Refactor mempool spend conflict checks to increase performance #2826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Oct 6, 2021
teor2345
reviewed
Oct 6, 2021
jvff
force-pushed
the
refactor-mempool-spend-conflict-checks
branch
from
0ce1796
to
6af8a40
Compare
teor2345
reviewed
Oct 7, 2021
This was referenced Oct 7, 2021
Keep track of the spent transparent outpoints and the revealed nullifiers. Clippy complained that the `ActiveState` had variants with large size differences, but that was expected, so I disabled that lint on that `enum`.
Clear them so that they remain consistent with the set of verified transactions.
Store new outputs into its respective `HashSet`, and abort if a duplicate output is found.
Restore the `HashSet` to its previous state.
Keep the mempool storage in a consistent state when a transaction is removed.
Ensure eviction also keeps the tracked outputs consistent with the verified transactions.
Move the code to handle the output caches into the new type. Also move the eviction code to make things a little simpler.
Centralize the code that handles the removal of a transaction to avoid mistakes.
Because the evicted transactions must be added to the rejected list.
Leftover from some temporary testing code. Co-authored-by: teor <teor@riseup.net>
It is more speculation than planning, so it doesn't add much value. Co-authored-by: teor <teor@riseup.net>
The verb should match the subject "transactions" which is plural. Co-authored-by: teor <teor@riseup.net>
There's a subtle but important detail in the implementation that should be made more visible to avoid mistakes in the future. Co-authored-by: teor <teor@riseup.net>
Left-over from the attempt to move the eviction into the `VerifiedSet`.
Rewrite the comment explaining why the Clippy lint was ignored.
Refactor to avoid API misuse.
Using two transactions, perform the same test adding a conflict to both of them to check if the second inserted transaction is properly rejected. Then remove any conflicts from the second transaction and add it again. That should work, because if it doesn't it means that when the second transaction was rejected it left things it shouldn't in the cache.
When removing multiple transactions from the mempool storage, all of the ones requested should be removed and any other transaction should be still be there afterwards.
jvff
force-pushed
the
refactor-mempool-spend-conflict-checks
branch
from
df362a1
to
8fc2832
Compare
|
I reviewed this and it looks good, but I think it's better for @teor2345 to take a last look. I can make any final adjustments if needed. |
If the mempool size is smaller than 4, these tests don't fail on a trivial removal bug. Because we need a minimum number of transactions in the mempool to trigger the bug. Also commit a proptest seed that fails on a trivial removal bug. (This seed fails if we remove indexes in order, because every index past the first removes the wrong transaction.)
And replace the very large proptest debug output with the new summary.
teor2345
approved these changes
Oct 10, 2021
There was a problem hiding this comment.
I increased the mempool size to 4, so that the new tests would actually detect multiple removal errors.
I also summarised the debug output of the tests, because it was very very large.
(It contained the entire contents of a vector of random transactions.)
Apart from the mempool size, I only changed test debug formatting.
So I think we should merge now to avoid conflicts.
This was referenced Oct 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
#2765 implemented rejection of mempool transactions that have spend conflicts (i.e., spend the same transparent outpoint or reveal the same nullifier) with other transactions already in the mempool. However, the implementation was simple and would not scale well in terms of performance as the mempool size is increased.
Solution
Replace the O(n * m) algorithm with an O(n) algorithm. Now the
mempool::StorageusesHashSets to keep track of the spent outpoints and revealed nullifiers by the verified transactions. This allows it to quickly check if there are conflicts when new transactions are added.Review
@teor2345
Reviewer Checklist
Follow Up Work