💬 theStack commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001370725)
> I ran the entire test suite from my `build` directory using `ctest --output-on-failure` and the modified test passed just fine there. Any idea on what might cause the discrepancy?

I assume you built bitcoind without USDT support. One simply way to do enable it is e.g. configuring CMake via `cmake --preset dev-mode`. What happens if you run the individual test, i.e. `$ ./build/test/functional/interface_usdt_net.py`?

💬 maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733763095)
lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0

💬 maflcko commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-2733810899)
(The arm CI failed)

🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2695196075)
re: https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2694724554

> lgtm, but would you like people to review #31992 first?

Could be helpful but we have a chain of PR's here #31992 #31741 #30975 #31802 that could be reviewed in any order even if they need to be merged in a particular order. Different reviewers will be familiar with different parts of the changes, so it is probably most helpful for reviewers to give feedback on whatever parts they are most familiar with.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001400939)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001144249

> Maybe add `--numeric-owner` here as suggested in https://reproducible-builds.org/docs/archives/ (and as already done further down in this file).

Previously I used that page as a reference and added options to make these tarballs always reproducible but I had to drop them because they were not supported on macos: https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621360345, https://github.com/r
...

💬 maflcko commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2733834009)
An alternative fix would be to adjust `wait_for_parent_requests` to accept any order and any sequence of messages, but this may be a larger diff.

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