-
Notifications
You must be signed in to change notification settings - Fork 688
Implement Execution/Consensus interface over RPC #3617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 46 commits
4f132e9
969a1b6
8219a5e
2a18ad2
eaad44a
901dbea
18097ee
f54a0f0
eeb55d3
f35e01b
acb0165
baca94a
61b8b17
55e3d05
7c9c449
e9b35e3
6894bbe
b71389d
f793c62
c65f6bf
25c70bd
6dac9b1
130a5ab
c2f6ebd
94aa08c
ac38683
75f885c
1f8c7c2
ffe9416
04b166e
5201dda
61a4017
2df87bc
cb34614
3b5fa3b
0acc651
f8b8f60
b71cb29
72458ac
4501faf
cd5bd77
3d7a2c5
6ce9b5b
cce9ac7
ceef817
ac94517
144c44e
c556785
310a61a
667db30
3f1cdd1
b584553
690b8e7
219dd11
04cdeb3
20e5888
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,13 +33,15 @@ import ( | |
| "github.com/offchainlabs/nitro/broadcastclients" | ||
| "github.com/offchainlabs/nitro/broadcaster" | ||
| "github.com/offchainlabs/nitro/cmd/chaininfo" | ||
| "github.com/offchainlabs/nitro/consensus" | ||
| "github.com/offchainlabs/nitro/consensus/rpcserver" | ||
ganeshvanahalli marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| "github.com/offchainlabs/nitro/daprovider" | ||
| daconfig "github.com/offchainlabs/nitro/daprovider/config" | ||
| "github.com/offchainlabs/nitro/daprovider/daclient" | ||
| "github.com/offchainlabs/nitro/daprovider/das" | ||
| "github.com/offchainlabs/nitro/daprovider/data_streaming" | ||
| "github.com/offchainlabs/nitro/execution" | ||
| "github.com/offchainlabs/nitro/execution/gethexec" | ||
| "github.com/offchainlabs/nitro/execution/rpcclient" | ||
| "github.com/offchainlabs/nitro/solgen/go/bridgegen" | ||
| "github.com/offchainlabs/nitro/solgen/go/precompilesgen" | ||
| "github.com/offchainlabs/nitro/staker" | ||
|
|
@@ -52,6 +54,7 @@ import ( | |
| "github.com/offchainlabs/nitro/util/headerreader" | ||
| "github.com/offchainlabs/nitro/util/redisutil" | ||
| "github.com/offchainlabs/nitro/util/rpcclient" | ||
| "github.com/offchainlabs/nitro/util/rpcserver" | ||
| "github.com/offchainlabs/nitro/util/signature" | ||
| "github.com/offchainlabs/nitro/wsbroadcastserver" | ||
| ) | ||
|
|
@@ -77,6 +80,8 @@ type Config struct { | |
| ResourceMgmt resourcemanager.Config `koanf:"resource-mgmt" reload:"hot"` | ||
| BlockMetadataFetcher BlockMetadataFetcherConfig `koanf:"block-metadata-fetcher" reload:"hot"` | ||
| ConsensusExecutionSyncer ConsensusExecutionSyncerConfig `koanf:"consensus-execution-syncer"` | ||
| RPCServer rpcserver.Config `koanf:"rpc-server"` | ||
| ExecutionRPCClient rpcclient.ClientConfig `koanf:"execution-rpc-client" reload:"hot"` | ||
| // SnapSyncConfig is only used for testing purposes, these should not be configured in production. | ||
| SnapSyncTest SnapSyncConfig | ||
| } | ||
|
|
@@ -119,6 +124,9 @@ func (c *Config) Validate() error { | |
| if c.TransactionStreamer.TrackBlockMetadataFrom != 0 && !c.BlockMetadataFetcher.Enable { | ||
| log.Warn("track-block-metadata-from is set but blockMetadata fetcher is not enabled") | ||
| } | ||
| if err := c.ExecutionRPCClient.Validate(); err != nil { | ||
ganeshvanahalli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return fmt.Errorf("error validating ExecutionRPCClient config: %w", err) | ||
| } | ||
| // Check that sync-interval is not more than msg-lag / 2 | ||
| if c.ConsensusExecutionSyncer.SyncInterval > c.SyncMonitor.MsgLag/2 { | ||
| log.Warn("consensus-execution-syncer.sync-interval is more than half of sync-monitor.msg-lag, which may cause sync issues", | ||
|
|
@@ -159,6 +167,8 @@ func ConfigAddOptions(prefix string, f *pflag.FlagSet, feedInputEnable bool, fee | |
| resourcemanager.ConfigAddOptions(prefix+".resource-mgmt", f) | ||
| BlockMetadataFetcherConfigAddOptions(prefix+".block-metadata-fetcher", f) | ||
| ConsensusExecutionSyncerConfigAddOptions(prefix+".consensus-execution-syncer", f) | ||
| rpcserver.ConfigAddOptions(prefix+".rpc-server", "consensus", f) | ||
| rpcclient.RPCClientAddOptions(prefix+".execution-rpc-client", f, &ConfigDefault.ExecutionRPCClient) | ||
| } | ||
|
|
||
| var ConfigDefault = Config{ | ||
|
|
@@ -183,6 +193,15 @@ var ConfigDefault = Config{ | |
| Maintenance: DefaultMaintenanceConfig, | ||
| ConsensusExecutionSyncer: DefaultConsensusExecutionSyncerConfig, | ||
| SnapSyncTest: DefaultSnapSyncConfig, | ||
| RPCServer: rpcserver.DefaultConfig, | ||
| ExecutionRPCClient: rpcclient.ClientConfig{ | ||
diegoximenes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| URL: "", | ||
| JWTSecret: "", | ||
| Retries: 3, | ||
| RetryErrors: "websocket: close.*|dial tcp .*|.*i/o timeout|.*connection reset by peer|.*connection refused", | ||
| ArgLogLimit: 2048, | ||
| WebsocketMessageSizeLimit: 256 * 1024 * 1024, | ||
| }, | ||
| } | ||
|
|
||
| func ConfigDefaultL1Test() *Config { | ||
|
|
@@ -526,6 +545,7 @@ func getBroadcastClients( | |
| func getBlockMetadataFetcher( | ||
| ctx context.Context, | ||
| configFetcher ConfigFetcher, | ||
| l2Config *params.ChainConfig, | ||
| arbDb ethdb.Database, | ||
| exec execution.ExecutionClient, | ||
| expectedChainId uint64, | ||
|
|
@@ -535,7 +555,7 @@ func getBlockMetadataFetcher( | |
| var blockMetadataFetcher *BlockMetadataFetcher | ||
| if config.BlockMetadataFetcher.Enable { | ||
| var err error | ||
| blockMetadataFetcher, err = NewBlockMetadataFetcher(ctx, config.BlockMetadataFetcher, arbDb, exec, config.TransactionStreamer.TrackBlockMetadataFrom, expectedChainId) | ||
| blockMetadataFetcher, err = NewBlockMetadataFetcher(ctx, config.BlockMetadataFetcher, arbDb, l2Config.ArbitrumChainParams.GenesisBlockNum, exec, config.TransactionStreamer.TrackBlockMetadataFrom, expectedChainId) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -1167,7 +1187,7 @@ func createNodeImpl( | |
| return nil, err | ||
| } | ||
|
|
||
| blockMetadataFetcher, err := getBlockMetadataFetcher(ctx, configFetcher, arbDb, executionClient, l2Config.ChainID.Uint64()) | ||
| blockMetadataFetcher, err := getBlockMetadataFetcher(ctx, configFetcher, l2Config, arbDb, executionClient, l2Config.ChainID.Uint64()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -1324,10 +1344,20 @@ func registerAPIs(currentNode *Node, stack *node.Node) { | |
| Public: false, | ||
| }) | ||
| } | ||
| config := currentNode.configFetcher.Get() | ||
| if config.RPCServer.Enable { | ||
| apis = append(apis, rpc.API{ | ||
| Namespace: consensus.RPCNamespace, | ||
| Version: "1.0", | ||
| Service: consensusrpcserver.NewConsensusRPCServer(currentNode), | ||
| Public: config.RPCServer.Public, | ||
| Authenticated: config.RPCServer.Authenticated, | ||
| }) | ||
| } | ||
| stack.RegisterAPIs(apis) | ||
| } | ||
|
|
||
| func CreateNodeExecutionClient( | ||
| func CreateConsensusNodeConnectedWithSimpleExecutionClient( | ||
| ctx context.Context, | ||
| stack *node.Node, | ||
| executionClient execution.ExecutionClient, | ||
|
|
@@ -1344,6 +1374,10 @@ func CreateNodeExecutionClient( | |
| blobReader daprovider.BlobReader, | ||
| latestWasmModuleRoot common.Hash, | ||
| ) (*Node, error) { | ||
| if configFetcher.Get().ExecutionRPCClient.URL != "" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL still being used here instead of the directConnect var.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. directConnect var doesnt apply here? because we aren't creating exec node here. This check is just for added safety
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CreateConsensusNodeConnectedWithSimpleExecutionClient should decide if RPC must be used or not through the useRPC var, in the same wayCreateConsensusNodeConnectedWithFullExecutionClient does. I don't understand what you mentioned with "This check is just for added safety"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if nodeConfig.ExecutionNode is false (which is when nitro.go calls this function), useRPC will be true so implying we will have to connect via URL anyway, so extra useRPC check is not needed? In which case one can choose to connect to external execution client via RPC or just not run with an execution client by keeping the URL empty
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CreateConsensusNodeConnectedWithSimpleExecutionClient is also used by system tests, that always create an ExecutionNode, and that can choose to use RPC or not. Relying on the URL here is correct 🙂 , but what I am advocating for is not about code correctness, is about improving code, and Nitro config, maintainability and readability. We can solely rely on URLs. We don't need the nodeConfig.ExecutionNode and nodeConfig.ConsensusExecutionInSameProcessUseRPC flags.
It is easier for node operators to understand if an Execution node will be run or not by checking a single config flag, i.e., nodeConfig.ExecutionNode, instead of reasoning about the 4 combinations in which (execution-rpc-server.URL, execution-rpc-client.URL) can be set/not set. We don't need the useRPC var. WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, fixed it |
||
| execConfigFetcher := func() *rpcclient.ClientConfig { return &configFetcher.Get().ExecutionRPCClient } | ||
| executionClient = executionrpcclient.NewExecutionRPCClient(execConfigFetcher, nil) | ||
ganeshvanahalli marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| if executionClient == nil { | ||
| return nil, errors.New("execution client must be non-nil") | ||
| } | ||
|
|
@@ -1355,13 +1389,10 @@ func CreateNodeExecutionClient( | |
| return currentNode, nil | ||
| } | ||
|
|
||
| func CreateNodeFullExecutionClient( | ||
| func CreateConsensusNodeConnectedWithFullExecutionClient( | ||
| ctx context.Context, | ||
| stack *node.Node, | ||
| executionClient execution.ExecutionClient, | ||
| executionSequencer execution.ExecutionSequencer, | ||
| executionRecorder execution.ExecutionRecorder, | ||
| arbOSVersionGetter execution.ArbOSVersionGetter, | ||
| fullExecutionClient execution.FullExecutionClient, | ||
| arbDb ethdb.Database, | ||
| configFetcher ConfigFetcher, | ||
| l2Config *params.ChainConfig, | ||
|
|
@@ -1375,10 +1406,17 @@ func CreateNodeFullExecutionClient( | |
| blobReader daprovider.BlobReader, | ||
| latestWasmModuleRoot common.Hash, | ||
| ) (*Node, error) { | ||
| if (executionClient == nil) || (executionSequencer == nil) || (executionRecorder == nil) || (arbOSVersionGetter == nil) { | ||
| return nil, errors.New("execution client, sequencer, recorder, and ArbOS version getter must be non-nil") | ||
| if fullExecutionClient == nil { | ||
| return nil, errors.New("full execution client must be non-nil") | ||
| } | ||
| var executionClient execution.ExecutionClient | ||
| if configFetcher.Get().ExecutionRPCClient.URL != "" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about being more explicit here, and relying on ExecutionNode and ConsensusExecutionUseRPC instead of ExecutionRPCClient.URL?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In cmd/nitro.go, nodeConfig.ExecutionNode is being used to decide if the gethexec.ExecutionNode will be created or not. That is why I proposed to have a unified way to verify if the RPC client should be used, that takes into consideration nodeConfig.ConsensusExecutionUseRPC and nodeConfig.ExecutionNode, possibly through a function, or creating a variable and passing it around, instead of relying on RPC URLs. Also, the ConsensusExecutionUseRPC name could indicate more clearly that is only used when NodeConfig.ExecutionNode is set to true, that is why I suggested to rename to something like ConsensusExecutionUseRPCWhenBothAreRunInTheSameProcess. WDYT? |
||
| execConfigFetcher := func() *rpcclient.ClientConfig { return &configFetcher.Get().ExecutionRPCClient } | ||
| executionClient = executionrpcclient.NewExecutionRPCClient(execConfigFetcher, nil) | ||
ganeshvanahalli marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } else { | ||
| executionClient = fullExecutionClient | ||
| } | ||
| currentNode, err := createNodeImpl(ctx, stack, executionClient, executionSequencer, executionRecorder, arbOSVersionGetter, arbDb, configFetcher, l2Config, l1client, deployInfo, txOptsValidator, txOptsBatchPoster, dataSigner, fatalErrChan, parentChainID, blobReader, latestWasmModuleRoot) | ||
| currentNode, err := createNodeImpl(ctx, stack, executionClient, fullExecutionClient, fullExecutionClient, fullExecutionClient, arbDb, configFetcher, l2Config, l1client, deployInfo, txOptsValidator, txOptsBatchPoster, dataSigner, fatalErrChan, parentChainID, blobReader, latestWasmModuleRoot) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -1387,27 +1425,12 @@ func CreateNodeFullExecutionClient( | |
| } | ||
|
|
||
| func (n *Node) Start(ctx context.Context) error { | ||
| execClient, ok := n.ExecutionClient.(*gethexec.ExecutionNode) | ||
| if !ok { | ||
| execClient = nil | ||
| } | ||
| if execClient != nil { | ||
| err := execClient.Initialize(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("error initializing exec client: %w", err) | ||
| var err error | ||
| if execRPCClient, ok := n.ExecutionClient.(*executionrpcclient.ExecutionRPCClient); ok { | ||
| if err = execRPCClient.Start(ctx); err != nil { | ||
| return fmt.Errorf("error starting exec rpc client: %w", err) | ||
| } | ||
| } | ||
| err := n.Stack.Start() | ||
| if err != nil { | ||
| return fmt.Errorf("error starting geth stack: %w", err) | ||
| } | ||
| if execClient != nil { | ||
| execClient.SetConsensusClient(n) | ||
| } | ||
| err = n.ExecutionClient.Start(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("error starting exec client: %w", err) | ||
| } | ||
| if n.BlobReader != nil { | ||
| err = n.BlobReader.Initialize(ctx) | ||
| if err != nil { | ||
|
|
@@ -1516,7 +1539,10 @@ func (n *Node) Start(ctx context.Context) error { | |
| }() | ||
| } | ||
| if n.blockMetadataFetcher != nil { | ||
| n.blockMetadataFetcher.Start(ctx) | ||
| err = n.blockMetadataFetcher.Start(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("error starting block metadata fetcher: %w", err) | ||
| } | ||
| } | ||
| if n.configFetcher != nil { | ||
| n.configFetcher.Start(ctx) | ||
|
|
@@ -1594,16 +1620,15 @@ func (n *Node) StopAndWait() { | |
| n.providerServerCloseFn() | ||
| } | ||
| if n.ExecutionClient != nil { | ||
| n.ExecutionClient.StopAndWait() | ||
| } | ||
| if err := n.Stack.Close(); err != nil { | ||
| log.Error("error on stack close", "err", err) | ||
| if _, ok := n.ExecutionClient.(*executionrpcclient.ExecutionRPCClient); ok { | ||
| n.ExecutionClient.StopAndWait() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func (n *Node) FindInboxBatchContainingMessage(message arbutil.MessageIndex) containers.PromiseInterface[execution.InboxBatch] { | ||
| func (n *Node) FindInboxBatchContainingMessage(message arbutil.MessageIndex) containers.PromiseInterface[consensus.InboxBatch] { | ||
| batchNum, found, err := n.InboxTracker.FindInboxBatchContainingMessage(message) | ||
| inboxBatch := execution.InboxBatch{ | ||
| inboxBatch := consensus.InboxBatch{ | ||
| BatchNum: batchNum, | ||
| Found: found, | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this step being run by CI?
Some possible strategies:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it used to run previously but this PR is pretty old and CI workflow was changed, I updated it to run along with defaults-A and defaults-B