Bugfix: remove conflicting transactions from memory pool #2033
Conversation
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c52f3b86ceb4b2d933f919e06275c670f31dbdb6 for binaries and test log. |
|
I just observed mempool transactions depending on spent inputs on v0.7.1 with this patch - maybe it doesn't work. |
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.
|
Ok, seems the implementation didn't actually remove recursively. Should be fixed now. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/231b399952fd620ee0f72b1947024dba9651630d for binaries and test log. |
|
ACK. I note we do not yet have any mempool unit tests. But I did test it with a bunch of real mainnet reorgs. |
|
ACK |
Bugfix: remove conflicting transactions from memory pool
|
did this fix a bug that had been present since version 1? |
|
@rebroad Yes, indeed. Not a bad one, though. |
|
@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? |
|
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. |
|
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. |
|
Anyone who controls a large amount of hash power has much more potent attacks available to them than this. (E.g. orphan flooding) |
|
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... |
|
Not orphan transactions. Orphan blocks. |
Bugfix: remove conflicting transactions from memory pool
* drop CMasternodeSync::ClearFulfilledRequests() * Cleanup netfulfilledman periodically * Do not expose RemoveFulfilledRequest() Conflicts: src/masternode-sync.cpp src/masternode-sync.h src/netfulfilledman.h src/privatesend.cpp
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 7e15b68 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.