-
Notifications
You must be signed in to change notification settings - Fork 701
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
vms/platformvm
: Prune mempool periodically
#2566
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after this I think this lgtm
It seems that with this change there are now two different verification paths for transactions in the P-Chain mempool. When transactions are issued, they are issued into the mempool from the networking code: avalanchego/vms/platformvm/network/gossip.go Line 102 in d825ec2
which uses the mempool tx verifier:
but the new code uses the block builder, which enforces slightly different logic by packing transactions into a block. This seems like it could get a bit error prone and make it more difficult to reason about the behavior of the P-Chain mempool. Would it make sense to better align this behavior in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - left one comment about two different verification paths for transactions in the P-Chain mempool with this change, but this seems like a good change regardless.
Yeah - we should probably remove the |
vm.ctx.Lock.Lock() | ||
defer vm.ctx.Lock.Unlock() | ||
|
||
blockTxs, err := vm.Builder.PackBlockTxs(math.MaxInt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this approach could lead to some transactions invalidating others (based on their position in the "fake block"). I assume this is fine but we may want to comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually a benefit for pruning this way. Happy for a comment though. Seems related to aaron's comment about different verification paths
} | ||
|
||
for _, tx := range blockTxs { | ||
if err := vm.Builder.Add(tx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make vm.Builder.Add
accept (...tx)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd then need to return []error
which seems like a pain
I've got a PR that removes it here: #2362 |
Why this should be merged
The only time the P-Chain mempool gets exhausted is when
BuildBlock
is called. If a node does not build a block, the mempool can continue to gain in size with no re-verification of the mempool txs against the preferred state. This PR adds a goroutine to periodically iterate through the mempool and remove invalid txs.How this works
Periodically builds an ephemeral block (verifies all mempool txs and marks them as dropped). After this block is built, the txs are re-added to the mempool.
Periodic re-verification was chosen over on-demand eviction for better maintainability as P-chain verification rules change. UTXO eviction could be improved to allow skipping during the re-verification without too much work. But tracking validator set modifications for eviction would likely be fairly fragile. We can reconsider this as a future improvement.
How this was tested