💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681210901)
The missed EOL causes "Hunk #1 succeeded at 7 with fuzz 1."

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681238832)
Unless this patch is submitted upstream, why not use the same version as our CMake-based build system?

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681232558)
Cannot find b163fc36a48ecdc87a3ecb4c6bba5f6b8965acaf in https://github.com/zeromq/libzmq. Did you mean https://github.com/zeromq/libzmq/commit/aa885c5a154256612108636b0fb22f44ae0e247a?

Perhaps we could refer to a PR number, as is done in other patches?

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681251382)
```suggestion
@@ -564,13 +564,6 @@ else()
```

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681252005)
```suggestion
@@ -588,9 +581,7 @@ if(WIN32 AND NOT CYGWIN)
```

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233601603)
> Hmm, what specifically would you like me to change here?

Just to have the changes in `src/ipc/capnp` be their own commit. I need those in https://github.com/Sjors/bitcoin/pull/48, while at the same I need to avoid including the rest of 4e1a4342f3b2a857e3211587cd14e36704197483.

> The purpose of createNewBlock2 is just to make sure serialization works when createNewBlock is changed to return BlockTemplate.

In that case I'll just add a commit to https://github.com/Sjors/bitcoin/pull/48
...

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681266250)
The current condition (for using DFS) is:

```c++
while (queue.size() - 1 + queue.front().und.Count() > queue.capacity()) {
```

In the original version of this PR it was:

```c++
const auto queuesize_for_front = queue.capacity() - queue.front().und.Count();
...
while (queue.size() > queuesize_for_front) {
```

There is an offset 1 difference between the two; it didn't take into account that choosing BFS would first delete an entry from the queue before adding new ones.

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681267687)
yes this was very insightful, thank you

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1681269853)
Responded in https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233601603

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233625152)
Updated 4e1a4342f3b2a857e3211587cd14e36704197483 -> e1ae38f4bbcdb1b3fa5e1eab1abfa9cf2232798c ([`pr/mine.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.3) -> [`pr/mine.4`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.3..pr/mine.4)) adding several unit tests, and also starting to split up the "Add bitcoin-mine test program" commit by moving CTransaction serialization support to a new commit.

👍 theuni approved a pull request: "Fix SSE4.1-related issues"
(https://github.com/bitcoin/bitcoin/pull/28893#pullrequestreview-2183328324)
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233652640)
Updated e1ae38f4bbcdb1b3fa5e1eab1abfa9cf2232798c -> e1fa475a5becca0083f59d7cafac9bc67c2f4f1c ([`pr/mine.4`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.4) -> [`pr/mine.5`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.4..pr/mine.5)) adding support for -loglevel and other logging parameters as suggested https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233469862

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681303764)
I doubt upstream is going to take any patches. We can just set it to 3.22.

🤔 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.