💬 maflcko commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2671092845)
> > I don't think they have a 32-bit mode, so we'd have to drop that:
>
> They seem to have Aarch32 support! To verify this i spun up a `m7g` (Graviton 3) instance, running Ubuntu:
Thanks for checking. I guess we'd still have to drop the 32-bit cross compilation (to compile the code here), if we wanted to do it in CI (or add a separate task)? Though, I wonder if CI is a good way to test such changes, because CI doesn't print the debug logs on success and part of the testing involves lookin
...
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2671092845)
> > I don't think they have a 32-bit mode, so we'd have to drop that:
>
> They seem to have Aarch32 support! To verify this i spun up a `m7g` (Graviton 3) instance, running Ubuntu:
Thanks for checking. I guess we'd still have to drop the 32-bit cross compilation (to compile the code here), if we wanted to do it in CI (or add a separate task)? Though, I wonder if CI is a good way to test such changes, because CI doesn't print the debug logs on success and part of the testing involves lookin
...
👍 dergoegge approved a pull request: "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data"
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2629415560)
utACK a6d5ef3f604e646ae56c3a5c1aada48f9e134435
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2629415560)
utACK a6d5ef3f604e646ae56c3a5c1aada48f9e134435
💬 laanwj commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2671133552)
> because CI doesn't print the debug logs on success and part of the testing involves looking at debug logs.
Right, to make it usefully tested in CI we'd need a specific test case for it, that checks the instruction set is successfully detected and used. Otherwise all you're testing is "it doesn't hang". Which is, something at least.
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2671133552)
> because CI doesn't print the debug logs on success and part of the testing involves looking at debug logs.
Right, to make it usefully tested in CI we'd need a specific test case for it, that checks the instruction set is successfully detected and used. Otherwise all you're testing is "it doesn't hang". Which is, something at least.
🤔 maflcko reviewed a pull request: "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data"
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2629457813)
left a question. Also, did you check the coverage is not decreased? Ref: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/utxo_snapshot.cpp.gcov.html
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2629457813)
left a question. Also, did you check the coverage is not decreased? Ref: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/utxo_snapshot.cpp.gcov.html
💬 maflcko commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963329376)
I don't understand this pull request and I don't think it is possible, but I may be missing something obvious.
My understanding is that the blockchain in the fuzzing binary may always be different, because it will have a weaker pow check, or is there something that ensures the pow check in the fuzzing binary in this fuzz target behaves identical to the one in the unit tests?
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963329376)
I don't understand this pull request and I don't think it is possible, but I may be missing something obvious.
My understanding is that the blockchain in the fuzzing binary may always be different, because it will have a weaker pow check, or is there something that ensures the pow check in the fuzzing binary in this fuzz target behaves identical to the one in the unit tests?
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963357596)
In 9917a47ca49456172b03fee1e75c79ee72c381a6 "psbt: Add sighash types to PSBT when not DEFAULT or ALL"
Better. Maybe also drop the `else`, since the `SIGHASH_MISMATCH` guard makes sense isolation.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963357596)
In 9917a47ca49456172b03fee1e75c79ee72c381a6 "psbt: Add sighash types to PSBT when not DEFAULT or ALL"
Better. Maybe also drop the `else`, since the `SIGHASH_MISMATCH` guard makes sense isolation.
⚠️ dergoegge opened an issue: "build: x86 ASan build broken "error: inline assembly requires more registers than available""
(https://github.com/bitcoin/bitcoin/issues/31913)
Dockerfile to reproduce: https://gist.github.com/dergoegge/fc97743028f60719759b5498f5f022bf
Building with `clang-19` and `-fsantize=address` fails with:
```
7.574 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.574 356 | "movq 32(%%rsi), %%r11\n"
7.574 | ^
7.575 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.575 /bitcoin/src/secp256k1/src/sc
...
(https://github.com/bitcoin/bitcoin/issues/31913)
Dockerfile to reproduce: https://gist.github.com/dergoegge/fc97743028f60719759b5498f5f022bf
Building with `clang-19` and `-fsantize=address` fails with:
```
7.574 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.574 356 | "movq 32(%%rsi), %%r11\n"
7.574 | ^
7.575 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.575 /bitcoin/src/secp256k1/src/sc
...
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963414552)
Ok, in 795e911ded2563705cec69e43ff622ace22d58d5 "psbt: use sighash type field to determine whether to remove non-witness utxos" it's now more clear that this just moves a check inside the `for` loop.
I still can't wrap my head around the logic of how and why we drop `non_witness_utxos`.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963414552)
Ok, in 795e911ded2563705cec69e43ff622ace22d58d5 "psbt: use sighash type field to determine whether to remove non-witness utxos" it's now more clear that this just moves a check inside the `for` loop.
I still can't wrap my head around the logic of how and why we drop `non_witness_utxos`.
💬 dergoegge commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963416648)
Actually valid PoW will pass the reduced fuzz PoW check as well. But you're right I think the block hashes in the chain produced in the fuzz test and the unit test will be different.
We could move the assertion to the init function in the fuzz test, that should fail right now (i think).
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963416648)
Actually valid PoW will pass the reduced fuzz PoW check as well. But you're right I think the block hashes in the chain produced in the fuzz test and the unit test will be different.
We could move the assertion to the init function in the fuzz test, that should fail right now (i think).
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2671258378)
Code review ACK 61885ad9d406e25ec3b6523d01918e886996f1c3
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2671258378)
Code review ACK 61885ad9d406e25ec3b6523d01918e886996f1c3
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963431018)
> IIRC we had some guidelines somewhere that all intermediate commits should be legit - compiling and tests passing. This would matter during `git-bisect(1)`.
Yes, but the only enforcement is with the test-each-commit CI job, so I would be surprised if there weren't many other intermediate commits where there is some build or test error on windows. Also:
- In this case the error is in a new executable that didn't exist before this PR, so if the bisect test is testing this executable., it m
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963431018)
> IIRC we had some guidelines somewhere that all intermediate commits should be legit - compiling and tests passing. This would matter during `git-bisect(1)`.
Yes, but the only enforcement is with the test-each-commit CI job, so I would be surprised if there weren't many other intermediate commits where there is some build or test error on windows. Also:
- In this case the error is in a new executable that didn't exist before this PR, so if the bisect test is testing this executable., it m
...
🤔 rkrux reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2629300353)
Asked a question for clarity.
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2629300353)
Asked a question for clarity.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963435037)
Agree, it's a subtle point but important to convey in the comment.
The 2 assertions on line 124 and 125 below are correct individually but incongruent with regards to the overall node state.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963435037)
Agree, it's a subtle point but important to convey in the comment.
The 2 assertions on line 124 and 125 below are correct individually but incongruent with regards to the overall node state.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963232871)
Thanks for entertaining this request, the readability was indeed pulling me towards it but with the consensus rule in place, I agree with leaving this as-is.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963232871)
Thanks for entertaining this request, the readability was indeed pulling me towards it but with the consensus rule in place, I agree with leaving this as-is.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963427006)
Re: https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961637704
Continuing this thread here because there doesn't seem to be a way to continue that thread while making it part of the review.
As per this comment and related code https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2669011985, I understand that `FlushStateMode::IF_NEEDED` mode is used and therefore I assume that makes it quite unlikely to flush chainstate in a functional test, so the test will not fail.
> T
...
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1963427006)
Re: https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961637704
Continuing this thread here because there doesn't seem to be a way to continue that thread while making it part of the review.
As per this comment and related code https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2669011985, I understand that `FlushStateMode::IF_NEEDED` mode is used and therefore I assume that makes it quite unlikely to flush chainstate in a functional test, so the test will not fail.
> T
...
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2671300966)
> Can you add the note about `FUZZ_LIBS` usage to the PR description?
Sure! Added.
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2671300966)
> Can you add the note about `FUZZ_LIBS` usage to the PR description?
Sure! Added.
💬 janb84 commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2671302403)
tACK [f43b6ba](https://github.com/bitcoin/bitcoin/pull/31907/commits/f43b6ba27690442721623a9fa57b3c59e55d8397)
- Ran unit and functional tests related to assumeutxo (test/functional/feature_assumeutxo.py).
- Ran all the functional tests
- Validated the code change.
I also wrote a [review note](https://github.com/janb84/Bitcoin-Core_review-notes/blob/0f6634af718c2acedba9de74884cb149c2a68e2a/reviews/PR-%2331907.md) on the PR
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2671302403)
tACK [f43b6ba](https://github.com/bitcoin/bitcoin/pull/31907/commits/f43b6ba27690442721623a9fa57b3c59e55d8397)
- Ran unit and functional tests related to assumeutxo (test/functional/feature_assumeutxo.py).
- Ran all the functional tests
- Validated the code change.
I also wrote a [review note](https://github.com/janb84/Bitcoin-Core_review-notes/blob/0f6634af718c2acedba9de74884cb149c2a68e2a/reviews/PR-%2331907.md) on the PR
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1963455462)
I agree.
I've picked that commit from one of @theuni's branch. Perhaps it fell out of the correct context.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1963455462)
I agree.
I've picked that commit from one of @theuni's branch. Perhaps it fell out of the correct context.
💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2671319664)
> but I don't see how this can be achieved without possibly breaking someones build-setup.
Given we are already breaking all build setups in the CMake switch, it seems like the right time to make any further changes?
It's pretty clear the code here is convoluted/overcomplicated. Just the fact that the release process derives the same version number from two different places is odd (and is what hides the bug here).
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2671319664)
> but I don't see how this can be achieved without possibly breaking someones build-setup.
Given we are already breaking all build setups in the CMake switch, it seems like the right time to make any further changes?
It's pretty clear the code here is convoluted/overcomplicated. Just the fact that the release process derives the same version number from two different places is odd (and is what hides the bug here).
👍 ryanofsky approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2629670115)
Code review ACK 587ad67bf3bee03a7088e1dd5be2828b7a4f4fd4. Only change since last review is removing another dummy vTxSigOpsCost assignment.
IMO, it would be probably be more ideal to drop the dummy values from the `CBlockTemplate::vTxFees` and `CBlockTemplate::vTxSigOpsCost` vectors. I don't think it matters if they have different indexing than `CBlock::vtx` because that is a different vector in a different class, and it would be confusing for that vector *not* to contain a placeholder transa
...
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2629670115)
Code review ACK 587ad67bf3bee03a7088e1dd5be2828b7a4f4fd4. Only change since last review is removing another dummy vTxSigOpsCost assignment.
IMO, it would be probably be more ideal to drop the dummy values from the `CBlockTemplate::vTxFees` and `CBlockTemplate::vTxSigOpsCost` vectors. I don't think it matters if they have different indexing than `CBlock::vtx` because that is a different vector in a different class, and it would be confusing for that vector *not* to contain a placeholder transa
...