Propagate cleanup boundary error in get_block#1121
Propagate cleanup boundary error in get_block#1121nikolaysamoil0ff wants to merge 2 commits intomagicblock-labs:masterfrom
Conversation
get_block() currently ignores the result of check_lowest_cleanup_slot(slot): ```rust let _lock = self.check_lowest_cleanup_slot(slot); ``` If the requested slot has already been cleaned up, the error is silently dropped and the method continues reading from storage. This can lead to returning partial or inconsistent block data past the cleanup boundary. The cleanup check is intended to enforce a strict boundary, so failures should be propagated instead of ignored. This change uses `?` to return the error early, ensuring predictable behavior and preventing invalid reads beyond the cleanup range.
📝 WalkthroughWalkthroughThe change updates error handling in ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-ledger/src/store/api.rs (2)
425-428:⚠️ Potential issue | 🟠 MajorReplace panic-prone metadata conversion with fallible error propagation.
On Line 427,
.unwrap()can panic in production if status meta conversion fails; this should return aLedgerErrorinstead.Proposed fix
- Ok(VersionedTransactionWithStatusMeta { - transaction, - meta: TransactionStatusMeta::try_from(meta).unwrap(), - }) + let meta = TransactionStatusMeta::try_from(meta).map_err(|e| { + LedgerError::TransactionConversionError( + format!( + "failed to convert transaction status meta at slot {}: {}", + slot, e + ), + ) + })?; + Ok(VersionedTransactionWithStatusMeta { + transaction, + meta, + })As per coding guidelines "Treat any usage of
.unwrap()or.expect()in production Rust code as a MAJOR issue."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-ledger/src/store/api.rs` around lines 425 - 428, The code currently uses TransactionStatusMeta::try_from(meta).unwrap() when constructing VersionedTransactionWithStatusMeta, which can panic; replace the unwrap with fallible error propagation by mapping the conversion error into the function's LedgerError result (e.g., using map_err or ? to convert into an appropriate LedgerError variant) and return Err(LedgerError::...) from the surrounding function instead of unwrapping, so the metadata conversion failure is reported as a LedgerError rather than causing a panic.
384-431: 🧹 Nitpick | 🔵 TrivialAdd a regression test for
get_blockon cleaned slots.Given this behavioral change, add a unit test that sets
lowest_cleanup_slot >= slotand assertsget_block(slot)returnsErr(LedgerError::SlotCleanedUp)rather thanOk(Some(_))/Ok(None).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-ledger/src/store/api.rs` around lines 384 - 431, Add a unit test for get_block that reproduces the cleaned-up-slot behavior by setting the store's lowest cleanup slot to be >= the target slot and asserting get_block(slot) returns Err(LedgerError::SlotCleanedUp); to implement, create a test that initializes the relevant store state, calls the helper that sets lowest_cleanup_slot (or directly manipulates the value checked by check_lowest_cleanup_slot), then call get_block(slot) and assert it returns the SlotCleanedUp error rather than Ok(Some(_))/Ok(None); reference the get_block method and the check_lowest_cleanup_slot helper and assert against LedgerError::SlotCleanedUp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@magicblock-ledger/src/store/api.rs`:
- Around line 425-428: The code currently uses
TransactionStatusMeta::try_from(meta).unwrap() when constructing
VersionedTransactionWithStatusMeta, which can panic; replace the unwrap with
fallible error propagation by mapping the conversion error into the function's
LedgerError result (e.g., using map_err or ? to convert into an appropriate
LedgerError variant) and return Err(LedgerError::...) from the surrounding
function instead of unwrapping, so the metadata conversion failure is reported
as a LedgerError rather than causing a panic.
- Around line 384-431: Add a unit test for get_block that reproduces the
cleaned-up-slot behavior by setting the store's lowest cleanup slot to be >= the
target slot and asserting get_block(slot) returns
Err(LedgerError::SlotCleanedUp); to implement, create a test that initializes
the relevant store state, calls the helper that sets lowest_cleanup_slot (or
directly manipulates the value checked by check_lowest_cleanup_slot), then call
get_block(slot) and assert it returns the SlotCleanedUp error rather than
Ok(Some(_))/Ok(None); reference the get_block method and the
check_lowest_cleanup_slot helper and assert against LedgerError::SlotCleanedUp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4645d85-c683-4387-bdfb-01e936733a6b
📒 Files selected for processing (1)
magicblock-ledger/src/store/api.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-ledger/src/store/api.rs (1)
1077-1084:⚠️ Potential issue | 🟠 MajorInconsistent error handling: same
.unwrap()pattern exists here.This code has the same issue that was fixed in
get_blockat lines 425-432. Thestatus.try_into().unwrap()can panic if the protobuf conversion fails. For consistency with the fix in this PR, this should use the samemap_errpattern.🔧 Proposed fix for consistency
result = self .transaction_status_cf .get_protobuf((signature, slot))? .map(|status| { - let status = status.try_into().unwrap(); - (slot, status) + TransactionStatusMeta::try_from(status).map(|meta| (slot, meta)) + }) + .transpose() + .map_err(|e| { + LedgerError::TransactionConversionError(format!( + "failed to convert transaction status meta at slot {}: {}", + slot, e + )) });As per coding guidelines:
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-ledger/src/store/api.rs` around lines 1077 - 1084, The code calls status.try_into().unwrap() inside the closure returned from transaction_status_cf.get_protobuf((signature, slot)), which can panic; update the closure to propagate the conversion error instead of unwrapping—use try_into() and convert its Err into the same StorageError used in the get_block fix (i.e., replace the unwrap with a map_err/from conversion so the closure returns Result<(Slot, Status), StorageError> and works with the surrounding ? operator), referencing transaction_status_cf.get_protobuf, the closure that maps status, and the try_into conversion to mirror the error-handling pattern used in get_block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@magicblock-ledger/src/store/api.rs`:
- Around line 1077-1084: The code calls status.try_into().unwrap() inside the
closure returned from transaction_status_cf.get_protobuf((signature, slot)),
which can panic; update the closure to propagate the conversion error instead of
unwrapping—use try_into() and convert its Err into the same StorageError used in
the get_block fix (i.e., replace the unwrap with a map_err/from conversion so
the closure returns Result<(Slot, Status), StorageError> and works with the
surrounding ? operator), referencing transaction_status_cf.get_protobuf, the
closure that maps status, and the try_into conversion to mirror the
error-handling pattern used in get_block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6467400-fa82-4605-9b82-dc3e662aeaa5
📒 Files selected for processing (1)
magicblock-ledger/src/store/api.rs
1 participant
Summary
get_block() currently ignores the result of check_lowest_cleanup_slot(slot):
If the requested slot has already been cleaned up, the error is silently dropped and the method continues reading from storage. This can lead to returning partial or inconsistent block data past the cleanup boundary.
The cleanup check is intended to enforce a strict boundary, so failures should be propagated instead of ignored.
This change uses
?to return the error early, ensuring predictable behavior and preventing invalid reads beyond the cleanup range.Compatibility
Testing
Checklist
Summary by CodeRabbit