🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026880589)
Split out a test commit, moved a rename to the scripted diff, clarified test refactors and testing strategies in commit messages.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026880589)
Split out a test commit, moved a rename to the scripted diff, clarified test refactors and testing strategies in commit messages.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211613705)
We can still do that, but given that we need to be able to display the contents of the obfuscation anyway (unless we go back to reading/writing vectors), I think the current way is probably simpler. Please let me know if you disagree.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211613705)
We can still do that, but given that we need to be able to display the contents of the obfuscation anyway (unless we go back to reading/writing vectors), I think the current way is probably simpler. Please let me know if you disagree.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211627396)
Many of those changes were [suggested during review](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837161349). `dbwrapper` was added because of
https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057969172, after I saw that IBD failed even after all tests were passing. I've extracted it a separate commit now and explained the changes - and moved a remaining rename to the later scripted diff.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211627396)
Many of those changes were [suggested during review](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837161349). `dbwrapper` was added because of
https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057969172, after I saw that IBD failed even after all tests were passing. I've extracted it a separate commit now and explained the changes - and moved a remaining rename to the later scripted diff.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211610091)
Do you recommend we apply any of that after the recent changes?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211610091)
Do you recommend we apply any of that after the recent changes?
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211605825)
These changes are meant to minimize the diffs for the later optimization commits later.
I have added them after you have requested [simplifying a more complicated optimization commit](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1879140048) - these were the parts I was able to extract such that the diff is minimal in the high-risk commits. Later commits can simply change the `vector` to `Obfuscation` because of these refactors. Would you like me to go into more details in the commi
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211605825)
These changes are meant to minimize the diffs for the later optimization commits later.
I have added them after you have requested [simplifying a more complicated optimization commit](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1879140048) - these were the parts I was able to extract such that the diff is minimal in the high-risk commits. Later commits can simply change the `vector` to `Obfuscation` because of these refactors. Would you like me to go into more details in the commi
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211598937)
Let me know if this is still relevant after the previously pushed changes
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211598937)
Let me know if this is still relevant after the previously pushed changes
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211596259)
The two tests are doing different things - one does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The second one does what you mentioned, so if I understand you correctly, what you suggest is rather dropping the roundtrip test? I think it's useful because if it fails it gives a higher-level error.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211596259)
The two tests are doing different things - one does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The second one does what you mentioned, so if I understand you correctly, what you suggest is rather dropping the roundtrip test? I think it's useful because if it fails it gives a higher-level error.
📝 mzumsande opened a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997)
The logic for DBHashKey / DBHeightKey handling and lookup of entries is shared by coinstatsindex and blockfilterindex, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/key.h` (using templates for the index-specific `DBVal`).
Also remove an unnecessary db query in `CustomAppend()` for the coinstatsindex.
(https://github.com/bitcoin/bitcoin/pull/32997)
The logic for DBHashKey / DBHeightKey handling and lookup of entries is shared by coinstatsindex and blockfilterindex, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/key.h` (using templates for the index-specific `DBVal`).
Also remove an unnecessary db query in `CustomAppend()` for the coinstatsindex.
💬 achow101 commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211716770)
Done
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211716770)
Done
🤔 furszy reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3027069074)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3027069074)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211768238)
It's not true that they aren't used for anything. The point of reading the block from disk is to check if it's the prev block that is expected because the values that are written for the current block depend on the prev block values. They are used not from the prev block but from the members instead though. I am not sure right now if this check can just be removed, but if they are still needed the expected prev block hash could probably be written into a member too and the DB read could be avoid
...
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211768238)
It's not true that they aren't used for anything. The point of reading the block from disk is to check if it's the prev block that is expected because the values that are written for the current block depend on the prev block values. They are used not from the prev block but from the members instead though. I am not sure right now if this check can just be removed, but if they are still needed the expected prev block hash could probably be written into a member too and the DB read could be avoid
...
📝 ajtowns opened a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998)
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type
...
(https://github.com/bitcoin/bitcoin/pull/32998)
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type
...
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3081648267)
The "introduce script_verify_flags typename" commit is probably best reviewed with `--word-diff` fwiw.
cc @instagibbs @darosior @benthecarman
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3081648267)
The "introduce script_verify_flags typename" commit is probably best reviewed with `--word-diff` fwiw.
cc @instagibbs @darosior @benthecarman
💬 nervana21 commented on pull request "log: unify `UpdateTip` values":
(https://github.com/bitcoin/bitcoin/pull/32996#issuecomment-3081669227)
tACK [b039305](https://github.com/bitcoin/bitcoin/pull/32996/commits/b039305d3590aaf84bfdf3b3547b9311897f52c7)
Before:
> 2025-07-16T22:31:33Z UpdateTip: new best=000000000000000000022dabca47b7aabc24d93d0241536ca75dc528d08d5eba height=905352 version=0x20c26000 log2_work=95.713336 tx=1212933015 date='2025-07-13T11:06:38Z' progress=0.998857 cache=8.8MiB(61209txo)
After:
> 2025-07-16T22:54:20Z UpdateTip: new best=0000000000000000000186eed935e4ad209e7aaa4ceeb2542851feaf4b8d426f height=905
...
(https://github.com/bitcoin/bitcoin/pull/32996#issuecomment-3081669227)
tACK [b039305](https://github.com/bitcoin/bitcoin/pull/32996/commits/b039305d3590aaf84bfdf3b3547b9311897f52c7)
Before:
> 2025-07-16T22:31:33Z UpdateTip: new best=000000000000000000022dabca47b7aabc24d93d0241536ca75dc528d08d5eba height=905352 version=0x20c26000 log2_work=95.713336 tx=1212933015 date='2025-07-13T11:06:38Z' progress=0.998857 cache=8.8MiB(61209txo)
After:
> 2025-07-16T22:54:20Z UpdateTip: new best=0000000000000000000186eed935e4ad209e7aaa4ceeb2542851feaf4b8d426f height=905
...
💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211796030)
ah, ok. I thought that it was used in the past for something but no longer is, because we load data into a local variable and let it go out of scope without using it. So the point is the `return false` / the logged warning? Is this a legitimitate situation we can get into outside of index corruption?
In any case, I'll drop that commit with the next push, it's not the main focus of this PR.
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211796030)
ah, ok. I thought that it was used in the past for something but no longer is, because we load data into a local variable and let it go out of scope without using it. So the point is the `return false` / the logged warning? Is this a legitimitate situation we can get into outside of index corruption?
In any case, I'll drop that commit with the next push, it's not the main focus of this PR.
💬 achow101 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3081753878)
BIP 54 states
> For each input in the transaction, count the number of CHECKSIG and CHECKMULTISIG in the input scriptSig and previous output's scriptPubKey, including the P2SH redeemScript
However I don't see where the P2SH redeemScript sigops are counted. `CheckSigopsBIP54` doesn't parse the redeemScript as a script to count it's sigops like how `AreInputsStandard` does for the `MAX_P2SH_SIGOPS` check.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3081753878)
BIP 54 states
> For each input in the transaction, count the number of CHECKSIG and CHECKMULTISIG in the input scriptSig and previous output's scriptPubKey, including the P2SH redeemScript
However I don't see where the P2SH redeemScript sigops are counted. `CheckSigopsBIP54` doesn't parse the redeemScript as a script to count it's sigops like how `AreInputsStandard` does for the `MAX_P2SH_SIGOPS` check.
💬 achow101 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2211833948)
In 644f811572656d77f242c6d9c67067f9baf88540 "qa: unit test standardness of inputs packed with legacy sigops"
Using `max_sigops_redeem_script` here makes that script the scriptSig, not pushes it to the scriptSig as we would expect for a P2SH redeem script. Instead, `max_sigops_redeem_script` needs to be itself pushed to a script.
Something like
```
std::vector<unsigned char> rs_bytes(max_sigops_redeem_script.begin(), max_sigops_redeem_script.end());
CScript scriptsig;
scriptsig << rs_b
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2211833948)
In 644f811572656d77f242c6d9c67067f9baf88540 "qa: unit test standardness of inputs packed with legacy sigops"
Using `max_sigops_redeem_script` here makes that script the scriptSig, not pushes it to the scriptSig as we would expect for a P2SH redeem script. Instead, `max_sigops_redeem_script` needs to be itself pushed to a script.
Something like
```
std::vector<unsigned char> rs_bytes(max_sigops_redeem_script.begin(), max_sigops_redeem_script.end());
CScript scriptsig;
scriptsig << rs_b
...
👍 theStack approved a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3027186934)
Code-review ACK 50024620b909fc30b68a3715680e963f048482a5
With two suggestions regarding sanity checks on `lower_bound` iterators. Probably more than just nits, but still fine to tackle in the follow-up IMHO.
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3027186934)
Code-review ACK 50024620b909fc30b68a3715680e963f048482a5
With two suggestions regarding sanity checks on `lower_bound` iterators. Probably more than just nits, but still fine to tackle in the follow-up IMHO.
💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211827948)
```suggestion
if (!Assume(it != index_by_wtxid.end() && it->m_tx->GetWitnessHash() == wtxid)) continue;
```
for a full belts and suspenders (though I guess if no `m_orphan` entry with this wtxid exists, it would still be caught with the next `Assume` below, as `std::distance` would return a negative(?) value 🤔 )
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211827948)
```suggestion
if (!Assume(it != index_by_wtxid.end() && it->m_tx->GetWitnessHash() == wtxid)) continue;
```
for a full belts and suspenders (though I guess if no `m_orphan` entry with this wtxid exists, it would still be caught with the next `Assume` below, as `std::distance` would return a negative(?) value 🤔 )
💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211794222)
these two `Assume` lines should be swapped I think, to prevent potential dereference of an end() iterator (which, AFAIR, would be UB)
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211794222)
these two `Assume` lines should be swapped I think, to prevent potential dereference of an end() iterator (which, AFAIR, would be UB)
💬 davidgumberg commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#issuecomment-3081817336)
ACK 060695c22ae7b2b0f2a1d
I think this change is good since we can't ever be certain that loading will always succeed after migration, but I think cases where we know migration will succeed but loading will fail should be addressed. i.e. shouldn't we also change migration so that the case used in the functional test never successfully migrated in the first place?
Nit: Update PR description now that functional test is included.
(https://github.com/bitcoin/bitcoin/pull/32984#issuecomment-3081817336)
ACK 060695c22ae7b2b0f2a1d
I think this change is good since we can't ever be certain that loading will always succeed after migration, but I think cases where we know migration will succeed but loading will fail should be addressed. i.e. shouldn't we also change migration so that the case used in the functional test never successfully migrated in the first place?
Nit: Update PR description now that functional test is included.