fix: credit RustChain destination on completed bridge withdraw#5566
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
BCOS-L1
LubuSeb
left a comment
LubuSeb
left a comment
There was a problem hiding this comment.
Disclosure: I am reviewing this under Scottcjn/rustchain-bounties#73 as @LubuSeb.
Requesting changes on head b4e4b0cd7f06dca9f8a452d4796036589b7a5155. The patch fixes the missing credit on the normal withdraw completion path, and the focused + bridge regression suite passes locally, but the new credit is not made as an atomic one-time transition.
update_external_confirmation() reads the transfer before taking any write lock or doing a conditional terminal-state update. The update at completion time then sets status = 'completed' with only WHERE tx_hash = ?, and the new withdraw credit runs whenever the stale in-memory transfer[direction] == withdraw. Two duplicate bridge callbacks can both read the pre-completed row, both run the completion branch, and both credit balances.amount_i64 for the destination wallet.
I reproduced the stale-read interleaving with two SQLite connections against this PR head:
created withdraw transfer for 10 RTC
connection 1 reads status=pending
connection 2 reads status=pending
connection 1 completes and credits RTCdest1234 by 10_000_000
connection 2 completes from its stale read and credits RTCdest1234 by another 10_000_000
expected balance: 10_000_000
actual balance: 20_000_000
This means a duplicated external confirmation callback can mint/credit the RustChain destination twice even though the bridge transfer ends in a single completed state.
Suggested fix: make completion and crediting a single one-time transition. For example, acquire BEGIN IMMEDIATE before re-reading/updating the transfer, or use a conditional update such as UPDATE bridge_transfers ... WHERE tx_hash = ? AND status NOT IN ('completed','failed','voided') and only release/credit when the update actually changed one row. Add a regression that simulates two duplicate completion calls and asserts the destination balance remains exactly one transfer amount.
Validation I ran:
git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py
python -B -m py_compile node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py
PYTHONPATH=node python -B -m pytest -q node/test_bridge_withdraw_completion_credits_destination_poc.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.py --tb=short
# 48 passed
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
I found a correctness blocker in the new credit side effect. The callback path can double-credit a withdraw if the same completed callback is processed concurrently or after a stale read: update_external_confirmation reads the transfer before taking a write lock, then updates with WHERE tx_hash = ? only, and the new balance credit runs whenever this invocation computed new_status == 'completed'. Two workers can both read a pending transfer; after the first commits, the second can still update the already-completed row and add amount_i64 again. Please make the terminal transition atomic before crediting, for example BEGIN IMMEDIATE plus UPDATE ... WHERE tx_hash = ? AND status IN ('pending','locked','confirming') and only credit when that update wins. Also add the required SPDX header to the new test file so the BCOS gate accepts it.
ZacharyZhang-NY
left a comment
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
Reviewed the bridge withdraw completion credit fix at head b4e4b0c. The patch carries amount_i64 through get_bridge_transfer_by_hash() and credits the RustChain destination balance only when a withdraw transfer reaches completed status. The existing terminal-status guard prevents a later confirmation call from crediting the same completed transfer again.\n\nValidation performed:\n- python -B -m py_compile node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py\n- git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py\n- PYTHONPATH=node python -B -m pytest -q node/test_bridge_withdraw_completion_credits_destination_poc.py --noconftest -> 1 passed\n- PYTHONPATH=node python -B -m pytest -q node/test_bridge_withdraw_completion_credits_destination_poc.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.py --noconftest -> 48 passed\n- Manual idempotency probe: completed withdraw credited the destination once; a repeat confirmation returned current_status=completed and left the balance at one credit.\n\nI did not find a blocking issue in this PR.
|
Update after atomic-credit review feedback:
Verification on head |
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved.
I reviewed this on head ec20f3ac8832b8dd479dcde1ebf3ebe7dd662d26. The important behavior is covered: get_bridge_transfer_by_hash() now exposes amount_i64, the confirmation update only transitions rows still in pending/locked/confirming under BEGIN IMMEDIATE, and completed withdraw transfers credit balances[dest_address] exactly once. That closes the completed external-to-RustChain withdraw gap without turning stale/repeated callbacks into duplicate credits.
Validation run:
git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py
python3 -B -m py_compile node/bridge_api.py node/test_bridge_withdraw_completion_credits_destination_poc.py
PYTHONPATH=node python3 -B -m unittest -v node.test_bridge_precision node.test_bridge_withdraw_completion_credits_destination_poc
# runtime probe: create withdraw, complete it, retry same callback, assert no second creditResults: focused unittest ran 5 cases OK, including the new destination-credit and stale-callback coverage. The runtime probe returned {'first_ok': True, 'second_ok': False, 'second_error': 'Cannot update completed/failed/voided transfer', 'current_status': 'completed', 'balance_i64': 7500000, 'expected_i64': 7500000}.
No blockers from me.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
8 participants
Summary
get_bridge_transfer_by_hash()withdrawtransfer reaches the required external confirmation thresholdWhy
direction="withdraw"represents an external-chain to RustChain transfer. Before this change,update_external_confirmation()could mark the bridge transfercompletedwithout adding anybalances.amount_i64credit for the destination RustChain wallet, leaving funds stuck even though the bridge state said completion succeeded.The existing terminal-transfer early return keeps repeat confirmations from double-crediting a completed withdraw.
Validation
Observed locally:
48 passed, py_compile passed, andgit diff --checkwas clean.Duplicate / safety checks
Before opening this PR I searched public issues and PRs for
bridge withdraw destination credit,update_external_confirmation withdraw balance,bridge withdraw completed no balance, andexternal_tx_hash completed withdraw balances amount_i64. I did not find an exact duplicate.No private keys, tokens, credentials, or account-internal data are included.