Conversation
❌ 9 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
be02672 to
b2f5213
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4386 +/- ##
==========================================
- Coverage 34.55% 34.28% -0.27%
==========================================
Files 497 497
Lines 58985 59016 +31
==========================================
- Hits 20381 20233 -148
- Misses 34998 35214 +216
+ Partials 3606 3569 -37 |
Conversily enable consensus only mode in nitro Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
e7dacb1 to
da045ab
Compare
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Enables running Nitro in “execution-only” or “consensus-only” mode (splitting normally co-located components) and adds configuration/validation to support RPC-based separation.
Changes:
- Add execution/consensus enablement gating in
cmd/nitro/nitro.goto allow running only one component in-process. - Tighten config validation for RPC-mode requirements and add
--chain.genesis-block-num. - Bump
nitro-testnodesubmodule and document the change in the changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| nitro-testnode | Updates testnode submodule revision to pick up supporting changes. |
| execution_consensus/init.go | Allows consensus node to be absent (nil) when starting/stopping nodes. |
| cmd/nitro/nitro.go | Introduces execution/consensus-only gating; adjusts initialization paths accordingly. |
| cmd/nitro/config/config.go | Adds RPC-mode validation rules for mutually exclusive execution vs consensus. |
| cmd/conf/chain.go | Adds genesis-block-num field/flag to L2 chain config. |
| changelog/bragaigor-nit-4241.md | Documents the new mode(s) and new chain config flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
pmikolajczyk41
left a comment
There was a problem hiding this comment.
couldn't spot any obvious problems
| } | ||
|
|
||
| if execNode != nil { | ||
| if executionNodeEnabled && execNode != nil { |
There was a problem hiding this comment.
shouldn't/aren't these two checks redundant?
There was a problem hiding this comment.
who are we if we don't have redundancy xD jokes apart you're right, I'll remove executionNodeEnabled
diegoximenes
left a comment
There was a problem hiding this comment.
Not to do in this PR.
But at some point we can have less code blocks gated by if executionNode { for example, and have a single one.
This is decreasing code readability a lot.
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
agreed and created ticket for it |
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
| consensusNodeEnabled = nodeConfig.Node.RPCServer.Enable | ||
| } else { | ||
| // same process: no RPC communication | ||
| // TODO: There should be a validation that in this scenario no RPC URL is set. |
| if consensusNodeEnabled { | ||
| var chainConfig *params.ChainConfig | ||
| // TODO: First try to get chainConfig from consensusParsedInitMsg (needs https://github.com/OffchainLabs/nitro/pull/4395) | ||
| chainConfig, err = chaininfo.GetChainConfig(new(big.Int).SetUint64(nodeConfig.Chain.ID), nodeConfig.Chain.Name, nil, nodeConfig.Chain.InfoFiles, nodeConfig.Chain.InfoJson) |
There was a problem hiding this comment.
There is this code block before:
chainInfo, err := chaininfo.ProcessChainInfo(nodeConfig.Chain.ID, nodeConfig.Chain.Name, nodeConfig.Chain.InfoFiles, nodeConfig.Chain.InfoJson)
if err != nil {
log.Error("error processing l2 chain info", "err", err)
return 1
}
This chainInfo is only retrieved in order to read the inner chain config.
Change this code block to use chaininfo.GetChainConfig, and remove this chaininfo.GetChainConfig from inside this if.
| defer valNode.Stop() | ||
| } | ||
| } | ||
| if err == nil { |
There was a problem hiding this comment.
Don't need to change this line
Enable execution only mode in nitro as well as consensus only mode
closes NIT-4241
relates to NIT-4202
pulls in OffchainLabs/nitro-testnode#177