💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r2183339114)
OK, looks like the PR won't update with my branch now that it's closed, but it's here: https://github.com/davidgumberg/bitcoin/tree/1-15-24-configure_warnings
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r2183339114)
OK, looks like the PR won't update with my branch now that it's closed, but it's here: https://github.com/davidgumberg/bitcoin/tree/1-15-24-configure_warnings
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183379788)
If the situation "previous blocks all have nSequenceId = x-1 and the tip has x" is weird, wouldn't that exact same situation occur anyway when the next block arrives via p2p (which would get `nSequenceId=2`, where all predecessors have `nSequenceId=1`)?
And if `nSequenceId` is supposed to be monotonically increasing (which I think would mean that no connectable block should have a lower `nSequenceId` than its predecessor), wouldn't that be an argument **for** only setting the tip rather than
...
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183379788)
If the situation "previous blocks all have nSequenceId = x-1 and the tip has x" is weird, wouldn't that exact same situation occur anyway when the next block arrives via p2p (which would get `nSequenceId=2`, where all predecessors have `nSequenceId=1`)?
And if `nSequenceId` is supposed to be monotonically increasing (which I think would mean that no connectable block should have a lower `nSequenceId` than its predecessor), wouldn't that be an argument **for** only setting the tip rather than
...
💬 KATHI160 commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3033399551)
> in the `multiprocess, i686, DEBUG` task in `self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)`,
>
> https://cirrus-ci.com/task/5704849158832128?logs=ci#L2967:
>
> ```
> node1 2025-06-26T14:05:40.072184Z [msghand] [net_processing.cpp:3452] [ProcessMessage] [net] received: pong (8 bytes) peer=0
> test 2025-06-26T14:05:43.222938Z TestFramework (ERROR): JSONRPC error
> Traceback (most recent call last
...
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3033399551)
> in the `multiprocess, i686, DEBUG` task in `self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)`,
>
> https://cirrus-ci.com/task/5704849158832128?logs=ci#L2967:
>
> ```
> node1 2025-06-26T14:05:40.072184Z [msghand] [net_processing.cpp:3452] [ProcessMessage] [net] received: pong (8 bytes) peer=0
> test 2025-06-26T14:05:43.222938Z TestFramework (ERROR): JSONRPC error
> Traceback (most recent call last
...
💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183627680)
> so having a chain of blocks where the previous blocks all have nSequenceId = x-1 and the tip has x
Sorry, I wrote that wrong. `:s/x-1/x+1/g`
In the case were we only change the tip, the rest of blocks will have `nSequenceId=1`, so we will end up with a chain like:
`1 -> 1 -> 1 -> 0 (tip)` that will evolve to `1 -> 1 -> 1 -> 0 -> 2 (tip)`
All other chains will have `nSequenceId=1` from start to end.
In the case proposed here, we'll have
`0 -> 0 -> 0 -> 0 (tip)` evolving to `
...
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183627680)
> so having a chain of blocks where the previous blocks all have nSequenceId = x-1 and the tip has x
Sorry, I wrote that wrong. `:s/x-1/x+1/g`
In the case were we only change the tip, the rest of blocks will have `nSequenceId=1`, so we will end up with a chain like:
`1 -> 1 -> 1 -> 0 (tip)` that will evolve to `1 -> 1 -> 1 -> 0 -> 2 (tip)`
All other chains will have `nSequenceId=1` from start to end.
In the case proposed here, we'll have
`0 -> 0 -> 0 -> 0 (tip)` evolving to `
...
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183651193)
oh right, I got confused in my earlier post, you are right. Still, maybe others could chime in if they think that this monotonicity is worth the added work or not.
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2183651193)
oh right, I got confused in my earlier post, you are right. Still, maybe others could chime in if they think that this monotonicity is worth the added work or not.
💬 tnndbtc commented on issue "Intermittent failure in feature_fee_estimation.py in check_raw_estimates feerate = float(e["feerate"]) KeyError: 'feerate'":
(https://github.com/bitcoin/bitcoin/issues/31944#issuecomment-3033451565)
@ismaelsadeeq @maflcko We currently do not have a good solution to fix this but have a good understanding on why it occasionally fails. Would it be helpful to add a error message when **feerate** is missing in the key and state that it's expected for certain random seeds, and a workaround is to re-run the test with a different random seed so other people wouldn't need to spend more time digging out the root cause?
(https://github.com/bitcoin/bitcoin/issues/31944#issuecomment-3033451565)
@ismaelsadeeq @maflcko We currently do not have a good solution to fix this but have a good understanding on why it occasionally fails. Would it be helpful to add a error message when **feerate** is missing in the key and state that it's expected for certain random seeds, and a workaround is to re-run the test with a different random seed so other people wouldn't need to spend more time digging out the root cause?
📝 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw opened a pull request: "New SVG, Icons, PNGs and X PixMaps"
(https://github.com/bitcoin/bitcoin/pull/32871)
This PR clean up the old SVG, Icons, PNGs and X PixMaps by providing and optimized much smaller file size versions while at the same time keeping resolutions untouched.
Shadows are removed.
Ping: @jonasschnelli because he was the author of the previous files. Please check the PR.
Thank you
(https://github.com/bitcoin/bitcoin/pull/32871)
This PR clean up the old SVG, Icons, PNGs and X PixMaps by providing and optimized much smaller file size versions while at the same time keeping resolutions untouched.
Shadows are removed.
Ping: @jonasschnelli because he was the author of the previous files. Please check the PR.
Thank you
💬 marcofleon commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3033602785)
ReACK fa501ea5ed874ffcba2b606806460e61a1204bd2
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3033602785)
ReACK fa501ea5ed874ffcba2b606806460e61a1204bd2
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2183756175)
Rebased and fixed the previous CI error. But now CI is showing a different error which is unrelated to my current change.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2183756175)
Rebased and fixed the previous CI error. But now CI is showing a different error which is unrelated to my current change.
💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2183768740)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768
> [04e51ba](https://github.com/bitcoin/bitcoin/commit/04e51ba1110e13598e0b11a35ce5abf4a1789f53): now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.
Good idea. Latest push now integrates the release notes from the other PR and describes the changes as a whole
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2183768740)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768
> [04e51ba](https://github.com/bitcoin/bitcoin/commit/04e51ba1110e13598e0b11a35ce5abf4a1789f53): now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.
Good idea. Latest push now integrates the release notes from the other PR and describes the changes as a whole
🤔 ryanofsky reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2984719522)
Thanks for the reviews!
Updated 705791cd436f237fe9bbac2cf52d63ab4b2a41c7 -> 05de8bc1735accc494291e22da718bebab163547 ([`pr/libexec.7`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.7) -> [`pr/libexec.8`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.7..pr/libexec.8)) updating commit message to match release description, and implementing Sjors `bitcoin` help suggestion https://github.com/bitcoin/bitcoin/p
...
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2984719522)
Thanks for the reviews!
Updated 705791cd436f237fe9bbac2cf52d63ab4b2a41c7 -> 05de8bc1735accc494291e22da718bebab163547 ([`pr/libexec.7`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.7) -> [`pr/libexec.8`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.7..pr/libexec.8)) updating commit message to match release description, and implementing Sjors `bitcoin` help suggestion https://github.com/bitcoin/bitcoin/p
...
✅ achow101 closed a pull request: "descriptors: Allow `H` as a hardened indicator"
(https://github.com/bitcoin/bitcoin/pull/32788)
(https://github.com/bitcoin/bitcoin/pull/32788)
💬 achow101 commented on pull request "descriptors: Allow `H` as a hardened indicator":
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-3033825918)
https://github.com/bitcoin/bips/pull/1888 was merged so this is no longer relevant.
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-3033825918)
https://github.com/bitcoin/bips/pull/1888 was merged so this is no longer relevant.
💬 achow101 commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-3033841061)
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-3033841061)
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
🚀 achow101 merged a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307)
(https://github.com/bitcoin/bitcoin/pull/29307)
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183868035)
Thanks for this suggestion, I've implemented this approach in https://github.com/bitcoin/bitcoin/pull/32273/commits/55e60bd9c76ea74a108cd5c7275e26554b822b4c, and I've reused `migrate_and_get_rpc()` in the other tests added here (except [`test_failed_migration_cleanup_relative_path()`](https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L1038) where it's not feasible)
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183868035)
Thanks for this suggestion, I've implemented this approach in https://github.com/bitcoin/bitcoin/pull/32273/commits/55e60bd9c76ea74a108cd5c7275e26554b822b4c, and I've reused `migrate_and_get_rpc()` in the other tests added here (except [`test_failed_migration_cleanup_relative_path()`](https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L1038) where it's not feasible)
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183873334)
As suggested by @furszy [above](https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274) I moved this logic to `migrate_and_get_rpc()` in https://github.com/bitcoin/bitcoin/commit/55e60bd9c76ea74a108cd5c7275e26554b822b4c:
https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L125-L147
And I've reused `migrate_and_get_rpc()` in the tests added in this PR.
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183873334)
As suggested by @furszy [above](https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274) I moved this logic to `migrate_and_get_rpc()` in https://github.com/bitcoin/bitcoin/commit/55e60bd9c76ea74a108cd5c7275e26554b822b4c:
https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L125-L147
And I've reused `migrate_and_get_rpc()` in the tests added in this PR.
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3033891866)
> I don't think belt and suspenders checks are a great idea in general, but even when they are appropriate I think they need to be labeled with "// This condition is never expected to trigger" or similar comments to avoid confusing people reading the code and trying understand why the checks are there.
OK, in lieu of an assert/assume, I've gone to the simplified form suggested, and I've also used the lambda+ternary over if+else form as you suggested. I've also taken advantage of the fact that
...
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3033891866)
> I don't think belt and suspenders checks are a great idea in general, but even when they are appropriate I think they need to be labeled with "// This condition is never expected to trigger" or similar comments to avoid confusing people reading the code and trying understand why the checks are there.
OK, in lieu of an assert/assume, I've gone to the simplified form suggested, and I've also used the lambda+ternary over if+else form as you suggested. I've also taken advantage of the fact that
...
💬 achow101 commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3033949761)
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3033949761)
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
✅ achow101 closed an issue: "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix [AssertionError: Estimated fee (0.00923427) out of range]"
(https://github.com/bitcoin/bitcoin/issues/32461)
(https://github.com/bitcoin/bitcoin/issues/32461)