🤔 BrandonOdiwuor reviewed a pull request: "rpc, rest: Improve getblock error when only header is available"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2183351347)
Concept ACK

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2233661982)
Rebased. Added all the missing newline/diff adjustments. Swapped CMake minimum patch to 3.22. Updated the PR reference in the Windows patch.

✅ fanquake closed an issue: "build: Configuring with `-mno-sse4.1` does not fail the sse4.1 instrinsics check"
(https://github.com/bitcoin/bitcoin/issues/28864)

🚀 fanquake merged a pull request: "Fix SSE4.1-related issues"
(https://github.com/bitcoin/bitcoin/pull/28893)

💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2233674121)
> When/why where these removed in guix? I thought they were pretty fundamental there.

I've added some more cleanup here. We can't remove them entirely (for Darwin) in Guix, because we still need some set during depends, for QT/qmake... However, we can unset them after that, for the main build.

👍 ryanofsky approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2183368508)
Code review ACK 377c4b59d8f43883803b879c334324d916cf2687

💬 ryanofsky commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1681316815)
re: https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1679851292

So you are saying if the values are too big at this point, it means there is a bug elsewhere in the code. If that is the case, I think it would be more appropriate to use `assert` or `Assert` rather than `Assume` so the bug does not go undetected until it is too late and eventually results in an invalid block being produced. I could be wrong, but I don't see a rationale for preferring assume over assert here.

👍 brunoerg approved a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453#pullrequestreview-2183407028)
ACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15

I verified that the normal behaviour is sending a version message only in reply to a received version. In this test, I verified that it sends a version message before, and since it already sent it, when it receives a version message from node, it skips replying to version with own version message (`on_connection_send_msg = None`).

👍 brunoerg approved a pull request: "fuzz: Limit parse_univalue input length"
(https://github.com/bitcoin/bitcoin/pull/30473#pullrequestreview-2183422233)
utACK fa8aadb8f22d322d813f91ea7d8aa7967403a820

Just verified that 10,000 is really more than enough for `UniValue`. I didn't see anything bigger than 200 in practice, so it's ok.

💬 brunoerg commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#discussion_r1681349588)
I don't think so, there is nothing special with them.

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233717971)
Updated e1fa475a5becca0083f59d7cafac9bc67c2f4f1c -> bdd68d5bca483071ca0729e6f0d2d71706ad21e7 ([`pr/mine.5`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.5) -> [`pr/mine.6`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.5..pr/mine.6)) fixing lint error and splitting up "Expand mining interface" commit into two parts.

re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233601603

> Just to have the c
...

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1681358842)
> So you are saying if the values are too big at this point, it means there is a bug elsewhere in the code

Yes

Changed to Assert.

💬 itornaza commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1681361423)
This is great! Despite that these values are hard coded, I also think it is safer to prefer an assert over Assume in this case. In that way we still have a safeguard in place during the whole integration process of Stratum V2 and even if the future run time checks somehow fail to deliver their intended purpose at least we have some feedback in debug mode. On the other hand the assert wont hurt anyone exactly because the values are hard coded.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681373369)
Reverting this, otherwise this will just be blocked by the CI (without other changes that I'm not making here), and matching the CMake version isn't a requirement.

👍 itornaza approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2183473585)
tested ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c

Reviewed all changes since my last https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2167070268. Built the commit and run all unit, functional and extended tests and all of them pass.

🤔 morehouse reviewed a pull request: "fuzz: Version handshake"
(https://github.com/bitcoin/bitcoin/pull/30413#pullrequestreview-2183356740)
Concept ACK

💬 morehouse commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1681380988)
Why increment the time instead of setting it directly?

💬 morehouse commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1681375314)
Would it make sense to limit message type to `VERSION` and `VERACK`? Are other messages valid during the handshake?

💬 morehouse commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1681310085)
How much does lazy init of bloom filters improve fuzz target performance?

👍 ryanofsky approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2183452602)
Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c