Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continue relaying transactions after they expire from mapRelay #16851

Merged
merged 1 commit into from Oct 24, 2019

Conversation

@ajtowns
Copy link
Contributor

ajtowns commented Sep 11, 2019

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 fanquake added the P2P label Sep 11, 2019
@ajtowns

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 11, 2019

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 12, 2019

Concept ACK

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

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 12, 2019

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 laanwj added this to Blockers in High-priority for review Sep 12, 2019
@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 16, 2019

Concept ACK.

@jonatack jonatack referenced this pull request Sep 17, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 18, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 18, 2019

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

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 18, 2019

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 ajtowns removed this from Blockers in High-priority for review Sep 19, 2019
@laanwj laanwj added the Feature label Sep 30, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 2, 2019

Needs rebase 👀

@ajtowns ajtowns force-pushed the ajtowns:201909-relayparents branch from 7b2dbf9 to aaca5d9 Oct 3, 2019
@DrahtBot DrahtBot removed the Needs rebase label Oct 3, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 9, 2019

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

@fanquake fanquake requested a review from sdaftuar Oct 9, 2019
Copy link
Member

MarcoFalke left a comment

ACK aaca5d9

Show signature and timestamp

Signature:

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

ACK aaca5d90c0859ea3883f71aa7bc7e30cfab87a21
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUinowv/ZU/xg+wTAlCYcpsbJLqeOJNvWWg5fiHNZvsGkcfX4/2GlD2FAELSICvm
PI7DlGDGzayY5tH6ceIWmqB5w5j2wmxQtuz3cKcIz8wkJQn+pOLxn3k4WKU7zlxR
SsJ5VZBMCnFOQQ+Pi6OVtnfrXYXbqxVzlDhjeoYBrkoMCjK92e/q3RrV4BwvsAGd
PM90Sej8UbYuFk6V3GrUQruIh1d6yIpuDWrY0+NWWuMwdK/5H4xqpq37+YknzgMi
BhAR9tg1XSxqyDtZworytY4zzPOQS7BG4foEyoM8Kb4zjEqYQ/oyWSoq2Xs/Wxm4
H7nAA3y2WdtuE5RCt7lpipi9vGmBj9DEr5abx1ZLUKugdUDy9lfbIM/oG+zylKXF
49F0RQRxsGH8A0JQYq6GzJOUYxp1+35ttVahEwGvWhiXG7AGRLYo1PcTwmkPbzS4
z2oJD5NtYQnKN37j9V24uaGAoh7AXn7FAxp9oakeZe9X9f6iwIoR4wElwIqvG96q
mkW7StXQ
=fRMY
-----END PGP SIGNATURE-----

Timestamp of file with hash 0970ab24f6dc0609f52b54188404a5e069b6a5372f89722e14db5cba3ea2a7fe -

src/net_processing.cpp Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke added this to the 0.20.0 milestone Oct 15, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 22, 2019

@ajtowns 🙏 ^

@ajtowns ajtowns force-pushed the ajtowns:201909-relayparents branch from aaca5d9 to 168b781 Oct 23, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 23, 2019

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

Show signature and timestamp

Signature:

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

Timestamp of file with hash b6d7996f849e87c5338fd963f408651c4f54bfa0784b702e5fefa91d5863fb38 -

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 23, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 23, 2019

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

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 23, 2019

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

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 23, 2019

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

This comment has been minimized.

Copy link
Member

sdaftuar commented Oct 23, 2019

re-ACK 168b781

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 24, 2019

ACK 168b781

MarcoFalke added a commit that referenced this pull request Oct 24, 2019
…apRelay

168b781 Continue relaying transactions after they expire from mapRelay (Anthony Towns)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    re-ACK 168b781 (only change is comment fixup)
  sdaftuar:
    re-ACK 168b781
  sipa:
    ACK 168b781

Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
@MarcoFalke MarcoFalke merged commit 168b781 into bitcoin:master Oct 24, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.