🤔 rkrux reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2611618216)
reACK 78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421

I reviewed the range diff.
```
git range-diff e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc...78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421
```

Newer changes are related to using the constant `LAST_KEYPOOL_INDEX` as suggested and use `migratewallet` instead of `restorewallet` due to which the format of `good_deriv_path` is changed from `'` to 'h'. Using `migratewallet` is fine because it also ends up calling `LoadLegacyWalletRecords` internally where
...

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1954042039)
This was using 12 earlier, now 11. Fine since the test passes.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1952466689)
I had in mind tying this 10 to the constant as well. Consider if you end up retouching, otherwise it's fine.

💬 maflcko commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954103791)
> * I don't understand why "-342 Service unavailable, RPC server started but is shutting down due to error" comment is being changed to "-342 Service unavailable, could be starting up or shutting down". These descriptions seem inconsistent and I don't know which one is correct since I don't see where -342 is being returned

I'd guess that it is coming from authproxy and is pretty general:

```
$ git grep '\-342\>' ./test/
test/functional/feature_taproot.py:# Test Taproot softfork (BIPs 340
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2655986646)
`964b7b0ad6...ebc7ba0216`: rebase due to conflicts

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#discussion_r1954130643)
Right, the logic here has been swapped since this comment was written 😄

i think that sentence is redundant. That the length is checked is implied, why pass it otherwise? The line below already describes which length is used when it's not passed.

✅ fanquake closed an issue: "cmake -P $buildDir/Coverage.cmake: Test execution for coverage fails. Wrapper resource "cov_tool_wrapper.sh.in" missing from build cache folder."
(https://github.com/bitcoin/bitcoin/issues/31638)

🚀 fanquake merged a pull request: "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree"
(https://github.com/bitcoin/bitcoin/pull/31722)

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656047171)
Thanks for testing!

DrahtBot shouldn't have re-requested review, i first need to address sipa's comment in the first place.

💬 maflcko commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656074342)
> DrahtBot shouldn't have re-requested review, i first need to address sipa's comment in the first place.

For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can't come up with something better, and it seems fine as long any other reviewer notices.

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2656074472)
Uploading the macOS binaries here, so I can test the download flow (which in the past behaved different from the scp flow):

- [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
- [bitcoin-096525e92cc2-x86_64-apple-darwin.zip](https://github.com/user-attachments/files/18781669/bitcoin-096525e92cc2-x86_64-apple-darwin.zip)

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2656093804)
> For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can't come up with something better, and it seems fine as long any other reviewer notices.

Yes the heuristic's usually correct, don't know anything better. Was just noting it, as github has no way to un-request review.

💬 willcl-ark commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656114029)
Also looks good to me!

A docker build (with `--no-cache` set, so only using a cached Ubuntu base image) on my 11700k went from:

```bash
[+] Building 1521.2s (9/9) FINISHED
```

to

```bash
[+] Building 196.8s (9/9) FINISHED
```

Should be a nice win on CI runs (and for users) where a rebuild would have previously been triggered (which can be caused by almost anything).

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1954219601)
Added `Assume(IsI2P(conn.peer))` and `Assume(IsI2P(conn.me))` in `ThreadI2PAccept()`.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954220947)
I think `@retval std::nullopt if the node is shut down` covers it? Whether shutdown happens before the timeout or exactly at the timeout doesn't matter for the caller.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954226223)
Ah, I think I see the problem. There's a potential scenario where node startup is unusually slow, longer than timeout.

I think it's better in that case to ignore the timeout.

🤔 hodlinator reviewed a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2613365399)
Thanks for bearing with me @ryanofsky. :)

Updated PR description based on feedback and addressed inline comments.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954047202)
- Saw the code still had an inner `if self.nodes`-check - removed in new push.
- Slightly clarified commit message, describing the change (even though it adds some negation) rather than describing then end state of the change.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953499371)
> I think "initialization phase" comment that was removed provided more context and was more understandable than the new "Suppress these in favor of" comment.

As the commit message specifies, we are changing `wait_for_rpc_connection()`, so the only value the "Initialization phase" comment had came from adding some context to a ~80 line long function which is part of initialization IMO. (The mention of `FailedToStartError` also gives a hint at what phase we are in).

> I don't see why it is
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954056589)
Thanks! This made me actually realize I could add a test for it and improve the exception message (+ commit message).