💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677599944)
Why do you add this function around here instead of using `m_chainstates` as well?

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677573399)
nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.

👍 hebasto approved a pull request: "OptionsDialog: Prefer to stretch actual options area rather than waste space"
(https://github.com/bitcoin-core/gui/pull/827#pullrequestreview-2177434930)
ACK b71bfd9eef8b0017cef3c05361c02ddbd837e6ac

💬 stickies-v commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1677661313)
Thanks, main concerns are addressed in force push. I'm fine with the current code, but is there a problem I'm not seeing with having `max_subs` be a `size_t`, too? No need to retouch, I'm just curious.

👍 stickies-v approved a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2177436401)
Light ACK bc34bc288824978ef4b98e8802b47cb863c8a8c2

Code LGTM and approach seems reasonable, but there may be fuzzing and miniscript nuances I'm not considering.

🚀 hebasto merged a pull request: "OptionsDialog: Prefer to stretch actual options area rather than waste space"
(https://github.com/bitcoin-core/gui/pull/827)

💬 hebasto commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2228264130)
Concept ACK.

💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r1677672363)
yepp, fixed, thanks!

💬 hebasto commented on pull request "wallet: Improve error log color in the console":
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2228299766)
cc @GBKS for a designer's opinion.

> I still think the softer color is better.

I'm not a designer, but I find the current implementation the most appealing.

As for implementation, a named constant should be used in the code.

💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2228301130)
Removed the accidentally included `src/common/sv2_messages.cpp`.

💬 maflcko commented on pull request "wallet: Add scan_utxo option to getbalances RPC":
(https://github.com/bitcoin/bitcoin/pull/28930#issuecomment-2228318061)
Are you still working on this? Seems stale for more than half a year, at least.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2228365527)
Also added `getCoinbaseTx()` and `getWitnessCommitmentIndex()` which are needed for the `NewTemplate` message.

Since this PR now contains Stratum v2 specific stuff anyway, I pulled in #30441. Still in the process of incorporating all this into #29432 to make sure I didn't miss anything.

💬 Sjors commented on pull request "[WIP] Add getCoinbaseMerklePath() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30441#issuecomment-2228366141)
Folded into #30440.

✅ Sjors closed a pull request: "[WIP] Add getCoinbaseMerklePath() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30441)

💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1677742992)
I just went for changing only that which was required. I think of `int` as "default" and chose to only deviate from the default when needed.

💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677771441)
Done.

💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677772287)
> Ok, I see, the peer.wait_for_disconnect() immediately returns, as there is no connection yet, and doesn't do anything.


I meant it as a check so that we know if `self._transport` got reset to `None` at some point of time. (`self._transport` is set in `connection_made()` when the connection is opened irrespective of whether p2p connection remains connected or disconnected)

> Why is it required to set a timeout here at all? The log message should happen before the disconnect, which is wai
...

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677780013)
Nit: You can use the existing `bumpmocktime` helper?

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677787182)
Can you explain what this means? `sendSet` is a boolean, so what does "populated" mean? Also, when removing `peer2` the test works fine, no? What would the "wrong" error message be?

🤔 marcofleon reviewed a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2177671042)
Code review ACK bc34bc288824978ef4b98e8802b47cb863c8a8c2. The added comments are useful, thanks for those. Tested on the three inputs in https://github.com/bitcoin/bitcoin/issues/28812 that caused the timeouts.