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

pull dgenr8 wants to merge 1 commits into bitcoin:master from dgenr8:already_have changing 1 files +7 −7

dgenr8 commented at 1:51 AM on August 25, 2015: contributor

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.
sipa commented at 9:39 PM on August 25, 2015: member

Concept ACK
dajohi commented at 2:03 PM on August 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.
gavinandresen commented at 2:43 PM on August 27, 2015: contributor

Untested ACK.

in src/main.cpp:None in 4d2117a13c outdated

4332 | -            // means it was rejected and we shouldn't ask for it again.
4333 | -            if (!mempool.exists(tx.GetHash())) {
4334 | -                assert(recentRejects);
4335 | -                recentRejects->insert(tx.GetHash());
4336 | -            }
4337 | +        } else

laanwj commented at 3:26 PM on September 3, 2015:

Nit: can you please keep the style the same here

laanwj commented at 3:27 PM on September 3, 2015: member

utACK, nice catch
laanwj added the label P2P on 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.

9524c4d35c

dgenr8 force-pushed on Sep 3, 2015
dgenr8 commented at 5:28 PM on September 3, 2015: contributor

@laanwj done
btcdrak commented at 11:37 PM on September 7, 2015: contributor

utACK
dcousens commented at 1:06 AM on September 8, 2015: contributor

utACK
jgarzik commented at 9:59 AM on October 1, 2015: contributor

tested ACK
jgarzik merged this on Oct 1, 2015
jgarzik closed this on Oct 1, 2015
jgarzik referenced this in commit cf9bb11f97 on Oct 1, 2015
dgenr8 deleted the branch on Sep 19, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6785 Backport to v0.11: In (strCommand == "tx"), return if AlreadyHave()