💬 rkrux 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_r2190230878)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

I don't think this test is called and it fails upon being called because I don't believe the wallet is created without private keys.

💬 rkrux 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_r2190214038)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

Shouldn't the descriptor checksum be added?

💬 rkrux 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_r2190224703)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

There's a redundant RPC.
```diff
- wallet.listdescriptors()
- for d in wallet.listdescriptors()["descriptors"]:
+ descs = wallet.listdescriptors()
+ for d in descs["descriptors"]:
```

💬 kevkevinpal commented on pull request "threading: use correct mutex name in reverse_lock fatal error messages":
(https://github.com/bitcoin/bitcoin/pull/32829#issuecomment-3045441605)
> Done in #32888

awesome going to squash [41f64bd](https://github.com/bitcoin/bitcoin/pull/32829/commits/41f64bd636f580851ef9aa27d56dc45885ace0c7) and [85c2848](https://github.com/bitcoin/bitcoin/pull/32829/commits/85c2848eb575f4abaa81fdd4e8f3b2048693dd98)

💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32891#issuecomment-3045444299)
@maflcko You mean this commit? https://github.com/bitcoin/bitcoin/commit/42f6a0c2b946af2cce86b721b349d35c4e21ce88
Yes the PNGs was optimized but not correctly, that's why you can see in my commit for example that my bitcoin.png is -256 KB (16%) smaller than the current file in the repo. https://github.com/bitcoin/bitcoin/pull/32891/files#diff-5e43a57fa6d8a58177b80b3d7d608e6fffcda2a75078da2aa47bb4e447b44f81 also the script only optimized PNG files unlike my commit that optimized all bitcoin log
...

👍 theuni approved a pull request: "threading: use correct mutex name in reverse_lock fatal error messages"
(https://github.com/bitcoin/bitcoin/pull/32829#pullrequestreview-2994166696)
ACK 41f64bd636f580851ef9aa27d56dc45885ace0c7.

Thanks. I had the same test fixup locally on my branch, just forgot to push it.

👍 theuni approved a pull request: "threading: use correct mutex name in reverse_lock fatal error messages"
(https://github.com/bitcoin/bitcoin/pull/32829#pullrequestreview-2994172589)
Re-ACKde4eef52d123b781b833841a9765d1788010ac6b

👍 TheCharlatan approved a pull request: "threading: use correct mutex name in reverse_lock fatal error messages"
(https://github.com/bitcoin/bitcoin/pull/32829#pullrequestreview-2994186848)
ACK de4eef52d123b781b833841a9765d1788010ac6b

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045465990)
> I don't think the test is useful. The change is being tested against a `Sync()` mechanism introduced within the test itself and not the real one, which can change over time (see for example #26966 that changes it completely). It would be better, and simpler for you, to share a patch that reproduces the issue (as @mzumsande mentioned, just adding a sleep() call and triggering the reorg is enough to reproduce it. See #32835 PR description for a patch example).
>
> Other than that, concept ACK
...

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045471393)
> I think that this assert could be hit if a reorg was happening while `Sync()` was going on:
>
> 1. `Sync()` syncs some blocks but isn't finished (doesn't update m_best_block)
> 2. The node undergoes a reorg, so that some of the synced blocks need to be rewinded.
> 3. `Rewind()` is called and the assert fails.
>
> I could trigger this on regtest with an added sleep in `Sync()`, freezing the sync so that I could do the reorg. I think it's going to be hard / impossible to trigger it in a
...

💬 theuni commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3045478804)
Is it necessary to use `-g` here? The output in #32829, for ex, doesn't show a backtrace. So at least in that case the debug symbols are just taking up space.

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045485948)
Hi all, I've written a test with tweaked Sync() to demonstrate this bug: [https://github.com/bitcoin/bitcoin/commit/e9f4ffcd7d8b7211cc049a8ab7632b19c06f7b11](url)

💬 maflcko commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3045502250)
No, `-g` isn't needed for the CI itself. It is only there for developers who happen to run this locally in a container and want to reproduce something in a debugger without having to modify the script and re-compile. Storage is cheap/free, so I thought it can't hurt to have it, but no strong opinion. Anything should be fine for the setting.

🚀 fanquake merged a pull request: "threading: use correct mutex name in reverse_lock fatal error messages"
(https://github.com/bitcoin/bitcoin/pull/32829)

📝 Sjors opened a pull request: "rpc: use anti-fee-sniping by default"
(https://github.com/bitcoin/bitcoin/pull/32892)
If the user does not specify `locktime` for `send`, `sendall` and `walletcreatefundedpsbt`, enable anti-fee-sniping so RPC behavior matches the GUI wallet.

💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2190343122)
Done

👋 fanquake's pull request is ready for review: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32863)

👍 stickies-v approved a pull request: "bench: Avoid tmp files in pwd"
(https://github.com/bitcoin/bitcoin/pull/32890#pullrequestreview-2994321094)
ACK fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12

Was able to reproduce both the pre- and post-fixed situation described, and can't find any other test instances that should be using the testing setup temp dir instead.

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2190390786)
Yes absolutely! I think it all comes down to https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2149379657:
- This PR changes MakeProcessNextHeaders/PopHeadersReadyForAcceptance to use headers as a I/O parameter (09c7d0f703067d455aae4ee458ba7953c29d72fb)
- #32579 changes MakeProcessNextHeaders to use span instead of vector for headers (4c45b5d349c59ed6a795819cdfbaec90bc189a24)

The reasons for switching to headers as I/O parameter are explained in sipa's commit (09c7d0f703067d455aae4e
...

🤔 mzumsande reviewed a pull request: "p2p: Relax BlockRequestAllowed to respond to advertised blocks"
(https://github.com/bitcoin/bitcoin/pull/32869#pullrequestreview-2994324888)
I'll have to think a bit more about the consequences of this - but if we do this change, can we remove the `need_activate_chain` / `ActivateBestChain` logic from `ProcessGetBlockData()`? It doesn't belong in `net_processing` from an architectural POV, and now it shouldn't matter anymore for the request if the block is fully validated / invalid.

Also, just noting that as a side effect we'd now serve blockfilters for invalid / unvalidated blocks (`PrepareBlockFilterRequest`).