Skip to content

fix(checkpoint): avoid withdrawal count truncation - #179

Open
storopoli wants to merge 1 commit into
mainfrom
codex/fix-batch-withdrawal-utxo-count-truncation
Open

fix(checkpoint): avoid withdrawal count truncation#179
storopoli wants to merge 1 commit into
mainfrom
codex/fix-batch-withdrawal-utxo-count-truncation

Conversation

@storopoli

Copy link
Copy Markdown
Member

Motivation

  • The checkpoint DepositPool previously cast each withdrawal multiple from u64 to u32 while accumulating required, causing truncation for very large withdrawals and allowing oversized withdrawals to pass verification. This could lead to the bridge attempting more assignments than available UTXOs and panicking.
  • The intent of the change is to ensure verification uses full-precision accounting so a checkpoint cannot undercount required UTXOs and drive a deterministic failure during bridge processing.

Description

  • Use a u64 accumulator for the required UTXO count in crates/subprotocols/checkpoint/verification/src/deposit_pool.rs so (amt / denom) is never truncated during accumulation.
  • Compare the u64 required against self.count as u64 and only cast back to u32 after the bounds check when producing VerifiedWithdrawals.remaining_count.
  • Add a regression test large_multi_denomination_intent_does_not_truncate_required_count that verifies a 1-sat denomination withdrawal of u32::MAX + 1 sats is rejected as insufficient funds instead of being truncated.

Testing

  • Ran cargo test -p strata-checkpoint-verification deposit_pool, and all tests passed (15 passed; 0 failed).
  • Ran cargo fmt --check --package strata-checkpoint-verification, which succeeded.
  • Ran git diff --check to ensure no whitespace or diff issues, which reported no problems.

Codex Task

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
...tocols/checkpoint/verification/src/deposit_pool.rs 98.24% <100.00%> (+0.09%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Commit: 92e516d
SP1 Execution Results

program cycles gas
asm-stf 136,419,339 134,134,768
moho 5,223,535 5,525,318

@prajwolrg prajwolrg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is incomplete. We still treat VerifiedWithdrawals.remaining_count as u32. I'm not sure what benefit we are getting with the changes here.

Comment on lines +324 to +325
let intents = vec![withdrawal(u32::MAX as u64 + 1)];
let err = pool.verify_withdrawals(&intents).unwrap_err();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test is doing anything helpful.

The error basically is saying we are trying to withdraw more than in the pool and I don't think this has anything to do with the use of u32. There will be the same error even if we are using withdrawal of 2 sat because we only have 1 sat in the pool.

I suggest having two commits, one where the test is failing because of this issue and the next commit where we fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants