Create ValidationOptions for each tx submission#818
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThe changes remove persistent storage of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EVM
participant EmulatorConfig
participant TxValidator
Client->>EVM: SendRawTransaction(ctx, data)
EVM->>EVM: Create new head, emulatorConfig, evmSigner, validationOptions
EVM->>TxValidator: Validate transaction with local context
TxValidator-->>EVM: Validation result
EVM-->>Client: Return transaction hash or error
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/requester/requester.go (1)
186-208: Consider making the reference block number configurableWhile the fixed block number (20,182,324) works for the current implementation, consider making this configurable to avoid hardcoding values that might need to change in future upgrades.
- head := &types.Header{ - Number: big.NewInt(20_182_324), + head := &types.Header{ + Number: big.NewInt(e.config.ReferenceBlockNumber),This would require adding a
ReferenceBlockNumberfield to your config struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/requester/requester.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
services/requester/requester.go (4)
112-114: Good removal of the stored state fieldsRemoving the persistent storage of
head,evmSigner, andvalidationOptionsfields from theEVMstruct is the right approach. This eliminates the issue where these values were only initialized at startup and never updated.
163-173: Constructor changes match struct modificationsThe removal of field initializations from the
NewEVMconstructor correctly aligns with the field removals from the struct definition.
186-208: Per-transaction validation context is a good solutionCreating a fresh validation context for each transaction submission ensures the Header time is current. This correctly addresses the issue where transaction submissions of type 4 were being rejected due to outdated Prague/Pectra upgrade phase detection.
The fixed block number (20,182,324) appears to be a safe reference point for validation. The current timestamp ensures accurate upgrade phase detection.
209-209: Transaction validation now uses fresh contextUsing the locally created values for transaction validation ensures that validation decisions are based on current conditions rather than stale state.
|
|
||
| if err := models.ValidateTransaction(tx, e.head, e.evmSigner, e.validationOptions); err != nil { | ||
| head := &types.Header{ | ||
| Number: big.NewInt(20_182_324), |
There was a problem hiding this comment.
why is this block number hardcoded?
There was a problem hiding this comment.
This one is not really important, as it's no longer used for hard-forks. Ethereum does upgrades with timestamps, instead of block numbers. But this is there for backwards-compatibility. I think this was the Ethereum mainnet latest block number, when we launched Flow EVM on mainnet.
There was a problem hiding this comment.
@m-Peter please add that as a comment, magic numbers not good :)
3 participants
Description
Previously this Header time was only created during startup, so it never got updated. This makes the EVM GW think that we're not yet in Pectra:
{ "jsonrpc": "2.0", "id": 2, "error": { "code": -32000, "message": "transaction type not supported: type 4 rejected, pool not yet in Prague" } }For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Summary by CodeRabbit