fix(backport): do not process btc outbound as inbound transaction#3953
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 📝 WalkthroughWalkthroughThe changes update Bitcoin inbound transaction processing to use Changes
Sequence Diagram(s)sequenceDiagram
participant Observer
participant BitcoinRPC
participant FeeCalculator
Observer->>BitcoinRPC: Fetch raw Bitcoin transaction
Observer->>BitcoinRPC: Fetch previous transaction for sender address
BitcoinRPC-->>Observer: Return transaction data
Observer->>Observer: Validate recipient and sender addresses
alt Sender is empty or matches TSS address
Observer-->>Observer: Skip transaction (outbound or invalid)
else
Observer->>FeeCalculator: Calculate depositor fee
FeeCalculator-->>Observer: Return fee
alt Output value < fee
Observer-->>Observer: Mark event as INSUFFICIENT_DEPOSITOR_FEE
else
Observer-->>Observer: Parse memo and create inbound event
end
end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 #3953 +/- ##
===========================================
- Coverage 64.21% 64.18% -0.03%
===========================================
Files 474 474
Lines 34796 34804 +8
===========================================
- Hits 22343 22338 -5
- Misses 11425 11437 +12
- Partials 1028 1029 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
zetaclient/chains/bitcoin/observer/inbound_tracker.go (1)
72-81: 💡 Verification agent🧩 Analysis chain
Critical fix correctly implemented, but missing test coverage.
The function call change from
GetBtcEventtoGetBtcEventWithWitnessproperly addresses the PR objective. Based on the implementation inzetaclient/chains/bitcoin/observer/witness.go(lines 66-69),GetBtcEventWithWitnessincludes logic to skip processing whenfromAddressequalstssAddress, preventing Bitcoin outbound transactions from being processed as inbound.However, static analysis indicates this line lacks test coverage, which is concerning for such a critical fix.
🏁 Script executed:
#!/bin/bash # Description: Verify test coverage exists for GetBtcEventWithWitness function calls # Expected: Find test cases that cover the new function call path # Search for tests covering GetBtcEventWithWitness usage in inbound tracker context rg -A 10 -B 5 "GetBtcEventWithWitness.*inbound_tracker|inbound_tracker.*GetBtcEventWithWitness" --type go # Search for tests of CheckReceiptForBtcTxHash function which contains the changed line rg -A 15 -B 5 "CheckReceiptForBtcTxHash" --type go --glob="*test*"Length of output: 174
🏁 Script executed:
#!/bin/bash # Search for any test files referencing GetBtcEventWithWitness rg 'GetBtcEventWithWitness' --glob='*_test.go' -A5 -B5 # Search for any test files referencing inbound_tracker rg 'inbound_tracker' --glob='*_test.go' -A5 -B5Length of output: 11082
Missing integration tests for inbound tracker logic
While
GetBtcEventWithWitnessis well-covered inwitness_test.go, there are no tests exercising its invocation ininbound_tracker.go. The critical path added at lines 72–81 is untested.Please add tests in a new or existing
inbound_tracker_test.goto cover:
- Successful event extraction via
inbound_tracker.gousingGetBtcEventWithWitness.- Skipping when
fromAddress == tssAddress.- Error propagation when the RPC client fails.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-72: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L72
Added line #L72 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
changelog.md(1 hunks)cmd/zetatool/chains/bitcoin.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(0 hunks)zetaclient/chains/bitcoin/observer/inbound_tracker.go(1 hunks)zetaclient/chains/bitcoin/observer/witness.go(3 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(6 hunks)zetaclient/common/env.go(1 hunks)
💤 Files with no reviewable changes (1)
- zetaclient/chains/bitcoin/observer/inbound_test.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/chains/bitcoin.gozetaclient/chains/bitcoin/observer/inbound_tracker.gozetaclient/chains/bitcoin/client/client_test.gozetaclient/common/env.gozetaclient/chains/bitcoin/observer/inbound.gozetaclient/chains/bitcoin/observer/witness.gozetaclient/chains/bitcoin/observer/witness_test.go
🧬 Code Graph Analysis (4)
cmd/zetatool/chains/bitcoin.go (1)
zetaclient/chains/bitcoin/observer/witness.go (1)
GetBtcEventWithWitness(35-118)
zetaclient/chains/bitcoin/observer/inbound_tracker.go (1)
zetaclient/chains/bitcoin/observer/witness.go (1)
GetBtcEventWithWitness(35-118)
zetaclient/chains/bitcoin/client/client_test.go (1)
zetaclient/common/env.go (1)
EnvBtcRPCTestnet4(19-19)
zetaclient/chains/bitcoin/observer/witness.go (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
GetSenderAddressByVin(229-256)
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound_tracker.go
[warning] 72-72: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L72
Added line #L72 was not covered by tests
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 162-171: zetaclient/chains/bitcoin/observer/inbound.go#L162-L171
Added lines #L162 - L171 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (10)
zetaclient/common/env.go (1)
15-22: LGTM: Environment variable constants properly defined.The addition of new Bitcoin network constants (
EnvBtcRPCSignet,EnvBtcRPCTestnet4) and TON RPC constant (EnvTONRPC) follows proper naming conventions and includes appropriate documentation.zetaclient/chains/bitcoin/client/client_test.go (1)
46-46: LGTM: Test configuration properly updated.The environment variable reference correctly updated from
EnvBtcRPCTestnettoEnvBtcRPCTestnet4, maintaining consistency with the environment variable changes inzetaclient/common/env.go.changelog.md (1)
9-12: LGTM: Changelog entry accurately documents the fix.The addition of the "Fixes" section with PR #3953 reference appropriately documents the Bitcoin outbound transaction processing fix.
cmd/zetatool/chains/bitcoin.go (1)
59-68: LGTM! Clean refactoring to use witness-aware event processing.The replacement of
GetBtcEventwithGetBtcEventWithWitnessaligns perfectly with the codebase refactoring objectives and maintains consistent Bitcoin transaction processing.zetaclient/chains/bitcoin/observer/witness.go (3)
7-7: Appropriate import addition for case-insensitive address comparison.The
stringspackage import is correctly added to support the case-insensitive comparison of Bitcoin addresses in the filtering logic.
26-34: Excellent documentation enhancement for memo type support.The expanded function documentation clearly explains the two supported memo types (OP_RETURN and Tapscript) and their prioritization, improving code maintainability.
64-75: Critical improvement: Early outbound transaction filtering implemented correctly.This change successfully addresses the PR objective by moving sender address retrieval and filtering logic earlier in the function flow. The implementation correctly:
- Retrieves the sender address immediately after recipient validation
- Uses case-insensitive comparison with
strings.EqualFoldfor address matching- Filters out transactions with empty senders or TSS address senders before expensive operations
This optimization prevents outbound transactions from being processed as inbound and avoids unnecessary fee calculations for filtered transactions.
zetaclient/chains/bitcoin/observer/witness_test.go (3)
66-68: Good test setup improvement with TSS pkScript variable.The addition of
tssPkScriptvariable with proper hex decoding provides reusable test infrastructure for TSS address-related test scenarios.
276-295: Excellent test coverage for P2WPKH output validation.This test case properly validates the function's ability to skip transactions with invalid P2WPKH outputs by modifying the script prefix from "0014" to "a914", ensuring robust input validation.
404-438: Comprehensive test for TSS address filtering logic.This test case effectively validates the core PR objective by ensuring transactions where the sender is the TSS address are properly filtered out. The test correctly mocks the previous transaction output with TSS pkScript and verifies the function returns nil.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
changelog.md(1 hunks)cmd/zetatool/chains/bitcoin.go(1 hunks)zetaclient/chains/bitcoin/client/client_test.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(0 hunks)zetaclient/chains/bitcoin/observer/inbound_tracker.go(1 hunks)zetaclient/chains/bitcoin/observer/witness.go(3 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(6 hunks)zetaclient/common/env.go(1 hunks)
💤 Files with no reviewable changes (1)
- zetaclient/chains/bitcoin/observer/inbound_test.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/common/env.gocmd/zetatool/chains/bitcoin.gozetaclient/chains/bitcoin/observer/inbound_tracker.gozetaclient/chains/bitcoin/client/client_test.gozetaclient/chains/bitcoin/observer/inbound.gozetaclient/chains/bitcoin/observer/witness.gozetaclient/chains/bitcoin/observer/witness_test.go
🧬 Code Graph Analysis (4)
cmd/zetatool/chains/bitcoin.go (1)
zetaclient/chains/bitcoin/observer/witness.go (1)
GetBtcEventWithWitness(35-118)
zetaclient/chains/bitcoin/observer/inbound_tracker.go (1)
zetaclient/chains/bitcoin/observer/witness.go (1)
GetBtcEventWithWitness(35-118)
zetaclient/chains/bitcoin/client/client_test.go (1)
zetaclient/common/env.go (1)
EnvBtcRPCTestnet4(19-19)
zetaclient/chains/bitcoin/observer/witness.go (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
GetSenderAddressByVin(229-256)
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound_tracker.go
[warning] 72-72: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L72
Added line #L72 was not covered by tests
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 162-171: zetaclient/chains/bitcoin/observer/inbound.go#L162-L171
Added lines #L162 - L171 were not covered by tests
🔇 Additional comments (14)
zetaclient/common/env.go (1)
15-19: LGTM: Environment variable constants properly updated for Bitcoin network support.The refactoring from a single testnet to specific signet and testnet4 constants provides better clarity and supports multiple Bitcoin test networks. The naming convention follows established patterns and maintains consistency.
changelog.md (1)
9-11: Proper changelog documentation for the Bitcoin transaction filtering fix.The changelog entry accurately captures the core purpose of this PR and follows the established format conventions.
zetaclient/chains/bitcoin/client/client_test.go (1)
46-46: Consistent environment variable reference update.The update to use
EnvBtcRPCTestnet4aligns properly with the environment variable constant changes inzetaclient/common/env.go.cmd/zetatool/chains/bitcoin.go (1)
59-68: Improved Bitcoin event processing with enhanced transaction filtering.The replacement of
GetBtcEventwithGetBtcEventWithWitnessaligns with the PR objective. The updated function includes logic to skip outbound transactions by comparing sender addresses to the TSS address, preventing incorrect processing of BTC outbound as inbound transactions.zetaclient/chains/bitcoin/observer/inbound_tracker.go (1)
72-81:Details
✅ Verification successful
Function call updated consistently with enhanced filtering logic.
The replacement with
GetBtcEventWithWitnessprovides the same transaction filtering improvements as implemented elsewhere in the codebase.Address test coverage gap for the updated function call.
The static analysis correctly identifies that line 72 lacks test coverage. Consider adding test cases that exercise this code path to ensure the function call change is properly validated.
🏁 Script executed:
#!/bin/bash # Search for existing test coverage of CheckReceiptForBtcTxHash function rg -A 10 -B 5 "CheckReceiptForBtcTxHash|GetBtcEventWithWitness" --type go --glob "*test*"Length of output: 15459
Approve inbound_tracker.go changes; test coverage already exists
The call to
GetBtcEventWithWitnessininbound_tracker.gois fully exercised by the comprehensive tests inzetaclient/chains/bitcoin/observer/witness_test.go, covering both success and error scenarios. No additional unit tests are required for this call site.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-72: zetaclient/chains/bitcoin/observer/inbound_tracker.go#L72
Added line #L72 was not covered by testszetaclient/chains/bitcoin/observer/inbound.go (1)
162-171:Details
✅ Verification successful
LGTM: Direct call to witness-aware function eliminates unnecessary indirection.
The replacement of
GetBtcEventwithGetBtcEventWithWitnessis correct and removes redundant wrapper logic. However, the static analysis indicates these lines lack test coverage.
🏁 Script executed:
#!/bin/bash # Verify test coverage for FilterAndParseIncomingTx function rg -A 10 -B 5 "FilterAndParseIncomingTx" --type goLength of output: 7538
🏁 Script executed:
#!/bin/bash # Display lines around the change in inbound.go to inspect context for GetBtcEventWithWitness sed -n '130,200p' zetaclient/chains/bitcoin/observer/inbound.goLength of output: 2124
LGTM: Direct call to GetBtcEventWithWitness is correct and test coverage exists.
The replacement of
GetBtcEventwithGetBtcEventWithWitnessin
zetaclient/chains/bitcoin/observer/inbound.go(lines 162–171) is valid and removes unnecessary indirection. Coverage forFilterAndParseIncomingTx(which invokes this code) is already provided by:
- Unit tests in
zetaclient/chains/bitcoin/client/client_test.go- E2E tests in
e2e/runner/bitcoin.goNo further changes or additional tests are required.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 162-171: zetaclient/chains/bitcoin/observer/inbound.go#L162-L171
Added lines #L162 - L171 were not covered by testszetaclient/chains/bitcoin/observer/witness.go (3)
7-7: Appropriate import addition for case-insensitive address comparison.The
stringspackage import supports the new case-insensitive TSS address comparison logic.
26-34: Excellent documentation enhancement clarifying memo type support.The expanded comments clearly explain the two memo types (OP_RETURN and tapscript) and their prioritization, improving code maintainability.
64-75: Critical fix: Early sender validation prevents outbound transactions from being processed as inbound.This change correctly addresses the PR objective by:
- Moving sender address retrieval earlier in the processing flow
- Adding explicit checks to skip transactions where sender is empty or equals TSS address
- Using case-insensitive comparison for address matching
The logic effectively prevents Bitcoin outbound transactions from being incorrectly processed as inbound transactions.
zetaclient/chains/bitcoin/observer/witness_test.go (5)
6-6: Appropriate import for test string operations.
66-69: Good test setup: Pre-computed TSS pkScript enables mock transaction creation.The hex-decoded TSS pkScript setup supports the new test scenarios for outbound transaction filtering.
276-295: Comprehensive test coverage for invalid P2WPKH output validation.This test correctly verifies that transactions with non-P2WPKH outputs (modified from
0014toa914for P2SH) are properly skipped.
346-346: Improved test function naming for clarity.The renamed function better describes its purpose regarding sender address retrieval errors.
404-438: Excellent test coverage for TSS sender address filtering.This test validates the critical fix by mocking a transaction where the sender address matches the TSS address, ensuring such outbound transactions are correctly skipped.
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Can we maybe keep a debug log?
Good idea though, commit ab3d986 |
3 participants
Description
This PR is to backport 3951 to
developHow Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Tests
Chores