feat(feynman): ensure that transition block timestamp is exact #1214
Conversation
WalkthroughThe comment on the Feynman transition storage slot in witness generation was clarified without code logic changes. A conditional timestamp override was added for the Feynman transition block in mining work preparation. The patch version constant was incremented from 58 to 59. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant ConsensusEngine
Worker->>Worker: newWork()
alt IsFeynmanTransitionBlock
Worker->>ConsensusEngine: Prepare(header, feynmanTimestamp)
else Not FeynmanTransitionBlock
Worker->>ConsensusEngine: Prepare(header, nil)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 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 (3)
✨ 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 (
|
| // Note: scroll-revm detects the Feynman transition block using this storage slot, | ||
| // since it does not have access to the parent block timestamp. We need to make | ||
| // sure that this is always present in the execution witness. | ||
| // Note: scroll-reth detects the Feynman transition block using the block | ||
| // timestamp, but since there might be multiple blocks with the same timestamp, | ||
| // it also needs this value to avoid applying the state update multiple times. | ||
| // So we need to make sure that this is always present in the execution witness. | ||
| statedb.GetState(rcfg.L1GasPriceOracleAddress, rcfg.IsFeynmanSlot) |
There was a problem hiding this comment.
You still include the witness for every block?
There was a problem hiding this comment.
See the explanation -- there might be multiple blocks with the same timestamp. Either we do this, or we make sure that the block after Feynman has timestamp at least Feynman + 1
There was a problem hiding this comment.
I'm not very familiar on the overhead this state read means for the prover, if it's minimal maybe we just leave has is and avoid over complicating things?
There was a problem hiding this comment.
it also needs this value to avoid applying the state update multiple times.
why set this can avoid the multiple applying, and what's the result after applying many times
There was a problem hiding this comment.
why set this can avoid the multiple applying, and what's the result after applying many times
I think it's because there's only one time that a block is in feynman time and IsFeynmanSlot is false, the later blocks with feynman time will have IsFeynmanSlot true.
|
Closing this in light of this discussion. |
1. Purpose or design rationale of this PR
Feynman requires us to apply a pre execution change at the top of the transition block, upgrading a predeploy contract.
Since we use timestamp-based forks, in Reth it's not possible to detect if the current block is the transition block, because the state transition function does not have access to the parent timestamp (see sub-optimal workaround in scroll-tech/reth@7a31b73).
As a workaround, we propose a way to ensure that the transition block uses the exact Feynman timestamp.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit