skip LastResultsHash check if giga executor is on#2866
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2866 +/- ##
=======================================
Coverage 57.18% 57.19%
=======================================
Files 2091 2091
Lines 171512 171514 +2
=======================================
+ Hits 98075 98089 +14
+ Misses 64692 64682 -10
+ Partials 8745 8743 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| if err != nil { | ||
| // Check if this is a LastResultsHash mismatch and log detailed info | ||
| if !bytes.Equal(block.LastResultsHash, state.LastResultsHash) { | ||
| if !types.SkipLastResultsHashValidation.Load() && !bytes.Equal(block.LastResultsHash, state.LastResultsHash) { |
There was a problem hiding this comment.
Does this actually work? I think you're just skipping the log, but still returning the error here, so I don't think SkipLastResultsHashValidation actually does anything.
There was a problem hiding this comment.
err comes from validateBlock, which wouldn't return a non-nil value because of https://github.com/sei-protocol/sei-chain/pull/2866/changes#diff-d3412156734f105e2fc032cd6850dd192eb7f1a039872a127df968c1bffde8f9R65
There was a problem hiding this comment.
That makes sense, but then why are we changing anything here at all? I think we can leave this file as-is.
There was a problem hiding this comment.
I think there's value in containing a change like this if possible.
There was a problem hiding this comment.
on a giga node, if there is indeed a validation error that's not LastResultsHash mismatch, but LastResultsHash does mismatch, then the log here would say "LastResultsHash error" instead of the real error.
it's a rare case though
arajasek
force-pushed
the
tony/skip-lastresult-hash
branch
from
cffaeaa to
a997ef7
Compare
Giga executors may yield different gas usage than v2 executors due to: - more parity with Ethereum - optimized state access pattern Since we only care about state consistency, which is covered by AppHash, we don't need to require lastResultsHash on nodes that enabled giga. patch on giga-enabled RPC nodes
3 participants
Describe your changes and provide context
Giga executors may yield different gas usage than v2 executors due to:
Since we only care about state consistency, which is covered by AppHash, we don't need to require lastResultsHash on nodes that enabled giga.
Testing performed to validate your change
patch on giga-enabled RPC nodes