💬 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.
💬 AMoynahan89 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932168428)
Why is this pr still open?
"It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101
https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932168428)
Why is this pr still open?
"It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101
https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896
🤔 mabu44 reviewed a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2889699475)
ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d
Reviewed the code, compiled it, and ran the new tests. Additionally, made modifications to both the code and the tests to intentionally cause the tests to fail in expected ways.
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2889699475)
ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d
Reviewed the code, compiled it, and ran the new tests. Additionally, made modifications to both the code and the tests to intentionally cause the tests to fail in expected ways.
💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2122002429)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2122002429)
Fixed
💬 achow101 commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2122004809)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2122004809)
Fixed
👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2889705787)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4, just rebased due to various conflicts since last review.
---
re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097
> We should treat the `libmultiprocess` subtree the same as all other subtrees and disable the `EXPORT_COMPILE_COMMANDS` property for _all_ its targets.
This could be reasonable, though I'm not very sure what the benefits of disabling linting here would be. Also, I think even with EXPORT_COMPIL
...
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2889705787)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4, just rebased due to various conflicts since last review.
---
re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097
> We should treat the `libmultiprocess` subtree the same as all other subtrees and disable the `EXPORT_COMPILE_COMMANDS` property for _all_ its targets.
This could be reasonable, though I'm not very sure what the benefits of disabling linting here would be. Also, I think even with EXPORT_COMPIL
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2122005059)
In commit "build: depends makes libmultiprocess by default" (adc1bcf784ae9763246f1f143ebc225214bf1730)
Not important, but might be nice to replace `multiprocess_packages` with `ipc_packages` to be more consistent.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2122005059)
In commit "build: depends makes libmultiprocess by default" (adc1bcf784ae9763246f1f143ebc225214bf1730)
Not important, but might be nice to replace `multiprocess_packages` with `ipc_packages` to be more consistent.