Feat/block proposal spec - #143
Conversation
Unit Test Results 5 files ± 0 670 suites +5 1m 5s ⏱️ -1s Results for commit df79e03. ± Comparison against base commit 4340747. This pull request removes 3 and adds 19 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
This doesn't really handle reorgs well -- if some tx's get used by a block, then that block is reorged, then they're back in play. But they're gone from the FIFO. |
…runing in Mempool
…or block ingestion and tip updates
…for improved compile-time evaluation
…ation flow and update proposal reconstruction to return granular error results
… dedicated module
…ter semantics for improved memory handling
… transaction application, epoch/market handling, multisig threshold derivation, fork and orphan-state pruning, and comprehensive tests.
| let nextExecutionGas = cumulativeExecutionGas.checkedAdd(execGas).valueOr: | ||
| continue | ||
| if nextExecutionGas > MAX_EXECUTION_GAS_PER_BLOCK: | ||
| continue |
There was a problem hiding this comment.
There's no early exit for these, so every block proposal scans the entire tx pool. While there are other avoidable inefficiencies here which can make that less problematic, there should be some mechanism by which it detects it's sufficiently close to full, that no other transactions in the pool will likely fit, or won't be likely enough to be worth searching, and stop. This could be because cumulativeExecutionGas is already close enough to MAX_EXECUTION_GAS_PER_BLOCK, for example, that it couldn't be improved much.
Because of the shared/diluted/anonymous leader reward it optimizing for the absolute greatest reward doesn't matter anyway. The ROI for doing a truly thorough search is low.
Other block building performance points:
- casual benchmarking suggests that a single Ed25519 signature verification involved in e.g.,
tryApplyTx(without, e.g., batch verification) on the Zen 5 machine I tried it's 120μs with the current version (and incidentally, BoringSSL provides one which benchmarks at around 90μs on the same machine). This is forChannelInscribe; - zkSig/Groth verification looks to be around 1.7ms (~15x slower) on the same machine;
- it's wasteful to actually compute
encodeSignedMantleTx(item[].tx)to computeencodeSignedMantleTx(item[].tx).len; - another case to consider is a tx pool full of invalid signatures which don't apply and don't consume execution or storage tx, which becomes fairly pathological with the current design;
- it's especially wasteful (though, it's not nearly as CPU-intense as these signature verifications) to do so twice, i.e.
mandatory_feesimmediately does so again; - and it's even more wasteful to verify signatures of any sort more than once at all across blocks -- once the signature is verified once, it should never have to be verified again. it does not become spontaneously invalid (and I'm not sure if there's any reason to keep invalid tx's in the pool, except to have some memory that they are invalid, so not to waste time verifying them again); and
- the
var candidate = workingLedgerand similar copies (sinkdoesn't actually work particularly well inrefc, I've tried; with luck it works withmove(), but otherwise apparently not at all that I've managed) aren't huge but they do end up copying slightly nontrivial things beyond the HAMT. This looks to be perf noise compared to the rest though, and changes with ORC anyway, wheresinkdoes do something useful, unlikerefcwhere it's ... not very functional.
As is, this block building effectively DoSes itself/the node with any reasonable number of pool tx's.
There was a problem hiding this comment.
Yeah, agreed. About workingLedger, the remaining candidate = workingLedger copy is real, but it's needed in case applying the candidate transaction fails. Switching tryApplyTx to var already removes the extra copy inside the call, so I think it's fine to leave this as is unless profiling shows the remaining copy is actually significant.
There was a problem hiding this comment.
perf issues will be managed by PR183 and gossipsub to validate txs at ingress
…d introduce slot clamping for robustness
This PR implements the Execution Market — Block Construction Specification and adds an in-memory transaction mempool.
It also integrates the mempool with the node and chain, including proposal reconstruction, validation, transaction pruning, and restoration after reorgs.
Main Changes
Mempool
Mempoolfor Mantle transactions.GraceCachefor proposal reconstruction.Proposal Reconstruction
proposal.nimmodule.Chain Integration
Design Decisions
Dynamic Fee Market
We don't use a priority queue because execution and storage prices are dynamic and can change independently between transaction insertion and proposal construction.
A transaction's effective cost is:
execGas × execPrice + storGas × storPriceTherefore, there is no stable priority value we can store when the transaction enters the mempool.
For now, transactions are kept in FIFO order and evaluated against the current ledger state during proposal construction.
Note Dependencies
Transactions can depend on notes created by other unconfirmed transactions.
For now, we process them in FIFO order using a temporary working ledger. Transactions that cannot be applied are skipped.
A dependency DAG could be added in a follow-up PR to allow better out-of-order selection.
Limitations
GraceCache.MempoolMinAgeSlots = 3is currently a placeholder and should be adjusted based on propagation benchmarks.