💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2056177529)
I had that one coming. :)

💬 furszy commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824481962)
> I think you can easily solve this by running a small JSON-RPC proxy that listens on the usual RPC port, injects a estimate_mode argument if none is provided and forwards that to the bitcoind instance? Then your clients won't need to make any changes for as long as you're willing to maintain backwards compatibility.

Or.. while the clients adapt, simpler to just downgrade or compile the repository yourself.

🤔 rkrux reviewed a pull request: "test: Test that migration automatically repairs corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2787605306)
LGTM to me at bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633, ACK - it's just one functional test now.

Couple of observations/questions after rotating the seed and unloading the wallet that I found noteworthy enough to share here.

1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.

2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was diff
...

💬 maflcko commented on issue "cmake: GUI build broken with `-stdlib=libc++`":
(https://github.com/bitcoin/bitcoin/issues/32331#issuecomment-2824604630)
IIRC it used to work with qt5, but it is unclear if that was intentional or by accident. I guess in any case this is an upstream issue that would need to be fixed in the distro or in qt6, if it is an issue at all?

💬 fanquake commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824608069)
Agree with the other comments here. Closing for now.

✅ fanquake closed a pull request: "Add rpcestimateconservativefees"
(https://github.com/bitcoin/bitcoin/pull/32329)

💬 juanmigdr commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824637123)
You're right, @ismaelsadeeq, @glozow, @stickies-v, @furszy, and @fanquake. Thanks for the feedback and suggestions. I see that adding this config option isn't the best solution, as this change is quite specific to my situation, and the cost of maintaining it outweighs the benefits. I'll consider alternative ways to handle this. Appreciate the help!

💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824676516)
@whitslack @eli-schwartz @thesamesam

Setting the required conditions aside, can you confirm whether the following patch resolves the issue on Gentoo:

```diff
--- a/src/qt/CMakeLists.txt
+++ b/src/qt/CMakeLists.txt
@@ -250,6 +250,8 @@ if(qt_lib_type STREQUAL "STATIC_LIBRARY")
)
endif()

+list(TRANSFORM CMAKE_CXX_LINK_OPTIONS_PIE REPLACE "${CMAKE_CXX_COMPILE_OPTIONS_PIE}" "${CMAKE_CXX_COMPILE_OPTIONS_PIC}")
+
add_executable(bitcoin-qt
main.cpp
../init/bitcoin-qt.cpp
```
?

💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824841846)
> Interesting! It should happen since https://gitlab.archlinux.org/archlinux/packaging/packages/qt6-base/-/commit/5cd71aed56de955b182e20263a50cd91bf7b6aaa.

This approach appears suitable for Gentoo's Qt packages as well, since, according to [this](https://github.com/qt/qtbase/commit/19b7f854a274812d9c95fc7aaf134a12530c105f) commit message, the `-Bsymbolic` and `-Bsymbolic-functions` functionality—underlying the `-reduce-relocations` option—is broken.

💬 w0xlt commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2056499667)
`FindNode()` is not bad, but I personally like `ConnectedToAddrPort()`. This name makes the purpose of the function very clear.
The parameter type (`CService` and `str`) makes the difference clear. It is not necessary for the function name to describe the parameter.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2824999837)
re-ACK 1c16944a4aff71e6560703916b11b2a544ea71ca

Diff from 2b34857ad54aebbf0a9271e742c2caee85577bc8 is trivial.

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056598543)
Makes sense, will do next time I push

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2056629920)
Done. Removed the `height > 0` condition, corrected `height - 1` for `height` and added a comment that the `height` variable does not actually correspond to the block height as it mind be confusing to readers. Verified correctness by rebasing my Consensus Cleanup PR on top of this one.

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2056631476)
Rebased on master.

💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2825129551)
> 1. The `listreceivedbyaddress` RPC doesn't show any entry for `addr` to which the 1 BTC was sent, but `listunspent` RPC does show.

This is expected since we didn't get `addr` through normal means, and it therefore is missing expected items for receiving addresses like an address book entry.

> 2. The `desc` field in the `bad_deriv_wallet.listunspent` and `bad_deriv_wallet_master.listunspent` calls for the same `addr` was different slightly as I see `'` was used in the former & `h` in the
...

💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056630902)
AFAICT the workaround is correct, and this test is already using `importdescriptors` on master.

This test succeeds as-is on this branch with this commit (https://github.com/bitcoin/bitcoin/pull/31250/commits/e6c900f9c0e1b143098663e1c5c80e1811946b2d) dropped.

💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056635528)
Nit: Extra "the".

💬 davidgumberg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2054565214)
For other reviewers: Unlike for legacy wallets, watch only[^1] descriptor wallets do not need/have their ScriptPubKeyMans set up during wallet [creation](https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3003).

https://github.com/bitcoin/bitcoin/blob/e5a00b24972461f7a181bc184dd461cedcce6161/src/wallet/wallet.cpp#L3073-L3080

---

Tangentially, it might be nice to refactor `SetupDescriptorsWallet()` to reuse `CWallet::Create()` rather
...

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056733702)
I certain that this commit was required when writing this PR originally, however, I no longer recall the error that it fixed. Dropping this commit.

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734208)
I think the `%` is your command line prompt?

This was missing a newline, fixed.

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2056734975)
Fixed