Add New Node Flow fixes #1712
Add New Node Flow fixes #1712
Conversation
WalkthroughThe recent changes across various files indicate a focus on improving the infrastructure for smart contract deployment and testing. Key updates include increasing gas limits for contract interactions, refining VM creation and management on Azure, enhancing error handling in enclave logic, and ensuring unique network configurations in testing environments. Additionally, there's a pause introduced in batch processing and a shift towards more robust network simulations with updated port configurations and improved process checks. Changes
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? 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/manual-deploy-testnet-validator.yml (2 hunks)
Additional comments: 3
.github/workflows/manual-deploy-testnet-validator.yml (3)
169-169: The change to use
secrets.ADD_NEW_NODE_PRIVATE_KEYinstead of a direct input parameter for the private key is a significant security enhancement. Ensure that the secret is properly set in the GitHub repository and has the correct access permissions.23-28: Ensure that there are no leftover references to the removed
node_private_keyinput parameter elsewhere in the codebase.Verification successful
The search for
node_private_keyin YAML files did not yield any results, indicating that there are no leftover references to the removed input parameter in the GitHub Actions workflow or other YAML files in the codebase.* 23-28: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-28]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any leftover references to 'node_private_key' in the codebase. rg --type yaml 'node_private_key'Length of output: 33
Please ensure that the PR checklist from the repository's internal documentation is reviewed and marked as completed before merging this pull request.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/manual-deploy-testnet-validator.yml (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/manual-deploy-testnet-validator.yml
otherview
temporarily deployed
to
sepolia-testnet
GitHub Actions
Inactive
otherview
temporarily deployed
to
sepolia-testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (14)
- .github/workflows/build-pr.yml (1 hunks)
- .github/workflows/manual-deploy-testnet-validator.yml (4 hunks)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (1 hunks)
- go/enclave/enclave.go (1 hunks)
- go/enclave/nodetype/sequencer.go (1 hunks)
- integration/common/constants.go (1 hunks)
- integration/common/utils.go (1 hunks)
- integration/constants.go (2 hunks)
- integration/eth2network/eth2_network.go (4 hunks)
- integration/simulation/network/socket.go (3 hunks)
- integration/simulation/simulation.go (1 hunks)
- integration/smartcontract/smartcontracts_test.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/build-pr.yml
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts
- integration/simulation/simulation.go
- integration/smartcontract/smartcontracts_test.go
Additional comments: 15
integration/constants.go (1)
- 5-16: The changes to port allocation constants need to be cross-checked for conflicts and reflected in all relevant configuration files and documentation.
integration/common/constants.go (1)
- 86-87: The significant increase in gas limits for
GasBatchExecutionLimitandGasLocalExecutionCapFlagshould be justified with performance metrics or testing results to ensure it does not negatively impact system performance or transaction costs.integration/common/utils.go (1)
- 102-102: Lowering the gas limit in the
PrefundWalletsfunction could affect the success rate of transactions, especially if they involve complex contract interactions. Ensure this change is tested thoroughly to prevent transaction failures.go/config/enclave_cli_flags.go (2)
- 58-58: The increase in the default value for
GasBatchExecutionLimitshould be consistent with similar changes in other parts of the codebase. Ensure that all documentation and configuration files are updated accordingly.Verification successful
To ensure thorough verification, let's expand the search to include
.goand.tomlfiles, which are common for configuration and code comments in Go projects.
The search in
.goand.tomlfiles for references toGasBatchExecutionLimithas produced several results. Here are the findings:
- The default value of
GasBatchExecutionLimitis set to30_000_000inintegration/common/constants.go.- In
integration/simulation/network/network_utils.go, theGasBatchExecutionLimitis set to half ofparams.MaxGasLimit.- The
GasBatchExecutionLimitis used inintegration/simulation/devnetwork/node.gowith the value fromdefaultCfg.- The flag
GasBatchExecutionLimitis declared ingo/config/enclave_cli_flags.gowith the new default value of30_000_000.- The
GasBatchExecutionLimitis used ingo/enclave/enclave.gofor initializing aBatchExecutorand settingBatchGasLimit.- Tests in
go/config/enclave_config_test.goare setting and asserting theGasBatchExecutionLimitvalue.- The
GasBatchExecutionLimitis part of theenclaveConfigstruct ingo/config/enclave_config.go.Based on these results, it appears that the new default value for
GasBatchExecutionLimitis consistently used in the codebase, and the tests have been updated to reflect this change. There is no indication that documentation outside of the code (like markdown or text files) references this flag, which suggests that either the documentation does not exist or does not need to be updated.* 65-65: The increase in the default value for `GasLocalExecutionCapFlag` should be consistent with similar changes in other parts of the codebase. Ensure that all documentation and configuration files are updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the new default values are reflected in the documentation and example configuration files. rg --type md --type txt --type yaml --type json "GasBatchExecutionLimit"Length of output: 72
Script:
#!/bin/bash # Expand the search to include .go and .toml files for references to GasBatchExecutionLimit rg "GasBatchExecutionLimit" -g "*.go" -g "*.toml"Length of output: 1299
Verification successful
The search results indicate that the new default value for
GasLocalExecutionCapFlagof40_000_000is present in thego/config/enclave_cli_flags.gofile and is also reflected in theintegration/common/constants.gofile. Additionally, the flag is used in various places within the Go source files, such asgo/enclave/enclave.go,go/config/enclave_config.go, andintegration/simulation/.However, the old default value of
3_000_000did not appear in the search results, suggesting that it may not have been hardcoded or documented in a way that was captured by the search within Go source files.Given the information from the search results, it seems that the new default value is consistently used where expected in the Go source files. There is no evidence of the old value needing to be updated in these files.
Based on the provided context and the search results:
- The new default value of
40_000_000forGasLocalExecutionCapFlagis present in the Go source files.- There is no indication that the old default value of
3_000_000is present and needs updating within the Go source files.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the new default values are reflected in the documentation and example configuration files. rg --type md --type txt --type yaml --type json "GasLocalExecutionCapFlag"Length of output: 74
Script:
#!/bin/bash # Search for the old default value of GasLocalExecutionCapFlag to check if it needs to be updated in documentation and configuration files. rg --type md --type txt --type yaml --type json "3_000_000"Length of output: 59
Script:
#!/bin/bash # Search for the old and new default values of GasLocalExecutionCapFlag in Go source files and other configuration files not covered by previous searches. rg "GasLocalExecutionCapFlag" -tgo rg "3_000_000" -tgo rg "40_000_000" -tgoLength of output: 1268
tools/hardhatdeployer/contract_deployer.go (1)
- 95-95: The increase in the gas limit for contract deployment should be justified with reasons such as the need for deploying more complex contracts. Ensure this change is tested to prevent unexpected network congestion.
integration/simulation/network/socket.go (1)
- 4-8: The addition of new imports "bufio," "os/exec," "regexp," and "strings" is noted. Ensure these packages are used effectively within the file and there are no unused imports.
.github/workflows/manual-deploy-testnet-validator.yml (3)
- 26-27: The input parameter
node_private_keyhas been renamed tonode_id. Verify that all references to this parameter in the workflow and any associated scripts or documentation have been updated accordingly.Verification successful
: The renaming of
node_private_keytonode_idhas been verified within the workflow YAML files, shell scripts, and markdown files in the repository, with no occurrences ofnode_private_keyfound. It is recommended to manually verify the rest of the codebase to ensure that all references have been updated accordingly.* 104-137: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [107-167]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_private_key` has been updated to `node_id` in all relevant places. rg --type yaml 'node_private_key' .github/workflows/Length of output: 52
Script:
#!/bin/bash # Check for any occurrences of 'node_private_key' in shell scripts or documentation files. rg 'node_private_key' --type sh --type mdLength of output: 41
The removal of
node_l1_ws_url,HOC_ERC20_ADDR, andPOC_ERC20_ADDRneeds to be verified to ensure that they are not required anymore and that their removal does not affect the workflow negatively.Verification successful
- The parameters
HOC_ERC20_ADDRandPOC_ERC20_ADDRare still in use in themanual-deploy-testnet-l2.ymlworkflow file, indicating that their removal frommanual-deploy-testnet-validator.ymlis specific to that workflow and does not impact the other.- The parameter
node_l1_ws_urldoes not appear in the search results, suggesting it is not used in any other workflow files, which supports the decision to remove it frommanual-deploy-testnet-validator.yml.* 104-137: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [107-167]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the removed parameters are not used in the workflow or any related scripts. rg --type yaml 'node_l1_ws_url|HOC_ERC20_ADDR|POC_ERC20_ADDR' .github/workflows/Length of output: 324
The initialization script for VMs has been updated. Ensure that the new script correctly reflects the changes in input parameters and that the VMs are initialized as expected.
Verification successful
The output from the executed script shows the occurrences of
az vm create,az vm open-port, andaz vm run-command invokecommands in various GitHub Actions workflow files. The specific file in question,.github/workflows/manual-deploy-testnet-validator.yml, is listed with the changes as described in the PR summary.Based on the output, it appears that the workflow file has been updated to use the new input parameters (
node_id,testnet_type, etc.) and the Azure CLI commands have been adjusted accordingly. The script also shows similar patterns in other workflow files, which suggests a consistent approach to VM management across different workflows.Given the information provided, the changes in the
.github/workflows/manual-deploy-testnet-validator.ymlfile seem to be consistent with the PR's objective of refactoring the logic for creating and managing VMs on Azure. The use ofnode_idand other parameters in the Azure CLI commands aligns with the changes mentioned in the review comment.However, without the ability to execute the Azure CLI commands and observe the actual behavior of the VM creation and initialization, we cannot fully verify the correctness of the script's logic. The script's syntax appears correct, but the operational verification would require actual execution in the Azure environment.
Therefore, based on the code review and the output from the shell script:
- The renaming of
node_private_keytonode_idis reflected in the Azure CLI commands.- The removal of
node_l1_ws_url,HOC_ERC20_ADDR, andPOC_ERC20_ADDRis not directly verifiable from the output, but no references to these variables are present in the provided script sections.- The script for initializing the VMs has been adjusted to accommodate the updated input parameters.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the initialization script for VMs is updated correctly. rg --type yaml 'az vm create|az vm open-port|az vm run-command invoke' .github/workflows/Length of output: 3093
go/enclave/nodetype/sequencer.go (1)
- 166-166: The introduction of
time.Sleep(time.Second)may be intended for synchronization or delay, but it can lead to unpredictable behavior and performance issues. Verify the necessity of this sleep and consider alternative approaches if applicable.integration/eth2network/eth2_network.go (4)
22-22: The import
gethcommon "github.com/ethereum/go-ethereum/common"has been added. Ensure that it is used within the file and not unnecessarily imported, which could lead to bloated binary sizes.211-213: The method
ensureNoDuplicatedNetworkis correctly invoked at the start of theStartmethod to ensure that there are no active nodes before starting a new network. This is a good practice to avoid conflicts with existing processes.516-518: The method
prefundedBalancesActiveis invoked after the merge event to ensure that accounts are prefunded. This is a critical step for the network's operation, and the error handling with a clear message is a good practice.618-626: The
ensureNoDuplicatedNetworkmethod checks for active nodes on the WebSocket ports. The error handling is clear, but it's important to ensure that the method does not produce false positives by mistaking unrelated services running on the same ports for geth nodes.
| func (n *Impl) prefundedBalancesActive(client *ethclient.Client) error { | ||
| for _, addr := range n.preFundedMinerAddrs { | ||
| balance, err := client.BalanceAt(context.Background(), gethcommon.HexToAddress(addr), nil) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to check balance for account %s - %w", addr, err) | ||
| } | ||
| if balance.Cmp(gethcommon.Big0) == 0 { | ||
| return fmt.Errorf("unexpected %s balance for account %s", balance.String(), addr) | ||
| } | ||
| fmt.Printf("Account %s prefunded with %s\n", addr, balance.String()) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The prefundedBalancesActive method checks if the prefunded accounts have a balance greater than zero. It's important to ensure that the balance check is against the expected minimum balance rather than just zero to avoid potential issues with underfunded accounts.
if balance.Cmp(expectedMinimumBalance) < 0 {
return fmt.Errorf("insufficient balance for account %s: %s", addr, balance.String())
}
go/enclave/enclave.go
Outdated
| headBatch := e.registry.HeadBatchSeq() | ||
| if headBatch != nil { | ||
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | ||
| if err == nil { | ||
| // todo - we should return an error when head state is not available, but for current test situations with race | ||
| // conditions we allow it to return zero while head state is uninitialized | ||
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | ||
| if err != nil { | ||
| return nil, responses.ToInternalError(err) | ||
| } | ||
| nonce = s.GetNonce(address) |
There was a problem hiding this comment.
The refactoring of the logic for fetching the head batch sequence and creating a state DB includes a new conditional check for headBatch. This check ensures that the headBatch is not nil before proceeding to fetch the batch by sequence number and create the state DB. This is a good practice as it prevents potential nil dereferences.
However, the error handling inside the conditional block could be improved. The comment on line 692 suggests that there are known race conditions that are being temporarily tolerated by allowing a zero nonce to be returned when the head state is uninitialized. This could lead to unexpected behavior and should be addressed as soon as possible.
Additionally, the error from e.storage.FetchBatchBySeqNo(headBatch.Uint64()) is being swallowed if it is not nil. It would be better to handle this error explicitly, even if it's just logging it for debugging purposes.
l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64())
if err == nil {
// TODO: We should return an error when head state is not available, but for current test situations with race
// conditions we allow it to return zero while head state is uninitialized
s, err := e.storage.CreateStateDB(l2Head.Hash())
if err != nil {
return nil, responses.ToInternalError(err)
}
nonce = s.GetNonce(address)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| headBatch := e.registry.HeadBatchSeq() | |
| if headBatch != nil { | |
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | |
| if err == nil { | |
| // todo - we should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| return nil, responses.ToInternalError(err) | |
| } | |
| nonce = s.GetNonce(address) | |
| headBatch := e.registry.HeadBatchSeq() | |
| if headBatch != nil { | |
| l2Head, err := e.storage.FetchBatchBySeqNo(headBatch.Uint64()) | |
| if err != nil { | |
| // Handle the error appropriately | |
| return nil, responses.ToInternalError(err) | |
| } | |
| // TODO: We should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| return nil, responses.ToInternalError(err) | |
| } | |
| nonce = s.GetNonce(address) |
| // getProcessesUsingPort returns a slice of process details using the specified port. | ||
| func checkProcessPort(errPort string) error { | ||
| re := regexp.MustCompile(`:(\d+):`) | ||
| matches := re.FindStringSubmatch(errPort) | ||
|
|
||
| if len(matches) < 2 { | ||
| return fmt.Errorf("no port found in string") | ||
| } | ||
|
|
||
| port := matches[1] | ||
|
|
||
| cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port)) //nolint:gosec | ||
|
|
||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var processes []string | ||
| scanner := bufio.NewScanner(strings.NewReader(string(output))) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, "LISTEN") || strings.Contains(line, "ESTABLISHED") { | ||
| processes = append(processes, line) | ||
| } | ||
| } | ||
|
|
||
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The checkProcessPort function uses exec.Command to run lsof which can introduce security risks if not properly sanitized. Ensure that the port variable derived from errPort is validated against a strict pattern to prevent command injection.
- cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port)) //nolint:gosec
+ // Ensure `port` is strictly a number to prevent command injection
+ if !regexp.MustCompile(`^\d+$`).MatchString(port) {
+ return fmt.Errorf("invalid port format")
+ }
+ cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // getProcessesUsingPort returns a slice of process details using the specified port. | |
| func checkProcessPort(errPort string) error { | |
| re := regexp.MustCompile(`:(\d+):`) | |
| matches := re.FindStringSubmatch(errPort) | |
| if len(matches) < 2 { | |
| return fmt.Errorf("no port found in string") | |
| } | |
| port := matches[1] | |
| cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port)) //nolint:gosec | |
| output, err := cmd.Output() | |
| if err != nil { | |
| return err | |
| } | |
| var processes []string | |
| scanner := bufio.NewScanner(strings.NewReader(string(output))) | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| if strings.Contains(line, "LISTEN") || strings.Contains(line, "ESTABLISHED") { | |
| processes = append(processes, line) | |
| } | |
| } | |
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) | |
| return nil | |
| } | |
| // getProcessesUsingPort returns a slice of process details using the specified port. | |
| func checkProcessPort(errPort string) error { | |
| re := regexp.MustCompile(`:(\d+):`) | |
| matches := re.FindStringSubmatch(errPort) | |
| if len(matches) < 2 { | |
| return fmt.Errorf("no port found in string") | |
| } | |
| port := matches[1] | |
| // Ensure `port` is strictly a number to prevent command injection | |
| if !regexp.MustCompile(`^\d+$`).MatchString(port) { | |
| return fmt.Errorf("invalid port format") | |
| } | |
| cmd := exec.Command("lsof", "-i", fmt.Sprintf(":%s", port)) | |
| output, err := cmd.Output() | |
| if err != nil { | |
| return err | |
| } | |
| var processes []string | |
| scanner := bufio.NewScanner(strings.NewReader(string(output))) | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| if strings.Contains(line, "LISTEN") || strings.Contains(line, "ESTABLISHED") { | |
| processes = append(processes, line) | |
| } | |
| } | |
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) | |
| return nil | |
| } |
| if len(matches) < 2 { | ||
| return fmt.Errorf("no port found in string") | ||
| } |
There was a problem hiding this comment.
The error message "no port found in string" could be more descriptive by including the original string that failed to match the expected pattern.
- return fmt.Errorf("no port found in string")
+ return fmt.Errorf("no port found in error string: %s", errPort)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if len(matches) < 2 { | |
| return fmt.Errorf("no port found in string") | |
| } | |
| if len(matches) < 2 { | |
| return fmt.Errorf("no port found in error string: %s", errPort) | |
| } |
| } | ||
| } | ||
|
|
||
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) |
There was a problem hiding this comment.
Directly printing to stdout may not be ideal for a library function. Consider returning the information to the caller to handle the output more flexibly.
- fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes)
+ // Return the processes information instead of printing
+ return processes, nilCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| fmt.Printf("Found processes still opened on port %s - %+v\n", port, processes) | |
| // Return the processes information instead of printing | |
| return processes, nil |
integration/constants.go
Outdated
| StartPortEth2NetworkTests = 10000 | ||
| StartPortNodeRunnerTest = 14000 | ||
| StartPortSimulationGethInMem = 18000 | ||
| StartPortSimulationInMem = 22000 | ||
| StartPortSimulationFullNetwork = 26000 | ||
| StartPortSmartContractTests = 30000 | ||
| StartPortContractDeployerTest = 34000 | ||
| StartPortWalletExtensionUnitTest = 38000 | ||
| StartPortFaucetUnitTest = 42000 | ||
| StartPortFaucetHTTPUnitTest = 48000 | ||
| StartPortTenscanUnitTest = 52000 | ||
| StartPortTenGatewayUnitTest = 56000 |
There was a problem hiding this comment.
Consider adding a comment explaining the rationale for choosing these specific port numbers, as they are now in a range that might be more commonly used by other applications.
otherview
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
otherview
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
otherview
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
otherview
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/manual-deploy-testnet-validator.yml (6 hunks)
- .github/workflows/runner-scripts/testnet-clear-loadbalancer.sh (1 hunks)
- go/enclave/nodetype/sequencer.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/manual-deploy-testnet-validator.yml
- go/enclave/nodetype/sequencer.go
| #az network nic ip-config address-pool remove \ | ||
| # --address-pool ${pool} \ | ||
| # --ip-config-name "${ipconfig_id}" \ | ||
| # --nic-name "${nic_id}" \ | ||
| # --resource-group Testnet \ | ||
| # --lb-name ${lb} \ |
There was a problem hiding this comment.
The block of code that removes an address pool from the network interface configuration has been commented out. If this is intentional to prevent the removal for now, ensure that there is a tracking mechanism (like a TODO or a ticket) to revisit this if the functionality needs to be restored in the future.
| @@ -43,12 +43,12 @@ | |||
| exit 0 | |||
There was a problem hiding this comment.
Consider using a non-zero exit status when the script exits due to not finding the nic_id or ipconfig_id. Exiting with a status of 0 usually indicates success, which might not be appropriate in this context.
| # --ip-config-name "${ipconfig_id}" \ | ||
| # --nic-name "${nic_id}" \ | ||
| # --resource-group Testnet \ | ||
| # --lb-name ${lb} \ | ||
|
|
||
|
|
||
| echo 'Load balancer removed successfully' |
There was a problem hiding this comment.
The message 'Load balancer removed successfully' is misleading because the code that performs the removal is commented out. This message should be updated to reflect the current behavior of the script.
otherview
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
otherview
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/manual-deploy-testnet-validator.yml (7 hunks)
Additional comments: 6
.github/workflows/manual-deploy-testnet-validator.yml (6)
6-20: The README instructions have been updated to reflect the new workflow. Ensure that the documentation is consistent with the actual behavior of the workflow and that all necessary updates have been made.
41-42: The
node_idparameter has been introduced. Verify that all references to the oldnode_private_keyhave been updated to usenode_idwhere applicable.Verification successful
The search results indicate that
node_private_keyhas been removed andnode_idhas been implemented in the workflow. To ensure thatnode_idis being used correctly, I will now verify that it is being used in the same context asnode_private_keywas used before.
The provided context confirms that
node_idis used throughout the workflow for VM creation, configuration, and health checks, replacing the previousnode_private_key. There are no indications ofnode_private_keybeing required in the updated workflow. The changes seem to be correctly implemented.* 122-138: The `node_id` is used in the VM creation and port opening steps. Ensure that the `node_id` is used consistently and correctly in these contexts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_private_key` is no longer used and `node_id` is correctly implemented. rg --type yaml 'node_private_key' .github/workflows/ rg --type yaml 'node_id' .github/workflows/Length of output: 2335
Script:
#!/bin/bash # Verify the context in which `node_id` is used compared to the old `node_private_key`. rg --type yaml -C 5 'node_id' .github/workflows/manual-deploy-testnet-validator.ymlLength of output: 4519
Verification successful
The search results confirm that
node_idis used consistently within themanual-deploy-testnet-validator.ymlworkflow file. It is utilized in the naming of the Azure VM, the DNS name for the public IP address, the port opening command, the VM run command, and the script for checking node health. Additionally,node_idis also referenced in other workflow files, but in different contexts (pull request and issue IDs).Based on the provided output, there are no inconsistencies with the usage of
node_idin the context of VM setup within themanual-deploy-testnet-validator.ymlfile.* 170-182: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [145-179]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_id` is used consistently in VM setup. rg --type yaml 'node_id' .github/workflows/Length of output: 2282
The
node_idis used in theStart Obscuro nodestep. Verify that thenode_idis correctly referenced and that the corresponding secrets and inputs are correctly utilized.Verification successful
The
node_idis consistently used across the.github/workflows/manual-deploy-testnet-validator.ymlfile to reference the specific node in various operations such as VM creation, port opening, and node startup on Azure. The usage appears to be correct as per the output provided.* 213-219: The `node_id` is used in the update-loadbalancer job. Confirm that the `node_id` is correctly used in the context of updating the load balancer.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_id` and corresponding secrets and inputs are correctly referenced in the 'Start Obscuro node' step. rg --type yaml 'node_id' .github/workflows/Length of output: 2282
Verification successful
The occurrences of
node_idin the.github/workflows/manual-deploy-testnet-validator.ymlfile are used consistently across various steps such as Azure VM creation, DNS naming, port opening, VM command invocation, and load balancer update. The usage pattern suggests thatnode_idis a dynamic input used to uniquely identify resources related to a specific node within the workflow.* 231-234: The `node_id` is used in the health check script. Confirm that the `node_id` is correctly used and that the health check script is functioning as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_id` is correctly used in the context of updating the load balancer. rg --type yaml 'node_id' .github/workflows/Length of output: 2282
Verification successful
The
node_idis consistently used throughout the workflow for various tasks, including the health check script, which suggests that it is being used correctly. No issues are apparent from the provided context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `node_id` is correctly used in the health check script. rg --type yaml 'node_id' .github/workflows/Length of output: 2282
| required: true | ||
| default: 3 | ||
| type: number | ||
| node_private_key: | ||
| description: 'Node Private Key' | ||
| node_id: | ||
| description: 'Node id' | ||
| required: true | ||
| type: string | ||
| node_account_address: | ||
| description: 'Node Account Address' | ||
| required: true | ||
| type: string | ||
| node_l1_ws_url: | ||
| description: 'Node L1 Connection String' | ||
| required: true | ||
| type: string | ||
| MGMT_CONTRACT_ADDR: | ||
| description: 'Management Contract Addr' | ||
| required: true |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The removal of HOC_ERC20_ADDR and POC_ERC20_ADDR from manual-deploy-testnet-validator.yml is confirmed. However, these variables are still referenced in .github/workflows/manual-deploy-testnet-l2.yml. This could be an oversight or intentional, depending on whether these workflows are interconnected or independent.
HOC_ERC20_ADDRandPOC_ERC20_ADDRare still used in.github/workflows/manual-deploy-testnet-l2.yml.
Analysis chain
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [41-60]
The declarations for node_l1_ws_url, HOC_ERC20_ADDR, and POC_ERC20_ADDR have been removed. Confirm that there are no lingering references to these variables in the workflow.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the removed variables.
rg --type yaml 'node_l1_ws_url' .github/workflows/
rg --type yaml 'HOC_ERC20_ADDR' .github/workflows/
rg --type yaml 'POC_ERC20_ADDR' .github/workflows/
Length of output: 394
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/manual-deploy-testnet-validator.yml (7 hunks)
Additional comments: 5
.github/workflows/manual-deploy-testnet-validator.yml (5)
6-20: The README instructions added in the workflow file provide clarity on the steps required to deploy a Testnet validator. This is a good practice as it helps maintainers and contributors understand the deployment process.
41-44: The renaming of
node_private_keytonode_idshould be checked across the entire codebase to ensure consistency and avoid breaking changes.Verification successful
The search for references to
node_private_keyin the.github/workflows/directory did not yield any results. This suggests that the variablenode_private_keyhas been successfully renamed tonode_idin the workflow files, and there are no remaining references to the old variable name that could cause issues.* 173-173: The variable `node_l1_ws_url` has been removed from the declarations but is still being used in the script. Confirm that this is intentional and that the variable is set elsewhere or no longer needed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `node_private_key` variable. rg --type yaml 'node_private_key' .github/workflows/Length of output: 52
- 192-222: The load balancer configuration code is commented out. If this is intentional, ensure that it does not negatively impact the deployment process and that the load balancer is updated elsewhere if necessary.
Verification successful
The search results show that load balancer configurations are referenced in other workflow files, such as
manual-deploy-testnet-l2.yml, and in a script namedtestnet-clear-loadbalancer.sh. This suggests that the load balancer management is handled elsewhere in the workflows, and the commenting out of the load balancer code inmanual-deploy-testnet-validator.ymlis likely intentional.* 234-237: Adding a script to wait until the node is healthy is a good practice, as it likely provides a more reliable and dynamic way to ensure the node is ready before proceeding.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other scripts or workflows that handle load balancer configuration. rg --type shell 'loadbalancer' .github/workflows/Length of output: 79
Script:
#!/bin/bash # Correcting the file type and re-running the search for load balancer configurations in the workflows directory. rg 'loadbalancer' .github/workflows/Length of output: 1061
| - name: 'Allow time for VM initialization' | ||
| shell: bash | ||
| run: sleep 60 |
There was a problem hiding this comment.
Using sleep 60 for VM initialization could lead to flaky behavior if the VM takes longer to initialize. Consider implementing a health check loop that confirms the VM is ready.
- sleep 60
+ while ! nc -z $VM_IP $VM_PORT; do
+ sleep 1 # wait for 1 second before check again
+ doneCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - name: 'Allow time for VM initialization' | |
| shell: bash | |
| run: sleep 60 | |
| - name: 'Allow time for VM initialization' | |
| shell: bash | |
| run: | | |
| while ! nc -z $VM_IP $VM_PORT; do | |
| sleep 1 # wait for 1 second before check again | |
| done |
Good question, I'll add that comment to the code . The Loadbalancer shouldn't be updated because it will take a good while for the node to bootstrap and catchup on the L1 and the L2 state. |
Why this change is needed
Please provide a description and a link to the underlying ticket
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks