Bugfix: remove conflicting transactions from memory pool #2033

pull sipa wants to merge 1 commits into bitcoin:master from sipa:kickconflicts changing 2 files +28 −3

sipa commented at 9:51 PM on November 24, 2012: member

When a transaction A is in the memory pool, while a transaction B (which shares an input with A) gets accepted into a block, A was kept forever in the memory pool.

This problem exists in probably all versions of Bitcoin ever. On v0.7.1, it can be demonstrating by mining with 7e15b68ae applied. Every few hours, it seems such a transactions that conflicts with the memory pool gets mined (successful double spends?).

Fixing this results in less transactions in the memory pool, and faster construction of new blocks.

This should apply cleanly on v0.7.0, v0.7.1 and HEAD.
BitcoinPullTester commented at 10:34 PM on November 24, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c52f3b86ceb4b2d933f919e06275c670f31dbdb6 for binaries and test log.
sipa commented at 1:26 AM on November 25, 2012: member

I just observed mempool transactions depending on spent inputs on v0.7.1 with this patch - maybe it doesn't work.

Bugfix: remove conflicting transactions from memory pool

When a transaction A is in the memory pool, while a transaction B
(which shares an input with A) gets accepted into a block, A was
kept forever in the memory pool.

This commit adds a CTxMemPool::removeConflicts method, which
removes transactions that conflict with a given transaction, and
all their children.

This results in less transactions in the memory pool, and faster
construction of new blocks.

231b399952

sipa commented at 11:51 AM on November 25, 2012: member

Ok, seems the implementation didn't actually remove recursively. Should be fixed now.
BitcoinPullTester commented at 6:18 PM on November 27, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/231b399952fd620ee0f72b1947024dba9651630d for binaries and test log.
gmaxwell commented at 5:37 PM on November 29, 2012: contributor

ACK. I note we do not yet have any mempool unit tests. But I did test it with a bunch of real mainnet reorgs.
gavinandresen commented at 12:20 AM on December 1, 2012: contributor

ACK
sipa referenced this in commit cd7fb7d1de on Dec 1, 2012
sipa merged this on Dec 1, 2012
sipa closed this on Dec 1, 2012
rebroad commented at 4:07 PM on December 20, 2012: contributor

did this fix a bug that had been present since version 1?
sipa commented at 4:22 PM on December 20, 2012: member

@rebroad Yes, indeed. Not a bad one, though.
rebroad commented at 5:33 PM on December 20, 2012: contributor

@sipa it looks like it was a bug that could have successfully created a DOS attack that may even have broken the network - i.e. create loads of conflicting transactions so that the mempool fills up to its limit and then the nodes would have stopped passing on new transactions... yes/no?
gmaxwell commented at 5:36 PM on December 20, 2012: contributor

No, not really— because it requires mining blocks that have later spends than everyone else had accepted. So it's naturally limited by the attacker's ability to mine blocks.
sipa deleted the branch on Dec 20, 2012
rebroad commented at 8:26 PM on December 20, 2012: contributor

Unless the attacker managed to attack a mining node so that they saw the later spends instead of the earlier ones.

Or any pool mining owner who wanted to do this presumeably could.
gmaxwell commented at 8:30 PM on December 20, 2012: contributor

Anyone who controls a large amount of hash power has much more potent attacks available to them than this. (E.g. orphan flooding)
rebroad commented at 1:41 PM on December 21, 2012: contributor

Why would orphan flooding be a more potent attack than filling the mempool? I'd have thought the orphan pool filling up would be far less of a problem than the mempool filling up. Sorry, probably a discussion for #bitcoin...
gmaxwell commented at 1:44 PM on December 21, 2012: contributor

Not orphan transactions. Orphan blocks.
laudney referenced this in commit 5b2811f86a on Mar 19, 2014
HashUnlimited referenced this in commit 29f6cde20a on Apr 19, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#2054 IDB and already have block errors