feat: forward original bincoded transaction#991
feat: forward original bincoded transaction#991bmuddha wants to merge 1 commit intobmuddha/accountsdb/checksumfrom
Conversation
keep original bincoded transaction along with the sanitized version, this can will be used by future ledger and replicator implementations
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughThe pull request introduces a mechanism to preserve original bincode-encoded transaction bytes as they flow through the system. A new wrapper type Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-aperture/src/requests/http/simulate_transaction.rs (1)
53-57:⚠️ Potential issue | 🟠 Major
simulate()receives stale encoded bytes — passtransaction.txninstead.When
replace_recent_blockhash=true,prepare_transactionreplaces the blockhash in the deserialized transaction before constructingWithEncoded. Theencodedfield therefore contains the original wire bytes (with the client's blockhash), whiletransaction.txnholds the modifiedSanitizedTransaction. Passing the fullWithEncoded<SanitizedTransaction>tosimulate()propagates this inconsistency:TransactionSchedulerHandle::sendcallssanitize_with_encoded(true), which returnsSome(stale_bytes), soProcessableTransaction { encoded: Some(stale_bytes), ... }reaches the scheduler with bytes that don't match the transaction being simulated.If any downstream consumer reads
encodedon a simulationProcessableTransaction, it will operate on the wrong bytes (wrong blockhash).The fix is to pass only the inner sanitized transaction so
encodedisNonein the resultingProcessableTransaction:🐛 Proposed fix
- let result = self - .transactions_scheduler - .simulate(transaction) - .await - .map_err(RpcError::transaction_simulation)?; + let result = self + .transactions_scheduler + .simulate(transaction.txn) + .await + .map_err(RpcError::transaction_simulation)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-aperture/src/requests/http/simulate_transaction.rs` around lines 53 - 57, The call to transactions_scheduler.simulate(...) is passing a WithEncoded<SanitizedTransaction> which contains stale encoded bytes (the original client wire bytes) because prepare_transaction replaced the blockhash on the deserialized txn but left encoded unchanged; change the call to pass the inner sanitized transaction (transaction.txn) instead so the scheduler receives a transaction with encoded == None and cannot propagate stale bytes—locate the simulate(...) invocation in simulate_transaction.rs and replace the WithEncoded argument with transaction.txn, leaving all other error mapping (RpcError::transaction_simulation) intact; this ensures TransactionSchedulerHandle::send and its sanitize_with_encoded(true) path produce a ProcessableTransaction without stale encoded bytes.magicblock-aperture/src/requests/http/send_transaction.rs (1)
17-35:⚠️ Potential issue | 🟡 Minor
signaturespan field is declared but never recorded.
fields(signature = tracing::field::Empty)creates a lazy span field that must be explicitly filled viaSpan::current().record(...). Since that call was removed (per PR summary), the field is always empty in traces — a regression in observability for this hot path.🔧 Proposed fix — record signature after extraction
+use tracing::Span; #[instrument(skip(self, request), fields(signature = tracing::field::Empty))] pub(crate) async fn send_transaction( &self, request: &mut JsonRequest, ) -> HandlerResult { // ... let signature = *transaction.txn.signature(); + Span::current().record("signature", tracing::field::display(&signature));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-aperture/src/requests/http/send_transaction.rs` around lines 17 - 35, The span field "signature" declared on send_transaction (fields(signature = tracing::field::Empty)) is never recorded; after extracting signature (the signature variable) call Span::current().record(...) to populate that field (use tracing::field::display(...) or the string form of signature) so the span contains the actual signature value; ensure this happens right after let signature = *transaction.txn.signature() within the send_transaction function.magicblock-aperture/src/requests/http/mod.rs (1)
176-215:⚠️ Potential issue | 🟠 Major
WithEncodedinvariant broken whenreplace_blockhash=true.After the blockhash replacement at line 201–204,
transaction.messageholds the new blockhash butencodedstill contains the original wire bytes. The returnedWithEncoded { txn, encoded }therefore hastxn.recent_blockhash != bincode::deserialize::<VersionedTransaction>(&encoded).recent_blockhash. The doc comment acknowledges bytes are "unused" for simulation, but the struct conveys no such contract —encodedlooks like it matchestxnto any future reader.The downstream impact is tracked in the
simulate_transaction.rscomment above. The fix there (passingtransaction.txntosimulate()) is sufficient, but you may also want to tighten the invariant here by only constructingWithEncodedwhen bytes are guaranteed consistent:💡 Alternative — conditionally preserve encoded bytes
- let txn = transaction.sanitize(sigverify)?; - Ok(WithEncoded { txn, encoded }) + let txn = transaction.sanitize(sigverify)?; + // Only preserve bytes when they still correspond to the sanitized txn + // (i.e., the blockhash was not replaced) + if replace_blockhash { + Ok(WithEncoded { txn, encoded: Vec::new() }) + } else { + Ok(WithEncoded { txn, encoded }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-aperture/src/requests/http/mod.rs` around lines 176 - 215, prepare_transaction currently mutates transaction.message when replace_blockhash is true but leaves `encoded` as the original wire bytes, breaking the invariant that `encoded` matches the returned `txn`; fix this in prepare_transaction by reserializing the modified `transaction` into `encoded` after calling set_recent_blockhash (e.g. call bincode::serialize(&transaction).map_err(RpcError::invalid_params) and assign to `encoded`) so that the returned WithEncoded { txn, encoded } stays consistent (alternatively, if you intentionally don't want to preserve bytes for simulations, return an empty/absent encoded only when replace_blockhash is true, but prefer reserializing to maintain the invariant).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-core/src/link/transactions.rs`:
- Around line 169-172: The bincode serialization failure in with_encoded is
incorrectly mapped to TransactionError::SanitizeFailure; update the error
handling to either (preferred) add a new enum variant like
TransactionError::SerializationError (or TransactionError::InvalidTransaction)
in the TransactionError definition and map bincode::serialize errors to that
variant, updating all match/uses accordingly, or (if adding a variant is not
possible now) add a clear inline comment above the map_err call explaining that
serialization failures are being mapped to SanitizeFailure as a deliberate
substitution and why; reference the with_encoded function and the WithEncoded
result wrapper when making the change.
- Around line 164-173: Remove the dead helper function by deleting the
with_encoded<T> function (the block that calls bincode::serialize, maps errors
to TransactionError::SanitizeFailure, and returns WithEncoded { txn, encoded });
also remove any now-unused imports or references introduced solely for that
function (e.g., bincode::serialize, Serialize bound, and WithEncoded if it is
only used by this helper) so there are no lingering unused symbols.
---
Outside diff comments:
In `@magicblock-aperture/src/requests/http/mod.rs`:
- Around line 176-215: prepare_transaction currently mutates transaction.message
when replace_blockhash is true but leaves `encoded` as the original wire bytes,
breaking the invariant that `encoded` matches the returned `txn`; fix this in
prepare_transaction by reserializing the modified `transaction` into `encoded`
after calling set_recent_blockhash (e.g. call
bincode::serialize(&transaction).map_err(RpcError::invalid_params) and assign to
`encoded`) so that the returned WithEncoded { txn, encoded } stays consistent
(alternatively, if you intentionally don't want to preserve bytes for
simulations, return an empty/absent encoded only when replace_blockhash is true,
but prefer reserializing to maintain the invariant).
In `@magicblock-aperture/src/requests/http/send_transaction.rs`:
- Around line 17-35: The span field "signature" declared on send_transaction
(fields(signature = tracing::field::Empty)) is never recorded; after extracting
signature (the signature variable) call Span::current().record(...) to populate
that field (use tracing::field::display(...) or the string form of signature) so
the span contains the actual signature value; ensure this happens right after
let signature = *transaction.txn.signature() within the send_transaction
function.
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 53-57: The call to transactions_scheduler.simulate(...) is passing
a WithEncoded<SanitizedTransaction> which contains stale encoded bytes (the
original client wire bytes) because prepare_transaction replaced the blockhash
on the deserialized txn but left encoded unchanged; change the call to pass the
inner sanitized transaction (transaction.txn) instead so the scheduler receives
a transaction with encoded == None and cannot propagate stale bytes—locate the
simulate(...) invocation in simulate_transaction.rs and replace the WithEncoded
argument with transaction.txn, leaving all other error mapping
(RpcError::transaction_simulation) intact; this ensures
TransactionSchedulerHandle::send and its sanitize_with_encoded(true) path
produce a ProcessableTransaction without stale encoded bytes.
| /// Encodes a transaction to bincode and wraps it with its encoded form. | ||
| /// Use for internally-constructed transactions that need the encoded bytes. | ||
| pub fn with_encoded<T>(txn: T) -> Result<WithEncoded<T>, TransactionError> | ||
| where | ||
| T: Serialize, | ||
| { | ||
| let encoded = bincode::serialize(&txn) | ||
| .map_err(|_| TransactionError::SanitizeFailure)?; | ||
| Ok(WithEncoded { txn, encoded }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all call sites of with_encoded across the repo
rg -n --type rust '\bwith_encoded\s*\(' -A 2 -B 2Repository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Broader search for with_encoded without strict pattern matching
rg 'with_encoded' --type rust -iRepository: magicblock-labs/magicblock-validator
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Search for direct calls to with_encoded function, excluding method references
rg '::with_encoded\s*\(|with_encoded\s*\(' --type rust -B 2 -A 2Repository: magicblock-labs/magicblock-validator
Length of output: 1670
🏁 Script executed:
#!/bin/bash
# Search for with_encoded in test files and check module exports
rg 'with_encoded' --type rust -C 3 | grep -E '(test|pub use|mod)'
# Also explicitly search test files
fd '\.rs$' | xargs grep -l 'with_encoded' | grep -i testRepository: magicblock-labs/magicblock-validator
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Final verification - simple search for function calls
rg 'with_encoded\s*\(' --type rust
# Also check if it's referenced in documentation or comments that might indicate intended usage
rg -i 'with_encoded' --type rust | grep -v 'sanitize_with_encoded' | grep -v 'pub fn with_encoded'Repository: magicblock-labs/magicblock-validator
Length of output: 445
Remove the unused with_encoded function.
The function has no call sites in the codebase and is dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-core/src/link/transactions.rs` around lines 164 - 173, Remove the
dead helper function by deleting the with_encoded<T> function (the block that
calls bincode::serialize, maps errors to TransactionError::SanitizeFailure, and
returns WithEncoded { txn, encoded }); also remove any now-unused imports or
references introduced solely for that function (e.g., bincode::serialize,
Serialize bound, and WithEncoded if it is only used by this helper) so there are
no lingering unused symbols.
| { | ||
| let encoded = bincode::serialize(&txn) | ||
| .map_err(|_| TransactionError::SanitizeFailure)?; | ||
| Ok(WithEncoded { txn, encoded }) |
There was a problem hiding this comment.
TransactionError::SanitizeFailure is misleading for a serialization error.
A bincode serialization failure in with_encoded has nothing to do with transaction sanitization. If TransactionError has no better variant (e.g., InvalidTransaction or SerializationError), at minimum add an inline comment explaining the substitution so future readers understand the intent.
let encoded = bincode::serialize(&txn)
- .map_err(|_| TransactionError::SanitizeFailure)?;
+ // No specific serialization error variant in TransactionError; SanitizeFailure is used as the closest proxy.
+ .map_err(|_| TransactionError::SanitizeFailure)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| let encoded = bincode::serialize(&txn) | |
| .map_err(|_| TransactionError::SanitizeFailure)?; | |
| Ok(WithEncoded { txn, encoded }) | |
| { | |
| let encoded = bincode::serialize(&txn) | |
| // No specific serialization error variant in TransactionError; SanitizeFailure is used as the closest proxy. | |
| .map_err(|_| TransactionError::SanitizeFailure)?; | |
| Ok(WithEncoded { txn, encoded }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-core/src/link/transactions.rs` around lines 169 - 172, The bincode
serialization failure in with_encoded is incorrectly mapped to
TransactionError::SanitizeFailure; update the error handling to either
(preferred) add a new enum variant like TransactionError::SerializationError (or
TransactionError::InvalidTransaction) in the TransactionError definition and map
bincode::serialize errors to that variant, updating all match/uses accordingly,
or (if adding a variant is not possible now) add a clear inline comment above
the map_err call explaining that serialization failures are being mapped to
SanitizeFailure as a deliberate substitution and why; reference the with_encoded
function and the WithEncoded result wrapper when making the change.
1 participant
Summary
Keep the original bincoded body of transaction along with sanitized
version. Forward it to the scheduler, so it can be reused for the
purposes of replication and future ledger persistence.
Compatibility
Testing
Checklist
Summary by CodeRabbit