Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mempool: Invert reorg transaction handling. #2956

Merged
merged 1 commit into from
Jun 12, 2022

Conversation

sefbkn
Copy link
Member

@sefbkn sefbkn commented May 24, 2022

This commit inverts the order that transactions are added to the mempool during a reorg so that they are accepted into the mempool in reverse block order.

  • Add new public function MaybeAcceptTransactions to mempool for processing not new transactions. It expects transactions to be provided in block order, though this restriction could be relaxed as a non-breaking change if it makes sense to do so at a later time. Some reasons behind adding a new public function is to
    • lock the mempool while adding the batch of transactions
    • make the transactions being added visible to the utxo view since they do not exist anywhere else

This PR addresses a scenario where statistics that are calculated for the initial sort used during block template generation may become inaccurate after a reorg under special circumstances.

A set of unconfirmed transactions in the mempool that spends or is spent by another in the set are referred to here as a transaction chain. Consider the following transaction chain in the mempool.

Block: []
Mempool: [A <— B <— C]

A scenario is possible where the tail of the transaction chain (C) is left in the mempool while the other 2 are included in the blockchain.

Block: [A <— B]
Mempool: [C]

If a reorg occurs causing the first two transactions to re-enter the mempool, then the aggregate statistics for the transaction already in the mempool will become inaccurate due to its ancestors being added out of order. Currently, transactions are added back to the mempool in block-order.

So, when transaction A is added to the mempool, there is no path to update the descendant C’s statistics to account for it since B is neither in the mempool nor the blockchain. When transaction B is added to the mempool, its statistics are applied to C’s aggregate statistics. The problem here is that C’s aggregate statistics are missing information about A. If A is removed from the mempool, leaving C and B in the mempool, then A’s statistics would be subtracted from C’s statistics despite not having been added to those statistics during the last reorg.

The solution here is to add transactions in reverse-block order so that there is continuity in unconfirmed transaction chains from the perspective of the mining view as they are added and removed from the mempool / transaction graph.

@davecgh davecgh added this to the 1.8.0 milestone May 24, 2022
@davecgh
Copy link
Member

davecgh commented May 27, 2022

Thanks for the PR. Would you mind editing the PR description to include the reasoning for the change? I personally know based on previous discussions, but for someone looking at this in isolation it probably isn't clear.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be rebased to resolve the conflict, and have the description updated as previously mentioned, but otherwise, I don't see any issues. Nice job!

internal/mempool/mempool_test.go Outdated Show resolved Hide resolved
This commit inverts the order that transactions
are added to the mempool during a reorg so that
they are added in reverse block order.
@sefbkn sefbkn force-pushed the reverse_transaction_handling_mempool branch from 6821bc0 to 27b90e0 Compare June 12, 2022 07:38
@sefbkn
Copy link
Member Author

sefbkn commented Jun 12, 2022

This needs to be rebased to resolve the conflict, and have the description updated as previously mentioned, but otherwise, I don't see any issues. Nice job!

The PR description has been updated and merge conflict resolved.

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

Successfully merging this pull request may close these issues.

3 participants