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

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

Merged
merged 2 commits into from Dec 1, 2015

Conversation

Projects
None yet
6 participants
@gmaxwell
Member

gmaxwell commented Nov 23, 2015

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.

kazcw and others added some commits Jul 16, 2014

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.
Limit setAskFor and retire requested entries only when a getdata retu…
…rns.

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.

@laanwj laanwj added the P2P label Nov 24, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 24, 2015

Member

concept ACK

Member

laanwj commented Nov 24, 2015

concept ACK

@laanwj laanwj added this to the 0.12.0 milestone Nov 24, 2015

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 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.

Contributor

dgenr8 commented Nov 25, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 25, 2015

Member

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

Member

gmaxwell commented Nov 25, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 25, 2015

Contributor

concept ACK

Contributor

dcousens commented Nov 25, 2015

concept ACK

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 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.

Contributor

dgenr8 commented Nov 25, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 25, 2015

Member

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.

Member

gmaxwell commented Nov 25, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 26, 2015

Member

Untested ACK

Member

sipa commented Nov 26, 2015

Untested ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 30, 2015

Member

utACK

Member

laanwj commented Nov 30, 2015

utACK

@laanwj laanwj merged commit ebb25f4 into bitcoin:master Dec 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 1, 2015

Merge pull request #7079
ebb25f4 Limit setAskFor and retire requested entries only when a getdata returns. (Gregory Maxwell)
5029698 prevent peer flooding request queue for an inv (kazcw)

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

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.

Github-Pull: #7079
Rebased-From: 5029698

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

Limit setAskFor and retire requested entries only when a getdata retu…
…rns.

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.

Github-Pull: #7079
Rebased-From: ebb25f4

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

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.

Github-Pull: #7079
Rebased-From: 5029698

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

Limit setAskFor and retire requested entries only when a getdata retu…
…rns.

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.

Github-Pull: #7079
Rebased-From: ebb25f4

zkbot pushed a commit to zcash/zcash that referenced this pull request Sep 20, 2016

zkbot
Auto merge of #1407 - bitcartel:master_bitcoin_7079, r=daira
Upstream patch: Prevent peer flooding inv request queue

bitcoin/bitcoin#7079
5029698
ebb25f4

Fuzzbawls added a commit to Fuzzbawls/Mintcoin-Desktop-Wallet that referenced this pull request Sep 26, 2016

Adjust header/block downloading logic
Merge some upstream commits that add
better handling of the header/block
download logic.

Includes:

Reduce download timeouts as blocks arrive
bitcoin/bitcoin#5976

Prevent peer flooding inv request queue
bitcoin/bitcoin#7079
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment