Fix mempool and chain locking #647
Merged
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
Makes no sense copying the Pool around.
Eliminate races between tx checks and adding them to the mempool, ensure the chain doesn't change while we're working with the new tx. Ensure only one block addition attempt could be in progress.
We can only add one block of the given height and we have two competing goroutines to do that --- consensus and block queue. Whomever adds the block first shouldn't trigger an error in another one. Fix block relaying for blocks added via the block queue also, previously one consensus-generated blocks were broadcasted.
Appending and not changing the real Items is utterly wrong.
Nobody outside should care about these details, mempool operates on transactions and that's it.
After the f0bb886 with all methods of Pool being pointer-based it makes no sense having this lock as a pointer.
They shouldn't depend on the chain state and for the same transaction they should always produce the same result. Thus, it makes no sense recalculating them over and over again.
It doesn't harm as we have transactions naturally ordered by fee anyway and it makes managing them a little easier. This also makes slices store item itself instead of pointers to it which reduces the pressure on the memory subsystem.
Chopping off the last element of the slice if way easier than doing it with the first one.
Which is way faster than sort.Sort'ing things all the time.
We not only need to remove transactions stored in the block, but also invalidate some potential double spends caused by these transactions. Usually new block contains a substantial number of transactions from the pool, so it's easier to make one pass over it only keeping valid items rather than remove them one by one and make an additional pass to recheck inputs/witnesses.
Our mempool only contains valid verified transactions all the time, it never has any unverified ones. Unverified pool made some sense for quick unverifying after the new block acceptance (and gradual background reverification), but reverification needs some non-trivial locking between blockchain and mempool and internal mempool state locking (reverifying tx and moving it between unverified and verified pools must be atomic). But our current reverification is fast enough (and has all the appropriate locks), so bothering with unverified pool makes little sense.
It's more efficient and keeps transactions sorted by priority.
golangcibot
reviewed
Feb 6, 2020
Codecov Report
@@ Coverage Diff @@
## master #647 +/- ##
==========================================
+ Coverage 63.9% 64.34% +0.44%
==========================================
Files 126 126
Lines 10574 10597 +23
==========================================
+ Hits 6757 6819 +62
+ Misses 3511 3475 -36
+ Partials 306 303 -3
Continue to review full report at Codecov.
|
Simplifies things a lot and removes useless code.
Fixes GolangCI: Error return value of (*github.com/CityOfZion/neo-go/pkg/core/mempool.Pool).Add is not checked (from errcheck) and allows us to almost completely forget about mempool here.
roman-khimov
force-pushed
the
fix-mempool-and-chain-locking
branch
from
037dab4 to
7445655
Compare
fyrchik
reviewed
Feb 6, 2020
| } | ||
| } | ||
| if num < len(mp.verifiedTxes)-1 { | ||
| mp.verifiedTxes = append(mp.verifiedTxes[:num], mp.verifiedTxes[num+1:]...) |
There was a problem hiding this comment.
I believe this line alone is enough. We should simplify it later.
fyrchik
approved these changes
Feb 6, 2020
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.
Problem
Our mempool sucks.
Solution
Make it great again! This allows us easily make 1000 Tx/s in 4-nodes privnet, fixes potential double spends and improves mempool testing coverage.