feat: cumulative confirmations tier per BTC block - #48
Conversation
frolvanya
left a comment
frolvanya
left a comment
There was a problem hiding this comment.
This is also a breaking change for our relayer, since it relies on get_confirmations method that was replaced by relayer_delta_for_predecessor, so we need to prepare all off-chain services for this before the upgrade
| dao_call( | ||
| &context, | ||
| "extend_relayer_white_list", | ||
| json!({ "relayer_ids": [context.relayer.id()] }), |
There was a problem hiding this comment.
Every test here operate only with whitelisted relayer, so confirmations_delta is 0, while old tests operate without whitelisted relayer, so we won't see possible regression. Let's add a couple new tests without whitelisting
There was a problem hiding this comment.
Pull request overview
Adds per-BTC-block cumulative deposit confirmation tiers to prevent split-deposit bypasses.
Changes:
- Introduces a fixed-capacity block-amount ring and callback-side confirmation checks.
- Integrates the light client’s inclusion-with-heights API across deposits and refunds.
- Adds migrations, mocks, and tests for cumulative tiers and ring behavior.
Reviewed changes
Copilot reviewed 19 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Ignores environment files. |
Cargo.lock |
Updates workspace package versions. |
contracts/mock-btc-light-client/Cargo.toml |
Bumps mock version. |
contracts/mock-btc-light-client/src/lib.rs |
Mocks inclusion height responses. |
contracts/satoshi-bridge/Cargo.toml |
Bumps bridge version. |
contracts/satoshi-bridge/src/api/bridge.rs |
Passes mandatory coinbase proofs. |
contracts/satoshi-bridge/src/api/management.rs |
Resizes the ring after configuration changes. |
contracts/satoshi-bridge/src/block_amount_ring.rs |
Implements cumulative per-block tracking. |
contracts/satoshi-bridge/src/btc_light_client/active_utxo_management.rs |
Preserves relayer confirmation deltas. |
contracts/satoshi-bridge/src/btc_light_client/deposit.rs |
Applies cumulative confirmation checks to deposits. |
contracts/satoshi-bridge/src/btc_light_client/mod.rs |
Adds inclusion proof and height APIs. |
contracts/satoshi-bridge/src/btc_light_client/withdraw.rs |
Preserves withdrawal confirmation behavior. |
contracts/satoshi-bridge/src/config.rs |
Adds maximum-confirmation helpers. |
contracts/satoshi-bridge/src/legacy.rs |
Migrates legacy state with an initialized ring. |
contracts/satoshi-bridge/src/lib.rs |
Adds ring state and versioned data. |
contracts/satoshi-bridge/src/refund.rs |
Requires maximum-tier depth for refunds. |
contracts/satoshi-bridge/src/upgrade.rs |
Migrates V6 contract state. |
contracts/satoshi-bridge/src/utils.rs |
Adds inclusion-result size limit. |
contracts/satoshi-bridge/src/zcash_utils/mod.rs |
Removes obsolete lint suppression. |
contracts/satoshi-bridge/tests/test_block_limit.rs |
Tests cumulative tiers and ring behavior. |
contracts/satoshi-bridge/tests/test_upgrade.rs |
Updates supported upgrade fixtures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| confirmations_delta: u64, | ||
| ) -> PromiseOrValue<bool> { | ||
| let result_bytes = env::promise_result_checked(0, MAX_BOOL_RESULT) | ||
| .expect("Call verify_transaction_inclusion failed"); | ||
| let is_valid = serde_json::from_slice::<bool>(&result_bytes) | ||
| .expect("verify_transaction_inclusion return not bool"); | ||
| require!(is_valid, "verify_transaction_inclusion return false"); | ||
| self.process_inclusion_and_check(&pending_utxo_info, confirmations_delta); |
There was a problem hiding this comment.
The cumulative only ever rises, so required only ever rises, therefore this can never accept an under-confirmed deposit and the worse case is delaying other transfers in same block
There was a problem hiding this comment.
Not fixing: the bump can only over-count, so failures only make the tier stricter (fail-closed). A correct undo needs eviction/saturation handling and adds more risk than it removes.
| let cumulative = self | ||
| .data_mut() | ||
| .block_bridge_amounts | ||
| .bump(block_height, amount) | ||
| .unwrap_or(u128::MAX); | ||
| let required = self.internal_config().get_confirmations(cumulative) + delta; |
| let is_valid = serde_json::from_slice::<bool>(&result_bytes) | ||
| .expect("verify_transaction_inclusion return not bool"); | ||
| require!(is_valid, "verify_transaction_inclusion return false"); | ||
| self.process_inclusion_and_check(&pending_utxo_info, confirmations_delta); |
There was a problem hiding this comment.
In verify_safe_deposit_callback, process_inclusion_and_check bumps the ring before the safe_mint cross-contract call. If safe_mint returns U128(0) (account not registered), safe_mint_callback removes verified_deposit_utxo and refunds the storage NEAR — but the ring bump is not unwound (it lives in a prior tx and can't be atomically reverted from a later callback).
- Attacker pays real BTC to lock at the deposit address, plus storage NEAR.
- Ring cumulative for that block is permanently inflated by the deposit amount.
- Future legitimate deposits at the same BTC block face a higher confirmations tier than they otherwise would.
- Attack strengthens the security requirement, doesn't weaken it. No fund loss.
There was a problem hiding this comment.
On the one hand, I completely agree with the point above. However, I'm not sure this actually needs to be fixed. Maybe we should leave it as is, and if we start seeing it become a problem, then we can fix it?
| } | ||
| } | ||
|
|
||
| pub fn resize(&mut self, new_capacity: usize) { |
There was a problem hiding this comment.
When the DAO changes config in a way that alters capacity_for(config), entries whose new-modulo slots collide are resolved by keeping the higher block height; the loser's cumulative_sats is discarded. Any subsequent deposit for a discarded block returns None from bump() → max tier. Fail-closed, but worth documenting that a config change during heavy deposit load transiently forces some in-flight blocks to max-tier.
There was a problem hiding this comment.
Shrinking is provably safe. On a resize collision the discarded entry is always the older block, and colliding heights differ by a multiple of the new capacity, so the discarded block is at least capacity_new deep. Since capacity_new = max_required_confirmations + slack, its depth already exceeds the max tier by more than the slack — the None → max tier fallback is trivially satisfied at the moment of eviction, so no deposit is actually delayed.
The real transitional effect is in the opposite direction: when the window grows, blocks the ring had legitimately forgotten re-enter the new window with their cumulative reset to zero, so a deposit from such a block can temporarily pass under a lower tier than the full block sum would require under the new policy. I consider this acceptable: the under-count per block is bounded by the amount deposited before the config change, and that amount was fully subject to the then-active policy — during the transition the system is never weaker than the old rules. The effect is one-off and disappears within capacity_new blocks after the change. Raising tiers is a rare DAO action, so I wouldn't add extra handling for this.
| } | ||
|
|
||
| pub fn get_confirmations(&self, config: &Config, satoshi_amount: u128) -> u64 { | ||
| /// Must be called at the synchronous entry point of a verify_* function — |
There was a problem hiding this comment.
For that reason I think it's less error-prone to keep predecessor call out of this method and instead pass it as "relayer_account_id"
There was a problem hiding this comment.
relayer_delta should be removed in the future
Confirmations tier now computed against the SUM of bridge tx amounts in a BTC
block, closing the split-deposit bypass (one big deposit chunked into many
small to fall under a lower tier).
Per-block sums in a fixed-capacity ring keyed by
block_height % cap. Usesverify_transaction_inclusion_with_heights(Near-One/btc-light-client-contract#140) to get tip + block height in one call; confirmations check moves to our callback.