Repository navigation
fix: correct transaction indexing - #603
Conversation
This commit introduces a correct way to index transactions within a block, which prevents entry overwrite bugs, which led to the broken ledger replay bugs and RPC method results
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
WalkthroughRefactors intra-slot transaction indexing from processor-managed atomic counters to a ledger-level per-slot AtomicU32 HashCache; adjusts logging from warn! to debug! in two HTTP handlers; updates tests and test helpers to align with automatic per-slot indexing and modifies a utility function's test-only visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler (old)
participant Executor as Executor (old)
participant LedgerOld as Ledger (old)
participant SchedulerNew as Scheduler (new)
participant LedgerCache as block_txn_indexes
participant LedgerNew as Ledger (new)
rect rgb(245, 245, 245)
Note over Scheduler,Executor: Old flow — processor-managed intra-slot index
Scheduler->>Executor: pass Arc<AtomicUsize> index
Executor->>LedgerOld: write_transaction(slot, transaction_slot_index)
LedgerOld-->>Executor: store using provided index
end
rect rgb(245, 255, 245)
Note over SchedulerNew,LedgerCache: New flow — ledger-managed per-slot index
SchedulerNew->>Executor: (no index passed)
Executor->>LedgerCache: entry(slot).or_default().1.fetch_add(1)
LedgerCache-->>Executor: per_slot_index
Executor->>LedgerNew: write_transaction(slot, per_slot_index)
LedgerNew-->>Executor: store using per-slot index
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-28T13:15:42.706ZApplied to files:
📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
📚 Learning: 2025-10-21T11:00:18.396ZApplied to files:
🧬 Code graph analysis (1)magicblock-ledger/src/store/api.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (9)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
magicblock-aperture/src/requests/http/send_transaction.rs(1 hunks)magicblock-aperture/src/requests/http/simulate_transaction.rs(1 hunks)magicblock-aperture/tests/transactions.rs(2 hunks)magicblock-ledger/Cargo.toml(1 hunks)magicblock-ledger/src/store/api.rs(12 hunks)magicblock-ledger/src/store/utils.rs(1 hunks)magicblock-ledger/tests/common.rs(1 hunks)magicblock-ledger/tests/get_block.rs(1 hunks)magicblock-ledger/tests/test_ledger_truncator.rs(4 hunks)magicblock-processor/src/executor/mod.rs(1 hunks)magicblock-processor/src/executor/processing.rs(0 hunks)magicblock-processor/src/scheduler.rs(1 hunks)
💤 Files with no reviewable changes (1)
- magicblock-processor/src/executor/processing.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-ledger/tests/common.rsmagicblock-processor/src/scheduler.rsmagicblock-processor/src/executor/mod.rsmagicblock-ledger/src/store/api.rsmagicblock-ledger/tests/get_block.rsmagicblock-ledger/tests/test_ledger_truncator.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-ledger/tests/common.rsmagicblock-ledger/Cargo.tomlmagicblock-ledger/src/store/api.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/scheduler.rsmagicblock-processor/src/executor/mod.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-processor/src/scheduler.rsmagicblock-ledger/src/store/api.rsmagicblock-ledger/tests/test_ledger_truncator.rs
🧬 Code graph analysis (2)
magicblock-ledger/tests/get_block.rs (1)
magicblock-ledger/tests/common.rs (1)
write_dummy_transaction(25-47)
magicblock-ledger/tests/test_ledger_truncator.rs (1)
magicblock-ledger/tests/common.rs (1)
write_dummy_transaction(25-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (15)
magicblock-aperture/src/requests/http/send_transaction.rs (1)
26-28: Verify that debug level is appropriate for transaction preparation failures.The logging level has been downgraded from
warn!todebug!for transaction preparation failures. While this reduces log noise, it may also hide important errors such as malformed transactions, encoding issues, or validation failures that could indicate client integration problems or attacks.Ensure that debug-level logging provides adequate observability for production monitoring and troubleshooting.
magicblock-aperture/tests/transactions.rs (2)
400-400: Clarify the purpose of advancing slots before generating test signatures.The addition of
env.advance_slots(1)before generating the test signatures appears related to the transaction indexing fix mentioned in the PR objectives. This likely ensures transactions are distributed across different slots for proper pagination testing.Please confirm whether this change is necessary to test the new per-slot transaction indexing behavior, and consider adding a comment explaining why the slot advance is required for this test.
424-424: Justify the pagination limit of 20.The "until" pagination test now includes
limit: Some(20), but the test expects exactly 3 results. This limit seems arbitrarily high for the test scenario.Please clarify:
- Why was a limit of 20 chosen when only 3 results are expected?
- Is this defensive against future test data expansion, or related to the indexing fix?
magicblock-aperture/src/requests/http/simulate_transaction.rs (1)
36-45: Clean error handling pattern with the same logging level consideration.The refactoring to use
inspect_errfor side-effect logging while propagating errors is idiomatic and improves code clarity. The error message appropriately includes context ("to simulate").However, the same consideration applies here as in
send_transaction.rs: downgrading transaction preparation failures todebug!level may reduce visibility of important errors in production environments. Ensure this aligns with your observability requirements.magicblock-ledger/src/store/utils.rs (1)
61-69: LGTM! Test-only helper properly scoped.The function is correctly restricted to test builds with
#[cfg(test)]and uses the fully-qualified type path.magicblock-ledger/Cargo.toml (1)
23-23: LGTM! Dependency correctly added.The
sccdependency is properly configured with workspace versioning and is used for theHashCacheconcurrent data structure in the ledger's per-slot transaction indexing.magicblock-ledger/tests/common.rs (1)
25-47: LGTM! Test helper correctly updated.The function signature has been properly updated to remove the explicit
transaction_slot_indexparameter, aligning with the internal per-slot index management now handled byblock_txn_indexesin the ledger.magicblock-ledger/tests/test_ledger_truncator.rs (1)
54-54: LGTM! Test calls correctly updated.All invocations of
write_dummy_transactionhave been properly updated to match the new signature without the explicit transaction index parameter.Also applies to: 83-83, 124-124, 180-180
magicblock-ledger/tests/get_block.rs (1)
42-43: LGTM! Test correctly validates transaction ordering.The test helper calls are properly updated, and the test continues to verify that transactions are retrieved in the correct order (lines 62-63, 67-68), which is crucial for validating the new per-slot indexing mechanism.
Also applies to: 51-52
magicblock-processor/src/executor/mod.rs (1)
1-1: LGTM! Per-executor indexing correctly removed.The removal of the
indexfield and associatedAtomicUsizeimport aligns with the architectural shift to ledger-level per-slot transaction indexing. The executor no longer needs to track transaction indexes locally.Also applies to: 31-106
magicblock-processor/src/scheduler.rs (1)
1-1: LGTM! Per-scheduler indexing correctly removed.The removal of the
indexfield and associated logic aligns with the shift to ledger-level transaction indexing. The scheduler no longer needs to manage or reset per-slot transaction indexes, and executor creation is properly updated.Also applies to: 26-78, 143-147
magicblock-ledger/src/store/api.rs (4)
74-74: LGTM! Concurrent per-slot index storage properly initialized.The
HashCache<Slot, AtomicU32>field provides lock-free concurrent access for per-slot transaction indexing, and is correctly initialized with the default constructor.Also applies to: 172-172
333-334: LGTM! Per-slot index correctly seeded.Each new slot is seeded with an atomic counter starting at 0. The ignored return value from
put()means if the slot already exists, the original counter is preserved, which prevents index resets and potential collisions.
1004-1009: LGTM! Per-slot transaction index correctly derived.The use of
entry(slot).or_default()withfetch_add(1, Ordering::Relaxed)correctly generates sequential per-slot transaction indexes in a thread-safe manner. TheRelaxedordering is appropriate since only atomicity of the increment is required, not synchronization with other operations.
610-665: LGTM! Middle range iteration logic looks correct.The iteration between upper and lower bounds correctly:
- Starts from the newest slot in reverse
- Stops at the limit, non-matching address, or oldest slot
- Skips entries where
tx_slot > newest_slot(lines 643-653), which might occur due to iterator positioning edge casesThe filtering logic properly collects signatures in the intended range.
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM, please address coderabbit comments though.
And I apologize, this is probably the darkest part of the ledger .. that code is heinous.
It was spaghetti in solana itself and I didn't do a good job of improving it much.
Sorry you hade to wade in that pit.
This commit introduces a correct way to index transactions within a block, which prevents entry overwrite bugs, which led to the broken ledger replay bugs and RPC method results <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Switched to per-slot automatic transaction indexing and simplified intra-slot ordering for more efficient ledger writes. * Adjusted logging verbosity in transaction prepare/dispatch paths (reduced from error/warn to debug). * Added a workspace dependency to support the new indexing mechanism. * **Tests** * Updated test helpers and pagination tests to match the new indexing and timing behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
This commit introduces a correct way to index transactions within a block, which prevents entry overwrite bugs, which led to the broken ledger replay bugs and RPC method results
Summary by CodeRabbit