Better handling of orphan transactions #4885

pull gavinandresen wants to merge 2 commits into bitcoin:master from gavinandresen:2014_09_orphant changing 4 files +70 −20

gavinandresen commented at 6:12 PM on September 9, 2014: contributor

This is an alternative to #4880

The second commit fixes a crashing bug I found when testing the rest of these commits.

The first is #4880. The third is a two-line change to remove orphans when a node is disconnected for any reason, rather than just because it is banned.

The fourth adds a -maxorphantx option, and drops the default maximum number of orphan transactions to store in memory from 10,000 to 100, which is much closer to the actual number of orphans at any time during normal operation. Lowering the maximum number of orphans stored reduces potential memory usage and mitigates a whole class of "make your node spend a lot of time validating invalid orphans" attacks.
gavinandresen commented at 6:16 PM on September 9, 2014: contributor

Oh, forgot:

Tested by running with: -debug=mempool -maxorphantx=0 and -debug=mempool -maxorphantx=4
gavinandresen force-pushed on Sep 9, 2014
sipa commented at 6:27 PM on September 9, 2014: member

Untested ACK. Testing.
jgarzik commented at 7:40 PM on September 9, 2014: contributor

One rare edge case I wonder about: When a reorg creates more than 100 orphans.
sipa commented at 7:47 PM on September 9, 2014: member

Jeff: you seem confused. Orphans (both blocks and transactions) as used in the source code means: things that can't be validated yet, as a parent is missing.

Disconnecting part of the block chain can moved blockchain transaction back to the memory pool, but certainly not to the orphan pool.

Or are you confusing with orphan blocks?
jgarzik commented at 7:54 PM on September 9, 2014: contributor

I meant invalid transactions, due to depending on unknown(non-existent) coinbases. But those wouldn't go into mapOrphan* presumably, as they are simply invalid.
sipa commented at 8:34 PM on September 9, 2014: member

Tested ACK
wtogami referenced this in commit 491d82c438 on Sep 10, 2014
wtogami referenced this in commit 105df6f6b7 on Sep 10, 2014
wtogami referenced this in commit 4834020e98 on Sep 10, 2014
wtogami referenced this in commit ae74ed06a8 on Sep 10, 2014
laanwj commented at 2:55 PM on September 10, 2014: member

I've cherry-picked 66aca7 "Fix crashing bug caused by orphan(s) with duplicate prevout.hash" into master as def2fdb to avoid crashes.
gavinandresen force-pushed on Sep 10, 2014
gavinandresen commented at 4:24 PM on September 10, 2014: contributor

Rebased on master, and reworked a bit to address @wtogami's comments-- now one bad signature breaks out of the "process all orphans that were waiting on this transaction" loop.
sipa commented at 4:32 PM on September 10, 2014: member

Won't breaking out of that loop mean that you end up with transactions in the orphan pool that never get processed anymore?

I think we should prevent processing transactions from punished sources instead.
gavinandresen commented at 5:14 PM on September 10, 2014: contributor

@sipa : mmm... I think you're right, I could construct a chain of valid orphans with invalid-but-expensive-to-validate children and cause a lot of unnecessary CPU work.
gavinandresen force-pushed on Sep 10, 2014
gavinandresen commented at 5:36 PM on September 10, 2014: contributor

Ok, the promote orphans into memory pool loop now keeps track of misbehaving peers, and skips processing of orphans from those peers. It doesn't remove ALL the orphans from those peers-- that still gets done when the peer is disconnected.

Tested by running -debug=mempool -printtoconsole and merging in a hacked-together disconnectpeer RPC call.
gavinandresen force-pushed on Sep 10, 2014

Stricter handling of orphan transactions

Prevent denial-of-service attacks by banning
peers that send us invalid orphan transactions
and only storing orphan transactions given to
us by a peer while the peer is connected.

c74332c678

Store fewer orphan tx by default, add -maxorphantx option

There is no reason to store thousands of orphan transactions;
normally an orphan's parents will either be broadcast or
mined reasonably quickly.

This pull drops the maximum number of orphans from 10,000 down
to 100, and adds a command-line option (-maxorphantx) that is
just like -maxorphanblocks to override the default.

aa3c697e90

gavinandresen force-pushed on Sep 10, 2014
BitcoinPullTester commented at 6:27 PM on September 10, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4885_aa3c697e90c02d5797a59a7bfb1ecac6fbd918cf/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
wtogami referenced this in commit 1a8a1b64d3 on Sep 11, 2014
wtogami referenced this in commit a09625b30e on Sep 11, 2014
wtogami commented at 5:48 AM on September 11, 2014: contributor

ACK
laanwj commented at 10:34 AM on September 11, 2014: member

I tested the orphan pool overflow as well as deletion of all orphans of a node on disconnect. I don't have anything to test the DoS/Misbehaving case at the moment, though.

ACK on code changes.
laanwj merged this on Sep 11, 2014
laanwj closed this on Sep 11, 2014
laanwj referenced this in commit 3fa1c81b94 on Sep 11, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4880 Remove peer's orphan transactions if they are banned