mempool: Filter new txs if they have insufficient gas#2385
Conversation
This also refactors the prior mempool to filter to be known as "precheck filter" and this new filter is called "postcheck filter" This PR also fixes a bug where the precheck filter previously didn't account for the amino overhead, which could a maliciously sized tx to halt blocks from getting any txs in them.
Codecov Report
@@ Coverage Diff @@
## develop #2385 +/- ##
===========================================
- Coverage 61.67% 61.62% -0.05%
===========================================
Files 198 198
Lines 16312 16356 +44
===========================================
+ Hits 10060 10080 +20
- Misses 5419 5446 +27
+ Partials 833 830 -3
|
node/node.go
Outdated
| return (len(tx) + aminoOverhead) <= maxDataBytes | ||
| }), | ||
| mempl.WithPostCheckFilter(func(tx types.Tx, res *abci.ResponseCheckTx) bool { | ||
| maxGas := state.ConsensusParams.BlockSize.MaxGas |
There was a problem hiding this comment.
Shouldn't this be outside? Otherwise there can be a DATA RACE. Note when it's outside, it's a copy. When inside, you're accessing state without mutex.
There was a problem hiding this comment.
You're right, thanks for catching this :)
state/execution.go
Outdated
| return (len(tx) + aminoOverhead) <= maxDataBytes | ||
| } | ||
| postFilter := func(tx types.Tx, res *abci.ResponseCheckTx) bool { | ||
| maxGas := state.ConsensusParams.MaxGas |
xla
left a comment
There was a problem hiding this comment.
Some changes of how we expose the filters and set them up.
mempool/mempool.go
Outdated
| // This is called before each check tx | ||
| preCheckFilter func(types.Tx) bool | ||
| // Filter mempool to only accept txs for which postCheckFilter(tx) returns true. | ||
| postCheckFilter func(types.Tx, *abci.ResponseCheckTx) bool |
There was a problem hiding this comment.
These should be typed a la:
type preCheckFilterFunc func(types.Tx) bool
type postCheckFilterFunc func(types.Tx, *abci.ResponseCheckTx) boolLikely also made public if the option functions are called from outside the mempool with it.
| }), | ||
| mempl.WithPreCheckFilter( | ||
| mempool.PreCheckAminoMaxBytes( | ||
| types.MaxDataBytesUnknownEvidence( |
There was a problem hiding this comment.
I think this change makes it harder to read than having maxDataBytes defined above
There was a problem hiding this comment.
Is this a comment regarding your personal taste or is there strong evidence how one is superior to the other?
There was a problem hiding this comment.
Personal preference, though I believe its founded on strong evidence. 3 function calls is less clear than two function calls on an explicitly defined variable name.
There was a problem hiding this comment.
The hierarchical nesting can be followed step-by-step to understand what is invoced, where with a variable one has to backtrack. Addtionally the scope of the NewNode function is already quite large, so avoiding adding more variables is desriable. Either case should be fine, if you have strong aversion towards the current formatting we can change it.
There was a problem hiding this comment.
nah no need to revert, I don't have a strong opinion :)
NewNode being large is a good point!
This also refactors the prior mempool to filter to be known as
"precheck filter" and this new filter is called "postcheck filter"
This PR also fixes a bug where the precheck filter previously didn't
account for the amino overhead, which could a maliciously sized tx to
halt blocks from getting any txs in them.
ref #2310