Fix build #1721
Fix build #1721
Conversation
|
Warning Rate Limit Exceeded@otherview has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 1 minutes and 12 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates reflect significant adjustments to gas limits for batch executions, local transactions, and smart contract deployments, suggesting optimizations for higher throughput or more complex operations. Port assignments for tests and services have been methodically increased, likely to avoid conflicts. A specific delay has been introduced in the sequencer logic, and additional checks for head state availability have been implemented. The integration and deployment scripts have been fine-tuned, with changes to test configurations and build retention policies. 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 (6)
- go/config/enclave_cli_flags.go (1 hunks)
- go/enclave/crosschain/message_bus_manager.go (2 hunks)
- integration/common/constants.go (1 hunks)
- integration/constants.go (2 hunks)
- integration/simulation/simulation.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Additional comments: 9
integration/constants.go (2)
6-16: The changes to the port numbers are consistent and appear to be correctly incremented by 1000 to avoid conflicts or align with new strategies.
27-29: The updates to the port offset constants are consistent and seem to standardize the offset to 1000, which may simplify configuration or align with other system changes.
integration/common/constants.go (1)
- 86-87: The changes to the gas limit constants from 3,000,000 to 5,000,000 are consistent with the PR's objectives to increase computational resources.
go/config/enclave_cli_flags.go (2)
58-58: The increase of
GasBatchExecutionLimitfrom 3,000,000 to 5,000,000 aligns with the PR's objectives to handle more resource-intensive operations.65-65: The increase of
GasLocalExecutionCapFlagfrom 3,000,000 to 5,000,000 aligns with the PR's objectives to handle more resource-intensive operations.tools/hardhatdeployer/contract_deployer.go (1)
- 95-95: The update of the
Gasfield within thedeployContractTxstruct from 2,000,000 to 4,000,000 is consistent with the PR's objectives to accommodate more complex or resource-intensive contracts.go/enclave/crosschain/message_bus_manager.go (2)
89-89: The increase of the gas limit in
GenerateMessageBusDeployTxfrom 2,000,000 to 4,000,000 aligns with the PR's objectives to handle more resource-intensive operations.222-222: The increase of the gas limit in
CreateSyntheticTransactionsfrom 2,000,000 to 4,900,000 aligns with the PR's objectives to handle more resource-intensive operations.integration/simulation/simulation.go (1)
- 230-230: The update of the
Gasfield within thedeployContractTxfrom 2,000,000 to 4,000,000 is consistent with the PR's objectives to accommodate more complex or resource-intensive contracts.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/simulation/simulation.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- integration/simulation/simulation.go
- tools/hardhatdeployer/contract_deployer.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/config/enclave_cli_flags.go (1 hunks)
- go/enclave/crosschain/message_bus_manager.go (2 hunks)
- integration/common/constants.go (1 hunks)
- integration/simulation/simulation.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- go/config/enclave_cli_flags.go
- go/enclave/crosschain/message_bus_manager.go
- integration/common/constants.go
- integration/simulation/simulation.go
- tools/hardhatdeployer/contract_deployer.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (1 hunks)
- go/enclave/crosschain/message_bus_manager.go (2 hunks)
- integration/common/constants.go (1 hunks)
- integration/simulation/simulation.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review due to trivial changes (1)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts
Files skipped from review as they are similar to previous changes (3)
- go/enclave/crosschain/message_bus_manager.go
- integration/simulation/simulation.go
- tools/hardhatdeployer/contract_deployer.go
Additional comments: 3
integration/common/constants.go (1)
- 86-87: The increase in gas limits from
3_000_000to10_000_000forGasLocalExecutionCapFlagand to12_000_000forGasBatchExecutionLimitshould be validated to ensure that these new limits are in line with the system's requirements and do not introduce any unexpected behavior or resource constraints.go/config/enclave_cli_flags.go (2)
58-58: The update to
GasBatchExecutionLimitflag's default value from3_000_000to10_000_000is a significant change. Ensure that this new default is consistent with the system's expected behavior and that it has been thoroughly tested to handle the increased gas limit without issues.65-65: The update to
GasLocalExecutionCapFlagflag's default value from3_000_000to12_000_000is a significant change. Ensure that this new default is consistent with the system's expected behavior and that it has been thoroughly tested to handle the increased gas limit without issues.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (1 hunks)
- integration/common/constants.go (1 hunks)
- integration/eth2network/eth2_network.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts
Additional comments: 5
integration/common/constants.go (1)
- 86-87: The increase in gas limits for
GasLocalExecutionCapFlagandGasBatchExecutionLimitis substantial. Ensure that these new limits have been tested to not negatively impact the system's performance or cost.go/config/enclave_cli_flags.go (1)
- 58-65: The increase in gas limits for
GasBatchExecutionLimitandGasLocalExecutionCapFlagis substantial. Ensure that these new limits have been tested to not negatively impact the system's performance or cost.integration/eth2network/eth2_network.go (3)
22-22: The addition of the
gethcommonimport is noted. Ensure that this package is used effectively in the new methods introduced.211-213: The integration of
ensureNoDuplicatedNetworkin theStartmethod is correct and ensures that the network is not started if there are duplicated nodes running. This is a good practice for network integrity.516-518: The call to
prefundedBalancesActiveis correctly placed after the merge event to ensure that the prefunded accounts are active. This is a good addition for validating the network's initial state.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/common/constants.go (1 hunks)
- integration/common/utils.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/common/constants.go
Additional comments: 1
integration/common/utils.go (1)
- 102-102: The gas value for transactions in
PrefundWalletshas been reduced from1,000,000to100,000. Ensure this change does not lead to failed transactions due to out-of-gas errors, especially if the transactions involve complex operations that require more gas.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/nodetype/sequencer.go (1 hunks)
- integration/eth2network/eth2_network.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/enclave/nodetype/sequencer.go
- integration/eth2network/eth2_network.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/build-pr.yml (1 hunks)
- go/enclave/enclave.go (1 hunks)
- integration/smartcontract/smartcontracts_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/build-pr.yml
Additional comments: 3
integration/smartcontract/smartcontracts_test.go (2)
41-41: The change of
TestTypefrom "noderunner" to "smartcontracts" should be consistent with the test suite's configuration. Confirm that this change aligns with the intended test categorization and that any dependent configurations or documentation have been updated accordingly.38-44: The modification in the
initfunction appears to be straightforward and isolated to the test configuration. If the change ofTestTypehas been verified and is correct, the rest of the hunk looks good.go/enclave/enclave.go (1)
- 688-693: The changes in the
GetTransactionCountmethod introduce a conditional check for an uninitialized head state, returning zero in such a case. While this may be intended for test scenarios with race conditions, it's important to ensure that this behavior is acceptable in production or if it should be guarded by a feature flag or environment check to prevent unintended side effects in a production environment.
otherview
force-pushed
the
pedro/fix_build
branch
from
51a8fdc to
3bde2ac
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/build-pr.yml (1 hunks)
- go/enclave/enclave.go (1 hunks)
- integration/smartcontract/smartcontracts_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/build-pr.yml
- go/enclave/enclave.go
- integration/smartcontract/smartcontracts_test.go
|
@CodeRabbit pause |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/network/socket.go (3 hunks)
Additional comments: 3
integration/simulation/network/socket.go (3)
4-8: The addition of imports "bufio," "os/exec," "regexp," and "strings" is noted. Ensure these are used within the file and are not extraneous to avoid unnecessary dependencies.
119-122: The error handling in node startup now includes a call to
checkProcessPort. Ensure that this function is robust and correctly interprets errors related to port issues.194-223: The new function
checkProcessPortuses system commands to check for port usage. Verify that this does not introduce security risks, such as command injection, and that it performs adequately without unnecessary resource usage.
otherview
force-pushed
the
pedro/fix_build
branch
from
0d39bae to
6f9c00e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/network/socket.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/simulation/network/socket.go
|
Stabilized Unit tests The red failure is a port re-use. |
| ObscuroGenesisFlag: flag.NewStringFlag(ObscuroGenesisFlag, "", "The json string with the obscuro genesis"), | ||
| L1ChainIDFlag: flag.NewInt64Flag(L1ChainIDFlag, 1337, "An integer representing the unique chain id of the Ethereum chain used as an L1 (default 1337)"), | ||
| ObscuroChainIDFlag: flag.NewInt64Flag(ObscuroChainIDFlag, 443, "An integer representing the unique chain id of the Obscuro chain (default 443)"), | ||
| UseInMemoryDBFlag: flag.NewBoolFlag(UseInMemoryDBFlag, true, "Whether the enclave will use an in-memory DB rather than persist data"), | ||
| ProfilerEnabledFlag: flag.NewBoolFlag(ProfilerEnabledFlag, false, "Runs a profiler instance (Defaults to false)"), | ||
| DebugNamespaceEnabledFlag: flag.NewBoolFlag(DebugNamespaceEnabledFlag, false, "Whether the debug namespace is enabled"), | ||
| GasLocalExecutionCapFlag: flag.NewUint64Flag(GasLocalExecutionCapFlag, 3_000_000, "Max gas usage when executing local transactions"), | ||
| GasLocalExecutionCapFlag: flag.NewUint64Flag(GasLocalExecutionCapFlag, 40_000_000, "Max gas usage when executing local transactions"), |
There was a problem hiding this comment.
Why is it now set to 40 M, while GasBatchExecutionLimit is 30M?
There was a problem hiding this comment.
I think we might have a bug somewhere, but it seems like if the GasLocalExecutionCapFlag <= GasBatchExecutionLimit we get more flaky errors, so I ended up bumping the local execution max gas to more than the batch size.
| StartPortFaucetHTTPUnitTest = 42000 | ||
| StartPortTenscanUnitTest = 43000 | ||
| StartPortTenGatewayUnitTest = 44000 | ||
| StartPortEth2NetworkTests = 10000 |
There was a problem hiding this comment.
What is the reason we need to change ports? Were there conflicts?
There was a problem hiding this comment.
Yes, that's right. There are still conflicts in ports, although less now. I've also added a log to tell us which process is occupying the conflicting port.
| @@ -227,7 +227,7 @@ func (s *Simulation) deployObscuroERC20s() { | |||
|
|
|||
| deployContractTx := types.DynamicFeeTx{ | |||
| Nonce: NextNonce(s.ctx, s.RPCHandles, owner), | |||
| Gas: 2_000_000, | |||
| Gas: 5_000_000, | |||
There was a problem hiding this comment.
What is the reason for increasing gas here, while decreasing it in func PrefundWallets?
Were previous estimates way off and we are adjusting to make them more optimised?
There was a problem hiding this comment.
Just making the gas limit equal everywhere pretty much.
Also, I'm not sure why we're using the DynamicFee tx in that particular tx and didn't want to over tweak it.
Why this change is needed
There are a bunch of latent issues that seem to creep up when the gas limits are added.
I have a few guesses on what might be happening, mainly relating with contract sizes being large, and blocks getting filled faster than the testing is used to. However I think this needs to be more carefully inspected.
This PR addresses the following:
Re-adds all reverted PRs:
Gaslimit + gascap flags #1682
Pedro/debug hh deployer #1710
Ziga/fix gateway spammy logs #1715
Adds missing log folders ( faucet, obscuroscan, tengateway, smartcontracts )
Fixes gas limits on calls on both go code and HardHat
Bump gas values to 30M ( Re-opened the ticket to reduce max batch size to 2.5M )
Bump ports - now have a 2k distance between them
Adds 1 sec delay between Genesis and Batch Protects against nil pointer in findRoundWinner. #2 ( producing 2 batches so close together was leading the synthetic contract deployment to be all screwy leading to complaints on the nonce and then to an unreliable batch state)
Added
ensureNoDuplicatedNetworkfor geth to ensure no lingering network is running when it shouldntAdded
prefundedBalancesActivemaking sure the prefunded balances are ok at the end of the eth2network start cycleWhat 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