Skip to content

fix(ledger): publish transaction data and indexes atomically - #1782

Merged
Dodecahedr0x merged 1 commit into
masterfrom
dode/fix-signature-publication
Oct 9, 2026
Merged

Dodecahedr0x merged 1 commit into
masterfrom
dode/fix-signature-publication

Conversation

@Dodecahedr0x

Copy link
Copy Markdown
Contributor

What changed

Summary

An address signature could become visible before its transaction bytes were persisted, causing a concurrent getTransaction lookup to return null and the TEE filter to omit the signature. Publish transaction bytes, metadata, and slot/address indexes in one atomic RocksDB write batch, then advance cached counts after the write succeeds.

Closes #1781

Impact

Breaking Changes

None. Storage keys, serialization, RPC behavior, and permission checks retain their existing formats and semantics. The change removes the partial-publication window without retries. It replaces individual database writes with batch construction and buffering; sustained throughput and allocation changes have not been benchmarked.

Reviewer notes

Test Plan

The concurrent regression requires every newly indexed signature to resolve to complete transaction bytes and matching status, and checks cached counts. The former status-only helper is now test-only for fixtures that deliberately construct incomplete index/status records. The affected invariant is that accepted transactions have a durable, queryable outcome; slot/index ordering and serialization are preserved.

The local reproduction submits valid noops with the queried account writable before 90 readonly accounts and polls signatures and transactions concurrently. This exercises the publication window through the original filter without injected RPC failures. Raw upstream responses from the deployed TEE remain unavailable.

@Dodecahedr0x Dodecahedr0x self-assigned this Oct 8, 2026
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: magicblock-labs/magicblock-validator/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 543907f5-8798-4bf1-a44d-48e5fcd72143

📥 Commits

Reviewing files that changed from the base of the PR and between cc32775 and 1078040.


📒 Files selected for processing (1)
  • magicblock-ledger/src/store/api.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.



📝 Walkthrough

Walkthrough

Ledger::write_transaction now writes transaction data, status, and lookup indexes in one database batch. Cached counts update only after the batch succeeds. A concurrency test checks that signatures exposed by address indexes resolve to transaction data and matching status.

Changes

Ledger transaction publication

Layer / File(s) Summary
Atomic transaction write and regression coverage
magicblock-ledger/src/store/api.rs
write_transaction batches transaction bytes, status, the slot-signature mapping, and writable/readonly address indexes. It updates cached counts after a successful batch. The status-only helper is test-only. A concurrency test checks indexed signatures against transaction data, status, and final counts.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: bmuddha


Merge Risk: ⚪ Minimal · up to 10780

The transaction-publication change is mergeable after normal checks; no actionable merge-blocking issue was identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check Passed Issue #1781 requires atomic publication of transaction bytes, status, slot/signature mapping, and address indexes. The PR summary reports that Ledger::write_transaction writes all of these records i…
Out of Scope Changes check Passed The reported changes are limited to the ledger write path, its cached-count update order, and supporting regression-test fixtures and coverage. Making write_transaction_status test-only supports inc…
Docstring Coverage Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files.
Title check Passed The title clearly identifies the main change: atomic publication of transaction data and indexes in the ledger.
Description check Passed The description directly explains the race condition, the atomic RocksDB batch change, the test plan, and the expected impact.


✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

redsuite: PR vs master

Single-run diff on shared runners — indicative only; statistical verdicts come from Bencher thresholds.

redline/ws_fanout_threshold/ws8
  delivery us                        median 516 → 189 (-63.4%)  p95 1189 → 378 (-68.2%)  ▼ better
  validator tx processing avg us     182.8 → 81.3 (-55.5%)  ▼ better
  (11 flat/mixed/info metric(s) not shown)

nothing worse than base

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Projectmagicblock-labs
Branchdode/fix-signature-publication
Testbedblacksmith-8vcpu-ubuntu-2404

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🐰 View full continuous benchmarking report in Bencher

@Dodecahedr0x
Dodecahedr0x marked this pull request as ready for review October 9, 2026 08:45
@Dodecahedr0x
Dodecahedr0x merged commit 1a79ddf into master Oct 9, 2026
42 checks passed
@Dodecahedr0x
Dodecahedr0x deleted the dode/fix-signature-publication branch October 9, 2026 08:49
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.

3 participants