💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2657465365)
Alright, we're back to two commits. Let's see how CI fares.

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2657467856)
Rebased 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b -> 9194e461518e6b843c1a707ba1b10ab7a000e096 ([`pr/mine.15`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.15) -> [`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.15-rebase..pr/mine.16)) due to conflict with #31834. Also added status line for the executable as suggested

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2657469079)
Rebased 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b -> 9194e461518e6b843c1a707ba1b10ab7a000e096 ([`pr/mine.15`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.15) -> [`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.15-rebase..pr/mine.16)) due to conflict with #31834. Also added status line for the executable as suggested

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955067561)
Good point, `getTip()` doesn't wait.

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2615975522)
Code review ACK 2d65225c4696dcc8312d546eb67f4eae11b9b8fe, just updating fee_threshold comment since last review

🤔 theuni reviewed a pull request: "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop"
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2615930348)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. Untested. A few (non-blocking) notes, assuming this works as-is.

1. It's unclear to me under what scenario the call might fail. OpenSSL does the same loop, I assume that's why it's done here? I guess it's pretty unlikely that it passes the <10 failures check at startup, then fails lots of times during execution.
2. The code duplication is unfortunate. I would've preferred to see a single factored-out function that takes a reference to
...

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955041233)
In case it helps other reviewers... no need for static for these because of the anon namespace.

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955045279)
I think `ok` and `test` should be initialized to sane values because some sanitizers (and reviewers :p) aren't clever enough to know if they're guaranteed to be set by the asm.

edit: I guess the same goes for the other copy of this.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1955078544)
re: https://github.com/bitcoin/bitcoin/pull/31283/commits/dcb75cbae61918a13310e3e22190c088d9b7605c#r1937644528

> I'd really like this to be easy to use for a Rust client as early as possible. It seems better to revert to the magic value for now.

To make this easy for rust (or other) cilents using this interface you can assign these fields default values. Maybe define MAX_MONEY and MAX_TIMEOUT constants using syntax shown https://capnproto.org/language.html#constants and assign them as defa
...

💬 laanwj commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2657497175)
Sure. It was good to have this test at the time to prove the issue was fixed. But it's been a decade, it's super unlikely that this specific toolchain ABI compatibility issue will ever pop up again in our builds, we can't keep the tests for every possible eventuality.

Generally we should have tests that test our code, we can't assure the sanity of the entire OS and system libraries.

💬 TheCharlatan commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955087765)
This comment seems to be lacking context now.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955088750)
Makes sense thanks.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955089683)
nit: There is still one remaining in line 180.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090027)
Yeah something like that that

In most of the fuzz test in `src/test/fuzz/cluster_linearize.cpp` you provided a really nice description , which is just an overview of what the test is going to do before jumping to the instructions.

Here we just jump into the instructions without that context.

But this is nitty, you can ignore this comment and resolve.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090136)
Yeah I was thinking something like

<details>
<summary>diff</summary>

```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index afaa46e6326..a9b3e446d26 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -1143,6 +1143,7 @@ void Cluster::SanityCheck(const TxGraphImpl& graph) const

// Verify m_linearization.
SetType m_done;
+ SetType last_chunk;
assert(m_depgraph.IsAcyclic());
for (auto lin_pos : m_linearization) {
assert(lin_pos < m_mappin
...

💬 theuni commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2657542305)
Concept ACK. Regarding the duplicate `--target`... this kind of thing has come up a few times now where we need to add something to cc/flags for depends but we don't want it exported in our toolchain. I have a branch where I've done something similar.

Additionally, for Xcode generators (and CMake 4.0), I think we'll want to handle the macOS sdk/include paths differently between depends and the toolchain file, since CMake has specific variables that it really wants set for handling those thing
...

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023)
I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.

💬 theuni commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657580903)
Hmm, I now see that @maflcko is still using the gcc coverage, so nuking it would break https://maflcko.github.io/b-c-cov/ .

@maflcko since you weren't on today's v29 milestone call: the consensus was to nuke the current gcc coverage support for v29 since it's "not working", and try to get llvm support in in its place, but that part wouldn't be a v29 release blocker.

But apparently it's working enough for you so I suspect you'd disagree with that?

👍 theuni approved a pull request: "depends: avoid an unset `CMAKE_OBJDUMP`"
(https://github.com/bitcoin/bitcoin/pull/31857#pullrequestreview-2616087124)
utACK 2434aeab62ba07c5380112838f3600b3dbbceef2

💬 jonasschnelli commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657584112)
I haven't looked at this closely, but it could be that the implementation needs an update.
The important point is *fingerprinting resistance*.

During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.

See the BIP:

> Counter-measures for peer fingerprinting
>
> Peers may have different prune depths (depending on the peers configuration, disk space, etc.) which can result in a fingerprinting wea
...