🤔 jonatack reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2889310631)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2889310631)
Concept ACK
💬 jonatack commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121737144)
Last I looked, enforcing this across the codebase may require large changes in some areas to untangle everything.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121737144)
Last I looked, enforcing this across the codebase may require large changes in some areas to untangle everything.
📝 fanquake converted_to_draft a pull request: "uint256 cxx-20 constexpr patch"
(https://github.com/bitcoin/bitcoin/pull/32663)
(https://github.com/bitcoin/bitcoin/pull/32663)
💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121791082)
@mzumsande pushed as a follow-up commit. lmk what you think.
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121791082)
@mzumsande pushed as a follow-up commit. lmk what you think.
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931818779)
> @bigspider what version of the Ledger test app is required? And can it just sign (or provide the nonce) the PSBT (via HWI) and register the policy on first use? Or do you have to first explicitly register the policy like [Moosig](https://github.com/LedgerHQ/moosig/blob/master/moosig.py) does?
@Sjors, musig2 is supported from version 2.4.0 of the Bitcoin app, which at this time is the latest version. Note that the firmware OS must be up to date, or you would only find older versions of the a
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931818779)
> @bigspider what version of the Ledger test app is required? And can it just sign (or provide the nonce) the PSBT (via HWI) and register the policy on first use? Or do you have to first explicitly register the policy like [Moosig](https://github.com/LedgerHQ/moosig/blob/master/moosig.py) does?
@Sjors, musig2 is supported from version 2.4.0 of the Bitcoin app, which at this time is the latest version. Note that the firmware OS must be up to date, or you would only find older versions of the a
...
💬 jonatack commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2931826727)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2931826727)
Concept ACK
💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931846833)
@bigspider Ledger Live says my LedgerX device is up to date. The mainnet Bitcoin app version is 2.4.0, but the testnet is at 2.3.0.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931846833)
@bigspider Ledger Live says my LedgerX device is up to date. The mainnet Bitcoin app version is 2.4.0, but the testnet is at 2.3.0.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121848421)
As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121848421)
As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121849100)
As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121849100)
As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121858146)
> they do not fall within the scope of the IWYU repository.
IWYU has mappings for intrinsics though?
- https://github.com/include-what-you-use/include-what-you-use/blob/master/gcc-8.intrinsics.imp
- https://github.com/include-what-you-use/include-what-you-use/blob/master/clang-6.intrinsics.imp
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121858146)
> they do not fall within the scope of the IWYU repository.
IWYU has mappings for intrinsics though?
- https://github.com/include-what-you-use/include-what-you-use/blob/master/gcc-8.intrinsics.imp
- https://github.com/include-what-you-use/include-what-you-use/blob/master/clang-6.intrinsics.imp
👍 ryanofsky approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2889196839)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4. Seems a lot nicer to track more reliable information instead of relying on best-effort heuristics if this is now possible.
This change looks ready to merge with all the reviews and discussion so far, but there are some suggestions from 3 weeks ago https://github.com/bitcoin/bitcoin/pull/31405/commits/ed172d3002da68a3ddbd5d13e7d3f0618c1378d4#pullrequestreview-2843060953 that could be responded to. Could merge this or hold off depending
...
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2889196839)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4. Seems a lot nicer to track more reliable information instead of relying on best-effort heuristics if this is now possible.
This change looks ready to merge with all the reviews and discussion so far, but there are some suggestions from 3 weeks ago https://github.com/bitcoin/bitcoin/pull/31405/commits/ed172d3002da68a3ddbd5d13e7d3f0618c1378d4#pullrequestreview-2843060953 that could be responded to. Could merge this or hold off depending
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121671164)
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59):
Even after reading PR overview I found this commit description really confusing, because it only explained why it was ok to set flags here, not why it was actually good, or how the code wasn't broken before this commit. It would be helpful if the commit said these things explicitly:
- This commit marks descendants of failed blocks with BLOCK_FAILED_CHILD and updates m_best_heade
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121671164)
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59):
Even after reading PR overview I found this commit description really confusing, because it only explained why it was ok to set flags here, not why it was actually good, or how the code wasn't broken before this commit. It would be helpful if the commit said these things explicitly:
- This commit marks descendants of failed blocks with BLOCK_FAILED_CHILD and updates m_best_heade
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121705267)
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59):
Commit description mentions "at the cost of looping over the entire block index" which is referring to the loop in `Chainstate::SetBlockFailureFlags`. Probably not important in this context, but that function does seem like it could trigger unnecessary work because it is [adding blocks to m_dirty_blockindex](https://github.com/bitcoin/bitcoin/blob/1c6602399be6de0148938a3fd7b51e6c48
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121705267)
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59):
Commit description mentions "at the cost of looping over the entire block index" which is referring to the loop in `Chainstate::SetBlockFailureFlags`. Probably not important in this context, but that function does seem like it could trigger unnecessary work because it is [adding blocks to m_dirty_blockindex](https://github.com/bitcoin/bitcoin/blob/1c6602399be6de0148938a3fd7b51e6c48
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121916755)
In commit "validation: remove m_failed_blocks" (ed172d3002da68a3ddbd5d13e7d3f0618c1378d4)
Found this commit message confusing because it is talking about changes that actually happened in previous commits, and only referring to the code that sets flags, not the code that checks them.
Would suggest a more specific description like "After previous changes, the `pindexPrev->nStatus` check in `AcceptBlockHeader` is now sufficient to detect invalid blocks and checking `m_failed_blocks` there is
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121916755)
In commit "validation: remove m_failed_blocks" (ed172d3002da68a3ddbd5d13e7d3f0618c1378d4)
Found this commit message confusing because it is talking about changes that actually happened in previous commits, and only referring to the code that sets flags, not the code that checks them.
Would suggest a more specific description like "After previous changes, the `pindexPrev->nStatus` check in `AcceptBlockHeader` is now sufficient to detect invalid blocks and checking `m_failed_blocks` there is
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121892715)
In commit "validation: in invalidateblock, mark children as invalid right away" (eb973022dc2a324af4d61d9b55134577788bc83e)
Comment in commit message about the candidate getting marked failed later seems like useful information to have in the actual code.
Maybe add comment here like "// Do not remove candidate from the highpow cache, because it might be a descendant of the block being invalidated which needs to be marked failed later."
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121892715)
In commit "validation: in invalidateblock, mark children as invalid right away" (eb973022dc2a324af4d61d9b55134577788bc83e)
Comment in commit message about the candidate getting marked failed later seems like useful information to have in the actual code.
Maybe add comment here like "// Do not remove candidate from the highpow cache, because it might be a descendant of the block being invalidated which needs to be marked failed later."
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121667185)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090791378
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59)
The suggestion to call InvalidBlockFound instead of InvalidChainFound to avoid duplicating logic seems like a good idea, and would be interested to see a response, even if the change is not worth making here.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121667185)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090791378
In commit "validation: call InvalidChainfound also from AcceptBlock" (65de18836e5ddbdb8388e3bbdbcebee84c3b4c59)
The suggestion to call InvalidBlockFound instead of InvalidChainFound to avoid duplicating logic seems like a good idea, and would be interested to see a response, even if the change is not worth making here.
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121727206)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090831336
In commit "validation: cache all headers with enough PoW in invalidateblock" (9304257d67e6f280cc1b45021118c6bb37489d89)
> In which case, I don't think this part of the commit message is correct, at least for this commit?
Yes, I was going to ask about this same thing. I don't understand what "Since we don't remove blocks after inserting into setBlockIndexCandidates anymore" is referring to.
EDIT: I guess it refer
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121727206)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090831336
In commit "validation: cache all headers with enough PoW in invalidateblock" (9304257d67e6f280cc1b45021118c6bb37489d89)
> In which case, I don't think this part of the commit message is correct, at least for this commit?
Yes, I was going to ask about this same thing. I don't understand what "Since we don't remove blocks after inserting into setBlockIndexCandidates anymore" is referring to.
EDIT: I guess it refer
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121807875)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090961175
In commit "validation: in invalidateblock, calculate m_best_header right away" (a335d003eba7bf8204db457a2a6dcca0b2f53a43)
It does seem surprising that if both RecalculateBestHeader and the CheckBlockIndex `pindex->nChainWork <= m_best_header->nChainWork` were changed to use CBlockIndexWorkComparator the check would fail. Would be good to know what causes this and it would be good to have a comment in CheckBlockIndex
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121807875)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090961175
In commit "validation: in invalidateblock, calculate m_best_header right away" (a335d003eba7bf8204db457a2a6dcca0b2f53a43)
It does seem surprising that if both RecalculateBestHeader and the CheckBlockIndex `pindex->nChainWork <= m_best_header->nChainWork` were changed to use CBlockIndexWorkComparator the check would fail. Would be good to know what causes this and it would be good to have a comment in CheckBlockIndex
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2932138830)
> This change looks ready to merge with all the reviews and discussion so far, but there are some suggestions from 3 weeks ago [#31405 (review)](https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2843060953) that could be responded to. Could merge this or hold off depending on your preference @mzumsande
Please hold off merging, I'll address @stickies-v and your comments later this week!
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2932138830)
> This change looks ready to merge with all the reviews and discussion so far, but there are some suggestions from 3 weeks ago [#31405 (review)](https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2843060953) that could be responded to. Could merge this or hold off depending on your preference @mzumsande
Please hold off merging, I'll address @stickies-v and your comments later this week!
💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932150196)
Nice! Good work, @hebasto :)
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932150196)
Nice! Good work, @hebasto :)