💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1898964160)
lgtm ACK c65fde483133a04964cc8757c96005b78d9e8ca8
Didn't review the other commits.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1898964160)
lgtm ACK c65fde483133a04964cc8757c96005b78d9e8ca8
Didn't review the other commits.
🚀 achow101 merged a pull request: "test: Remove all-lint.py script"
(https://github.com/bitcoin/bitcoin/pull/29228)
(https://github.com/bitcoin/bitcoin/pull/29228)
💬 achow101 commented on pull request "rpc: Fix race in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898966431)
Should this be backported?
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898966431)
Should this be backported?
💬 achow101 commented on pull request "rpc: Fix race in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898969740)
ACK 5555d8db3313f893609eb0cf549bb597361d4466
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898969740)
ACK 5555d8db3313f893609eb0cf549bb597361d4466
💬 maflcko commented on pull request "rpc: Fix race in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898971746)
I don't think assumeutxo params were specified for the main chain, and a wrong RPC result output in some test cases seems harmless.
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1898971746)
I don't think assumeutxo params were specified for the main chain, and a wrong RPC result output in some test cases seems harmless.
🚀 achow101 merged a pull request: "rpc: Fix race in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29262)
(https://github.com/bitcoin/bitcoin/pull/29262)
💬 DocBrown101 commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1898994959)
I have the same problem on a raspberry pi 64 ...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1898994959)
I have the same problem on a raspberry pi 64 ...
💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898995069)
> The previous thread was closed by Bitcoin Core, due to obvious lack of consensus, yet for some reason this new thread that duplicates the agenda of the previous thread was created. I am confused? People who voiced objection in the previous thread appear largely to be ignoring this thread, since their points were already made.
Every objection made both here and in the original PR was quite thoroughly and clearly debunked as not having any bearing whatsoever on the issue at hand. The post ab
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898995069)
> The previous thread was closed by Bitcoin Core, due to obvious lack of consensus, yet for some reason this new thread that duplicates the agenda of the previous thread was created. I am confused? People who voiced objection in the previous thread appear largely to be ignoring this thread, since their points were already made.
Every objection made both here and in the original PR was quite thoroughly and clearly debunked as not having any bearing whatsoever on the issue at hand. The post ab
...
💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1898996304)
Quick update:
I did confirm (on another machine) that more contemporary versions of gcc (`12.2.0` in this case) do show a speed _up_ of about 4%, with slightly more time spent in kernel:
### #29169 vs. f8ca1357 (relative)
| bench name | x | #29169 | f8ca1357 |
| ---------------------------------------------------------- | --: | -----------------: | -----------------: |
| ibd.local.range.dbcache=4000.667200.697200.to
...
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1898996304)
Quick update:
I did confirm (on another machine) that more contemporary versions of gcc (`12.2.0` in this case) do show a speed _up_ of about 4%, with slightly more time spent in kernel:
### #29169 vs. f8ca1357 (relative)
| bench name | x | #29169 | f8ca1357 |
| ---------------------------------------------------------- | --: | -----------------: | -----------------: |
| ibd.local.range.dbcache=4000.667200.697200.to
...
💬 maflcko commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1899010205)
There'd be https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344713537 if someone wants to tackle it.
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1899010205)
There'd be https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344713537 if someone wants to tackle it.
👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1830011358)
Code review ACK e3592ee55756c08bda3075b54cbff719d3fb964f. Nice improvement eliminating g_signals and letting libbitcoinkernel application code control where signals are sent and how they are processed. (I think this could be mentioned in the commit description. I think it is a bigger accomplishment than removing a small dependency on CScheduler code).
This PR is definitely an improvement over status the quo, but I did make a number of suggestions below to improve internal consistency and nami
...
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1830011358)
Code review ACK e3592ee55756c08bda3075b54cbff719d3fb964f. Nice improvement eliminating g_signals and letting libbitcoinkernel application code control where signals are sent and how they are processed. (I think this could be mentioned in the commit description. I think it is a bigger accomplishment than removing a small dependency on CScheduler code).
This PR is definitely an improvement over status the quo, but I did make a number of suggestions below to improve internal consistency and nami
...
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457678045)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Slight preference for passing validation_signals dependency parameter first and mempool_opts options parameter second, for consistency with ChainstateManager, and since signals dependency is required in theory it should be possible to omit the options parameter.
It might also be worth asking whether the signals parameter actually should be required, since mempool class does not need to send sig
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457678045)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Slight preference for passing validation_signals dependency parameter first and mempool_opts options parameter second, for consistency with ChainstateManager, and since signals dependency is required in theory it should be possible to omit the options parameter.
It might also be worth asking whether the signals parameter actually should be required, since mempool class does not need to send sig
...
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457662881)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Mentioned earlier, but you should drop the new parameter here and just call `wallet->chain().waitForNotificationsIfTipChanged({})` instead of `SyncWithValidationInterfaceQueue`, for the same reasons
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457662881)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Mentioned earlier, but you should drop the new parameter here and just call `wallet->chain().waitForNotificationsIfTipChanged({})` instead of `SyncWithValidationInterfaceQueue`, for the same reasons
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457639359)
In commit "scripted-diff: Rename MainSignals to ValidationSignals" (0fd494c4a856d7aefd4fd94a4d5f18ab5e59c2a2)
Not important, but I think it would be a little nicer if scripted diff was last commit instead of first commit, because it's hard to tell whether renames are good or bad when the code is in flux, and better when you can see that names match state of the final code. Moving the scripted diff would also reduce churn within the PR so commit diffs are smaller and lines like this (which are
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457639359)
In commit "scripted-diff: Rename MainSignals to ValidationSignals" (0fd494c4a856d7aefd4fd94a4d5f18ab5e59c2a2)
Not important, but I think it would be a little nicer if scripted diff was last commit instead of first commit, because it's hard to tell whether renames are good or bad when the code is in flux, and better when you can see that names match state of the final code. Moving the scripted diff would also reduce churn within the PR so commit diffs are smaller and lines like this (which are
...
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457671335)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Note: This is ugly, but #24230 replaces it with `m_chain->attachChain()`. Other changes to indexing code in this PR, which also don't look great, have similar replacements.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457671335)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Note: This is ugly, but #24230 replaces it with `m_chain->attachChain()`. Other changes to indexing code in this PR, which also don't look great, have similar replacements.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457667372)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Would suggest organizing the parameters {validation_signals, interrupt, chainman_opts, blockman_opts} so ChainstateManager dependencies are first and ChainstateManager options are last. Also so dependencies and options are both ordered from high level to low level.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457667372)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Would suggest organizing the parameters {validation_signals, interrupt, chainman_opts, blockman_opts} so ChainstateManager dependencies are first and ChainstateManager options are last. Also so dependencies and options are both ordered from high level to low level.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457656786)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
These wallet tests should be calling `wallet.chain().waitForNotificationsIfTipChanged({})` instead of `test_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue`. This would make them more realistic and less tied to test and node internals.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457656786)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
These wallet tests should be calling `wallet.chain().waitForNotificationsIfTipChanged({})` instead of `test_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue`. This would make them more realistic and less tied to test and node internals.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457683117)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Note: At some point we should see if it makes sense to deduplicate this function with `AppInit` and `BasicTestingSetup::BasicTestingSetup` since all three functions are doing very similar things.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457683117)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Note: At some point we should see if it makes sense to deduplicate this function with `AppInit` and `BasicTestingSetup::BasicTestingSetup` since all three functions are doing very similar things.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457687097)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Would again suggest making make signals parameter first. `signals` is the hard dependency and input, `notifications` is the option and output.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457687097)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
Would again suggest making make signals parameter first. `signals` is the hard dependency and input, `notifications` is the option and output.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457699474)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
`m_signals` declaration should be moved directly above `m_proxy` declaration. IMO it is a bad practice to mix up method declarations and variable declarations in a class definition. The most important thing you can know about a class is what it information contains and what it represents, and that is difficult to do if you can't easily see a list of its member variables.
Additionally, all membe
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457699474)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)
`m_signals` declaration should be moved directly above `m_proxy` declaration. IMO it is a bad practice to mix up method declarations and variable declarations in a class definition. The most important thing you can know about a class is what it information contains and what it represents, and that is difficult to do if you can't easily see a list of its member variables.
Additionally, all membe
...