💬 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

💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2056768817)
We're continually updating this doc and for now we're adding more and more to the mac section - it's not getting any simpler for now, seems we still need the mac section for now. And when that changes, we'll update the doc.

💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2825349060)
> it would be good to mention [Wiki: AddressSanitizerContainerOverflow (false positives) (google/sanitizers)](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives)

Added the links to the doc and added another workaround of setting the target architecture an Apple Silicon for errors like:
```bash
usr/include/malloc/_malloc_type.h:66:126: error: unknown type name 'size_t'; did you mean 'std::size_t'?
```

🤔 janb84 reviewed a pull request: "guix: Remove unused `file` package"
(https://github.com/bitcoin/bitcoin/pull/32242#pullrequestreview-2788550078)
Code review and Tested ACK [513e202](https://github.com/bitcoin/bitcoin/commit/513e2020a9acdd366d4933780b331f97bac85597)

- Did a code review
- Full GUIX build under nixos on macos via UTM, works !

💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2825373348)
I've pushed an update to check more broadly. The new issues (for just Polish) include some false positives around (change/reszta). Though, the others largely apply as far as I can see:

#### Erroneous translation:
<source>Warning: If you encrypt your wallet and lose your passphrase, you will &lt;b&gt;LOSE ALL OF YOUR BITCOINS&lt;/b&gt;!</source>
<translation>hasłoOstrzeżenie: Jeśli zaszyfrujesz swój portfel i zgubisz hasło - &lt;b&gt;STRACISZ WSZYSTKIE SWOJE BITCONY&lt;/b&gt;!</t
...

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056790435)
Rather than explaining the line below, I added this comment to add context that I think isn't immediately available from the test, namely what the (obscure in both use and name) `listwalletdir` rpc does, I think your suggested comment would repeat what the test is asserting.

🤔 achow101 reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-2788566889)
There should also be a test where multiple things are imported, one with a timestamp, and one with `never`. There should be a rescan in this case.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2056798497)
While this verifies that the address was imported, what we care about here is whether a rescan occurred. It would be better to send to the address for the descriptor being imported, then check to see that after importing, the transaction is not detected.