💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2054053341)
This was the result of an unfortunate battle with the linter. Seems to not even have worked on older versions:

https://stackoverflow.com/questions/51775950/why-isnt-it-possible-to-use-backslashes-inside-the-braces-of-f-strings-how-can

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2054051131)
Took part of the simplification.

💬 hebasto commented on pull request "depends: Fix cross-compiling on macOS":
(https://github.com/bitcoin/bitcoin/pull/32215#issuecomment-2821251222)
> I'm trying to test this PR, but having trouble verifying it. Am I correct to conclude that the fix is really mac only ( not nix-shell on macos? or GUIX cross-compile ? )
>
> Can confirm that the `mv -t` option is not supported on MacOS .

This PR is specific to cases like https://github.com/bitcoin/bitcoin/issues/32208.

👍 hebasto approved a pull request: "ci: switch to LLVM 20 in tidy job"
(https://github.com/bitcoin/bitcoin/pull/32226#pullrequestreview-2784021261)
ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6.

This PR effectively bumps two tools: `clang-tidy` and `include-what-you-use`. Release notes for the latter are available [here](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.24).

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054067055)
These are equivalent. In both cases the coinbase transaction will still be valid, just not timelocked to the block. I think it's marginally better to not make it modulo `LOCKTIME_THRESHOLD` for simplicity and to avoid rewinding back to values already used. But really, i think anything past year 2106 shouldn't matter. :)

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2821309098)
Although `test_direct_file` doesn't currently fail, adding a check for the wallet's name in the backup path does make it fail, e.g.

That's a good point. Instead of backing up `plainfile` as `plainfile_123.legacy.bak` 47174476c04a7f1138fbd32eb0c9c38e85946393 backs it up as `wallets_123.legacy.bak` because it assumes the name of the parent directory is the name of the wallet. Following would seem to be a good fix:

```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4499,13
...

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2054107249)
> This is needed to be able to read without obfuscation - as the comment states.

Why does it need to be set to zero again when it's been set in the initializer list of this same ctor?

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054112074)
It feels out of place in mining_basic, and the PR that introduces validation checks for nSequence/nLockTime in coinbase does check various combinations in feature_block, including this one. So i think it's better left there?

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

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

💬 fanquake commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821359451)
I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?

💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821394743)
> I remember us already having code to work around something similar to this. i.e: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/build-aux/m4/bitcoin_qt.m4#L182? Looks like it was just dropped, rather than ported to CMake?

Was related to LTO?

💬 thesamesam commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2821405387)
It's the same problem. It just shows up worse with LTO because Qt's sanity check can't fire to tell you and give you a build failure instead.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2821410082)
Changed the serialization of `Obfuscation` from `vector` -> `array` without re-testing my suggestions towards the end. Forgot that one serializes the size and the other does not. Seems to be responsible for some of the test failures on my suggestion-branch.

💬 maflcko commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821488921)
Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.

💬 hebasto commented on pull request "test: Treat executable paths in tests consistently":
(https://github.com/bitcoin/bitcoin/pull/32324#issuecomment-2821506859)
> Seems fine. An alternative could be to remove `BUILDDIR` and replace it with `BINDIR`, now that all bins are in one dir? `BINDIR` could include the multi-config subdir path element, if it exists.

The question is how to choose the configuration for testing.

📝 maflcko opened a pull request: "ci: Add missing -Wno-error=array-bounds to valgrind fuzz"
(https://github.com/bitcoin/bitcoin/pull/32325)
Due to an upstream GCC issue, any debug/fuzz build which aborts on failed assumes will print a false positive array-bounds warning in `src/test/fuzz/txgraph.cpp`.

This also affects one CI task.

Fix the CI task by ignoring the error for now.

Fixes https://github.com/bitcoin/bitcoin/issues/32276

🤔 furszy reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2784336700)
utACK cad39f86fb5a81

> Sounds like this should be an actual test then instead of a benchmark, no?

I'd keep it as is for now and drive all our focus towards #31250. It would be nice to cut down the tree first - we can pick up any leftover later.

💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2821541818)
I've written a tool to check translations: https://github.com/maflcko/DrahtBot/tree/main/check_translations

This finds the above issues, as well as one more:

```
$ cargo run -- --llm-api-key hi --translation-file /path/to/src/qt/locale/bitcoin_pl.ts --cache-dir cache


Erroneous translation:

<source>Create Wallet</source>
<translation type="unfinished">Stwórz potrfel</translation>

Erroneous translation:

<source>%1 (%2 blocks)</source>
<translation type="
...

📝 vasild opened a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.

Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...