In (strCommand == "tx"), return if AlreadyHave() #6588

Merged
merged 1 commit into from Oct 1, 2015

Conversation

Projects
None yet
8 participants
@dgenr8
Contributor

dgenr8 commented Aug 25, 2015

A live DoS attack observed by several nodes in recent days involved repeated rejection of duplicate transactions.

Add a call to AlreadyHave when an unsolicited full tx is received, as was the case in the observed attack. AlreadyHave uses the recentRejects filter.

I tested this change on mainnet while the actual attack was still underway.

The recentRejects filter is cleared when the tip is updated, so nothing stops attacker from re-transmitting a load of rejectable txes after a new block, and in fact our attacker was doing this. But the duplicates are stopped between blocks and the attack could get arbitrarily heavy if multiple attacking peers were involved.

@dgenr8 dgenr8 referenced this pull request in bitcoinxt/bitcoinxt Aug 25, 2015

Merged

Track recently rejected transactions; don't reprocess #50

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 25, 2015

Member

Concept ACK

Member

sipa commented Aug 25, 2015

Concept ACK

@dajohi

This comment has been minimized.

Show comment
Hide comment
@dajohi

dajohi Aug 26, 2015

Contributor

What are you thoughts on disallowing unsolicited tx's -- peers that send a tx without a getdata request. That would break bitcoinj though.

Contributor

dajohi commented Aug 26, 2015

What are you thoughts on disallowing unsolicited tx's -- peers that send a tx without a getdata request. That would break bitcoinj though.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Aug 27, 2015

Contributor

Untested ACK.

Contributor

gavinandresen commented Aug 27, 2015

Untested ACK.

@laanwj

View changes

src/main.cpp
- assert(recentRejects);
- recentRejects->insert(tx.GetHash());
- }
+ } else

This comment has been minimized.

@laanwj

laanwj Sep 3, 2015

Member

Nit: can you please keep the style the same here

@laanwj

laanwj Sep 3, 2015

Member

Nit: can you please keep the style the same here

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 3, 2015

Member

utACK, nice catch

Member

laanwj commented Sep 3, 2015

utACK, nice catch

@laanwj laanwj added the P2P label Sep 3, 2015

In (strCommand == "tx"), return if AlreadyHave()
The main effect is to exit processing for recently-rejected hashes,
in case they are pushed to us without prior advertisement.  This
behavior was seen in the wild.

An additional effect is to do early checks for mempool or mapOrphan
existence.  No logging or nDoS tracking is needed for failures of
these checks.
@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Sep 3, 2015

Contributor

@laanwj done

Contributor

dgenr8 commented Sep 3, 2015

@laanwj done

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Sep 7, 2015

Member

utACK

Member

btcdrak commented Sep 7, 2015

utACK

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 8, 2015

Contributor

utACK

Contributor

dcousens commented Sep 8, 2015

utACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Oct 1, 2015

Contributor

tested ACK

Contributor

jgarzik commented Oct 1, 2015

tested ACK

@jgarzik jgarzik merged commit 9524c4d into bitcoin:master Oct 1, 2015

1 check passed

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

jgarzik added a commit that referenced this pull request Oct 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment