Add maxBatchInterval to skip some empty batches #1599
Conversation
WalkthroughThe changes primarily introduce a new feature to control the batch processing interval in the system. A new parameter Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files ignored due to filter (3)
- go/common/rpc/generated/enclave.pb.go
- go/common/rpc/generated/enclave.proto
- go/common/rpc/generated/enclave_grpc.pb.go
Files selected for processing (21)
- .github/workflows/manual-deploy-testnet-l2.yml (2 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (2 hunks)
- go/common/enclave.go (1 hunks)
- go/config/host_config.go (4 hunks)
- go/enclave/components/batch_executor.go (4 hunks)
- go/enclave/components/interfaces.go (1 hunks)
- go/enclave/components/rollup_compression.go (1 hunks)
- go/enclave/enclave.go (2 hunks)
- go/enclave/nodetype/interfaces.go (1 hunks)
- go/enclave/nodetype/sequencer.go (7 hunks)
- go/enclave/rpc_server.go (1 hunks)
- go/host/container/cli.go (5 hunks)
- go/host/container/cli_flags.go (2 hunks)
- go/host/enclave/guardian.go (3 hunks)
- go/host/rpc/enclaverpc/enclave_client.go (1 hunks)
- go/node/cmd/cli.go (3 hunks)
- go/node/cmd/cli_flags.go (2 hunks)
- go/node/cmd/main.go (1 hunks)
- go/node/config.go (2 hunks)
- go/node/docker_node.go (1 hunks)
- testnet/launcher/docker.go (2 hunks)
Files skipped from review due to trivial changes (3)
- .github/workflows/manual-deploy-testnet-l2.yml
- go/host/container/cli_flags.go
- go/node/config.go
Additional comments (Suppressed): 39
go/common/enclave.go (1)
- 112-114: The function signature for
CreateBatchhas been changed to include askipIfEmptyboolean parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the new parameter is being used correctly within the function to control whether to skip batch creation if no transactions are ready to be included.go/enclave/enclave.go (2)
584-587: The function signature of
CreateBatchhas been changed to include a new boolean parameterskipBatchIfEmpty. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the new parameter is being used correctly within the function.593-593: The
CreateBatchmethod of theSequencerinterface has been updated to include the newskipBatchIfEmptyparameter. Ensure that the implementation of this method in theSequencerinterface has been updated to handle this new parameter.go/enclave/nodetype/interfaces.go (1)
- 24-27: The function signature of
CreateBatchhas been changed to include a new boolean parameterskipBatchIfEmpty. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the logic of the function has been updated to handle this new parameter correctly.go/node/cmd/cli_flags.go (2)
28-34: The new flag
maxBatchIntervalFlaghas been added to the list of flags. Ensure that this flag is being used correctly in the codebase and that it is documented in the user-facing documentation.63-69: The description for the new
maxBatchIntervalFlagis clear and informative. It explains the purpose of the flag and how to format its value.go/node/cmd/main.go (1)
- 34-37: The new
MaxBatchIntervalconfiguration option has been added to thenodeCfgobject. Ensure that thecliConfig.maxBatchIntervalvalue is properly validated and sanitized before being passed to theWithMaxBatchIntervalfunction. Also, verify that theWithMaxBatchIntervalfunction handles this new configuration option correctly.go/enclave/components/interfaces.go (1)
- 67-71: The function signature of
ComputeBatchhas been changed to include a new boolean parameterfailForEmptyBatch. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify the logic wherefailForEmptyBatchis used to skip batch production is working as expected.go/enclave/components/rollup_compression.go (1)
- 550-553: The
CreateBatchfunction call has been updated with a new boolean parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify the implications of thefalsevalue for theskipIfEmptyparameter in this context.go/host/rpc/enclaverpc/enclave_client.go (2)
402-411: The function signature of
CreateBatchhas been changed to include a new parameterskipIfEmpty. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that theskipIfEmptyflag is being used correctly in the context of the application logic.408-408: The
CreateBatchRequestnow includes aSkipIfEmptyfield. Ensure that the server-side implementation ofCreateBatchhas been updated to handle this new field correctly.go/node/cmd/cli.go (3)
42-48: The new
maxBatchIntervalfield has been added to theNodeConfigCLIstruct. This field represents the maximum interval between batches. The format is a string that can be parsed bytime.ParseDuration(), such as "500ms" or "2s".78-84: The
maxBatchIntervalflag has been added to the command-line interface. The default value is "1s". Ensure that the usage information for this flag has been added to theflagUsageMap.109-115: The parsed
maxBatchIntervalflag value is being assigned to themaxBatchIntervalfield in theNodeConfigCLIstruct. This is consistent with the handling of other flags..github/workflows/manual-upgrade-testnet-l2.yml (2)
66-69: The new environment variable
L2_MAX_BATCH_INTERVALis being echoed. Ensure that this variable is correctly set in the environment where this workflow is running.178-182: The
-max_batch_intervalflag is being passed to the upgrade command. Ensure that the command being run supports this new flag and behaves as expected when it is provided.go/enclave/rpc_server.go (1)
- 280-284: The function signature of
CreateBatchhas been changed to include a new parameterr *generated.CreateBatchRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, the error handling does not return the error to the caller. Consider returning the error to the caller after logging it.- sysError := s.enclave.CreateBatch(r.SkipIfEmpty) - if sysError != nil { - s.logger.Error("Error creating batch", log.ErrKey, sysError) - } + sysError := s.enclave.CreateBatch(r.SkipIfEmpty) + if sysError != nil { + s.logger.Error("Error creating batch", log.ErrKey, sysError) + return nil, sysError + }go/node/docker_node.go (1)
- 117-123: The new configuration option
maxBatchIntervalis being added to the command-line arguments for the Docker container. Ensure that the Docker container's entrypoint script or application can handle this new argument. Also, verify that themaxBatchIntervalvalue is being correctly set in thed.cfgstruct.go/host/container/cli.go (5)
47-50: The new
MaxBatchIntervalfield has been added to theHostConfigTomlstruct. This field represents the maximum interval between batches. Ensure that this new field is properly documented and its usage is clear to other developers.89-92: The
MaxBatchIntervalfield is now included in the command-line flags. This allows the maximum interval between batches to be set when starting the host. Ensure that themaxBatchIntervalNameflag is properly documented and its usage is clear to users.138-144: The
MaxBatchIntervalfield is parsed from the command-line flags. If the parsing fails, an error is returned. This is a good practice as it ensures that the program fails fast if the provided input is not valid.170-182: The
MaxBatchIntervalfield is parsed from the TOML configuration file. If the parsing fails, a default value of 1 second is used. This is a good practice as it provides a sensible default value if the configuration file does not specify a value forMaxBatchInterval.211-217: The
MaxBatchIntervalfield is included in theHostInputConfigobject that is returned by thefileBasedConfig()function. This ensures that the maximum interval between batches is included in the configuration that is used by the host.go/host/enclave/guardian.go (3)
68-91: The
Guardianstruct has been updated to include two new fields:maxBatchIntervalandlastBatchCreated. ThemaxBatchIntervalis set from theHostConfigand represents the maximum interval between batches. ThelastBatchCreatedis a timestamp that records the last time a batch was created. These fields are used to control the skipping of empty batches. TheNewGuardianfunction has been updated to initialize these new fields.511-520: The
periodicBatchProductionfunction has been updated to include logic for skipping the creation of empty batches. IfmaxBatchIntervalis greater thanbatchIntervaland the elapsed time since the last batch creation is less thanmaxBatchInterval, theCreateBatchfunction is called withskipBatchIfEmptyset totrue. This will skip the creation of a batch if there are no transactions ready to be included. Ensure that theCreateBatchfunction in theEnclaveClientinterface has been updated to handle this new parameter correctly.614-620: The
lastBatchCreatedtimestamp is updated whenever a new batch is produced. This is used to track the time since the last batch was created, which is used in the logic for skipping empty batches. This change is consistent with the goal of the PR to optimize batch creation.go/config/host_config.go (4)
84-93: The new
MaxBatchIntervalfield is introduced in theHostConfigstruct. This field represents the maximum interval between batches. If this interval is greater than the regularBatchInterval, the host will not create empty batches until theMaxBatchIntervalis reached or a transaction is received. This change is in line with the PR's goal of optimizing batch creation by skipping the creation of empty batches.138-138: The
MaxBatchIntervalfield is correctly added to theToHostConfig()method, ensuring that this new configuration option is correctly converted fromHostInputConfigtoHostConfig.166-168: The
MaxBatchIntervalfield is added to theNodeConfigCLIstruct. This field is used to set the maximum interval between batches from the command line. The comment explains its purpose well.267-268: The
MaxBatchIntervalfield is correctly set in theDefaultHostParsedConfig()function. This ensures that a default value is provided if it is not specified in the configuration file or command line.However, there seems to be a minor formatting issue on line 267 where two fields are set on the same line. This could be a typo or a formatting error. It's recommended to keep each field on a separate line for better readability.
- DebugNamespaceEnabled: false, BatchInterval: 1 * time.Second, + DebugNamespaceEnabled: false, + BatchInterval: 1 * time.Second,go/enclave/nodetype/sequencer.go (6)
101-104: The function signature of
CreateBatchhas been changed to include a boolean parameterskipBatchIfEmpty. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.117-117: The
createNewHeadBatchfunction is now being called with an additional argumentskipBatchIfEmpty. Ensure that the function definition and all its calls have been updated accordingly.152-155: The function signature of
createNewHeadBatchhas been changed to include a boolean parameterskipBatchIfEmpty. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.189-196: The
produceBatchfunction is now being called with an additional argumentskipBatchIfEmpty. Ensure that the function definition and all its calls have been updated accordingly. Also, the error handling has been updated to log and return nil when there are no transactions to process, which is a good practice for skipping empty batches.212-218: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [205-218]
The function signature of
produceBatchhas been changed to include a boolean parameterfailForEmptyBatch. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- 326-329: The
produceBatchfunction is now being called with an additional argumentfalse. Ensure that the function definition and all its calls have been updated accordingly.testnet/launcher/docker.go (1)
- 109-109: Similar to the previous comment, the
MaxBatchIntervalis set to the same value asBatchInterval. This means that the system will not skip any empty batches, as the maximum interval between batches is the same as the regular interval. If the intention is to allow the system to skip empty batches,MaxBatchIntervalshould be set to a value greater thanBatchInterval.- node.WithMaxBatchInterval("1s"), + node.WithMaxBatchInterval("5s"),go/enclave/components/batch_executor.go (2)
133-133: The function signature of
ComputeBatchhas been changed to include a new boolean parameterfailForEmptyBatch. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.264-264: The
ComputeBatchmethod is called withfailForEmptyBatchset tofalse. This is consistent with the comment that this execution is not used when first producing a batch, and we never want to fail for empty batches.
testnet/launcher/docker.go
Outdated
| @@ -63,6 +63,7 @@ | |||
| node.WithLogLevel(4), | |||
| node.WithEdgelessDBImage("ghcr.io/edgelesssys/edgelessdb-sgx-4gb:v0.3.2"), // default edgeless db value | |||
| node.WithBatchInterval("1s"), | |||
| node.WithMaxBatchInterval("1s"), | |||
There was a problem hiding this comment.
The MaxBatchInterval is set to the same value as BatchInterval. This means that the system will not skip any empty batches, as the maximum interval between batches is the same as the regular interval. If the intention is to allow the system to skip empty batches, MaxBatchInterval should be set to a value greater than BatchInterval.
- node.WithMaxBatchInterval("1s"),
+ node.WithMaxBatchInterval("5s"),| } | ||
|
|
||
| executor.populateHeader(©Batch, allReceipts(txReceipts, ccReceipts)) | ||
| if failForEmptyBatch && | ||
| len(txReceipts) == 0 && | ||
| len(ccReceipts) == 0 && | ||
| len(transactionsToProcess) == 0 && | ||
| len(crossChainTransactions) == 0 && | ||
| len(messages) == 0 && | ||
| len(transfers) == 0 { | ||
| return nil, ErrNoTransactionsToProcess | ||
| } |
There was a problem hiding this comment.
The new code checks if the batch is empty when failForEmptyBatch is true and returns an error if so. This is a good practice as it prevents unnecessary computation on empty batches. However, consider moving this check to a separate function for better readability and maintainability.
+ func (executor *batchExecutor) isBatchEmpty(txReceipts []*types.Receipt, ccReceipts []*types.Receipt, transactionsToProcess []*types.Transaction, crossChainTransactions []*types.Transaction, messages []*types.CrossChainMessage, transfers []*types.CrossChainTransfer) bool {
+ return len(txReceipts) == 0 &&
+ len(ccReceipts) == 0 &&
+ len(transactionsToProcess) == 0 &&
+ len(crossChainTransactions) == 0 &&
+ len(messages) == 0 &&
+ len(transfers) == 0
+ }
+
executor.populateHeader(©Batch, allReceipts(txReceipts, ccReceipts))
- if failForEmptyBatch &&
- len(txReceipts) == 0 &&
- len(ccReceipts) == 0 &&
- len(transactionsToProcess) == 0 &&
- len(crossChainTransactions) == 0 &&
- len(messages) == 0 &&
- len(transfers) == 0 {
+ if failForEmptyBatch && executor.isBatchEmpty(txReceipts, ccReceipts, transactionsToProcess, crossChainTransactions, messages, transfers) {
return nil, ErrNoTransactionsToProcess
}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- go/enclave/components/batch_executor.go (4 hunks)
- go/node/config.go (2 hunks)
- testnet/launcher/docker.go (2 hunks)
Files skipped from review due to trivial changes (1)
- testnet/launcher/docker.go
Additional comments (Suppressed): 7
go/node/config.go (3)
50-53: The
maxBatchIntervalfield has been added to theConfigstruct. This field is used to set the maximum interval between batches. Ensure that this new field is properly used and validated throughout the codebase.59-66: The
maxBatchIntervalfield is initialized with a default value of "1s" in theNewNodeConfigfunction. This is consistent with the PR summary and the new field added to theConfigstruct.320-324: The
WithMaxBatchIntervalfunction has been added. This function returns anOptionthat sets themaxBatchIntervalfield of aConfigobject. Ensure that this function is used correctly where needed.go/enclave/components/batch_executor.go (4)
32-32: The error
ErrNoTransactionsToProcessis defined but not used anywhere in the old hunks. Ensure that it is used appropriately in the new hunks.133-133: The function signature of
ComputeBatchhas been changed to include a new boolean parameterfailForEmptyBatch. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.212-220: The new code introduces a condition to return an error when there are no transactions to process and
failForEmptyBatchis true. This is a significant change in behavior that could potentially cause issues if not handled properly. Ensure that all callers of this function are prepared to handle this new error condition.264-264: The
ComputeBatchfunction is called withfailForEmptyBatchset to false. This means that it will not fail even if there are no transactions to process. If this is the intended behavior, then it is fine. However, if there are cases where you would want this call to fail for empty batches, then this hard-coded value might be a problem.
| len(crossChainTransactions) == 0 && | ||
| len(messages) == 0 && | ||
| len(transfers) == 0 { | ||
| return nil, ErrNoTransactionsToProcess |
There was a problem hiding this comment.
The AI bunny is right to be concerned I think. I don't like adding the lint ignore above and this approach does feel messy. But by bailing out within this method it's easy to be sure there was nothing important going to be in the batch and it also avoid committing anything to the stateDB cache.
Any ideas for how to do this more cleanly?
| @@ -62,8 +62,6 @@ func (t *Testnet) Start() error { | |||
| node.WithDebugNamespaceEnabled(true), | |||
| node.WithLogLevel(4), | |||
| node.WithEdgelessDBImage("ghcr.io/edgelesssys/edgelessdb-sgx-4gb:v0.3.2"), // default edgeless db value | |||
| node.WithBatchInterval("1s"), | |||
There was a problem hiding this comment.
These values are defaulted in the node config so this was redundant, (and the linter was getting upset at how much duplication there was in here).
| @@ -65,6 +65,7 @@ jobs: | |||
| echo "WORKER_ADDR=${{vars.WORKER_ADDR}}" | |||
|
|
|||
| echo "BATCH_INTERVAL=${{vars.BATCH_INTERVAL}}" | |||
| echo "L2_MAX_BATCH_INTERVAL=${{vars.L2_MAX_BATCH_INTERVAL}}" | |||
There was a problem hiding this comment.
This config key looks a bit different because it's using the new naming convention (got a PR in flight to update the rest).
| if _, err := s.produceBatch(sequencerNo.Add(sequencerNo, big.NewInt(1)), l1HeadBlock.Hash(), headBatch.Hash(), transactions, uint64(time.Now().Unix()), skipBatchIfEmpty); err != nil { | ||
| if errors.Is(err, components.ErrNoTransactionsToProcess) { | ||
| // skip batch production when there are no transactions to process | ||
| s.logger.Info("Skipping batch production, no transactions to execute") |
There was a problem hiding this comment.
we might want to eventually make this a DEBUG, as we'll see quite a lot of it
There was a problem hiding this comment.
Yeah 100%, it will be implied by the silence I guess. But yeah I'll leave it there for now just to make sure it's behaving as expected until we're confident.
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
Why this change is needed
Enclave database is getting clogged up and it's mostly full of empty batches. We want to buy some time by avoiding producing a batch unless a transaction is ready to include or unless some 'maxInterval' time has passed.
What changes were made as part of this PR
CreateBatch enclave method now takes a flag that tells it whether it can skip empty batches. The host sets that flag based on its config and on how long since the prev batch.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks