feat: forward original bincoded transaction#991
feat: forward original bincoded transaction#991bmuddha wants to merge 2 commits 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 PR preserves original bincode-encoded transaction bytes by introducing WithEncoded and adding an optional encoded: Option<Vec> field to ProcessableTransaction. SanitizeableTransaction gains sanitize_with_encoded, and prepare_transaction now returns WithEncoded so entry-point bytes are captured and propagated. Several callsites and schedulers were updated to accept and forward the encoded payload. bincode and serde were added as workspace dependencies to support serialization. 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.
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 (2)
magicblock-aperture/src/requests/http/mod.rs (2)
186-199: 🧹 Nitpick | 🔵 TrivialNaming of
encodedlocal variable may mislead future readersThe variable
encoded(line 186) holds bytes obtained by decoding the base58/base64 client string. They are indeed the bincode form, but the flow — decode base58/base64 → call the resultencoded→bincode::deserialize(&encoded)— reads counter-intuitively. The former namedecoded(per the AI summary) was more accurate at the point of assignment, whilebincode_bytesorwire_byteswould be unambiguous in both contexts.This is purely a readability nit; no behaviour is affected.
🤖 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 186 - 199, Rename the local variable currently named `encoded` (used in the match on `UiTransactionEncoding` and passed to `bincode::deserialize`) to a clearer name such as `decoded`, `bincode_bytes`, or `wire_bytes` so the flow "decode base58/base64 → deserialize bincode" reads unambiguously; update all references (the match arms producing the Vec<u8> and the subsequent `bincode::deserialize(&encoded)` call that constructs the `VersionedTransaction`) to use the new identifier.
176-215:⚠️ Potential issue | 🟡 Minor
WithEncodedinvariant violated whenreplace_blockhash=trueWhen
replace_blockhash=true,transaction.messageis mutated (new blockhash set) before sanitization, butencodedstill holds the original wire bytes. The returnedWithEncoded { txn, encoded }is therefore inconsistent: deserializingencodedyields a transaction with a different blockhash thantxn.message().recent_blockhash().The doc comment acknowledges "bytes are unused" for simulation, and today's callers (
simulate_transaction.rs) do correctly dropencoded. However, this bakes a broken invariant into the return type:WithEncodedimpliesencodedis the canonical serialized form oftxn, which is false here. Any future caller that reaches forencodedin the simulation path will silently get stale bytes.Consider either:
- Returning
Option<Vec<u8>>for the encoded field (orNonewhenreplace_blockhash=true), or- Splitting this into two functions so the simulation path never produces a
WithEncodedat all.🤖 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, The prepare_transaction function currently returns WithEncoded { txn, encoded } even when replace_blockhash=true, violating the invariant that encoded serializes txn; to fix, change the return to omit or nullify encoded for the simulation path: update WithEncoded usage or type so its encoded field is Option<Vec<u8>> (or change prepare_transaction to return Result<SanitizedTransaction> for replace_blockhash=true and Result<WithEncoded> otherwise), ensure prepare_transaction (and callers like simulate_transaction.rs) construct WithEncoded only when encoded matches txn (i.e., when replace_blockhash is false) and return None/absent encoded when the blockhash was replaced before sanitize; adjust signatures and call sites to reflect the new optional/alternative return so no stale encoded bytes are exposed.
♻️ Duplicate comments (1)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
204-208: Same misleading "64KB" comment as inmagicblock-api/src/tickers.rs— see note there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-accounts/src/scheduled_commits_processor.rs` around lines 204 - 208, The comment beside the with_encoded(intent_sent_transaction) match is misleading (mentions "64KB" constraint); update the comment and/or log to accurately reflect the real reason for bincode failure: either remove the incorrect "all intent transactions are smaller than 64KB by construction" clause or replace it with a factual note (e.g., "unexpected bincode serialization failure — should be unreachable under normal inputs"); keep the error! call as-is (error!("Failed to bincode intent transaction");) and ensure the code references the same symbols (with_encoded and intent_sent_transaction) so the comment now correctly documents why this branch is considered unreachable.
🤖 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-aperture/src/requests/http/request_airdrop.rs`:
- Around line 38-40: The code currently constructs SerdeSignature from
txn.signatures.first().cloned().unwrap_or_default(), which silently returns an
all-zero signature if the invariant is violated; change this to fail hard and
surface the error instead: in the scope that builds the signature (referencing
SerdeSignature and txn.signatures), replace unwrap_or_default() with an explicit
check or expect() that returns an Err (or propagates with ?/bail!) when
txn.signatures.first() is None so the handler returns a clear error rather than
a zero signature.
In `@magicblock-api/src/tickers.rs`:
- Around line 93-97: The comment incorrectly attributes infallibility of
bincode::serialize to a 64KB size guarantee; instead update the comment and log
to state that serialization should not fail because the Transaction type (and
all its fields) implements Serialize, not because of any size limit. Locate the
with_encoded(tx) call and its surrounding comment/error (the block that
currently says "Unreachable case, all schedule commit txns are smaller than 64KB
by construction" and the error!("Failed to bincode intent transaction")),
replace the explanatory text to mention the Serialize invariant (or remove the
unreachable claim) and adjust the log message to reflect an unexpected
serialization failure rather than a size-related one; apply the same fix in the
analogous spot in scheduled_commits_processor.rs.
---
Outside diff comments:
In `@magicblock-aperture/src/requests/http/mod.rs`:
- Around line 186-199: Rename the local variable currently named `encoded` (used
in the match on `UiTransactionEncoding` and passed to `bincode::deserialize`) to
a clearer name such as `decoded`, `bincode_bytes`, or `wire_bytes` so the flow
"decode base58/base64 → deserialize bincode" reads unambiguously; update all
references (the match arms producing the Vec<u8> and the subsequent
`bincode::deserialize(&encoded)` call that constructs the
`VersionedTransaction`) to use the new identifier.
- Around line 176-215: The prepare_transaction function currently returns
WithEncoded { txn, encoded } even when replace_blockhash=true, violating the
invariant that encoded serializes txn; to fix, change the return to omit or
nullify encoded for the simulation path: update WithEncoded usage or type so its
encoded field is Option<Vec<u8>> (or change prepare_transaction to return
Result<SanitizedTransaction> for replace_blockhash=true and Result<WithEncoded>
otherwise), ensure prepare_transaction (and callers like
simulate_transaction.rs) construct WithEncoded only when encoded matches txn
(i.e., when replace_blockhash is false) and return None/absent encoded when the
blockhash was replaced before sanitize; adjust signatures and call sites to
reflect the new optional/alternative return so no stale encoded bytes are
exposed.
---
Duplicate comments:
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 204-208: The comment beside the
with_encoded(intent_sent_transaction) match is misleading (mentions "64KB"
constraint); update the comment and/or log to accurately reflect the real reason
for bincode failure: either remove the incorrect "all intent transactions are
smaller than 64KB by construction" clause or replace it with a factual note
(e.g., "unexpected bincode serialization failure — should be unreachable under
normal inputs"); keep the error! call as-is (error!("Failed to bincode intent
transaction");) and ensure the code references the same symbols (with_encoded
and intent_sent_transaction) so the comment now correctly documents why this
branch is considered unreachable.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
magicblock-account-cloner/src/lib.rsmagicblock-accounts/src/scheduled_commits_processor.rsmagicblock-aperture/src/requests/http/mod.rsmagicblock-aperture/src/requests/http/request_airdrop.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-api/src/tickers.rs
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