💬 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 :)
💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121986744)
Added a check for `persistent`.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121986744)
Added a check for `persistent`.
💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987027)
Changed.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987027)
Changed.
💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987972)
It's generally a pattern in the wallet that we update in memory before writing to disk.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987972)
It's generally a pattern in the wallet that we update in memory before writing to disk.
💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2121989228)
Do these defines actually do anything in our case, or are they just here for the sake of being explicit?
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2121989228)
Do these defines actually do anything in our case, or are they just here for the sake of being explicit?
💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121989490)
It is a bit weird, but changing the interface is probably not going to happen as it would be a breaking change.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121989490)
It is a bit weird, but changing the interface is probably not going to happen as it would be a breaking change.