nit: Comment doesn't seem to match the assert. Could just add...

assert_equal(len(self.nodes[0].listunspent()), 1)

Commit "Don't relay tx data to peers until after tx announcement"

Doesn't seem quite obvious that std::set is worst, why did you picked std::unordered_set?

Commit "Don't relay tx data to peers until after tx announcement"

nit, s/_map/_set?

Constant time lookup seemed better to me than log_n lookup but I dunno, is there a reason you think std::set is better here? I suspect it doesn't really matter.

I dunno too. Considering the size is small I agree it doesn't matter.

Commit "Don't relay tx data to peers until after tx announcement"

From developer notes:

- By default, declare single-argument constructors `explicit`.

  - *Rationale*: This is a precaution to avoid unintended conversions that might
    arise when single-argument constructors are used as implicit conversion
    functions.

Commit "Don't relay tx data to peers until after tx announcement"

nit, could use emplace(k,v) since you are touching this line.

This is moved and updated in the next commit.

Commit "Add ancestors of announced transactions to mapRelay"

nit, could drop std::make_pair.

nit: distribuiton (spelling)

This test, being stochastic, looks like it'll fail spuriously. I guess it if happens too often, we can up REPEATS or something.

Someone could run it for 1000 times and see if it fails at all

./test/functional/test_runner.py -j 15 $(for i in {0..999}; do echo p2p_leak_tx ; done)

Looks like it fails about 9% of the time for me. :(

Sorry for that. I haven't actually run the test substantially after writing. I seems you could set EXPECTED_MEASURED_LEAK = .42 or so without making the test useless. (On master the leak is always 1.00, so setting the expected measured leak to any number less than 1 should be fine)

(measured leak on x-axis)

I bumped it up to 0.45, and it failed once in 1000 runs, so i bumped it up to 0.50 and ran it 5000 times with no failures. I have not really looked at the test or analyzed the statistics at all to verify that this makes sense.

nit: unrelated change?

fixed

Sorry for that. I haven't actually run the test substantially after writing. I seems you could set EXPECTED_MEASURED_LEAK = .42 or so without making the test useless. (On master the leak is always 1.00, so setting the expected measured leak to any number less than 1 should be fine)

(measured leak on x-axis)

I don't understand what this test is supposed to be testing. Is it:

(i) that we don't leak txs to inbound peers that we haven't yet announced to (as stated in the docstring); or
(ii) that txs are probabilistically announced to outbound peers before inbound peers.

If (i), can't we just test that for every tx, either we receive a NOTFOUND message for the tx or we've already received an INV message for the tx?

The assert_greater_than() call here seems to be simply testing (ii). Or maybe I'm misunderstanding?

Makes sense! That makes the test easier to understand as well:

(to be applied on current HEAD commit of this branch)

diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py
index 21d1d4ed2d..a0ceaedd67 100755
--- a/test/functional/p2p_leak_tx.py
+++ b/test/functional/p2p_leak_tx.py
@@ -13,10 +13,17 @@ from test_framework.util import (
     wait_until,
 )
 
+import time
+
+
+class P2PNode(P2PDataStore):
+    def on_inv(self, msg):
+        pass
+
 
 class P2PLeakTxTest(BitcoinTestFramework):
     def set_test_params(self):
-        self.num_nodes = 2
+        self.num_nodes = 1
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -26,67 +33,28 @@ class P2PLeakTxTest(BitcoinTestFramework):
         gen_node.generate(1)
         self.sync_all()
 
-        inbound_peer = self.nodes[0].add_p2p_connection(P2PDataStore())  # An "attacking" inbound peer
-        outbound_peer = self.nodes[1]  # Our outbound peer
+        inbound_peer = self.nodes[0].add_p2p_connection(P2PNode())  # An "attacking" inbound peer
 
-        # In an adversarial setting we can generally assume that inbound peers
-        # are more likely to spy on us than outbound peers. Thus, on average,
-        # we announce transactions first to outbound peers, then to (all)
-        # inbound peers. Inbound peers must not be able to successfully request a
-        # transaction if they haven't yet received the announcement for it.
-        #
-        # With only one outbound peer, we expect that a tx is first announced
-        # to (all) inbound peers (and thus present a potential leak) in 28.5% of
-        # the cases.
-        #
-        # Probability( time_ann_inbound < time_ann_outbound )                 =
-        # ∫ f_in(x)                           * F_out(x)                   dx =
-        # ∫ (lambda_in * exp(-lambda_in * x)) * (1 - exp(-lambda_out * x)) dx =
-        # 0.285714
-        #
-        # Where,
-        # * f_in is the pdf of the exponential distribution for inbound peers,
-        #   with lambda_in = 1 / INVENTORY_BROADCAST_INTERVAL = 1/5
-        # * F_out is the cdf of the expon. distribution for outbound peers,
-        #   with lambda_out = 1 / (INVENTORY_BROADCAST_INTERVAL >> 1) = 1/2
-        #
-        # Due to measurement delays, the actual monte-carlo leak is a bit
-        # higher. Assume a total delay of 0.6 s (Includes network delays and
-        # rpc delay to poll the raw mempool)
-        #
-        # Probability( time_ann_inbound < time_ann_outbound + 0.6 )           =
-        # ∫ f_in(x)                           * F_out(x + 0.6)             dx =
-        # ∫ (lambda_in * exp(-lambda_in * x)) * (1 - exp(-lambda_out * (x+.6))) dx =
-        # 0.366485
-        # EXPECTED_MEASURED_LEAK = .366485
-        # Because this test is empirical and our testing framework isn't set up
-        # to handle tests that fail with some expected likelihood, we bump this
-        # value up to decrease the false positive rate.
-        EXPECTED_MEASURED_LEAK = .50
-
-        REPEATS = 100
-        measured_leak = 0
-        self.log.info('Start simulation for {} repeats'.format(REPEATS))
+        REPEATS = 10
+        self.log.info('Start test for {} repeats'.format(REPEATS))
         for i in range(REPEATS):
             self.log.debug('Run {}/{}'.format(i, REPEATS))
             txid = gen_node.sendtoaddress(gen_node.getnewaddress(), 0.033)
+
+            time.sleep(5)
+
             want_tx = msg_getdata()
             want_tx.inv.append(CInv(t=1, h=int(txid, 16)))
-
-            wait_until(lambda: txid in outbound_peer.getrawmempool(), lock=mininode_lock)
             inbound_peer.send_message(want_tx)
             inbound_peer.sync_with_ping()
 
             if inbound_peer.last_message.get('notfound'):
+                self.log.debug('tx {} was not yet announced to us.'.format(txid))
                 assert_equal(inbound_peer.last_message['notfound'].vec[0].hash, int(txid, 16))
                 inbound_peer.last_message.pop('notfound')
             else:
-                measured_leak += 1
-
-        measured_leak /= REPEATS
-        self.log.info('Measured leak of {}'.format(measured_leak))
-
-        assert_greater_than(EXPECTED_MEASURED_LEAK, measured_leak)
+                self.log.debug('tx {} was announced to us.'.format(txid))
+                assert int(txid, 16) in [inv.hash for inv in inbound_peer.last_message['inv'].inv]
 
 
 if __name__ == '__main__':

Concept ACK the change to the test!

Can we add multiple P2P connections to the node to get more test repeats in parallel, or does the change to tx propagation in #13298 mean that we'll announce to all those peers at the same time and not get any additional benefit?

Currently we can only easily add inbound mininode peers (which are in the same bucket for tx propagation), so right now it couldn't run in parallel that way.

I am happy to adjust the test as soon as we can add outbound connections to mininode peers.

Thanks!

I think a comment here would be helpful (since mi->second.m_node_set.count(pfrom->GetId()) isn't immediately obvious when read outside the context of this PR). Something like:

// Only send the transaction if we've previously announced it to this peer

Note that we can leak mempool contents (and lose privacy) to a peer if it sends us a MEMPOOL message. That is unchanged by this PR, but the logic here is a bit confusing and we should probably deprecate support for MEMPOOL in future.

One relatively easy solution (I think), is to only announce transactions that have been longer than say 60s in the mempool at the time the MEMPOOL message is received, and to enforce the same time restriction in this block of code (so you can't fetch transactions that were received less than 60s before the last MEMPOOL request).

I've now reviewed this test. Sadly, it doesn't test the new code (and does not fail on master).

Since the node-under-test has only one peer, the transaction won't get added to mapRelay until we send the INV to that peer. That means in the if/else branches at the bottom of this test:

• if the node has already sent us an INV for the tx, then it'll be in the mapRelay and the node will send us the TX.
• if the node has not yet sent us an INV for the tx, then it won't be in the mapRelay and we'll fail mi != mapRelay.end() and not even reach && mi->second.m_node_set.count(pfrom->GetId())

To test the new code, we want the to be in mapRelay, but the peer to not be in the RelayEntry. That's not possible if the node has only one peer.

Perhaps we should remove the test from this PR and add it under a future PR. If you do so, please tag me in that PR and I'll commit to reviewing the test.

Use m_txref(std::move(tx)) here. tx in this context is not an rvalue reference (as it's been bound to variable), it's only initialized by accepting an rvalue reference.

I had no idea, thank you for pointing this out!

One relatively easy solution (I think), is to only announce transactions that have been longer than say 60s in the mempool at the time the MEMPOOL message is received, and to enforce the same time restriction in this block of code (so you can't fetch transactions that were received less than 60s before the last MEMPOOL request).

There are at most 9 buckets, so a set of outbound NodeIds and a flag for inbound ones might demand less memory?