Skip to content

fix(fullnode): avoid rewriting pending proofs on retry#3176

Open
tonykim525 wants to merge 8 commits intochainwayxyz:nightlyfrom
tonykim525:3168-fix-pendingproofs-disk-usage
Open

fix(fullnode): avoid rewriting pending proofs on retry#3176
tonykim525 wants to merge 8 commits intochainwayxyz:nightlyfrom
tonykim525:3168-fix-pendingproofs-disk-usage

Conversation

@tonykim525
Copy link
Contributor

@tonykim525 tonykim525 commented Mar 4, 2026

Description

This PR fixes unnecessary disk-write amplification in the PendingProof retry flow.

When a proof remains pending, retry processing could re-store the same pending proof every L1 cycle. This increases write pressure/disk usage without changing logical state.

This PR includes:

  • a precheck reproduction test (sov-db)
  • the fullnode retry-path fix (da_block_handler.rs)

Implementation summary:

  • Added ProofSource (FromL1, FromPendingRetry).
  • Fresh L1 proofs use FromL1; pending-table retries use FromPendingRetry.
  • store_pending_proof(...) now runs only for FromL1.
  • Retry-origin pending proofs are kept pending without rewrite.
  • Updated logs to match actual behavior.

Logical semantics are preserved: while a proof is still pending, its existing PendingProof entry is kept without rewrite across retry cycles.

Testing

Targeted reproduction:

  • cargo test -p sov-db --features "sov-schema-db/test-utils" test_pending_proofs_current_flow_rewrite_increases_disk_usage -- --nocapture

Observed:

  • current_rewrites=40, fixed_rewrites=0
  • current_pre_flush_db_bytes=2981196, fixed_pre_flush_db_bytes=357396
  • current_post_flush_db_bytes=428461, fixed_post_flush_db_bytes=362817
  • current_live_bytes=66616, fixed_live_bytes=66616
  • current_total_sst_bytes=66616, fixed_total_sst_bytes=66616
  • Result: 1 passed; 0 failed; 7 filtered out (targeted test-name filter)

Interpretation:

  • Current flow rewrites once per cycle; fixed flow skips rewrites.
  • Pre-flush disk footprint is significantly higher in current flow (~8.34x).
  • Live data/SST converge in this synthetic setup, so logical state is preserved while redundant writes are removed.

Full sov-db package test run:

  • cargo test -p sov-db --features "sov-schema-db/test-utils" -- --nocapture
  • Result: 8 passed; 0 failed (doc-tests: 0 passed; 0 failed; 2 ignored)

Additional validation:

  • rustfmt +nightly --check crates/fullnode/src/da_block_handler.rs
  • cargo check -p citrea-fullnode (passed)
  • cargo test -p citrea-fullnode --no-run (passed)
  • cargo test -p citrea --features testing --test bitcoin_e2e --no-run (passed)

Docs

No public API/interface changes.
No docs update required.

@tonykim525 tonykim525 requested a review from a team as a code owner March 4, 2026 10:13
@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 80.64516% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.5%. Comparing base (a030fa8) to head (2838b74).

Files with missing lines Patch % Lines
crates/fullnode/src/da_block_handler.rs 80.6% 6 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/fullnode/src/da_block_handler.rs 86.0% <80.6%> (-0.2%) ⬇️

... 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tonykim525
Copy link
Contributor Author

tonykim525 commented Mar 4, 2026

test(sov-db): make pending proof repro test clippy-compliant - 0d950d3

Only Commands run:

  1. rustfmt +nightly crates/sovereign-sdk/full-node/db/sov-db/src/ledger_db/tests.rs
  2. cargo clippy -p sov-db --all-targets --all-features --features "sov-schema-db/test-utils"
  3. SKIP_GUEST_BUILD=1 make lint

@tonykim525
Copy link
Contributor Author

I can see the Validate PR Title check is failing on this PR.

It did pass once earlier, and now appears to be failing due to workflow-side behavior.
From my side, this does not look fixable in this fork.
Could the maintainers please take a look and help unblock this check?

image

I addressed the two previous CI issues in follow-up commits:

  • fixed lint/format and clippy issues
  • added the required changelog entry
스크린샷 2026-03-04 21 08 23 스크린샷 2026-03-04 21 07 53

Could you please re-run CI on the latest commit?
If there’s anything else you’d like me to adjust, I’m happy to update it (tests, coverage, etc.).

@jfldde
Copy link
Contributor

jfldde commented Mar 4, 2026

Hey @tonykim525,
Just approved to re-run on latest commit. Thanks for the PR, it looks great. I'll give it a review tomorrow and try it out. Don't worry about the Validate PR Title for now, I'll open an issue regarding this.
Regarding #3168, I was more thinking about reducing PendinfProofs disk usage by storing bitcoin (block idx, tx idx) instead of storing the proof itself and going through a RPC call on re-processing.

jfldde
jfldde previously approved these changes Mar 6, 2026
Copy link
Contributor

@jfldde jfldde left a comment

Choose a reason for hiding this comment

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

Great PR. Removed the Fixes #3168 as this doesn't directly address it.

@tonykim525
Copy link
Contributor Author

Sorry — I moved the changelog entry before merging nightly. I’ve now resolved the changelog conflict and placed it under Unreleased as requested.

Removed the pending-proof disk-usage repro test, moved the changelog entry under Unreleased, and resolved the nightly merge conflict.

Validated with:

  • cargo test -p sov-db ledger_db::tests --no-run
  • rustfmt +nightly --check /Users/kim-yewon/IdeaProjects/citrea/crates/sovereign-sdk/full-node/db/sov-db/src/ledger_db/tests.rs

Both passed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants