💬 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

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2656192834)
`7b63b4ca1c...c4ab7f82d6`: rebase due to conflicts and address a suggestion

📝 vasild opened a pull request: "net: reduce CAddress usage to CService or CNetAddr"
(https://github.com/bitcoin/bitcoin/pull/31854)
Using `CAddress` when only `CService` or `CNetAddr` is needed is excessive and confusing. Fix those occurrences to use the class they need:

* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`, thus change its argument.

* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a dummy `CAddress` from `CService`, so use `CService` instead.

* `GetBindAddress()` only needs to return `CService`.

* `CNode::addrBind` only needs to be `CService`.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2656212656)
Rebased https://github.com/bitcoin/bitcoin/commit/1c6b886465df0f00549e7d10c3bfefd27be7f1c2 to https://github.com/bitcoin/bitcoin/pull/31689/commits/bd27a83efcc3d678f33041ee34eeb019777a2405

I improved some variable and function names based on received feedback, and also made style changes to align with `doc/developer-notes.md`.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2656216742)
Extracted the first commit into https://github.com/bitcoin/bitcoin/pull/31854. It is not strictly related to this PR and makes sense on its own. If merged will reduce the size of this PR.

📝 tianzedavid opened a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31855)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 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_r1954288305)
I'm introducing a new `RPC_SHUTDOWN_ERROR` which makes sense here. I don't think the message itself matters, since the user knows they shut down the node - and the exact details of where and why the RPC call fails aren't important.

💬 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_r1954295417)
Indeed, but on the other hand code changes over time, stuff appears between the if statement and the usage of `tip`, then someone changes the if statement, and now we have a crash...

💬 vasild 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_r1954297095)
Yes, this scenario I had in mind.

> in that case to ignore the timeout and instead keep waiting for the node to set a tip

Hmm. What if the tip is not being connected for whatever reason? Wait forever? The current behavior of respecting the timeout looks safer. Would be an odd user experience to provide a timeout and the program to decide to wait longer for whatever reason.

💬 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_r1954298757)
It seems the compiler doesn't know about this default inside the implementation file.

💬 vasild 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_r1954300543)
Ok

💬 willcl-ark commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656266441)
Thanks for the contribution, but this should probably be closed.

In this project we prefer typos to be fixed organically as that area of the codebase is worked on to avoid diluting precious developer review time. See our https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy

💬 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#issuecomment-2656278649)
I changed `waitTipChanged()` to only return `nullopt` during shutdown. This is achieved by extending the `timeout` in the unlikely event that node initialization takes longer.

I introduced a new `RPC_SHUTDOWN_ERROR` code and use it in case `waitfornewblock` and friends are called really early and the node shuts down (probably very hard to reach in practice). See https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952373326

On question about long polling I still need to investigate:
...