refactor: switch zetaclient to confirmation params; unify inbound observation as block range based
#3496
Conversation
…bservation using block range; add confirmation related helper functions to base observer
|
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 📝 WalkthroughWalkthroughThe pull request updates the confirmation logic by introducing a new structured field, ConfirmationParams, while deprecating confirmation_count. Changes span updates to both Bitcoin and EVM observers and related tests. New helper methods are added for calculating block ranges and verifying block confirmations. Several functions have been refactored for improved error handling and logging, including the removal of redundant methods and test cases. Documentation in the changelog and configuration has been updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant O as Observer
participant RPC as RPC Client
participant CP as Chain Params Logic
O->>RPC: updateLastBlock(ctx)
RPC-->>O: Return current block number
O->>O: Calculate unscanned block range (CalcUnscannedBlockRangeInboundSafe/Fast)
O->>CP: Check block confirmation (IsBlockConfirmedForInbound/OutboundSafe/Fast)
CP-->>O: Return confirmation result
O->>O: Process inbound/outbound transactions accordingly
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
|
ws4charlie
added
zetaclient
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
zetaclient/testutils/mocks/chain_params.go (1)
34-34: Add TODO to track ConfirmationCount removal.While the comment explains why the deprecated field is still present, it would be helpful to add a TODO comment with a tracking issue to ensure this technical debt is addressed after the chain parameters validation is updated.
- ConfirmationCount: confirmation, // it is deprecated still needed to by pass chain params validation + ConfirmationCount: confirmation, // TODO(#ISSUE): Remove after updating chain params validation. Currently needed to bypass validation despite being deprecatedzetaclient/chains/evm/observer/observer.go (1)
275-292: Enhance method documentation and error handling.While the implementation is solid, consider the following improvements:
- Expand the comment to explain why keeping the last block up-to-date is crucial for confirmation accuracy.
- Include chain name in error messages for better debugging in multi-chain environments.
- Add debug level logging for successful block updates.
Here's the suggested implementation:
-// updateLastBlock is a helper function to update the last block number. -// Note: keep last block up-to-date helps to avoid inaccurate confirmation. +// updateLastBlock updates the last known block number from the EVM client. +// Keeping the last block number up-to-date is crucial for: +// 1. Ensuring accurate transaction confirmation calculations +// 2. Preventing potential security issues from block number inconsistencies +// 3. Maintaining reliable chain synchronization state func (ob *Observer) updateLastBlock(ctx context.Context) error { blockNumber, err := ob.evmClient.BlockNumber(ctx) switch { case err != nil: return errors.Wrap(err, "error getting block number") case blockNumber < ob.LastBlock(): - return fmt.Errorf("block number should not decrease: current %d last %d", blockNumber, ob.LastBlock()) + return fmt.Errorf("block number should not decrease on chain %s: current %d last %d", ob.Chain().Name, blockNumber, ob.LastBlock()) default: ob.WithLastBlock(blockNumber) + ob.Logger().Chain.Debug(). + Uint64("old_block", ob.LastBlock()). + Uint64("new_block", blockNumber). + Msg("Updated last block number") } // increment prom counter metrics.GetBlockByNumberPerChain.WithLabelValues(ob.Chain().Name).Inc() return nil }zetaclient/chains/base/observer_helper.go (1)
79-85: Minor variable naming consideration.
Renaming “confHeight” to something like “minConfirmedBlock” could improve clarity, though this is optional.zetaclient/chains/bitcoin/observer/inbound.go (2)
24-28: Test coverage needed for inbound observation initialization.
These lines are not covered in tests, which risks missing regressions in logger setup and last-block updates.Would you like help with unit tests to ensure this logic is fully validated?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-28: zetaclient/chains/bitcoin/observer/inbound.go#L24-L28
Added lines #L24 - L28 were not covered by tests
56-104: Uncovered inbound observation paths.
Though the logic is comprehensive, these lines are missed in tests, leaving error handling unverified.Would you like me to suggest a test scenario covering block retrieval errors and inbound parsing?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-68: zetaclient/chains/bitcoin/observer/inbound.go#L56-L68
Added lines #L56 - L68 were not covered by tests
[warning] 85-86: zetaclient/chains/bitcoin/observer/inbound.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 95-96: zetaclient/chains/bitcoin/observer/inbound.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 103-103: zetaclient/chains/bitcoin/observer/inbound.go#L103
Added line #L103 was not covered by testszetaclient/chains/evm/observer/inbound.go (2)
102-107: Improve test coverage for block update logic.
The newly added update step is not covered by tests, potentially hiding future regressions.I can outline a unit test approach to confirm that 'ObserveInbound' correctly updates the last scanned block.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 102-102: zetaclient/chains/evm/observer/inbound.go#L102
Added line #L102 was not covered by tests
[warning] 104-107: zetaclient/chains/evm/observer/inbound.go#L104-L107
Added lines #L104 - L107 were not covered by tests
403-410: Confirmation blocks lack test coverage.
Code that checks block confirmation for inbound safety remains untested, which can hide logic bugs.Do you want me to propose a new test suite exercising these confirmation checks?
Also applies to: 454-461, 504-510
zetaclient/chains/bitcoin/observer/inbound_tracker.go (1)
73-76: Consider adding context to error message.The error message could be more informative by including the current block height and required confirmations.
- return "", fmt.Errorf("block %d is not confirmed yet", blockVb.Height) + return "", fmt.Errorf("block %d is not confirmed yet (current height: %d, required confirmations: %d)", + blockVb.Height, ob.LastBlock(), ob.GetConfirmationParams().SafeInboundCount)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-76: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L74-L76
Added lines #L74 - L76 were not covered by testszetaclient/chains/evm/observer/v2_inbound_tracker.go (2)
34-40: Consider adding context to error message.Similar to the Bitcoin observer, include current block height and required confirmations in the error message for better debugging.
- "inbound %s has not been confirmed yet: receipt block %d", - tx.Hash, - receipt.BlockNumber.Uint64(), + "inbound %s has not been confirmed yet: receipt block %d (current height: %d, required confirmations: %d)", + tx.Hash, + receipt.BlockNumber.Uint64(), + ob.LastBlock(), + ob.GetConfirmationParams().SafeInboundCount,
42-97: Consider refactoring event parsing logic.The current implementation has repeated patterns for three different event types. Consider extracting the common logic into a helper function to improve maintainability.
type eventParser struct { parseFunc func(log) (interface{}, error) newVoteFunc func(interface{}) interface{} } func (ob *Observer) handleEvent(ctx context.Context, parser eventParser, log Log, tx *client.Transaction) error { event, err := parser.parseFunc(log) if err != nil { return nil } if !ob.isEventProcessable(...) { return fmt.Errorf("event from inbound tracker %s is not processable", tx.Hash) } msg := parser.newVoteFunc(event) _, err = ob.PostVoteInbound(ctx, &msg, zetacore.PostVoteInboundExecutionGasLimit) return err }zetaclient/chains/bitcoin/observer/observer.go (1)
229-232: Enhance error handling with more context.The error wrapping could provide more context about the RPC call.
- blockNumber, err := ob.rpc.GetBlockCount(ctx) - if err != nil { - return errors.Wrapf(err, "error getting block number") - } + blockNumber, err := ob.rpc.GetBlockCount(ctx) + if err != nil { + return errors.Wrapf(err, "failed to get block count from Bitcoin RPC") + }zetaclient/chains/base/observer_helper_test.go (2)
23-31: Enhance test case descriptions for better clarity.The test case names could be more descriptive to better explain the scenarios being tested.
- name: "no unscanned blocks", + name: "should return [0,0) when last block is within confirmation window", - name: "1 unscanned blocks", + name: "should return [91,92) when one block is available for scanning", - name: "10 unscanned blocks", + name: "should return [91,101) when ten blocks are available for scanning",Also applies to: 33-41, 43-51
75-76: Add assertions for individual components.Consider adding separate assertions for start and end values to make test failures more descriptive.
- require.Equal(t, tt.expectedBlockRange, [2]uint64{start, end}) + require.Equal(t, tt.expectedBlockRange[0], start, "start block mismatch") + require.Equal(t, tt.expectedBlockRange[1], end, "end block mismatch")x/observer/types/chain_params_test.go (1)
174-188: Improve test readability with table-driven tests.Consider refactoring the tests to use table-driven tests for better organization and easier addition of test cases.
+ tests := []struct { + name string + safeInboundCount uint64 + fastInboundCount uint64 + expectedCount uint64 + }{ + { + name: "should return fast count when enabled", + safeInboundCount: 2, + fastInboundCount: 1, + expectedCount: 1, + }, + { + name: "should fallback to safe count when fast is disabled", + safeInboundCount: 2, + fastInboundCount: 0, + expectedCount: 2, + }, + } + + for _, tt := range tests { + s.T().Run(tt.name, func(t *testing.T) { + copy := copyParams(s.evmParams) + copy.ConfirmationParams.SafeInboundCount = tt.safeInboundCount + copy.ConfirmationParams.FastInboundCount = tt.fastInboundCount + confirmation := copy.InboundConfirmationFast() + require.Equal(t, tt.expectedCount, confirmation) + }) + }Also applies to: 190-204
changelog.md (1)
12-12: Enhance the changelog entry with more details.Consider expanding the changelog entry to include:
- The motivation behind the change to block range-based observation
- The impact on performance and end-to-end tests
- The deprecation notice for
ConfirmationCountExample:
-* [3496](https://github.com/zeta-chain/node/pull/3496) - zetaclient uses `ConfirmationParams` in stead of old `ConfirmationCount`; use block ranged based observation for btc and evm chain. +* [3496](https://github.com/zeta-chain/node/pull/3496) - Enhance inbound observation performance: + * Replace `ConfirmationCount` with `ConfirmationParams` (deprecated: `ConfirmationCount`) + * Unify Bitcoin and EVM chain observation using block range-based scanning + * Improve end-to-end test speed and block catching efficiency
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
changelog.md(1 hunks)cmd/zetatool/inbound/bitcoin.go(1 hunks)cmd/zetatool/inbound/evm.go(1 hunks)x/observer/types/chain_params.go(1 hunks)x/observer/types/chain_params_test.go(1 hunks)zetaclient/chains/base/observer.go(0 hunks)zetaclient/chains/base/observer_helper.go(1 hunks)zetaclient/chains/base/observer_helper_test.go(1 hunks)zetaclient/chains/base/observer_test.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound.go(3 hunks)zetaclient/chains/bitcoin/observer/inbound_tracker.go(1 hunks)zetaclient/chains/bitcoin/observer/observer.go(2 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(0 hunks)zetaclient/chains/bitcoin/observer/outbound.go(1 hunks)zetaclient/chains/evm/observer/inbound.go(10 hunks)zetaclient/chains/evm/observer/inbound_test.go(10 hunks)zetaclient/chains/evm/observer/observer.go(2 hunks)zetaclient/chains/evm/observer/outbound.go(2 hunks)zetaclient/chains/evm/observer/outbound_test.go(1 hunks)zetaclient/chains/evm/observer/v2_inbound_tracker.go(1 hunks)zetaclient/config/config_chain.go(1 hunks)zetaclient/testutils/mocks/chain_params.go(1 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/chains/base/observer.go
- zetaclient/chains/bitcoin/observer/observer_test.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/inbound/bitcoin.gozetaclient/chains/evm/observer/v2_inbound_tracker.gozetaclient/testutils/mocks/chain_params.gozetaclient/config/config_chain.gocmd/zetatool/inbound/evm.gozetaclient/chains/bitcoin/observer/outbound.gox/observer/types/chain_params.gozetaclient/chains/base/observer_test.gozetaclient/chains/evm/observer/inbound_test.gozetaclient/chains/evm/observer/outbound.gozetaclient/chains/evm/observer/outbound_test.gozetaclient/chains/evm/observer/observer.gox/observer/types/chain_params_test.gozetaclient/chains/bitcoin/observer/observer.gozetaclient/chains/bitcoin/observer/inbound.gozetaclient/chains/bitcoin/observer/inbound_tracker.gozetaclient/chains/base/observer_helper.gozetaclient/chains/base/observer_helper_test.gozetaclient/chains/evm/observer/inbound.go
📓 Learnings (3)
zetaclient/testutils/mocks/chain_params.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3461
File: x/observer/types/chain_params.go:59-62
Timestamp: 2025-02-04T06:12:41.760Z
Learning: The legacy `ConfirmationCount` field in the ChainParams struct is planned to be deprecated after a full upgrade to the new confirmation count system that uses SafeInboundCount, FastInboundCount, SafeOutboundCount, and FastOutboundCount fields.
x/observer/types/chain_params.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3461
File: x/observer/types/chain_params.go:59-62
Timestamp: 2025-02-04T06:12:41.760Z
Learning: The legacy `ConfirmationCount` field in the ChainParams struct is planned to be deprecated after a full upgrade to the new confirmation count system that uses SafeInboundCount, FastInboundCount, SafeOutboundCount, and FastOutboundCount fields.
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-11-12T13:20:12.658Z
Learning: ObserveInbound coverage will be improved in future refactor.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 151-152: zetaclient/chains/bitcoin/observer/outbound.go#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/bitcoin/observer/outbound.go#L155
Added line #L155 was not covered by tests
zetaclient/chains/bitcoin/observer/observer.go
[warning] 228-235: zetaclient/chains/bitcoin/observer/observer.go#L228-L235
Added lines #L228 - L235 were not covered by tests
[warning] 238-251: zetaclient/chains/bitcoin/observer/observer.go#L238-L251
Added lines #L238 - L251 were not covered by tests
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 24-28: zetaclient/chains/bitcoin/observer/inbound.go#L24-L28
Added lines #L24 - L28 were not covered by tests
[warning] 32-33: zetaclient/chains/bitcoin/observer/inbound.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 38-40: zetaclient/chains/bitcoin/observer/inbound.go#L38-L40
Added lines #L38 - L40 were not covered by tests
[warning] 44-48: zetaclient/chains/bitcoin/observer/inbound.go#L44-L48
Added lines #L44 - L48 were not covered by tests
[warning] 51-51: zetaclient/chains/bitcoin/observer/inbound.go#L51
Added line #L51 was not covered by tests
[warning] 56-68: zetaclient/chains/bitcoin/observer/inbound.go#L56-L68
Added lines #L56 - L68 were not covered by tests
[warning] 85-86: zetaclient/chains/bitcoin/observer/inbound.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 95-96: zetaclient/chains/bitcoin/observer/inbound.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 103-103: zetaclient/chains/bitcoin/observer/inbound.go#L103
Added line #L103 was not covered by tests
zetaclient/chains/bitcoin/observer/inbound_tracker.go
[warning] 16-20: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L16-L20
Added lines #L16 - L20 were not covered by tests
[warning] 22-35: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L22-L35
Added lines #L22 - L35 were not covered by tests
[warning] 38-38: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L38
Added line #L38 was not covered by tests
[warning] 42-46: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L42-L46
Added lines #L42 - L46 were not covered by tests
[warning] 48-51: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L48-L51
Added lines #L48 - L51 were not covered by tests
[warning] 53-56: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-61: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L58-L61
Added lines #L58 - L61 were not covered by tests
[warning] 63-65: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L63-L65
Added lines #L63 - L65 were not covered by tests
[warning] 67-70: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L67-L70
Added lines #L67 - L70 were not covered by tests
[warning] 74-76: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L74-L76
Added lines #L74 - L76 were not covered by tests
[warning] 79-91: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L79-L91
Added lines #L79 - L91 were not covered by tests
[warning] 93-95: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L93-L95
Added lines #L93 - L95 were not covered by tests
[warning] 97-100: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L97-L100
Added lines #L97 - L100 were not covered by tests
[warning] 102-104: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L102-L104
Added lines #L102 - L104 were not covered by tests
[warning] 106-106: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L106
Added line #L106 was not covered by tests
zetaclient/chains/evm/observer/inbound.go
[warning] 102-102: zetaclient/chains/evm/observer/inbound.go#L102
Added line #L102 was not covered by tests
[warning] 104-107: zetaclient/chains/evm/observer/inbound.go#L104-L107
Added lines #L104 - L107 were not covered by tests
🔇 Additional comments (20)
zetaclient/chains/evm/observer/observer.go (1)
270-273: LGTM!The added logging statement improves visibility of successful RPC health checks.
zetaclient/chains/base/observer_helper.go (3)
3-11: Efficient unscanned block range approach.
The calculations for inbound safe confirmation appear logically sound and well encapsulated.
30-36: Check fallback logic.
The doc comment states that fast confirmation is disabled with a fallback to safe confirmations, but the code does not appear to handle that scenario. Please confirm if this is the intended behavior.
53-77: Robust unscanned block range calculation.
Limiting the block range using the provided block limit is straightforward and correct, ensuring no inadvertent overscanning.zetaclient/chains/bitcoin/observer/inbound.go (2)
32-35: Early termination for empty scanning range.
Short-circuiting the scan when startBlock >= endBlock is an efficient check. Consider adding a targeted test.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: zetaclient/chains/bitcoin/observer/inbound.go#L32-L33
Added lines #L32 - L33 were not covered by tests
38-41: Partial scanning design is clear.
Handling partial failures while continuing the observation process is well-structured.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-40: zetaclient/chains/bitcoin/observer/inbound.go#L38-L40
Added lines #L38 - L40 were not covered by testszetaclient/chains/evm/observer/inbound.go (1)
115-120: Clean early return on empty blocks.
This minor optimization prevents unnecessary inbound scans and is well implemented.zetaclient/config/config_chain.go (1)
10-11: LGTM! Clear and descriptive constant naming.The rename from
MaxBlocksPerPeriodtoBlockLimitPerScanbetter reflects its purpose, and the added comment enhances clarity.zetaclient/chains/bitcoin/observer/inbound_tracker.go (2)
15-39: Add test coverage for ProcessInboundTrackers.This function handles critical inbound tracking functionality but currently lacks test coverage. Consider adding comprehensive tests to verify:
- Error handling for GetInboundTrackersForChain
- Processing of multiple trackers
- Error propagation from CheckReceiptForBtcTxHash
Would you like me to help create a test suite for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-20: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L16-L20
Added lines #L16 - L20 were not covered by tests
[warning] 22-35: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L22-L35
Added lines #L22 - L35 were not covered by tests
[warning] 38-38: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L38
Added line #L38 was not covered by tests
42-107: Add test coverage for CheckReceiptForBtcTxHash.This function performs critical Bitcoin transaction validation but lacks test coverage. Consider adding tests for:
- Invalid transaction hash handling
- Block confirmation validation
- TSS address retrieval errors
- Various error conditions
Would you like me to help create a test suite for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-46: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L42-L46
Added lines #L42 - L46 were not covered by tests
[warning] 48-51: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L48-L51
Added lines #L48 - L51 were not covered by tests
[warning] 53-56: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-61: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L58-L61
Added lines #L58 - L61 were not covered by tests
[warning] 63-65: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L63-L65
Added lines #L63 - L65 were not covered by tests
[warning] 67-70: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L67-L70
Added lines #L67 - L70 were not covered by tests
[warning] 74-76: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L74-L76
Added lines #L74 - L76 were not covered by tests
[warning] 79-91: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L79-L91
Added lines #L79 - L91 were not covered by tests
[warning] 93-95: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L93-L95
Added lines #L93 - L95 were not covered by tests
[warning] 97-100: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L97-L100
Added lines #L97 - L100 were not covered by tests
[warning] 102-104: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L102-L104
Added lines #L102 - L104 were not covered by tests
[warning] 106-106: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L106
Added line #L106 was not covered by testscmd/zetatool/inbound/bitcoin.go (1)
74-74: LGTM! Proper use of SafeInboundCount.The change correctly implements the switch from ConfirmationCount to ConfirmationParams.SafeInboundCount, aligning with the PR objectives.
zetaclient/chains/bitcoin/observer/observer.go (1)
226-252: Add test coverage for the newupdateLastBlockmethod.The method contains critical logic for block number validation and node status management, but lacks test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 228-235: zetaclient/chains/bitcoin/observer/observer.go#L228-L235
Added lines #L228 - L235 were not covered by tests
[warning] 238-251: zetaclient/chains/bitcoin/observer/observer.go#L238-L251
Added lines #L238 - L251 were not covered by testscmd/zetatool/inbound/evm.go (1)
82-82: LGTM! Correctly updated to use structured confirmation parameters.The change appropriately uses
SafeInboundCountfrom the newConfirmationParamsstructure.zetaclient/chains/base/observer_test.go (1)
42-44: LGTM! Test suite correctly uses the new confirmation parameters.The test suite has been updated to use
ConfirmationParams.SafeInboundCountinstead of the deprecatedConfirmationCount, aligning with the PR objectives.x/observer/types/chain_params.go (1)
158-174: LGTM! Well-designed fallback mechanism for fast confirmation counts.The implementation elegantly handles the fallback to safe confirmation counts when fast mode is disabled. The methods are well-documented and follow a consistent pattern.
zetaclient/chains/evm/observer/outbound.go (2)
38-41: LGTM! Essential block update before processing.The addition of
updateLastBlockensures accurate confirmation checks by maintaining up-to-date block information.
484-489: LGTM! Simplified confirmation check logic.The confirmation check has been improved by using
IsBlockConfirmedForOutboundSafe, making the code more maintainable and consistent with the new confirmation parameters system.zetaclient/chains/evm/observer/inbound_test.go (1)
42-43: LGTM! Comprehensive test updates for new confirmation system.All test cases have been systematically updated to use
ConfirmationParams.SafeInboundCount, ensuring proper test coverage for the new confirmation parameters system.Also applies to: 60-61, 78-79, 127-128, 145-146, 163-164, 489-490
zetaclient/chains/bitcoin/observer/outbound.go (1)
151-155: Add test coverage for confirmation parameter changes.The switch from
ConfirmationCounttoConfirmationParams.SafeOutboundCountis not covered by tests. Add test cases to verify:
- The correct confirmation count is used
- The confirmation check behaves correctly for both successful and unsuccessful cases
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 151-152: zetaclient/chains/bitcoin/observer/outbound.go#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/bitcoin/observer/outbound.go#L155
Added line #L155 was not covered by testszetaclient/chains/evm/observer/outbound_test.go (1)
424-425: LGTM! Test updates correctly reflect the parameter changes.The test has been properly updated to use
ConfirmationParams.SafeOutboundCountand the block number calculation is correct.Also applies to: 431-432
| return nil | ||
| // save last scanned block to both memory and db | ||
| if lastScannedNew > ob.LastBlockScanned() { | ||
| logger.Debug().Uint64("from", startBlock).Uint64("to", lastScannedNew).Msg("observed blocks for inbounds") |
There was a problem hiding this comment.
Why if observeInboundInBlockRange already emit logs?
There was a problem hiding this comment.
This one is on Debug level and will not be printed in Datadog. It just allows us to debug locally and see the last scanned block without metrics, should we remove it?
…actor-confirmation-params-and-evm-btc-inbound-scanning
| @@ -7,7 +7,8 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| MaxBlocksPerPeriod = 100 | |||
| // BlockLimitPerScan is the maximum number of blocks to scan in one ticker | |||
| BlockLimitPerScan = 100 | |||
There was a problem hiding this comment.
| BlockLimitPerScan = 100 | |
| MaxBlocksPerScan = 100 |
| @@ -420,6 +448,17 @@ func GetDefaultZetaPrivnetChainParams() *ChainParams { | |||
|
|
|||
| // ChainParamsEqual returns true if two chain params are equal | |||
| func ChainParamsEqual(params1, params2 ChainParams) bool { | |||
| equalConfirmParams := func(confmParam1, confmParam2 *ConfirmationParams) bool { | |||
There was a problem hiding this comment.
let's move this to a separate confirmationParamsEqual(a, b *ConfirmationParams) bool
if a == nil || b == nil {
return false
}
return *a == *b| // isBlockConfirmed checks if the block number is confirmed. | ||
| // | ||
| // Note: block 100 is confirmed if the last block is 100 and confirmation count is 1. | ||
| func isBlockConfirmed(blockNumber uint64, confirmation uint64, lastBlock uint64) bool { |
There was a problem hiding this comment.
nit:
| func isBlockConfirmed(blockNumber uint64, confirmation uint64, lastBlock uint64) bool { | |
| func isBlockConfirmed(blockNumber, confirmation, lastBlock uint64) bool { |
| } | ||
|
|
||
| // increment prom counter | ||
| metrics.GetBlockByNumberPerChain.WithLabelValues(ob.Chain().Name).Inc() |
There was a problem hiding this comment.
nit: could be moved to WithLastBlock
|
|
||
| // CalcUnscannedBlockRangeInboundFast calculates the unscanned block range using inbound fast confirmation count. | ||
| // It returns a range of blocks [from, end (exclusive)) that need to be scanned. | ||
| func (ob *Observer) CalcUnscannedBlockRangeInboundFast(blockLimit uint64) (from uint64, end uint64) { |
There was a problem hiding this comment.
WDYT about this naming? IMO it's clearer
| func (ob *Observer) CalcUnscannedBlockRangeInboundFast(blockLimit uint64) (from uint64, end uint64) { | |
| func (ob *Observer) GetScanRangeInboundFast(blockLimit uint64) (from uint64, end uint64) { |
| ob.logger.Inbound.Info(). | ||
| Str("tracker.hash", tracker.TxHash). | ||
| Str("tracker.coin-type", tracker.CoinType.String()). | ||
| Msgf("checking tracker") |
There was a problem hiding this comment.
| Msgf("checking tracker") | |
| Msg("Processing inbound tracker") |
| if err != nil { | ||
| return err | ||
| } | ||
| ob.logger.Inbound.Info(). |
There was a problem hiding this comment.
i think we already log inbounds in zetacore client, right?
| } | ||
|
|
||
| // CheckReceiptForBtcTxHash checks the receipt for a btc tx hash | ||
| func (ob *Observer) CheckReceiptForBtcTxHash(ctx context.Context, txHash string, vote bool) (string, error) { |
There was a problem hiding this comment.
Please wrap the errors to understand where they come from.
| } | ||
|
|
||
| ob.WithLastBlock(blockNumber) | ||
| logger := ob.Logger().Inbound.With().Str(logs.FieldMethod, "ObserveInbound").Logger() |
There was a problem hiding this comment.
| logger := ob.Logger().Inbound.With().Str(logs.FieldMethod, "ObserveInbound").Logger() | |
| logger := ob.Logger().Inbound.With().Str(logs.FieldMethod, "observe_inbound").Logger() |
| logger.Debug(). | ||
| Msgf("tx included but not confirmed, receipt block %d current block %d", receipt.BlockNumber.Uint64(), lastHeight) | ||
| Msgf("tx included but not confirmed, receipt block %d current block %d", receipt.BlockNumber.Uint64(), ob.LastBlock()) |
Description
In order to implement the feature of fast observation, some preparation is done.
Make a switch in
zetaclientto useConfirmationParamsunconditionally, deprecating the existingConfirmationCount. TheConfirmationParamswill be available in chain state right after migration.Unify and simplify
EVMandBitcoininbound observation style, making both of them block range-based scanning style. We may (or may not) differentiatescanning Safe blocksfromscanning Fast blocksin the upstream of the flow, we'll see. Making Bitcoin inbound scanning range-based speeds up E2E test by a lot (see screenshot below); it also makes block catching up much faster as well.The observation function
ObserveInboundis split into two:updateLastBlock+observeInboundInBlockRangeGroup a few confirmation calculation related help functions into
base observer. BothEVMandBitcoinobservers will use them:CalcUnscannedBlockRangeInboundSafe,CalcUnscannedBlockRangeInboundFast,IsBlockConfirmedForInboundSafe, etc.Bitcoin inbound tracker function like
ProcessInboundTrackersis moved to separate fileinbound_tracker.goHow Has This Been Tested?