💬 maflcko commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2657052286)
(Looks like there are 4 conflicts, so best to wait until there are 0. Otherwise there will be useless churn)

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1954788703)
ah hm, maybe we don't need to wait? I guess if you send a ping, you don't get a pong until all 24 are processed.

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954800384)
Up to this point you have not applied dependencies, so the new transaction is being considered its own singleton cluster, which is in-congruent with the actual goal. If you apply dependencies via CheckMemPoolPolicyLimits, then the resulting cluster is porentially oversized and violates the requirements set forth in the TxGraph interface.

see:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index 61576851cd..affababd10 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2657080863)
Can you update the PR description to say that this is based on #31854? The convention then is to mark this PR draft until that one is merged.

Additionally, it's 25% easier to review #31854 if you rebase this PR on the exact same commit. Since then I can just range-diff it with what I already reviewed here.

💬 furszy commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657093782)
q: isn't the test just wrong?. `min_viable_change` is initialized to the future input fee without increasing the window by one as we do in the actual code. Thats why there is an overlap here. See the wallet code:
https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/wallet/spend.cpp#L1131-L1132

While you are manually setting it to:
```
coin_params.min_viable_change = coin_params.m_discard_feerate.GetFee(coin_params.change_spend_size);
```

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954822133)
I think the mistake here is that in my call to `CountDistinctClusters()`, I should be setting `main_only` to true, and it looks like I omitted that so it'll call it on the staging graph instead. The same issue may apply with the invocation of `GetDescendants()`; the intent is to just call it on the main graph.

Would that resolve the concern you're bringing up, or is there something else I'm overlooking?

💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2657122152)
Thanks for adding that, I think it makes the script more complete. The only thing missing is the `runs=1` arg in `run_single` for when the entry is the full corpus dir. Right now it just runs continuously. But other than that, lgtm.

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1954834091)
> I should be setting main_only to true,

I think that works? Will think more on it.

> GetDescendants

Missed that one!

💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2657136142)
@tdb3 @danielabrozzoni can you give this another look? The changes should be trivial

💬 ryanofsky 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_r1954836649)
In commit "Have createNewBlock() wait for a tip" (854766b886b6c2ee564b867db64bdbce80064b3b)

Another alternative but probably not better is:

```diff
- if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
+ if (!static_cast<Mining*>(this)->waitTipChanged(uint256::ZERO)) return {};
```

I think in longer term after we add an initial waitNext() method and make improvements to it, it will be natural to move more functionality out of Mining class methods i
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2657139469)
(there seems to be a delay in Github processing my latest push)

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2657139983)
Ah, I didn't test with libFuzzer. I guess to support that I'll have to port https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/test/fuzz/test_runner.py#L170

I'll do that tomorrow.

✅ fanquake closed a pull request: "cmake: Install man pages for configured targets only"
(https://github.com/bitcoin/bitcoin/pull/31765)

💬 fanquake commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2657151761)
Closing given the concept ACK in the other thread.

💬 fanquake commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2657172457)
https://github.com/bitcoin/bitcoin/pull/31662/checks?check_run_id=37171033299:
```bash
[10:53:16.049] gmake[2]: Entering directory '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu'
[10:53:16.093] [ 72%] Linking CXX executable fuzz
[10:53:16.093] cd /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz && /usr/bin/cmake -E cmake_link_script CMakeFiles/fuzz.dir/link.txt --verbose=1
[10:53:16.117] /usr/bin/clang++-20 -ftrivial-auto-var-init=pattern -O2 -g -fsanitize=fuz
...

💬 sipa commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2657179844)
Mild concept ACK. I think having all binaries in one place is a nice, but not critical improvement. If we're doing it, we should do it now, though.

💬 ryanofsky 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_r1954865233)
In commit "rpc: handle shutdown during long poll and wait methods" (0c06c8108fb43be317d86199750ecd3b7db9a5d2)

I think this code is not quite right because it is assuming that if `getTip()` returns null it means the node is shutting down, when in reality it is starting up. Also the IsRPCRunning() call unnecessarily complicates things and is incorrect as we found previously in #30635. Would also be nice to reduce number of variables used in this code and remove CHECK_NONFATAL calls that can't t
...

💬 glozow commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657198949)
@hebasto @theuni can you open a PR to prune the gcc stuff for v29?

💬 sipa commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2657199198)
I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.

Adding an LLVM-based coverage mechanism then seems like an independent improvement.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1954895162)
I'll defer to you, but IIUC we'll do ~1 orphan processing per message processing step, so it might take a bit more to process all 24 from orphanage? Alternatively e could query the *first* tx and wait until the descendant count hits DEFAULT_ANCESTOR_LIMIT