feat: add configurable signer latency correction #3317
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the ZetaChain's operational flags mechanism, specifically focusing on adding a configurable signer block time offset. The changes span multiple files across the project, introducing new functionality for managing the time offset between block generation and signing operations. The modifications enable more precise synchronization of cross-chain transactions by allowing dynamic configuration of the signing latency. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3317 +/- ##
===========================================
- Coverage 61.75% 61.68% -0.08%
===========================================
Files 440 440
Lines 31171 31198 +27
===========================================
- Hits 19251 19245 -6
- Misses 11057 11089 +32
- Partials 863 864 +1
|
gartnera
added
ADMIN_TESTS
gartnera
force-pushed
the
zetaclient-signer-latency-correction
branch
3 times, most recently
from
e62d1a1 to
8f718ec
Compare
gartnera
force-pushed
the
zetaclient-signer-latency-correction
branch
from
8f718ec to
8993c28
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
e2e/e2etests/test_operational_flags.go (1)
66-107: Time-based test stability and clarity.
While this test validates the signer offset correctly, consider mitigating potential flakiness by using a mock or stable time fixture. Time-based tests can sometimes behave unpredictably in slower or highly variable CI environments.docs/openapi/openapi.swagger.yaml (1)
58115-58117: Enhance property documentationThe description could be more specific about:
- The expected format of the duration value
- What
zetaclient_core_block_latencyrepresents- Example values for common scenarios
Apply this diff to improve the documentation:
description: |- Offset from the zetacore block time to initiate signing. - Should be calculated and set based on max(zetaclient_core_block_latency). + Should be calculated and set based on max(zetaclient_core_block_latency), + which represents the observed delay between zetaclient and core block processing. + + Format: A duration string (e.g. "10s", "1m") + Example values: + - Mainnet: "30s" (higher latency environment) + - Testnet: "10s" (lower latency environment)proto/zetachain/zetacore/observer/operational.proto (1)
15-18: Consider enhancing field documentationWhile the current documentation is good, consider adding:
- The expected range of values
- The impact on system behavior
- Example values for mainnet vs testnet
x/observer/types/operational.go (1)
22-24: Consider adding context to error messageThe error wrapping is good, but consider including the limit value in the error message for better debugging.
- return cosmoserrors.Wrapf(ErrOperationalFlagsSignerBlockTimeOffsetLimit, "(%s)", signerBlockTimeOffset) + return cosmoserrors.Wrapf(ErrOperationalFlagsSignerBlockTimeOffsetLimit, "(%s exceeds limit of %s)", signerBlockTimeOffset, signerBlockTimeOffsetLimit)x/observer/types/operational_test.go (1)
31-57: Add missing edge case testsConsider adding the following test cases:
- Zero duration for signer offset
- Exactly at the limit (10 seconds)
- Nil signer offset
x/observer/client/cli/tx_update_operational_flags.go (1)
64-64: Enhance flag description for better clarityThe current description could be more specific about the purpose and impact of this setting.
-cmd.Flags().Duration(signerBlockTimeOffsetFlag, 0, "Offset from the zetacore block time to initiate signing") +cmd.Flags().Duration(signerBlockTimeOffsetFlag, 0, "Time offset from the zetacore block time to initiate signing (e.g., '5s', '10s'). Use positive values to delay signing, negative to advance it.")x/observer/keeper/msg_server_update_operational_flags_test.go (1)
31-36: Enhance test coverage with edge casesThe test uses simple values but should include edge cases to ensure robust validation.
signerBlockTimeOffset := ptr.Ptr(time.Second) + +// Add test cases for edge values +t.Run("can handle maximum allowed offset", func(t *testing.T) { + msg.OperationalFlags.SignerBlockTimeOffset = ptr.Ptr(24 * time.Hour) + _, err = srv.UpdateOperationalFlags(sdk.WrapSDKContext(ctx), &msg) + require.NoError(t, err) +}) + +t.Run("can handle zero offset", func(t *testing.T) { + msg.OperationalFlags.SignerBlockTimeOffset = ptr.Ptr(time.Duration(0)) + _, err = srv.UpdateOperationalFlags(sdk.WrapSDKContext(ctx), &msg) + require.NoError(t, err) +})x/observer/types/message_update_operational_flags_test.go (2)
40-51: Expand test coverage for signer block time offset validationThe current test only covers negative offset. Add tests for edge cases and maximum allowed values.
{ name: "invalid signer block time offset", msg: types.NewMsgUpdateOperationalFlags( sample.AccAddress(), types.OperationalFlags{ SignerBlockTimeOffset: ptr.Ptr(-time.Second), }, ), err: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, "invalid request") }, }, +{ + name: "rejects excessive time offset", + msg: types.NewMsgUpdateOperationalFlags( + sample.AccAddress(), + types.OperationalFlags{ + SignerBlockTimeOffset: ptr.Ptr(25 * time.Hour), + }, + ), + err: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, "exceeds maximum allowed offset") + }, +},
Line range hint
29-38: Consider using constants for test valuesDefine test constants for better maintainability and clarity of test intentions.
+const ( + invalidRestartHeight = -1 + validRestartHeight = 100 +) + { name: "invalid restart height", msg: types.NewMsgUpdateOperationalFlags( sample.AccAddress(), types.OperationalFlags{ - RestartHeight: -1, + RestartHeight: invalidRestartHeight, }, ),docs/spec/observer/messages.md (1)
172-172: Fix markdown formatting: Replace hard tab with spacesThe line contains a hard tab which should be replaced with spaces for consistent formatting.
- OperationalFlags operational_flags = 2; + OperationalFlags operational_flags = 2;🧰 Tools
🪛 Markdownlint (0.37.0)
172-172: Column: 1
Hard tabs(MD010, no-hard-tabs)
zetaclient/orchestrator/orchestrator_test.go (1)
545-595: LGTM! Consider enhancing test case names.The test implementation is well-structured and covers the essential scenarios for
syncObserverOperationalFlags. The use of table-driven tests with clear setup and assertions is commendable.Consider making the test case names more descriptive of the specific scenarios being tested:
- t.Run("no flags set", func(t *testing.T) { + t.Run("should set zero offset when no flags are set", func(t *testing.T) { - t.Run("flags set", func(t *testing.T) { + t.Run("should set expected offset when flags are set", func(t *testing.T) { - t.Run("flags updated", func(t *testing.T) { + t.Run("should update offset when flags are modified", func(t *testing.T) {docs/cli/zetacored/cli.md (1)
13375-13375: Enhance documentation for the signer block time offsetThe description for
--signer-block-time-offsetcould be more detailed to help users understand its purpose and impact. Consider expanding it to include:
- The purpose (network latency correction)
- Expected format for the duration value
- Example values for different networks (mainnet vs testnet)
- --signer-block-time-offset duration Offset from the zetacore block time to initiate signing + --signer-block-time-offset duration Duration to offset signing from the zetacore block time (e.g. '5s' for testnet, '10s' for mainnet). Used to adjust for network latency differences between environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
typescript/zetachain/zetacore/observer/operational_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/observer/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/operational.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (18)
cmd/zetae2e/local/local.go(1 hunks)docs/cli/zetacored/cli.md(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)docs/spec/observer/messages.md(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_operational_flags.go(2 hunks)proto/zetachain/zetacore/observer/operational.proto(1 hunks)proto/zetachain/zetacore/observer/tx.proto(2 hunks)testutil/sample/observer.go(3 hunks)x/observer/client/cli/tx_update_operational_flags.go(4 hunks)x/observer/keeper/msg_server_update_operational_flags_test.go(4 hunks)x/observer/types/errors.go(1 hunks)x/observer/types/message_update_operational_flags_test.go(3 hunks)x/observer/types/operational.go(1 hunks)x/observer/types/operational_test.go(2 hunks)zetaclient/metrics/metrics.go(1 hunks)zetaclient/orchestrator/orchestrator.go(4 hunks)zetaclient/orchestrator/orchestrator_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
x/observer/types/operational.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_update_operational_flags_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
zetaclient/metrics/metrics.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/operational.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/client/cli/tx_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/message_update_operational_flags_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/errors.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/operational_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 Markdownlint (0.37.0)
docs/spec/observer/messages.md
172-172: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🪛 buf (1.47.2)
proto/zetachain/zetacore/observer/operational.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 GitHub Check: codecov/patch
zetaclient/orchestrator/orchestrator.go
[warning] 141-146: zetaclient/orchestrator/orchestrator.go#L141-L146
Added lines #L141 - L146 were not covered by tests
[warning] 330-336: zetaclient/orchestrator/orchestrator.go#L330-L336
Added lines #L330 - L336 were not covered by tests
[warning] 815-822: zetaclient/orchestrator/orchestrator.go#L815-L822
Added lines #L815 - L822 were not covered by tests
[warning] 824-824: zetaclient/orchestrator/orchestrator.go#L824
Added line #L824 was not covered by tests
[warning] 827-833: zetaclient/orchestrator/orchestrator.go#L827-L833
Added lines #L827 - L833 were not covered by tests
[warning] 840-841: zetaclient/orchestrator/orchestrator.go#L840-L841
Added lines #L840 - L841 were not covered by tests
🔇 Additional comments (17)
e2e/e2etests/test_operational_flags.go (2)
14-16: Constants look clear and aligned with naming conventions.
These newly introduced metrics constants are well-named and straightforward.
Line range hint 19-65: Test logic is robust and correctly sequences the restart flow.
The test effectively schedules a future block-based restart and verifies the successful restart by comparing the new start time against the original. The use of require.Eventually is appropriate here.
zetaclient/orchestrator/orchestrator.go (4)
71-73: Field addition for block time offset appears well-structured.
The new signerBlockTimeOffset field is integrated in a concurrency-safe way, thanks to the existing lock usage elsewhere in the code.
814-858: Enhance coverage for operational flags synchronization.
This function elegantly retrieves and applies the new offset. Ensure you have a test that verifies a scenario where the offset is nil and another where the offset changes mid-run, to confirm concurrency robustness and correct application of changes.
✅ Verification successful
Let me check the test file content to fully understand the test coverage.
Test coverage for operational flags synchronization is comprehensive.
The test suite includes all critical scenarios: nil flags, initial flag setting, and mid-run flag updates. The test cases verify both the concurrency safety through mutex usage and correct application of changes. No additional test coverage is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Searching for unit tests or integration tests that call syncObserverOperationalFlags to confirm coverage
rg "syncObserverOperationalFlags" -A 10
Length of output: 4995
Script:
#!/bin/bash
# Get the full content of the test file
cat zetaclient/orchestrator/orchestrator_test.go
Length of output: 22957
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 815-822: zetaclient/orchestrator/orchestrator.go#L815-L822
Added lines #L815 - L822 were not covered by tests
[warning] 824-824: zetaclient/orchestrator/orchestrator.go#L824
Added line #L824 was not covered by tests
[warning] 827-833: zetaclient/orchestrator/orchestrator.go#L827-L833
Added lines #L827 - L833 were not covered by tests
[warning] 840-841: zetaclient/orchestrator/orchestrator.go#L840-L841
Added lines #L840 - L841 were not covered by tests
330-336: Verify negative and large offset scenarios.
The code sets the sleep duration to zero if it’s negative, preventing an unintended delay. However, you may want to test scenarios where the offset is extremely large or negative more thoroughly, both to confirm correctness and to increase coverage.
✅ Verification successful
Let me check the test coverage for this functionality.
Negative and large offset scenarios are properly validated
The codebase already implements robust validation for the signerBlockTimeOffset:
- Negative values are rejected through validation
- Large values are capped at 10 seconds (
signerBlockTimeOffsetLimit) - The sleep duration is set to zero when negative in the orchestrator, providing an additional safety net
These safeguards effectively prevent both negative and excessive delays, making additional testing unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to new block offset usage to confirm presence of negative or large offset tests
rg "signerBlockTimeOffset"
Length of output: 2950
Script:
#!/bin/bash
# Check test cases for signerBlockTimeOffset validation
ast-grep --pattern 'func Test$_($_, $t *testing.T) {
$$$
signerBlockTimeOffset
$$$
}'
# Also check the validation logic
cat x/observer/types/operational.go
Length of output: 749
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 330-336: zetaclient/orchestrator/orchestrator.go#L330-L336
Added lines #L330 - L336 were not covered by tests
141-146: Add test coverage for new background worker.
The new runSyncObserverOperationalFlags background worker is crucial for synchronization logic. Consider adding a targeted test to ensure it starts and handles errors properly, raising confidence in production.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-146: zetaclient/orchestrator/orchestrator.go#L141-L146
Added lines #L141 - L146 were not covered by tests
docs/openapi/openapi.swagger.yaml (1)
58113-58114:
Align type definition with protobuf schema
The property type is defined as "string", but the corresponding protobuf field uses google.protobuf.Duration. Consider using the OpenAPI duration format to maintain consistency.
Apply this diff to fix the type definition:
signer_block_time_offset:
- type: string
+ type: string
+ format: durationLikely invalid or redundant comment.
proto/zetachain/zetacore/observer/operational.proto (1)
4-5:
Fix missing proto import
The static analysis indicates that gogoproto/gogo.proto is missing. Ensure the proto file is available in your proto path or buf.yaml configuration.
🧰 Tools
🪛 buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
x/observer/types/errors.go (3)
49-53: LGTM: Consistent error formatting
The reformatting of ErrInboundDisabled improves readability while maintaining functionality.
59-63: LGTM: Consistent error formatting
The reformatting of ErrOperationalFlagsRestartHeightNegative improves readability while maintaining functionality.
64-73: LGTM: Well-structured error definitions for signer block time offset
The new error variables provide clear validation boundaries for the signer block time offset feature, aligning with the PR objectives for configurable signer latency correction.
proto/zetachain/zetacore/observer/tx.proto (1)
147-147: LGTM: Consistent protobuf field naming
The field name change to operational_flags follows protobuf naming conventions and maintains consistency with other message definitions.
testutil/sample/observer.go (1)
292-293: LGTM: Appropriate initialization of SignerBlockTimeOffset
The initialization with a one-second duration pointer is suitable for test scenarios, and the use of pointer type appropriately handles optional settings.
zetaclient/metrics/metrics.go (1)
97-102: LGTM: Well-structured metric addition
The new gauge metric follows the established pattern and provides valuable monitoring capability for signer latency adjustment.
cmd/zetae2e/local/local.go (1)
365-366: LGTM: Improved test granularity
The split into separate tests for signer offset and restart height provides better test isolation and clarity.
e2e/e2etests/e2etests.go (1)
135-136: LGTM! Well-integrated test definitions.
The new test definitions for TestZetaclientRestartHeightName and TestZetaclientSignerOffsetName are properly integrated into the test suite, following the established patterns and conventions.
Also applies to: 884-893
docs/cli/zetacored/cli.md (1)
13350-13374: LGTM: Standard CLI options are well-documented
The documentation for standard CLI options follows consistent formatting and provides clear descriptions for transaction signing, gas configuration, and broadcasting modes.
Also applies to: 13376-13378
gartnera
force-pushed
the
zetaclient-signer-latency-correction
branch
from
76989af to
d5d0c22
Compare
| @@ -318,6 +327,13 @@ func (oc *Orchestrator) runScheduler(ctx context.Context) error { | |||
| continue | |||
| } | |||
|
|
|||
| sleepDuration := time.Until(newBlock.Block.Time.Add(oc.signerBlockTimeOffset)) | |||
There was a problem hiding this comment.
Can we abstract this from orchestrator and move it to zetacore client? E.g.
oc.zetacoreClient.NewBlockSubscriber(ctx, true) // syncBlock=true
There was a problem hiding this comment.
Maybe but I'm not sure if we'll want the same latency offset for different processes.
Add a
signer_block_time_offsetfield in theOperationalFlags. This will allow us to adjust our signer synchronization on a per network basis (we see much different latencies across mainnet and testnet).Closes #3260
Related to:
Summary by CodeRabbit
New Features
zetacoredCLI documentation.signer_block_time_offsetin the OpenAPI specification.signerBlockTimeOffsetFlagfor operational flags.Orchestratorwith a new field for dynamic signer block time offset.Bug Fixes
SignerBlockTimeOffsetto prevent negative values and exceed limits.Documentation
zetacoredCLI documentation with new and modified subcommands.Tests