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

💬 s373nZ commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2656143427)
ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954234113)
I renamed `num_taproot` and `num_nontaproot` to `num_schnorr` and `num_ecdsa` respectively in https://github.com/bitcoin/bitcoin/pull/31689/commits/4b37efee90ee339b7bde7777b24a5268501cee9d

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954234524)
Done in https://github.com/bitcoin/bitcoin/pull/31689/commits/4b37efee90ee339b7bde7777b24a5268501cee9d

🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2614507411)
ACK 9911ba82be9ec2243b86bb149d6ec8509d9717d7

I reviewed the range diff this time, my earlier query is answered.

```
git range-diff c99e1bc359263d44a85460032c04f7c1f3c688c7...9911ba82be9ec2243b86bb149d6ec8509d9717d7
```

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1954211603)
Thanks, I get it now. I asked because I noticed `SignPSBTInput` being called but I see now the `DUMMY_SIGNING_PROVIDER` argument passed to it as well.
I see the reason why the test is contrived such that it requires manually updating the sighash and calling `finalisepsbt` again.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954237730)
Done in https://github.com/bitcoin/bitcoin/pull/31689/commits/4b37efee90ee339b7bde7777b24a5268501cee9d

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954238194)
Done in https://github.com/bitcoin/bitcoin/pull/31689/commits/4b37efee90ee339b7bde7777b24a5268501cee9d

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1954239176)
Adjusted the code to look more like this in https://github.com/bitcoin/bitcoin/pull/31689/commits/4b37efee90ee339b7bde7777b24a5268501cee9d