Continue relaying transactions after they expire from mapRelay

ajtowns commented at 5:10 am on September 11, 2019: member

This change allows peers to request transactions even after they’ve expired from mapRelay and even if they’re not doing mempool requests. This is intended to allow for CPFP of old transactions – if parent tx P wasn’t relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.

fanquake added the label P2P on Sep 11, 2019

ajtowns commented at 5:14 am on September 11, 2019: member

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.

sdaftuar commented at 5:56 pm on September 11, 2019: member

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.

sdaftuar commented at 8:13 pm on September 11, 2019: member

ACK 7b2dbf9e4bdbacd16ccd4923a6ebf4f93d7a799b

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.

laanwj commented at 10:03 am on September 12, 2019: member

Concept ACK

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

ajtowns commented at 12:05 pm on September 12, 2019: member

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.

sdaftuar commented at 12:56 pm on September 12, 2019: member

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.

laanwj added this to the "Blockers" column in a project

promag commented at 10:09 am on September 16, 2019: member

Concept ACK.

MarcoFalke commented at 7:58 pm on September 18, 2019: member

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

MarcoFalke commented at 8:00 pm on September 18, 2019: member

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.

DrahtBot commented at 8:28 pm on September 18, 2019: member

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.

ajtowns removed this from the "Blockers" column in a project

laanwj added the label Feature on Sep 30, 2019

MarcoFalke commented at 2:57 pm on October 2, 2019: member

Needs rebase :eyes:

MarcoFalke added the label Needs rebase on Oct 2, 2019

ajtowns force-pushed on Oct 3, 2019

DrahtBot removed the label Needs rebase on Oct 3, 2019

laanwj commented at 9:24 am on October 9, 2019: member

code review ACK aaca5d90c0859ea3883f71aa7bc7e30cfab87a21

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).

fanquake requested review from sdaftuar on Oct 9, 2019

in src/net_processing.cpp:42 in aaca5d90c0 outdated

38@@ -39,6 +39,8 @@
39 static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
40 /** Minimum time between orphan transactions expire time checks in seconds */
41 static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;
42+/** Expiration time for normal transaction relay */

MarcoFalke commented at 7:39 pm on October 15, 2019:

The comment sounds a bit misleading to me.

This is not an “expire” time, but how long txs are cached for relay in mapRelay until we fall back to getting them from the mempool, right?

MarcoFalke commented at 7:42 pm on October 15, 2019:

Maybe

0/** How long we cache mempool txs in mapRelay to reply to getdata requests TODO: remove the mapRelay cache */
1RELAY_TX_CACHE_DURATION

ajtowns commented at 1:36 am on October 23, 2019:

/** How long to cache transactions in mapRelay for normal relay */ and RELAY_TX_CACHE_TIME ? Doesn’t seem like a great place to add the TODO so I’ve left that out

MarcoFalke commented at 8:27 pm on October 15, 2019: member

ACK aaca5d90c0859ea3883f71aa7bc7e30cfab87a21

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK aaca5d90c0859ea3883f71aa7bc7e30cfab87a21
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUinowv/ZU/xg+wTAlCYcpsbJLqeOJNvWWg5fiHNZvsGkcfX4/2GlD2FAELSICvm
 8PI7DlGDGzayY5tH6ceIWmqB5w5j2wmxQtuz3cKcIz8wkJQn+pOLxn3k4WKU7zlxR
 9SsJ5VZBMCnFOQQ+Pi6OVtnfrXYXbqxVzlDhjeoYBrkoMCjK92e/q3RrV4BwvsAGd
10PM90Sej8UbYuFk6V3GrUQruIh1d6yIpuDWrY0+NWWuMwdK/5H4xqpq37+YknzgMi
11BhAR9tg1XSxqyDtZworytY4zzPOQS7BG4foEyoM8Kb4zjEqYQ/oyWSoq2Xs/Wxm4
12H7nAA3y2WdtuE5RCt7lpipi9vGmBj9DEr5abx1ZLUKugdUDy9lfbIM/oG+zylKXF
1349F0RQRxsGH8A0JQYq6GzJOUYxp1+35ttVahEwGvWhiXG7AGRLYo1PcTwmkPbzS4
14z2oJD5NtYQnKN37j9V24uaGAoh7AXn7FAxp9oakeZe9X9f6iwIoR4wElwIqvG96q
15mkW7StXQ
16=fRMY
17-----END PGP SIGNATURE-----

Timestamp of file with hash 0970ab24f6dc0609f52b54188404a5e069b6a5372f89722e14db5cba3ea2a7fe -

MarcoFalke added this to the milestone 0.20.0 on Oct 15, 2019

MarcoFalke added the label Waiting for author on Oct 17, 2019

MarcoFalke commented at 12:14 pm on October 22, 2019: member

@ajtowns :pray: ^

Continue relaying transactions after they expire from mapRelay 168b781fe7

ajtowns force-pushed on Oct 23, 2019

MarcoFalke removed the label Waiting for author on Oct 23, 2019

MarcoFalke commented at 3:58 pm on October 23, 2019: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a (only change is comment fixup)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhnRwwAqG6wIDfJsUAzCjuL+5wzxZKZpHUitPdQxW73bzHtuZnO9LT+1+y06Arg
 8+Lk0GgxLxmuBdT3U2lEVtcboNUYBazCeJg5e3xbb6B5PNLYgst4dd0o6M7VM6nhE
 9I9nJxCiwoqq6nmzFf+Y6Yk86ROmBQ/1gP0/GmziuHvMW8mPYP5/Aw+SGj6O6RXWU
102hBWb5jIFg6mkuQDGRkNoAGi7rdBawLdUYWvhVCkuMIg/OeBO//kSrjcXNnrBm6C
1175igedEd6Ps0ob9CIZ5CX/u9y/Evmsdz2F3eRpHaQeLICAewJ8DztjziXVZD2yzp
12ymDN349uFqe9eY5B84JI9cFnfS9S4lSknP7KTffNPfBm/ZrE83Z5NiHxrXjZRW1S
13hrUtSw+YqIpWiWJk7rrt6YzUhS6640qmvaW22rEXAEDJhcBD1+XK+mFiGivH6Er4
14owWaFknwp/hdNd2zwM6To8oWaDZdv2IT5jmTrLMHYQl+5Wn+ZuOiYVDeTvazQKsL
15HOIFMTQM
16=fBZj
17-----END PGP SIGNATURE-----

Timestamp of file with hash b6d7996f849e87c5338fd963f408651c4f54bfa0784b702e5fefa91d5863fb38 -

sipa commented at 4:07 pm on October 23, 2019: member

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

MarcoFalke commented at 4:18 pm on October 23, 2019: member

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.

MarcoFalke commented at 4:51 pm on October 23, 2019: member

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

sipa commented at 4:54 pm on October 23, 2019: member

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.

sdaftuar commented at 6:41 pm on October 23, 2019: member

re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a

sipa commented at 6:06 pm on October 24, 2019: member

ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a

MarcoFalke referenced this in commit fce7c75422 on Oct 24, 2019

MarcoFalke merged this on Oct 24, 2019

MarcoFalke closed this on Oct 24, 2019

JeremyRubin added this to the "Package Relay" column in a project

deadalnix referenced this in commit 482680ec58 on Jun 15, 2020

DrahtBot locked this on Dec 16, 2021

Continue relaying transactions after they expire from mapRelay #16851

Conflicts