💬 janb84 commented on pull request "depends: Fix cross-compiling on macOS":
(https://github.com/bitcoin/bitcoin/pull/32215#issuecomment-2821181976)
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 .

💬 rkrux commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2053991401)
Nit:
```diff
- # Retrieve all records that have the keymeta prefix
+ # Retrieve all records that have the "keymeta" prefix, rest of the key data is the pubkey that differs for each record
```

🤔 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-2783901872)
Nice that the migration flow can fix it automatically and no new custom handling is required now while loading the legacy wallets!

💬 rkrux commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2053994529)
Isn't [076b65796d6574, 076b65796d6575) a little too wide of a search range?

"keymeta" in hex is "6b65796d657461", prepending it with the compact size length "07" should make the key prefix "076b65796d657461" in the serialised form imo, making the range as [076b65796d657461, 076b65796d657462)

💬 rkrux commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2053996701)
```diff
- # Make sure that this is being detected and automatically cleaned up
+ # Make sure that this it is being automatically cleaned up during migration
```

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2054042572)
This is needed to be able to read without obfuscation - as the comment states. Can you suggest how to reword that to make it obvious?

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2054043508)
I'm moving code, I never know what that implies

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054047295)
Of course. My point is that it does not change anything for a miner if its coinbase transaction has `nLockTime` 0 or height-1.

💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-2821242063)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/32324, which addresses https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2026642378, and drafted.

📝 hebasto converted_to_draft a pull request: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233)
This PR:

1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.

2. Disables tests instead of ignoring them.

For example:
- building without Python available:
```
$ cmake -B build
$ cmake --build build -j 16
$ # ctest --test-dir build -j 16 -R "^util_"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 113: util_tests
Start 114: util_threadnames_tests

...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2054050094)
Adjusted in similar direction.

💬 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.