Prevent peer flooding inv request queue (redux) (redux) #7079

pull gmaxwell wants to merge 2 commits into bitcoin:master from gmaxwell:setAskFor changing 3 files +12 −1

gmaxwell commented at 2:13 AM on November 23, 2015: contributor

Currently it's possible for peers to send duplicate invs of the same object and push the inv request time far back.

PR #4547 sought to address this, but was closed with the expectation that it would be replaced by the more comprehensive #4831 but the latter hasn't yet reached escape velocity. I agree a comprehensive rework is better, but I think we should not let a simple improvement wait for that.

PR #6306 tried a simpler version, but by reusing the setInventoryKnown it gets the logic somewhat wrong, e.g. not allowing duplicate INVs even for objects we'd mempool retired but now would be accept again.

Here I bring back #4547 and add size limiting and make an important change to the logic (which I believe was originally flawed, though no one seems to have commented on that in the original PR):

The earlier PR would remove the suppression as soon as the getdata was requested, rather than removing it when response was returned. This one removes only when we decide to not getdata (because we already have) or when the getdata returns. This means that a peer can only have one in-flight INV for a particular tx at once; which is what we want.

prevent peer flooding request queue for an inv

mapAlreadyAskedFor does not keep track of which peer has a request queued for a
particular tx. As a result, a peer can blind a node to a tx indefinitely by
sending many invs for the same tx, and then never replying to getdatas for it.
Each inv received will be placed 2 minutes farther back in mapAlreadyAskedFor,
so a short message containing 10 invs would render that tx unavailable for 20
minutes.

This is fixed by disallowing a peer from having more than one entry for a
particular inv in mapAlreadyAskedFor at a time.

5029698186

Limit setAskFor and retire requested entries only when a getdata returns.

The setAskFor duplicate elimination was too eager and removed entries
 when we still had no getdata response, allowing the peer to keep
 INVing and not responding.

ebb25f4c23

laanwj added the label P2P on Nov 24, 2015
laanwj commented at 8:22 AM on November 24, 2015: member

concept ACK
laanwj added this to the milestone 0.12.0 on Nov 24, 2015
laanwj cross-referenced this on Nov 24, 2015 from issue Prevent peer flooding inv request queue (redux) by dgenr8
dgenr8 commented at 2:16 AM on November 25, 2015: contributor

e.g. not allowing duplicate INVs even for objects we'd mempool retired but now would be accept again.

That's a bug in CTxMemPool::Expire. It should clean the tx out of all setInventoryKnowns anyway, else we won't try to re-inv it to peers when it comes in again.
gmaxwell commented at 5:05 AM on November 25, 2015: contributor

@dgenr8 I don't think it is, or at least it's debatable... we won't reinv to peers we already INVed it to-- they may well still have it-- we should still reinv to anyone we haven't.
dcousens commented at 9:09 AM on November 25, 2015: contributor

concept ACK
dgenr8 commented at 10:02 PM on November 25, 2015: contributor

@gmaxwell Ah well, as long as the attack is mitigated. It's also possible to drop the timeout to 20s (from 2m) and only miss a response 1% of the time. .4% at 30s.
gmaxwell commented at 10:35 PM on November 25, 2015: contributor

The timeouts must be set very conservatively or otherwise a congested node (e.g. one under dos attack) can exhaust itself with repeated transmissions. You cannot count on the average case measured on a node with good connectivity.

I'm also not particularly interested if someone-- with a bunch of work-- can delay a transaction by a couple minutes; it's not a very interesting attack compared to self congestion collapse (which can happen absent any attacker at all). 2 minutes is the appropriate scaling from the 20 minute value used elsewhere, with respect to a the maximum size standard transaction being 1/10th the block size.
sipa commented at 9:15 PM on November 26, 2015: member

Untested ACK
laanwj commented at 11:57 AM on November 30, 2015: member

utACK
laanwj merged this on Dec 1, 2015
laanwj closed this on Dec 1, 2015
laanwj referenced this in commit 1b5118bfa0 on Dec 1, 2015
luke-jr referenced this in commit 45c7ab2db2 on Jan 10, 2016
luke-jr referenced this in commit 3b21a5ecd2 on Jan 10, 2016
luke-jr referenced this in commit b83ab95d8d on Jan 10, 2016
luke-jr referenced this in commit 0d43146081 on Jan 10, 2016
defuse cross-referenced this on Aug 16, 2016 from issue Merge upstream anti DoS patches by bitcartel
bitcartel cross-referenced this on Sep 16, 2016 from issue Upstream patch: Prevent peer flooding inv request queue by bitcartel
zkbot referenced this in commit 5ef7fecf14 on Sep 20, 2016
Fuzzbawls referenced this in commit 50285d0427 on Sep 26, 2016
rebroad cross-referenced this on Dec 27, 2016 from issue large orphan transactions are left in askfor queue even when rejected by rebroad
rebroad cross-referenced this on Dec 27, 2016 from issue requesting txs that have already been added to the pool by rebroad
adamjonas cross-referenced this on Oct 22, 2019 from issue potential problem with nodes ignoring getdata requests by rebroad
furszy cross-referenced this on May 3, 2021 from issue Net code updates [Step 2] by furszy
random-zebra referenced this in commit 71275c1896 on Jun 9, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.12.0

Linked (view graph)

#1157 potential problem with nodes ignoring getdata requests #3774 large orphan transactions are left in askfor queue even when rejected #4461 requesting txs that have already been added to the pool #4547 prevent peer flooding request queue for an inv #4831 net: Better askfor request management #6306 Prevent peer flooding inv request queue (redux)