Skip to content

fix(giga): match v2 correctness checks#3071

Open
arajasek wants to merge 2 commits intomainfrom
asr/giga-correctness
Open

fix(giga): match v2 correctness checks#3071
arajasek wants to merge 2 commits intomainfrom
asr/giga-correctness

Conversation

@arajasek
Copy link
Contributor

Describe your changes and provide context

This PR improves and cleans up the validation performed by the Giga executor before running a tx:

  • Calls EvmStatelessChecks directly
  • Moves all other validation into its own method
  • Bumps nonce if and only if the nonce is correct, as per v2
    • Basically "if nonce is correct, it will get bumped, regardless of any other potential failures"
  • Orders the various validation checks to match v2, so that in the edge-case where multiple things fail, we respect v2's behaviour
  • Matches v2 panic recovery
    • Seq mode needs to trap panics
    • OCC mode needs to lower panics to the appropriate error code: ErrOCCAbort, ErrOutOfGas, or ErrPanic

Testing performed to validate your change

Bunch of new AI-generated tests. Will confirm it syncs mainnet shortly.

@arajasek
Copy link
Contributor Author

This PR subsumes #3017. #3045, #3061

@github-actions
Copy link

github-actions bot commented Mar 13, 2026

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMar 13, 2026, 6:56 PM

@codecov
Copy link

codecov bot commented Mar 13, 2026

Codecov Report

❌ Patch coverage is 38.17204% with 115 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.40%. Comparing base (bb2c5b3) to head (5f624e7).

Files with missing lines Patch % Lines
app/app.go 38.17% 102 Missing and 13 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3071      +/-   ##
==========================================
- Coverage   58.41%   58.40%   -0.01%     
==========================================
  Files        2081     2081              
  Lines      171790   171918     +128     
==========================================
+ Hits       100352   100410      +58     
- Misses      62504    62572      +68     
- Partials     8934     8936       +2     
Flag Coverage Δ
sei-chain-pr 55.15% <38.17%> (?)
sei-db 70.41% <ø> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app/app.go 67.33% <38.17%> (-2.00%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arajasek arajasek force-pushed the asr/giga-correctness branch from a510d81 to d987428 Compare March 13, 2026 18:07
if r := recover(); r != nil {
logger.Error("panic in giga synchronous executor", "panic", r, "stack", string(debug.Stack()))
result = &abci.ExecTxResult{
Code: sdkerrors.ErrPanic.ABCICode(),
Copy link
Collaborator

@masih masih Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to differentiate between panic types otherwise returning everything as ABCICode would be apphash breaking i think.

We have 2 off the top of my head: scheduler.Abort and sdk.ErrorOutOfGas. We would want to check for OOG error type here jsut to be on the safe side.

}
}
}()
result, execErr = app.executeEVMTxWithGigaExecutor(ctx, evmMsg, cache)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm @arajasek we don't return result or execErr out from this func.

I recommend changing the function signature and use named return types prefixed with _ and use those instead. That is far less error prone against future refactors too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants