💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2671015861)
Rebased on the latest https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2627160751.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2671015861)
Rebased on the latest https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2627160751.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1963275816)
Indeed there can be quite a delay in when miners submit jobs. And it's also possible that not every new template we create is actually deployed on every ASIC, as pool software might filter them (not the case at the moment afaik).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1963275816)
Indeed there can be quite a delay in when miners submit jobs. And it's also possible that not every new template we create is actually deployed on every ASIC, as pool software might filter them (not the case at the moment afaik).
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1963284841)
The phrasing is a bit unclear, but "alternate" refers to what happens within each tick: we first check if the tip changed, then check if the fees went up.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1963284841)
The phrasing is a bit unclear, but "alternate" refers to what happens within each tick: we first check if the tip changed, then check if the fees went up.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963287780)
IIRC we had some guidelines somewhere that all intermediate commits should be legit - compiling and tests passing. This would matter during `git-bisect(1)`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963287780)
IIRC we had some guidelines somewhere that all intermediate commits should be legit - compiling and tests passing. This would matter during `git-bisect(1)`.
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2671077699)
> This PR is an immediate win because we don't have to perform checks like we do in
No this PR doesn't change anything there. It's just that people suggested using `-vTxFees[]` directly there instead of doing the calculation. This PR removes that temptation (and makes it easier to get rid of `vTxFees` entirely at some point).
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2671077699)
> This PR is an immediate win because we don't have to perform checks like we do in
No this PR doesn't change anything there. It's just that people suggested using `-vTxFees[]` directly there instead of doing the calculation. This PR removes that temptation (and makes it easier to get rid of `vTxFees` entirely at some point).
💬 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
...