docs: ADR-010 Execution API - initial draft #1947
Conversation
WalkthroughThe pull request introduces a new Execution API for Rollkit, designed to enhance interoperability by removing ABCI-centric code and creating a generic interface for various execution environments. Key changes include an update to the Changes
Assessment against linked issues
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1947 +/- ##
==========================================
- Coverage 42.70% 42.61% -0.09%
==========================================
Files 79 79
Lines 12765 12765
==========================================
- Hits 5451 5440 -11
- Misses 6589 6599 +10
- Partials 725 726 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RollkitBot
requested review from
a team,
MSevey,
tuxcanfly and
yarikbratashchuk
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
specs/lazy-adr/adr-010-exec-api.md (4)
29-44: Consider defining specific error types for common failure scenarios.The interface is well-structured, but could benefit from defining specific error types for common failure scenarios (e.g., InvalidStateError, TimeoutError) to help implementers handle errors consistently.
Also, consider documenting recommended timeout values for the context parameter in each method's documentation.
134-137: Enhance error specification for SetFinal.The error output specification should detail specific error cases that implementers need to handle, such as:
- Block height not found
- Block already finalized
- Attempting to finalize future blocks
153-176: Enhance sequence diagram with error handling flows.The block production sequence diagram should include error handling paths, showing how the system behaves when:
- ExecuteTxs fails
- DA layer submission fails
- SetFinal fails
201-204: Consider documenting migration strategy.As Rollkit transitions from an ABCI client to an orchestrator, consider documenting:
- Step-by-step migration path for existing implementations
- Backward compatibility considerations
- Required changes for dependent projects
🧰 Tools
🪛 LanguageTool
[uncategorized] ~202-~202: The hyphen in fully-fledged is redundant.
Context: ...ed from rollkit. Rollkit will turn from fully-fledged ABCI client to kind of orchestrator, gl...(ADVERB_LY_HYPHEN_FIX)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.markdownlint.yaml(1 hunks)specs/lazy-adr/adr-010-exec-api.md(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / yamllint
.markdownlint.yaml
[failure] 7-7:
7:1 [empty-lines] too many blank lines (1 > 0)
🪛 GitHub Actions: CI and Release
.markdownlint.yaml
[error] 7-7: Too many blank lines (1 > 0)
🪛 LanguageTool
specs/lazy-adr/adr-010-exec-api.md
[uncategorized] ~16-~16: Possible missing article found.
Context: ...on. 2. Migrate to Engine API. 3. Create new API generic enough to handle any arbitr...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~202-~202: The hyphen in fully-fledged is redundant.
Context: ...ed from rollkit. Rollkit will turn from fully-fledged ABCI client to kind of orchestrator, gl...
(ADVERB_LY_HYPHEN_FIX)
[style] ~261-~261: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...execution environment. ### Neutral 1. Need to introduce new API exposed by rollkit. ...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
.markdownlint.yaml (1)
6-6: LGTM! Configuration update looks good.The change from
allow_different_nestingtosiblings_onlyfor MD024 rule is valid and will help maintain consistent header uniqueness within the same nesting level.specs/lazy-adr/adr-010-exec-api.md (1)
115-122: Clarify transaction ordering requirements.The expected behavior mentions "Respect the ordering of transactions" but doesn't specify if this is the exact order provided in the input slice or if there's flexibility for the execution environment to reorder transactions based on certain criteria (e.g., nonce ordering for account-based systems).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.markdownlint.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.markdownlint.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: lint / yamllint
.markdownlint.yaml
[failure] 6-6:
6:22 [new-line-at-end-of-file] no new line character at the end of file
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test CLI (windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
.markdownlint.yaml (1)
6-6: LGTM! The configuration change looks good.The change to
siblings_only: truefor the MD024 rule is appropriate for ADR documentation as it allows duplicate headings at different nesting levels while still preventing duplicate sibling headings.🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: lint / yamllint
[failure] 6-6:
6:22 [new-line-at-end-of-file] no new line character at the end of file
gupadhyaya
left a comment
gupadhyaya
left a comment
There was a problem hiding this comment.
i feel like maxBytes is confusing and may be we should get rid of it altogether and go with the assumption that:
- sequencer will make fast batches of smaller sizes such that the batch will likely never exceed the execution client's block gas limits.
i am recommending this because, maxBytes is kind of useless. what do you think?
|
The Another idea for consideration - return As a result rollkit would pass Yet another idea - remove explicit |
got another idea. how about,
|
| ##### Inputs | ||
|
|
||
| - `ctx` (`context.Context`): Context for managing request timeouts and cancellations. | ||
| - `txs` (`[]types.Tx`): A slice of transactions to be executed. |
There was a problem hiding this comment.
types.Tx is just []byte, as it's not interpreted by rollkit in any way. I'll include information about types in ADR.
There was a problem hiding this comment.
why not use []byte? what is the type preventing here?
There was a problem hiding this comment.
TBH there is no good reason for this. ADR was reflecting code. We should change this in docs and code. Especially after evstack/go-sequencing#35
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
makes sense, it seems there are a few assumptions being made on the execution environment. Like verifying txs needs to be handled in the state machine. it could be good to mention these allowing readers to understand what is needed by them
facundomedica
left a comment
facundomedica
left a comment
There was a problem hiding this comment.
A couple of questions:
- Why do we need to return maxBytes in InitChain and ExecTxs?
- I'm not sure what this means: "More difficult deployment (another binary is needed)."
Max bytes should be defined in genesis file, and possibly can be changed during execution. So the max bytes is inherently defined by execution client, but sequencer also needs to know this, so it can build batches according to the limit.
Before ADR-010 introduction, rollkit and app can be in the same binary, and this is kind of "default" option. With separate executor, node operators always have to run separate node for rollkit, and separate for execution client. |
tzdybal
force-pushed
the
tzdybal/adr-010
branch
from
5ac7d33 to
3aca8c2
Compare
tzdybal
requested review from
facundomedica,
gupadhyaya,
tac0turtle and
yarikbratashchuk
Included explanations for new abstract types `types.Hash` and `types.Tx` in the Execution API.
tzdybal
force-pushed
the
tzdybal/adr-010
branch
from
cfbdaa9 to
163eed1
Compare
|
just referencing here: evstack/go-execution-evm#25, we may need further modification to the a couple of apis. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
specs/lazy-adr/adr-010-exec-api.md (4)
64-64: Clarify maxBytes behavior across methods.The
maxBytesparameter appears in bothInitChainandExecuteTxsoutputs. Based on the PR objectives, there's confusion about its purpose and behavior. Consider adding explicit documentation about:
- When and why
maxBytesmight change during execution- How the sequencer should handle changes to
maxBytes- Whether the sequencer must respect this limit strictly
Also applies to: 112-113
145-147: Enhance error handling documentation.While the general notes mention "robust error handling practices", consider adding:
- Specific error types or codes for common failure scenarios
- Error recovery strategies
- Guidelines for error propagation
- Examples of meaningful error messages
162-185: Document error paths in sequence diagrams.The sequence diagrams effectively show the happy path but should also illustrate:
- Error handling flows
- Retry mechanisms
- Timeout scenarios
- Recovery procedures
Also applies to: 189-206
219-238: Document migration strategy for existing users.The package changes are significant. Consider adding:
- Migration guide for existing ABCI implementations
- Backward compatibility strategy
- Deprecation timeline for removed functionality
- Example upgrade paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.markdownlint.yaml(1 hunks)specs/lazy-adr/adr-010-exec-api.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .markdownlint.yaml
🧰 Additional context used
🪛 LanguageTool
specs/lazy-adr/adr-010-exec-api.md
[uncategorized] ~16-~16: Possible missing article found.
Context: ...on. 2. Migrate to Engine API. 3. Create new API generic enough to handle any arbitr...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~211-~211: The hyphen in fully-fledged is redundant.
Context: ...ed from rollkit. Rollkit will turn from fully-fledged ABCI client to kind of orchestrator, gl...
(ADVERB_LY_HYPHEN_FIX)
[uncategorized] ~217-~217: Possible missing comma found.
Context: ...ollkit enables further refactoring, for example removing all CometBFT dependencies. ##...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~245-~245: Possible missing article found.
Context: ...sts should be introduced to ensure that entire flow (from InitChain to SetFinal) w...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~270-~270: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...execution environment. ### Neutral 1. Need to introduce new API exposed by rollkit. ...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test CLI (windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
specs/lazy-adr/adr-010-exec-api.md (1)
241-246: Expand testing strategy section.The testing section should include:
- Specific test scenarios for each API method
- Performance testing requirements
- Compatibility testing with different execution environments
- Error scenario testing
🧰 Tools
🪛 LanguageTool
[uncategorized] ~245-~245: Possible missing article found.
Context: ...sts should be introduced to ensure that entire flow (fromInitChaintoSetFinal) w...(AI_HYDRA_LEO_MISSING_THE)
| ExecuteTxs(ctx context.Context, txs []types.Tx, blockHeight uint64, timestamp time.Time, prevStateRoot types.Hash) (updatedStateRoot types.Hash, maxBytes uint64, err error) | ||
|
|
||
| // SetFinal marks a block at the given height as final. | ||
| SetFinal(ctx context.Context, blockHeight uint64) error |
There was a problem hiding this comment.
how come we cant assume execution is this step? is there a scenario in which an app would not set final after executetxs?
There was a problem hiding this comment.
if VerifyBatch call from rollkit/app to sequencer fails then the header producer may decide to not call setFinal. This is not implemented yet. Idea is to continue execution as soon as sequencer batch is available and not wait for the sequencer hard confirmation of the batch (GetNextBatch is only the soft confirmation from the sequencer).
| ##### Outputs | ||
|
|
||
| - `stateRoot` (`types.Hash`): The resulting state root after initializing the chain. | ||
| - `maxBytes` (`uint64`): Maximum block size in bytes, as defined by the execution client's genesis configuration. |
There was a problem hiding this comment.
why does this need to be passed back?
There was a problem hiding this comment.
To separate concerns - maxBytes as a genesis parameter is strictly related to execution layer. Sequencer needs to know maxBytes, but we don't want to parse execution layer genesis file in sequencer, and keeping this in API makes it more error prone (no need to match sequencer config with param in genesis).
|
|
||
| ##### Inputs | ||
|
|
||
| - `ctx` (`context.Context`): Context for managing request timeouts and cancellations. |
There was a problem hiding this comment.
where is the block created and how does the app know how many txs to return? does it return the whole mempool?
There was a problem hiding this comment.
Entire mempool contents should be returned. Sequencer should peek the best transactions that fits in single block (maxBytes) and send them back. Transactions included in block must be removed from mempool.
| ##### Outputs | ||
|
|
||
| - `updatedStateRoot` (`types.Hash`): The resulting state root after applying the transactions. | ||
| - `maxBytes` (`uint64`): Maximum block size in bytes, as allowed for the block being produced. |
There was a problem hiding this comment.
ditto, may be good to have a larger explainer as to why this is needed
There was a problem hiding this comment.
this is to update rollkit/sequencer if the maxbytes requirement has changed for the next block. execution layer may have congestion etc and needs to reduce the txs then it can be done via returning maxbytes for the next block.
|
as i look into the code i get more confused on this api. i think it may need to be redesigned or the adr isnt explaining things deeply enough |
Overview
Resolves #1931.
Summary by CodeRabbit
Documentation
New Features