🤔 mzumsande reviewed a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065#pullrequestreview-2695264617)
Two thoughts:
- As far as I understand it (https://geti2p.net/en/about/performance/future), i2p tunnels last for 10 minutes by default. If we don't use these extra sessions before they expire, their creation would be a waste of ressources. I would image this to be particularly relevant in a scenario where i2p is used together with other networks. When we have no need for new regular outbounds because we are full and none of our peers disconnect, we make feeler connections every 2 minutes and ex
...

📝 maflcko opened a pull request: "test: Fix intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/pull/32092)
The test may fail intermittently when the `net` thread is lagging while calling `DeleteNode`. This may result in a split `getdata`, meaning that `peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])` fails.

Fix it by adding a sync on the `net` thread.

Fixes #31700

💬 maflcko commented on pull request "test: Fix intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32092#issuecomment-2733932368)
Can be tested by the diff provided by @tnndbtc in https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318 (thanks!):

```diff
diff --git a/src/net.cpp b/src/net.cpp
index 735985a841..2bb6db0cf7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1965,6 +1965,9 @@ void CConnman::DisconnectNodes()
// Destroy the object only after other threads have stopped using it.
if (pnode->GetRefCount() <= 0) {
m_nodes_disconnected.remove(pnode);
+
...

💬 fjahr commented on pull request "test: switch wallet_crosschain.py to signet and drop testnet4":
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734008623)
utACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5

Looks good to me and CI failure seems unrelated.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2734048299)
Rebased 623c0f0df878968dabbaa271a7654c638603f369 -> a9935d4dae8847d0549ec51ddc4349c7e8c61763 ([`pr/wrap.23`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.23) -> [`pr/wrap.24`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.23-rebase..pr/wrap.24)) due to conflict with #32019

👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2695588548)
ACK a9935d4dae8847d0549ec51ddc4349c7e8c61763

💬 maflcko commented on pull request "test: switch wallet_crosschain.py to signet and drop testnet4":
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734205095)
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK cec14ee47d71d8dd
...

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2001653317)
How much resources can a (mempool) transaction waste by violating this rule in the last input / output after making the interpreter do a bunch of hashing work?

And is that worse than existing taproot script allows? And would it be better with a restriction on the number of combinations, e.g. if inputs can't refer to other inputs? Or should these bounds checks be done earlier for the whole transaction?

👍 vasild approved a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2695684854)
ACK 4e265debdc0319bbfcea915d9026b33810b810f8

💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2734250524)
(According to https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2691174968 the test commits are the same, so any reviewer here can probably also ack the other pull)

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2001668320)
The docs at https://clang.llvm.org/docs/SourceBasedCodeCoverage.html don't mention anything about `-O` flags.

🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2694691922)
Preliminary ACK 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92

I think it's okay to merge this code:
* as of this PR, it isn't actually used, so should not introduce bugs immediately
* the fuzz testing is pretty good, so there should not be any catastrophic bugs even if it were used
* while maybe the design could be improved, that can be done incrementally after it's merged
* I'm reasonably confident I understand what's going on and can help find/fix bugs/problems if they occur
* I think
...

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001126453)
Most of the `Cluster` methods require a `TxGraphImpl` to keep the quality/staging/mapping things consistent; feels a bit like it might be simpler just to have almost all this code stay in `TxGraphImpl`?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001315529)
I think this is a close to accurate diagram of the data structures here:


To me, the cycles in that graph seem like a code smell. The `GraphIndex` backrefs make sense (or are unavoidable), as some stable way to refer to each tx is needed. But, I think it would probably be better for a Cluster not to be aware of its quality level or staging level (removing the `m_setindex/m_quality/m_level`
...

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001430348)
`ClusterSet m_main_clusterset; std::optional<ClusterSet> m_staging_clusterset;` might be clearer -- the vector stuff seems more confusing than helpful?

Does it ever make sense to have multiple nested staging levels? Parallel staging levels could make sense (I want to compare three RBFs in parallel on multiple cores, having checked that they're all independent and don't interact with each others' clusters) but seems probably more complicated than would be worthwhile.

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001521035)
Avoid side-effects inside asserts if possible?

💬 Chand-ra commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001701966)
> What happens if you run the individual test, i.e. $ ./build/test/functional/interface_usdt_net.py

You were right, I didn't compile bitcoind with USDT support. Running the individual test results in the following output:
```
2025-03-18T18:06:36.018000Z TestFramework (INFO): PRNG seed is: 6897053979890084060
2025-03-18T18:06:36.021000Z TestFramework (WARNING): Test Skipped: bitcoind has not been built with USDT tracepoints enabled.
2025-03-18T18:06:36.074000Z TestFramework (INFO): Stoppi
...

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001723093)
I ran into a somewhat contrived use-case, but I have a fairly strong YNGNI feeling of this aspect of the code. That said, I don't think it hurts understanding too much as is.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001731756)
Curious to know this contrived use case, I also think it's a bit of an overkill in a previous comment https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2618143580, but @sipa made a compelling argument for it https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2666866811

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2734449766)
@ajtowns Responding to some of the more conceptual comments

* I'm indeed not entirely happy with the cycles between `TxGraphImpl` and `Cluster`, though there is a design concern that isn't really materialized in the current code yet: `Cluster` may become a virtual class with multiple implementations for the purpose of memory usage (e.g., there could be a specialized `SingletonCluster` which just stores a single transaction, and no DepGraph, linearization, ..., avoiding several vectors and the
...