💬 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.
💬 ajtowns commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3081928184)
cf #32945 maybe?
Changing test_runner.py from python to cmake logic might make running the functional and unit tests in parallel more plausible?
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3081928184)
cf #32945 maybe?
Changing test_runner.py from python to cmake logic might make running the functional and unit tests in parallel more plausible?
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2211907184)
Thank you, fixed!
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2211907184)
Thank you, fixed!
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3081990855)
Rebased to address @pablomartin4btc and merge conflict.
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3081990855)
Rebased to address @pablomartin4btc and merge conflict.
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233)
CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
> Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directiv
...
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233)
CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
> Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directiv
...
💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3082477608)
> Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly. I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
Without the lint changes, having multiple test fixtures in a single cpp file causes CI failures. Without the cmake changes, the additional test fixtures aren't executed via ctest. So I don't think it makes
...
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3082477608)
> Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly. I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
Without the lint changes, having multiple test fixtures in a single cpp file causes CI failures. Without the cmake changes, the additional test fixtures aren't executed via ctest. So I don't think it makes
...
🤔 l0rinc requested changes to a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3027286087)
Concept ACK, we should indeed make the error messages more user friendly.
It seems to me the PR is heading in the right direction, but we need to separate handling unrelated code paths - bech32 and base58 are different enough that they deserve their own helper method, not just buried somewhere in an if condition.
The functional tests could also use better separation, without favoring one or the other in the default params and the commit messages should explain the context better.
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3027286087)
Concept ACK, we should indeed make the error messages more user friendly.
It seems to me the PR is heading in the right direction, but we need to separate handling unrelated code paths - bech32 and base58 are different enough that they deserve their own helper method, not just buried somewhere in an if condition.
The functional tests could also use better separation, without favoring one or the other in the default params and the commit messages should explain the context better.
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211854675)
Q: when do we refer to it as `Bech32` and when `Bech32(m)`?
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211854675)
Q: when do we refer to it as `Bech32` and when `Bech32(m)`?