op-acceptance-test: add flashblock acceptance tests for rule-based op-rbuilder #18723
Conversation
…-rbuilder features
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #18723 +/- ##
========================================
Coverage 73.53% 73.53%
========================================
Files 189 189
Lines 11252 11252
========================================
Hits 8274 8274
Misses 2834 2834
Partials 144 144
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
teddyknox
left a comment
teddyknox
left a comment
There was a problem hiding this comment.
LGTM. Main concern is the potential for tests to silently pass when transactions don't land in the same block.
| // The rulesConfig specifies the path to the rules configuration file. | ||
| // Note: This function expects the orchestrator to be initialized via DoMain with | ||
| // WithSingleChainSystemWithFlashblocksAndRules(rulesConfig) option. | ||
| func NewSingleChainWithFlashblocksAndRules(t devtest.T) *SingleChainWithFlashblocks { |
There was a problem hiding this comment.
This function is identical to NewSingleChainWithFlashblocks and is never called. The tests use NewSingleChainWithFlashblocks directly. Either remove it or use it in the tests (replacing NewSingleChainWithFlashblocks in rules tests) to make intent clearer.
| logger.Info("Transactions in different blocks - comparing indices for transactions in same block", | ||
| "high_block", receiptHigh.BlockNumber, | ||
| "medium_block", receiptMedium.BlockNumber, | ||
| "low_block", receiptLow.BlockNumber, | ||
| "normal_block", receiptNormal.BlockNumber, | ||
| ) |
There was a problem hiding this comment.
In TestBoostPriorityOrdering, TestBoostedVsNonBoostedOrdering, and TestMultipleSendersWithMixedPriorities, the tests only verify ordering if transactions land in the same block. If they don't, the test either does weaker checks or logs and moves on. This could lead to tests silently passing without actually verifying the intended behavior.
| return nil | ||
| } | ||
|
|
||
| // Handle timeout errors - only continue if we haven't received any other error type |
There was a problem hiding this comment.
consider rephrasing to "Timeout errors are recoverable - keep reading"
Description
This PR adds new flashblock acceptance tests for op-rbuilder with rule-based ordering.
This allows op-rbuilder to score individual transaction based on some specific ruleset, and include the transactions with the score ordering.
See corresponding op-rbuilder PR for more detail: https://github.com/ethereum-optimism/op-rbuilder/pull/1
To run these new tests, set flag like this: