💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938953860)
Ok, I think using `MAX_MONEY` as magic value to mean "no threshold" would be nice to avoid but is not a blocker for this PR. Accompanying boolean "has" field is fine too.

🤔 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-2589198945)
I reviewed range-diff, asked a question.

```
git range-diff fefe0636d4ae7c246042276cacd60b22f5fc6bb9...c99e1bc359263d44a85460032c04f7c1f3c688c7
```

All the new changes address the previous review comments, namely: removing duplication in the functional test rpc_psbt, adding Taproot input in the test, fixing comments, and changing `FillPSBT()` argument default.

💬 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_r1938961739)
This is the final hex when I run the test, which is same as the one before modifying the `sighash` type as per the test:
```
0200000000010208bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960000000000fdffffff08bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960100000000fdffffff0280f0fa0200000000160014f57919cd765629cd0d2ba39021a146ee065d697508c2f00800000000160014c7109263e9f5db5d2162ce40a1f898f6ac3933ad02473044022067caa211634d7f25bcac48704165ca7d257ff80a7d0b764825877c7a4
...

💬 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_r1938961976)
`provided`

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938960467)
nit:
```suggestion
// Helper to check if the tip has changed, and also update tip_changed.
```

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938970989)
`wait_until()` will first call the predicate, so `check_tip_changed() || ` is not needed. The above code is equivalent to:

```cpp
notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
return check_tip_changed() || chainman().m_interrupt;
});
```

✅ hebasto closed a pull request: "MacOS updated Bitcoin-Core gui icon in accordance with Apple design docs"
(https://github.com/bitcoin/bitcoin/pull/31780)

💬 hebasto commented on pull request "MacOS updated Bitcoin-Core gui icon in accordance with Apple design docs":
(https://github.com/bitcoin/bitcoin/pull/31780#issuecomment-2630308019)
GUI-related proposal should be opened in https://github.com/bitcoin-core/gui.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939003712)
Ah oops, we're going in circles, I forgot about this as well. Will add a comment.

✅ hebasto closed an issue: "Organize the address types on the "Receiving addresses" tab"
(https://github.com/bitcoin-core/gui/issues/646)

💬 hebasto commented on issue "Organize the address types on the "Receiving addresses" tab":
(https://github.com/bitcoin-core/gui/issues/646#issuecomment-2630333317)
Given the developments in https://github.com/bitcoin-core/gui-qml, it doesn’t seem worthwhile to add a new feature solely based on the rationale that "it would be nicer".

Feel free to reopen if anyone disagrees.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939023678)
@ryanofsky pointed out the same mistake in a different (now closed) PR: https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848409060

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939026787)
Actually, I forgot that `wait_until` already checks the predicate once before waiting. See https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938970989

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2630362917)
Dropped unnecessary `check_tip_changed()` check before `wait_until`..

💬 Sjors 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_r1939034855)
> at the same time

You mean a transaction with both a taproot and non-taproot input?

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939042762)
It's easier to read in doxygen html, and can always be expanded.

💬 hebasto commented on pull request "depends: Fix compiling `libevent` package on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/31500#issuecomment-2630389701)
> > Can you link to the upstream issue / PR.
>
> > LGTM, but yeah, it'd be helpful to upstream this.
>
> Sure thing!
>
> Here is an upstream PR: [libevent/libevent#1768](https://github.com/libevent/libevent/pull/1768).

The upstream changes have been merged. So this branch was rebased, and the upstream commit was mentioned.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1939050902)
(Resolved by now merged #31433).

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939056704)
> then there's no wait to test its internals

Should that be "no way"?

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939058572)
IIUC the difference between release and close is only relevant in edge cases. The main purpose of this function it to close them. I would keep the name, but maybe document the function to point out the edge case.