This is particularly useful with support for package relay, see #16401.

It would be possible to go a bit further and drop mapRelay entirely; but we want to delay relaying transactions until we've inv'ed them, and currently we use mapRelay for that logic -- ie, we receive a tx, add it to setInventoryTxToSend, wait for a random poisson delay, INV the tx to a peer, add it to mapRelay, and only then are we willing to relay the tx. The poisson delay maxes out at about 3 minutes, so any tx that's been around for <15m is either to new to relay, or is already in mapRelay.

Ideally we would have some per-peer logic so that we only relay new txs to peer X if we've INV'ed the tx to peer X; but saving that work for a different PR in order to keep this one simple.

Code review ACK, will test.

I believe this will be useful even without package relay -- whenever a node starts up with an empty (or stale) mempool, transactions with parents fail to make it into the mempool because our peers won't serve parents that were previously relayed.

But I would also like this bug to be fixed as soon as possible, so that a package-acceptance scheme like what I'm proposing in #16401 is immediately useful, even before proposing a p2p protocol extension for package relay.

ACK 7b2dbf9

I ran a new node connected only to a node running this patch, and observed that as transaction relay started (after IBD) that the node started receiving orphan transactions that were then accepted to the mempool after the parents were fetched.

Concept ACK

I don't think so, but asking for sake of completeness: this will not affect privacy (fingerprinting) negatively, will it?

I don't think so, but asking for sake of completeness: this will not affect privacy (fingerprinting) negatively, will it?

I don't think so; I think you can already tell if tx "X" is in a peer's mempool by constructing a tx with "X:0" and "Y:0" as inputs (Y being a non-existent txid, providing invalid sigs for both, relies on X being rbf'able or X:0 not being spent yet), and see if they request both X and Y or just Y.

I agree with @ajtowns that this shouldn't add any additional privacy leak, because it's already possible to test the presence of a transaction in the mempool.

On further thought, I wonder if this may need additional anti-DoS protection. In ProcessGetData(), I believe we rate limit tx-getdata requests by (a) the contents of mapRelay (which should be bounded by our inv-timers), and (b) mempool requests are rate-limited by the poisson timers. I guess also (c) we can respond to multiple tx-getdata requests in a single ProcessGetData() only up until the send buffer for the peer is full. However, this logic for responding to getdata requests directly from the mempool bypasses measures (a) and (b) -- should we add some additional rate limiter here?

Edit: on further thought, I think this concern is basically misplaced. The mempool command is not rate-limited by poisson timers (just the initial response is delayed once), and at any rate there are trivial ways to use bandwidth that are not exacerbated by this change.

Concept ACK.

This missed the deadline for 0.19, so maybe it should be removed from high prio and targeted for 0.20?

Concept ACK. I've written #16908, so that those ugly multiplications by 1000000 go away. They aren't particularly review friendly.

Will take a closer look some time later.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16698 ([WIP] Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)

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.

Needs rebase 👀

code review ACK aaca5d9

I think this is a good candidate for early merge for 0.20 (to be able to notice issues with this change in behavior far before a relase).

@ajtowns 🙏 ^

re-ACK 168b781 (only change is comment fixup)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a (only change is comment fixup)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhnRwwAqG6wIDfJsUAzCjuL+5wzxZKZpHUitPdQxW73bzHtuZnO9LT+1+y06Arg
+Lk0GgxLxmuBdT3U2lEVtcboNUYBazCeJg5e3xbb6B5PNLYgst4dd0o6M7VM6nhE
I9nJxCiwoqq6nmzFf+Y6Yk86ROmBQ/1gP0/GmziuHvMW8mPYP5/Aw+SGj6O6RXWU
2hBWb5jIFg6mkuQDGRkNoAGi7rdBawLdUYWvhVCkuMIg/OeBO//kSrjcXNnrBm6C
75igedEd6Ps0ob9CIZ5CX/u9y/Evmsdz2F3eRpHaQeLICAewJ8DztjziXVZD2yzp
ymDN349uFqe9eY5B84JI9cFnfS9S4lSknP7KTffNPfBm/ZrE83Z5NiHxrXjZRW1S
hrUtSw+YqIpWiWJk7rrt6YzUhS6640qmvaW22rEXAEDJhcBD1+XK+mFiGivH6Er4
owWaFknwp/hdNd2zwM6To8oWaDZdv2IT5jmTrLMHYQl+5Wn+ZuOiYVDeTvazQKsL
HOIFMTQM
=fBZj
-----END PGP SIGNATURE-----

I'm not sure if we should just give up on trying to prevent testing the contents of our mempool?

I'm not sure if we should just give up on trying to prevent testing the contents of our mempool?

Long term it probably make sense to give up on it. Though, as long as the default wallet behaviour is to put the txs in the mempool immediately, we should keep the privacy effect of mapRelay.

This change has no adverse effect on privacy unless I am mistaken. It is a nice tx relay improvement.

I deleted my earlier comment, as I realized I missed the distinction between what this PR does (making txn available after expiry) and the idea of dropping mapRelay entirely (making everything always available). I was confused by the "this is not a privacy leak because mempool presence can be tested in different ways" response; while that's true, it's also not a privacy leak even if there were no other ways to test for memool presence.

re-ACK 168b781

ACK 168b781