🤔 paplorinc reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2182966578)
Started reviewing, but got a bit stuck at the first commit, will continue a bit later

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681189065)
> we do not change behavior by exiting early before attempting to sign if there is a failure with the merkle_root logic.

Previously, when an error occurred, subsequent calculations weren't performed.
Since `&=` doesn't short-circuit (and even if it did, `pubkey_bytes` would be allocated and `ComputeTapTweakHash` would run even if `secp256k1_keypair_xonly_pub` failed), the methods could be called even though a previous dependency failed.
Are you sure this is just a refactoring and not a beh
...

📝 Sjors opened a pull request: "Stratum v2 Template Provider common functionality"
(https://github.com/bitcoin/bitcoin/pull/30475)
Based on #30332. Parent PR #29432.

This contains all Template Provider functionality that can be used by both #29432 and the IPC based alternative in [link to PR].

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2233560101)
I split off the Template Provider class into #30475. The non-base commits in this PR start the TP from `init.cpp` and add some other things that are useful for testing (e.g. testnet4).

A WIP for the IPC sidecar approach can be found [insert PR]. It's also based on #30475.

🚀 fanquake merged a pull request: "test: bump mocktime only after node has received and sent bytes"
(https://github.com/bitcoin/bitcoin/pull/30468)

💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2233580171)
@hebasto, I couldn't reproduce the issue described in https://github.com/bitcoinknots/bitcoin/issues/83, trying both `master` and this PR, also checked the [fix in knots](https://github.com/bitcoinknots/bitcoin/releases/tag/v27.1.knots20240621) which is similar to this PR code change. It would be good to know what was the state of the "mask value" during the use case described in https://github.com/bitcoinknots/bitcoin/issues/83, the only thing I can think of it's that the issue is the same one
...

🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2183191027)
Approach ACK f2712ed278ec44ad100bdbe765de1716191f238b.

I've reviewed all patches for now.

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

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

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681214480)
```suggestion
@@ -599,7 +599,7 @@ if(NOT MSVC)
```

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681211106)
nit: missed EOL.

💬 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