net/rpc: Report inv information for debugging

ajtowns commented at 8:34 am on September 21, 2025: contributor

Adds per-peer entries to getpeerinfo for the size of the inv_to_send queue and the mempool sequence number as at the last INV. Can be helpful for debugging tx relay performance and privacy/fingerprinting issues.

rpc/net: add per-peer inv_to_send sizes adefb51c54

DrahtBot commented at 8:34 am on September 21, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33448.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, instagibbs
Concept ACK	0xB10C
Stale ACK	danielabrozzoni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33029 (net: make m_mempool optional in PeerManager by naiyoma)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/rpc_net.py:169 in ab58b26ca5 outdated

165@@ -166,6 +166,7 @@ def test_getpeerinfo(self):
166                 "permissions": [],
167                 "presynced_headers": -1,
168                 "relaytxes": False,
169+                "inv_to_send": 0,

l0rinc commented at 7:59 pm on September 21, 2025:

The CI failures indicate that we need to add the other field as well:

0                "last_inv_sequence": 0,
1                "inv_to_send": 0,

tested locally that it fixes the tests:

0TEST                     | STATUS    | DURATION
1
2rpc_net.py --v1transport | ✓ Passed  | 12 s
3rpc_net.py --v2transport | ✓ Passed  | 10 s

rpc/net: report per-peer last_inv_sequence 77b2ebb811

ajtowns force-pushed on Sep 22, 2025

0xB10C commented at 6:49 am on September 22, 2025: contributor

Concept ACK. I had exposing the inv-to-send size on my todo list too.

danielabrozzoni commented at 1:25 pm on September 22, 2025: contributor

ACK 77b2ebb811824899f56976f8e3113914706edc97

Code looks good to me, I have done some minimal testing on my own node (running getpeerinfo and inspecting the result)

Noteworthy change: the PR changes the mutex that guards m_last_inv_sequence: previously it was g_msgproc_mutex, but since the mutex is for “anything that is only accessed via the msg processing thread”, this PR changes it to m_tx_invetory_mutex. I manually checked that in any place where we use m_last_inv_sequence, the correct mutex is locked.

DrahtBot requested review from 0xB10C on Sep 22, 2025

maflcko commented at 1:37 pm on September 22, 2025: member

lgtm, but could add a small test? Maybe by modifying test_tx_in_block, or by copy-pasting test_tx_in_block into a newly created sub-test? A hacky diff:

 0diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py
 1index a1a00751d1..54eaaef7ab 100755
 2--- a/test/functional/p2p_leak_tx.py
 3+++ b/test/functional/p2p_leak_tx.py
 4@@ -36,8 +36,17 @@ class P2PLeakTxTest(BitcoinTestFramework):
 5 
 6         self.log.debug("Generate transaction and block")
 7         inbound_peer.last_message.pop("inv", None)
 8+        import time
 9+        self.gen_node.setmocktime(int(time.time()))
10         wtxid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"]
11+        pi=self.gen_node.getpeerinfo()[0]
12+        assert_equal(pi["last_inv_sequence"],1)
13+        assert_equal(pi["inv_to_send"],1)
14+        self.gen_node.setmocktime(0)
15         inbound_peer.wait_until(lambda: "inv" in inbound_peer.last_message and inbound_peer.last_message.get("inv").inv[0].hash == int(wtxid, 16))
16+        pi=self.gen_node.getpeerinfo()[0]
17+        assert_equal(pi["last_inv_sequence"],2)
18+        assert_equal(pi["inv_to_send"],0)
19         want_tx = msg_getdata(inv=inbound_peer.last_message.get("inv").inv)
20         self.generate(self.gen_node, 1)
21

test: validate behaviour of getpeerinfo last_inv_sequence and inv_to_send

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

2738b63e02

ajtowns force-pushed on Sep 23, 2025

ajtowns commented at 4:58 am on September 23, 2025: contributor

Added the test after slightly cleaning it up

sipa commented at 12:28 pm on September 23, 2025: member

utACK 2738b63e025d240618b3c72c28243c3e4d7d9c79

DrahtBot requested review from danielabrozzoni on Sep 23, 2025

in test/functional/p2p_leak_tx.py:40 in 2738b63e02

36@@ -36,8 +37,24 @@ def test_tx_in_block(self):
37 
38         self.log.debug("Generate transaction and block")
39         inbound_peer.last_message.pop("inv", None)
40+

instagibbs commented at 1:08 pm on September 23, 2025:

nit: double-checked before and after to check effects of test

 0diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py
 1index 42e586fca3..d302a68ebf 100755
 2--- a/test/functional/p2p_leak_tx.py
 3+++ b/test/functional/p2p_leak_tx.py
 4@@ -39,4 +39,9 @@ class P2PLeakTxTest(BitcoinTestFramework):
 5         inbound_peer.last_message.pop("inv", None)
 6 
 7+        assert_equal(self.gen_node.getrawmempool(False, True), {'txids': [], 'mempool_sequence': 1})
 8+        pi = self.gen_node.getpeerinfo()[0]
 9+        assert_equal(pi["last_inv_sequence"], 1) # sequence starts at 1
10+        assert_equal(pi["inv_to_send"], 0) # nothing has been queued
11+
12         self.gen_node.setmocktime(int(time.time())) # pause time based activities
13         wtxid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["wtxid"]
14@@ -44,5 +49,5 @@ class P2PLeakTxTest(BitcoinTestFramework):
15         pi = self.gen_node.getpeerinfo()[0]
16         assert_equal(rawmp["mempool_sequence"], 2) # our tx cause mempool activity
17-        assert_equal(pi["last_inv_sequence"], 1) # that is after the last inv
18+        assert_equal(pi["last_inv_sequence"], 1) # unchanged until queue is drained for peer
19         assert_equal(pi["inv_to_send"], 1) # and our tx has been queued
20         self.gen_node.setmocktime(0)

instagibbs approved

instagibbs commented at 1:09 pm on September 23, 2025: member

ACK 2738b63e025d240618b3c72c28243c3e4d7d9c79

fanquake merged this on Sep 23, 2025

fanquake closed this on Sep 23, 2025

in src/net_processing.cpp:1738 in 2738b63e02

1733+        stats.m_inv_to_send = tx_relay->m_tx_inventory_to_send.size();
1734     } else {
1735         stats.m_relay_txs = false;
1736         stats.m_fee_filter_received = 0;
1737+        stats.m_inv_to_send = 0;
1738     }

vasild commented at 2:53 pm on September 23, 2025:

In the else branch, why set one of the new variables stats.m_inv_to_send to 0 but not the other new variable stats.m_last_inv_seq? Both have default value of 0. I understand that if this code enters the if / true branch, it will never go to the else branch in subsequent calls. That is - peer->GetTxRelay() will not return != nullptr once and later return nullptr. So, this means that there is no need to zero out stale values in the else branch, right? Then stats.m_inv_to_send = 0; is not needed, or if it is needed then also stats.m_last_inv_seq = 0; is needed?

ajtowns commented at 2:08 pm on September 24, 2025:

It’s not needed the way we use it (passing in a fresh Stats argument each call), but arguably both should be zeroed out in case a previously-used object were reused.

vasild commented at 2:59 pm on September 23, 2025: contributor

ACK 2738b63e025d240618b3c72c28243c3e4d7d9c79

net/rpc: Report inv information for debugging #33448

Code Coverage & Benchmarks

Reviews

Conflicts