💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718625)
Thanks! Reworked.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121720547)
Done.

🤔 jonatack reviewed a pull request: "fuzz: set the output argument of FuzzedSock::Accept()"
(https://github.com/bitcoin/bitcoin/pull/31316#pullrequestreview-2889292340)
utACK 83199523c90591d57cd5046212c878a4d54d621d

This commit was also pulled into #28584 as https://github.com/bitcoin/bitcoin/pull/28584/commits/cf0a49723bbbbb22d47ccb8f0e030a51e6757609 and is identical at the time of review.

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121738729)
Ok, so no an issue if we actually land the patch (and remove the related code in the CI).

🤔 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

💬 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.

📝 fanquake converted_to_draft a pull request: "uint256 cxx-20 constexpr patch"
(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.

💬 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
...

💬 jonatack commented on pull request "headerssync: Keep tests ahead of increasing params":
(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.

💬 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.

💬 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.

💬 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

👍 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
...

💬 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
...

💬 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
...

💬 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
...

💬 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."

💬 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.