fix: cherry pick mainnet bitcoin fix to be able to revert missed inbound w…#3754
Conversation
📝 WalkthroughWalkthroughThis pull request updates Bitcoin deposit handling by ensuring that deposits revert when the memo output is missing. The changes include a new changelog entry referencing PR [3754] and adjustments in transaction construction to conditionally include an OP_RETURN output only when a memo is provided. Error handling has been enhanced in the event decoding and witness processing functions, and accompanying tests have been expanded to cover scenarios with nil, empty, or invalid memos. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Processor
participant Runner
participant Observer
participant Witness
User->>Processor: Submit Bitcoin deposit transaction
Processor->>Runner: Call sendToAddrFromDeployerWithMemo(memo?)
alt Memo Provided
Runner->>Processor: Create transaction with OP_RETURN output
Processor->>Observer: Process transaction normally
else No Memo Provided
Runner->>Observer: Check for memo (nil)
Observer-->>Runner: Return error ("no memo found in inbound")
Runner->>Witness: Pass error status and trigger revert
Witness->>Processor: Mark transaction as reverted (using "noMemoFound")
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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:
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
mainnet
bug
zetaclient
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3754 +/- ##
========================================
Coverage 64.33% 64.34%
========================================
Files 462 462
Lines 32910 32914 +4
========================================
+ Hits 21173 21177 +4
Misses 10767 10767
Partials 970 970
🚀 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: 0
🧹 Nitpick comments (1)
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
13-16: Verify new imports are needed.The added imports appear to be unused in the changed code sections. Please verify these are necessary or remove them if not needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go(1 hunks)e2e/runner/bitcoin.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(6 hunks)zetaclient/chains/bitcoin/observer/witness.go(3 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(1 hunks)
🧰 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/chains/bitcoin/observer/event_test.gozetaclient/chains/bitcoin/observer/witness_test.goe2e/runner/bitcoin.goe2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.gozetaclient/chains/bitcoin/observer/event.gozetaclient/chains/bitcoin/observer/witness.gozetaclient/chains/bitcoin/observer/inbound_test.go
🧠 Learnings (2)
zetaclient/chains/bitcoin/observer/event_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2025-03-24T14:49:37.949Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
zetaclient/chains/bitcoin/observer/event.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2025-03-24T14:49:37.949Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
🧬 Code Definitions (3)
zetaclient/chains/bitcoin/observer/event_test.go (2)
zetaclient/chains/bitcoin/observer/event.go (3)
event(53-76)event(80-139)BTCInboundEvent(23-50)zetaclient/types/event.go (2)
event(76-95)event(98-117)
zetaclient/chains/bitcoin/observer/witness_test.go (2)
zetaclient/chains/bitcoin/observer/event.go (4)
BTCInboundEvent(23-50)event(53-76)event(80-139)err(82-82)zetaclient/chains/bitcoin/observer/witness.go (1)
GetBtcEventWithWitness(27-109)
zetaclient/chains/bitcoin/observer/inbound_test.go (2)
zetaclient/testutils/testrpc/rpc_btc.go (1)
CreateBTCRPCAndLoadTx(61-80)zetaclient/testutils/testdata.go (1)
LoadBTCInboundRawResult(137-148)
🔇 Additional comments (14)
changelog.md (1)
73-73: LGTM: Clear changelog entry for the fix.The changelog entry accurately describes the fix, making it clear that Bitcoin deposits will now revert when the memo output is missing.
zetaclient/chains/bitcoin/observer/event_test.go (1)
171-178: Good test coverage for memo not found case.This test case properly verifies the error handling when no memo is found in an inbound Bitcoin transaction, ensuring that the system will respond with the expected error message.
zetaclient/chains/bitcoin/observer/event.go (1)
88-92: LGTM: Proper error handling for missing memo.The implementation correctly checks for the absence of a memo and returns an appropriate error message, ensuring that inbound transactions without a memo will be reverted. This is consistent with the updated behavior described in the changelog.
zetaclient/chains/bitcoin/observer/witness_test.go (1)
108-148: Comprehensive test for handling transactions without memo.The test effectively simulates a Bitcoin transaction without an OP_RETURN output, validating that the
GetBtcEventWithWitnessfunction correctly handles this case by setting the appropriate memo bytes value. This ensures that downstream processing can properly identify and revert these transactions.zetaclient/chains/bitcoin/observer/witness.go (3)
19-22: Good addition with clear purpose.The new constant
noMemoFoundis well-defined with a descriptive comment explaining its role as a placeholder for missing memos in Bitcoin inbound transactions.
74-75: Clear comment update reflecting the new behavior.The updated comment accurately describes the new logic where a placeholder is set when no memo is found, rather than exiting the function early.
84-85: Critical change in error handling flow.This implementation change is the core of the fix - instead of returning early with
nil, nilwhen no memo is found, the function now sets a special placeholder value. This ensures that the transaction will be processed and properly reverted later.e2e/runner/bitcoin.go (2)
247-259: Good implementation of conditional memo handling.The code now properly handles the case when no memo is provided, making it suitable for testing no-memo edge cases. The OP_RETURN output is only added when a memo is present, which aligns with the PR's purpose of enhancing error handling for missing memo outputs.
267-270: Improved transaction output management.The code now dynamically determines the index of the change output based on the actual number of outputs, making the solution more robust when handling transactions with or without memo outputs.
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go (2)
18-29: Well-structured test case for missing memo.This test case properly verifies that a Bitcoin deposit without a memo output is correctly reverted with the appropriate error status. The test is clear, focused, and directly targets the behavior introduced by this PR.
30-40: Complete coverage with empty memo test case.This additional test case checks for proper handling of empty memo byte slices, providing comprehensive test coverage for the different memo scenarios. The assertions correctly verify that the transaction is reverted with the INVALID_MEMO status.
zetaclient/chains/bitcoin/observer/inbound_test.go (3)
476-507: Test correctly updated to validate new behavior.This test has been properly updated to verify the new behavior with missing memos. Instead of expecting the transaction to be skipped, it now correctly asserts that the transaction is processed with the
noMemoFoundplaceholder.
586-612: Aligned test expectations with new behavior.The test correctly expects that transactions with invalid OP_RETURN outputs should be processed (not skipped) according to the new behavior. The updates to the setup and assertions are appropriate.
614-640: Consistent test update for memo decoding failures.This test has been properly updated to expect that transactions with memo decoding failures should be processed rather than skipped, aligning with the other changes in behavior for handling invalid memos.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/runner/bitcoin.go (1)
267-269: Good improvement to output swapping logic.Using the dynamic index calculation for the change output makes the code more resilient when dealing with variable numbers of transaction outputs. However, consider adding a comment explaining the expected ordering of transaction outputs after swapping for better maintainability.
// swap the TxOut[0] and the last output (change) idxChange := len(tx.TxOut) - 1 tx.TxOut[0], tx.TxOut[idxChange] = tx.TxOut[idxChange], tx.TxOut[0] +// After swapping: TxOut[0] is the payment to recipient, the optional memo is TxOut[1] (if present), and the last output is change to deployer address
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go(1 hunks)e2e/runner/bitcoin.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(6 hunks)zetaclient/chains/bitcoin/observer/witness.go(3 hunks)zetaclient/chains/bitcoin/observer/witness_test.go(1 hunks)
🧰 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.
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.gozetaclient/chains/bitcoin/observer/event_test.gozetaclient/chains/bitcoin/observer/witness_test.gozetaclient/chains/bitcoin/observer/event.gozetaclient/chains/bitcoin/observer/inbound_test.gozetaclient/chains/bitcoin/observer/witness.goe2e/runner/bitcoin.go
🧠 Learnings (3)
zetaclient/chains/bitcoin/observer/event_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2025-03-24T14:49:37.949Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
zetaclient/chains/bitcoin/observer/event.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2025-03-24T14:49:37.949Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
zetaclient/chains/bitcoin/observer/witness.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2025-03-24T14:49:37.949Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
🧬 Code Definitions (4)
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go (3)
e2e/runner/bitcoin.go (16)
r(37-70)r(74-94)r(98-102)r(105-137)r(140-157)r(161-191)r(193-199)r(201-325)r(328-374)r(377-381)r(384-386)r(390-399)r(402-418)r(423-444)txHash(111-111)err(110-110)e2e/runner/require.go (2)
r(19-27)r(30-49)testutil/sample/crosschain.go (2)
InboundParams(176-191)Status(248-260)
zetaclient/chains/bitcoin/observer/witness_test.go (2)
zetaclient/chains/bitcoin/observer/event.go (4)
BTCInboundEvent(23-50)event(53-76)event(80-139)err(82-82)zetaclient/chains/bitcoin/observer/observer.go (1)
Logger(70-76)
zetaclient/chains/bitcoin/observer/inbound_test.go (2)
zetaclient/testutils/testrpc/rpc_btc.go (1)
CreateBTCRPCAndLoadTx(61-80)zetaclient/testutils/testdata.go (1)
LoadBTCInboundRawResult(137-148)
e2e/runner/bitcoin.go (3)
e2e/runner/accounting.go (6)
r(36-50)r(52-87)r(89-137)r(140-171)r(173-188)r(190-216)e2e/runner/setup_bitcoin.go (4)
r(13-27)r(30-49)r(52-70)r(73-109)e2e/runner/logger.go (1)
Logger(22-27)
🔇 Additional comments (13)
changelog.md (1)
73-73: LGTM: Clear and descriptive changelog entry.The entry clearly communicates that Bitcoin deposits will now revert when memo output is missing, which aligns with the implementation in the code.
zetaclient/chains/bitcoin/observer/event_test.go (1)
171-178: LGTM: Well-structured test case for missing memo scenario.This test effectively verifies the new behavior where Bitcoin deposits with missing memos should revert, ensuring the system handles this edge case properly.
zetaclient/chains/bitcoin/observer/event.go (1)
88-92: LGTM: Proper error handling for missing memo condition.The implementation correctly identifies transactions with missing memos and returns an appropriate error, which will trigger a revert of the Bitcoin deposit as intended by the PR.
zetaclient/chains/bitcoin/observer/witness.go (3)
19-22: LGTM: Well-defined constant for missing memo identification.The
noMemoFoundconstant clearly communicates its purpose through both its name and comment.
74-76: LGTM: Improved comment clarity.The updated comment accurately describes the new behavior of setting a placeholder when no memo is found.
84-84: LGTM: Proper handling of missing memo scenario.Instead of leaving the memo as nil, the code now explicitly sets it to a recognizable placeholder value, enabling proper error handling in downstream components.
zetaclient/chains/bitcoin/observer/witness_test.go (1)
108-148: Well-structured test for the no-memo scenario.This test case properly validates the handling of Bitcoin transactions without a memo output, ensuring the system returns a standardized "no memo found" message rather than failing. The test's setup effectively simulates this edge case by removing the OP_RETURN output and verifies the expected behavior.
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go (2)
18-29: Good test coverage for nil memo scenario.This case effectively tests the system's behavior when a deposit is made without any memo output, ensuring that such transactions are properly reverted with an
INVALID_MEMOstatus rather than causing failures elsewhere in the system.
30-40: Appropriate test for empty memo case.This test properly verifies that empty memos are handled consistently with the system's requirements, resulting in a reverted transaction with the correct status. This complements the nil memo test by covering another important edge case.
e2e/runner/bitcoin.go (1)
247-259: Good implementation of optional memo handling.The conditional handling of memo data appropriately supports the testing of no-memo scenarios for transaction reversion. This change aligns well with the PR's objective to fix handling of Bitcoin deposits with missing memo outputs.
zetaclient/chains/bitcoin/observer/inbound_test.go (3)
476-507: Properly implemented test for transactions without memo.This test case correctly verifies that transactions without a memo are now processed (not skipped) and include the standardized "no memo found" message. This change is consistent with the PR's objective to improve handling of Bitcoin deposits with missing memo outputs.
586-612: Updated test to match new transaction handling policy.This test has been appropriately modified to verify that transactions with non-OP_RETURN outputs are now processed rather than skipped. This reflects the shift from silently dropping problematic transactions to processing them with proper error handling.
614-640: Correctly updated test for memo decoding failures.This test now properly verifies that transactions with memo decoding failures are processed rather than skipped, which allows for more robust error reporting and tracking in the system. The test setup effectively simulates the error condition by modifying the memo length.
…und w… (#3754) * cherry pick mainnet bitcoin fix to be able to revert missed inbound without memo * add changelog entry
* parse withdraw payload * prepare withdraw and call and add payload parsing * initialize withdraw and call test * add format payload function * changelog * fix parsing * improve testing * change returns * Update pkg/contracts/sui/payload.go Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com> * fix: cherry pick mainnet bitcoin fix to be able to revert missed inbound w… (#3754) * cherry pick mainnet bitcoin fix to be able to revert missed inbound without memo * add changelog entry * chore(e2e): print stack trace on nil dereference (#3751) * chore(e2e): print stack trace on nil dereference * fix go vet * Update e2e/runner/run.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix imports * use const for abi args * create payloadCall object * add argument for payload * refactor rawPayload --------- Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com> Co-authored-by: Charlie Chen <34498985+ws4charlie@users.noreply.github.com> Co-authored-by: Alex Gartner <alexg@zetachain.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Description
This PR is to cherry pick the mainnet
v29fix: #3752How Has This Been Tested?
Summary by CodeRabbit