📝 hebasto reopened a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/31306)
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.
New "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been addressed.
(https://github.com/bitcoin/bitcoin/pull/31306)
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.
New "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been addressed.
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#issuecomment-2502140063)
> No need to close this if we can change the approach.
Rebased on top of the merged #31305 and #31364.
> I think we can agree on an approach where we (in this order):
>
> * Fix the actual bugs
>
> * Notate the false-positives
>
> * Enable the checks.
Done.
> It sounds like it also is worth adding a line in `developer-notes.md` that describes when a `NOLINT` is/is not appropriate.
Left this change for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31306#issuecomment-2502140063)
> No need to close this if we can change the approach.
Rebased on top of the merged #31305 and #31364.
> I think we can agree on an approach where we (in this order):
>
> * Fix the actual bugs
>
> * Notate the false-positives
>
> * Enable the checks.
Done.
> It sounds like it also is worth adding a line in `developer-notes.md` that describes when a `NOLINT` is/is not appropriate.
Left this change for a follow-up.
💬 laanwj commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2502145119)
> Just wanted to clarify that I'd love to see this get in sooner rather than later, as it can provide a valuable speedup and I'd be ready to ACK with a reduced (default) size selected, for the reasons I outlined above.
Yes, i think there's been enough analysis here to confirm that changing the default to 16MB or 32MB would give almost all of the gain, with the least risk, let's go for that.
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2502145119)
> Just wanted to clarify that I'd love to see this get in sooner rather than later, as it can provide a valuable speedup and I'd be ready to ACK with a reduced (default) size selected, for the reasons I outlined above.
Yes, i think there's been enough analysis here to confirm that changing the default to 16MB or 32MB would give almost all of the gain, with the least risk, let's go for that.
📝 furszy opened a pull request: "wallet: fix crash during migration due to invalid multisig descriptors"
(https://github.com/bitcoin/bitcoin/pull/31378)
Ensure legacy wallet migration skips non-standard or consensus-invalid multisig
scripts. Treating them as valid causes migration to crash because we are enforcing
this rules within the descriptors parsing logic.
Testing Notes:
This can be verified by cherry-picking and running the test commit on master.
It will crash there but pass on this branch.
(https://github.com/bitcoin/bitcoin/pull/31378)
Ensure legacy wallet migration skips non-standard or consensus-invalid multisig
scripts. Treating them as valid causes migration to crash because we are enforcing
this rules within the descriptors parsing logic.
Testing Notes:
This can be verified by cherry-picking and running the test commit on master.
It will crash there but pass on this branch.
🤔 darosior reviewed a pull request: "wallet: fix crash during migration due to invalid multisig descriptors"
(https://github.com/bitcoin/bitcoin/pull/31378#pullrequestreview-2462972517)
Ruling out consensus-invalid scripts makes sense, but discarding non-standard scripts is a much bigger leap. Also it would be a wack-a-mole: does the migration crash on any non-standard script? How much of that do you want to cover? Is it not possible to make the migration not crash on valid but not standard scripts instead of changing `IsMine` for those?
(https://github.com/bitcoin/bitcoin/pull/31378#pullrequestreview-2462972517)
Ruling out consensus-invalid scripts makes sense, but discarding non-standard scripts is a much bigger leap. Also it would be a wack-a-mole: does the migration crash on any non-standard script? How much of that do you want to cover? Is it not possible to make the migration not crash on valid but not standard scripts instead of changing `IsMine` for those?
💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1859491491)
Hmm, okay. I think it's better to remove it from this PR and maybe address the subject in another issue so it doesn’t interfere with the other commits.
So, the PR will include only:
- The --skip-missing-binaries option
- The error prompt to inform the user if no binaries are found in the build directory path.
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1859491491)
Hmm, okay. I think it's better to remove it from this PR and maybe address the subject in another issue so it doesn’t interfere with the other commits.
So, the PR will include only:
- The --skip-missing-binaries option
- The error prompt to inform the user if no binaries are found in the build directory path.
💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2502387159)
Pushed a [small fix](https://github.com/bitcoin/bitcoin/compare/1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3..0667ee2e5bbaba1d2965ab95644bdb27f881ffa1) addressing `blockhash` nullability.
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2502387159)
Pushed a [small fix](https://github.com/bitcoin/bitcoin/compare/1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3..0667ee2e5bbaba1d2965ab95644bdb27f881ffa1) addressing `blockhash` nullability.
💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1859531317)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1859531317)
Fixed.
👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2463413227)
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
The changes are very scoped to address the comment. Nice use of `get()`.
Repeated test in https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177 for sanity.
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2463413227)
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
The changes are very scoped to address the comment. Nice use of `get()`.
Repeated test in https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177 for sanity.
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2502698185)
Rebased 2f9c2f4c6dcf8c45514e99d1018301aba61940fc -> 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e ([`pr/mine.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.12) -> [`pr/mine.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.13), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.12-rebase..pr/mine.13)) fixing silent merge conflicts and adding a functional test and updating gen-manpages
---
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2394194530
r
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2502698185)
Rebased 2f9c2f4c6dcf8c45514e99d1018301aba61940fc -> 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e ([`pr/mine.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.12) -> [`pr/mine.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.13), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.12-rebase..pr/mine.13)) fixing silent merge conflicts and adding a functional test and updating gen-manpages
---
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2394194530
r
...
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1783158715)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1782379621
It would be surprising if current code couldn't hit this condition at all, though not surprising if this condition is just hard to hit because the tip is set to the genesis block very quickly on startup. Maybe could experiment with starting the node with reindex options or calling bitcoin-mine in a loop if it's worth debugging.
It would seem reasonable to not return optional though if getTip will not be available to
...
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1783158715)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1782379621
It would be surprising if current code couldn't hit this condition at all, though not surprising if this condition is just hard to hit because the tip is set to the genesis block very quickly on startup. Maybe could experiment with starting the node with reindex options or calling bitcoin-mine in a loop if it's worth debugging.
It would seem reasonable to not return optional though if getTip will not be available to
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860025340)
I mean the delay specifically, not the set. If we talk about an abstract "reconciliation set" (which is a combination of two), you apply the delay for an addition, but not for the removal action.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860025340)
I mean the delay specifically, not the set. If we talk about an abstract "reconciliation set" (which is a combination of two), you apply the delay for an addition, but not for the removal action.
💬 maflcko commented on pull request "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS":
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503135628)
> The current CI runners don't need this, as they aren't ephemeral.
Currently, each runner is their own user account, with their own podman image layer cache, so in theory, they'd benefit from this as well.
However, the images are quite large (especially msan), so my worry would be that after a sufficient number of builds, the workers will run out of storage, as the cache isn't cleared like the local images are.
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503135628)
> The current CI runners don't need this, as they aren't ephemeral.
Currently, each runner is their own user account, with their own podman image layer cache, so in theory, they'd benefit from this as well.
However, the images are quite large (especially msan), so my worry would be that after a sufficient number of builds, the workers will run out of storage, as the cache isn't cleared like the local images are.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860106036)
4b4d99fb2df2c60a2214487cec627bc560f50f53
this could be moved into the lambda :)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860106036)
4b4d99fb2df2c60a2214487cec627bc560f50f53
this could be moved into the lambda :)
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860077605)
8a73b1ca91b1ff47b56451ca4074f73ef71b9f79
Mind handling the result of erasure here? Either `Assert/Assume` or `if`
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860077605)
8a73b1ca91b1ff47b56451ca4074f73ef71b9f79
Mind handling the result of erasure here? Either `Assert/Assume` or `if`
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860061020)
```
std::multimap<uint16_t, NodeId> parents_by_peer;
for (const auto &[peer_id, state_or_salt]: m_states) {
if (const auto state = std::get_if<TxReconciliationState>(&state_or_salt)) {
const size_t parent_count = std::count_if(parents.begin(), parents.end(),
[=](const auto& wtxid){return state->m_local_set.find(wtxid) != state->m_local_set.end();});
parents_by_peer.insert(std::make_pair(parent_count, peer_id
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860061020)
```
std::multimap<uint16_t, NodeId> parents_by_peer;
for (const auto &[peer_id, state_or_salt]: m_states) {
if (const auto state = std::get_if<TxReconciliationState>(&state_or_salt)) {
const size_t parent_count = std::count_if(parents.begin(), parents.end(),
[=](const auto& wtxid){return state->m_local_set.find(wtxid) != state->m_local_set.end();});
parents_by_peer.insert(std::make_pair(parent_count, peer_id
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860131262)
(this could also be done in 4b4d99fb2df2c60a2214487cec627bc560f50f53 `GetFanoutTargets`, although it's not that much an improvement over there)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860131262)
(this could also be done in 4b4d99fb2df2c60a2214487cec627bc560f50f53 `GetFanoutTargets`, although it's not that much an improvement over there)
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860064472)
Multimap is ordered by key, and for same-key (same `parent_count`) since c++11 the order is consistent ([documented here](https://stackoverflow.com/a/13337612)). Feel free to include this as a comment, although I'm not even sure we care about the same exact output of multiple `SortPeersByFewestParents` calls.
(also it depends on the consistent ordering of `m_states` iterator too, which could be documented along the same lines, here and elsewhere it is iterated over)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860064472)
Multimap is ordered by key, and for same-key (same `parent_count`) since c++11 the order is consistent ([documented here](https://stackoverflow.com/a/13337612)). Feel free to include this as a comment, although I'm not even sure we care about the same exact output of multiple `SortPeersByFewestParents` calls.
(also it depends on the consistent ordering of `m_states` iterator too, which could be documented along the same lines, here and elsewhere it is iterated over)
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860143301)
Yeah, and perhaps good time to squash this into eb2b19d2c237f2cc7b4ff3e7cefec52a37eb24b3, it got much stale by now.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860143301)
Yeah, and perhaps good time to squash this into eb2b19d2c237f2cc7b4ff3e7cefec52a37eb24b3, it got much stale by now.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860146357)
yes
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860146357)
yes