feat: forward original bincoded transaction#991
Open
bmuddha wants to merge 3 commits intobmuddha/accountsdb/checksumfrom
Open
feat: forward original bincoded transaction#991bmuddha wants to merge 3 commits intobmuddha/accountsdb/checksumfrom
bmuddha wants to merge 3 commits intobmuddha/accountsdb/checksumfrom
Conversation
5 tasks
Collaborator
Author
|
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. |
3 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds WithEncoded to carry pre-encoded bincode bytes alongside transactions, updates SanitizeableTransaction with sanitize_with_encoded, and extends ProcessableTransaction to include an encoded: Option<Vec> field. prepare_transaction now returns WithEncoded, and callers (schedulers, aperture handlers, account-cloner, tickers, scheduled commits) were updated to accept and propagate the encoded bytes. bincode and serde were added as dependencies to support encoding. Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ 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}
Contributor
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.
Contributor
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
4 tasks
2 tasks
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
from
3ed3c71 to
25651e8
Compare
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
from
9edc6c4 to
378e32d
Compare
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
from
25651e8 to
5f00021
Compare
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
2 times, most recently
from
618b99c to
26235a1
Compare
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
2 times, most recently
from
1d4ffb8 to
1bae5f8
Compare
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
from
26235a1 to
b8d302a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-accounts/src/scheduled_commits_processor.rs`:
- Around line 204-210: The current early return when
with_encoded(intent_sent_transaction) fails prevents sent-commit signaling;
change the logic to fall back to executing the original intent_sent_transaction
unencoded: attempt with_encoded(intent_sent_transaction) and if it Ok(txn) use
that, otherwise log a warning and set txn to the original
intent_sent_transaction (clone or reference as needed) and continue to call
internal_transaction_scheduler.execute(txn). Ensure the fallback preserves the
same error handling path so sent-commit signaling still occurs after execute.
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 55-56: The simulation call is discarding the original bincode
bytes by passing transaction.txn; preserve the encoded wrapper produced by
prepare_transaction by passing the full wrapper instead. Update the
.simulate(...) invocation to pass the whole transaction wrapper (the variable
named transaction returned from prepare_transaction) rather than transaction.txn
so the encoded payload flows through the scheduler to downstream components
(refer to prepare_transaction and the simulate method).
In `@magicblock-api/src/tickers.rs`:
- Around line 93-99: The current pre-encode path returns early on
with_encoded(tx) failure which skips processing; instead, when with_encoded(...)
returns Err, log a warning and fall back to calling
transaction_scheduler.execute with the original raw transaction (rather than
returning). Preserve the original transaction variable (e.g., rename the encoded
result to encoded_tx or keep raw_tx) so that if with_encoded fails you can still
call transaction_scheduler.execute(raw_tx). Keep the existing error logging for
execute(...).await and only treat encode failure as non-fatal fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a10d0080-11e9-413a-91fa-b2c142170fa3
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
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/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-api/src/tickers.rsmagicblock-core/Cargo.tomlmagicblock-core/src/link/transactions.rsmagicblock-processor/src/scheduler/tests.rs
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
from
b8d302a to
1d366ad
Compare
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
from
1bae5f8 to
2a14e70
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
204-209:⚠️ Potential issue | 🟠 MajorDon’t abort sent-commit signaling when pre-encoding fails.
Line 204 returns early on
with_encoded(...)failure, so the scheduler execution path is skipped. Since encoded bytes are optional, this should fall back to executing the raw transaction.Suggested patch
- let Ok(txn) = with_encoded(intent_sent_transaction) else { - // Unreachable case, all intent transactions are smaller than 64KB by construction - error!("Failed to bincode intent transaction"); - return; - }; - match internal_transaction_scheduler.execute(txn).await { + let exec_result = match with_encoded(intent_sent_transaction.clone()) { + Ok(txn) => internal_transaction_scheduler.execute(txn).await, + Err(err) => { + error!( + error = ?err, + "Failed to pre-encode intent transaction; falling back to unencoded transaction" + ); + internal_transaction_scheduler + .execute(intent_sent_transaction) + .await + } + }; + match exec_result {🤖 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 - 209, The current early return when with_encoded(intent_sent_transaction) fails aborts the sent-commit signaling; instead, if with_encoded(...) returns Err, fall back to executing the raw intent_sent_transaction payload via internal_transaction_scheduler.execute so the scheduling path still runs. Update the block around with_encoded(intent_sent_transaction) to try using the encoded txn when Ok(txn) and on Err call internal_transaction_scheduler.execute with the original intent_sent_transaction (or its unencoded representation), ensuring error logging remains but no early return prevents scheduling.magicblock-aperture/src/requests/http/simulate_transaction.rs (1)
55-56:⚠️ Potential issue | 🟠 MajorDon’t unwrap away encoded bytes before scheduling simulation.
Line 55 passes
transaction.txn, which drops the original encoded payload captured byprepare_transaction. Pass the wrapper sosanitize_with_encodedcan forward bytes consistently.Suggested patch
- .simulate(transaction.txn) + .simulate(transaction)🤖 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 55 - 56, The call to .simulate(...) is passing transaction.txn which discards the encoded payload prepared by prepare_transaction; instead pass the full wrapper (the transaction struct/value returned by prepare_transaction) so sanitize_with_encoded can access and forward the original encoded bytes consistently—locate the simulate invocation (the .simulate(...) chain) and replace the argument transaction.txn with the transaction wrapper variable (the value returned from prepare_transaction) so downstream sanitize_with_encoded receives the wrapper rather than raw/decoded bytes.magicblock-core/src/link/transactions.rs (1)
170-171: 🧹 Nitpick | 🔵 TrivialClarify why serialization failures are mapped to
SanitizeFailure.Line 170–171 maps a bincode serialization error to
TransactionError::SanitizeFailure, which is non-obvious and easy to misread during debugging. Please document this substitution inline (or route through a more precise error path if one exists).Suggested patch
- let encoded = bincode::serialize(&txn) - .map_err(|_| TransactionError::SanitizeFailure)?; + let encoded = bincode::serialize(&txn) + // TransactionError has no serialization-specific variant here; + // use SanitizeFailure as the closest existing error category. + .map_err(|_| TransactionError::SanitizeFailure)?;🤖 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 170 - 171, The bincode::serialize(&txn) call maps any serialization error to TransactionError::SanitizeFailure, which is confusing; either add an inline comment next to the serialization call explaining why a serialization failure is considered a sanitize failure for txn (e.g., serialization indicates invalid transaction structure after sanitization), or change the error mapping to a more precise variant (create/return TransactionError::SerializationFailure or wrap the original bincode error) and preserve the underlying error info; update the map_err usage around the encoded = bincode::serialize(&txn) expression and reference TransactionError::SanitizeFailure and the txn variable when making the change.magicblock-api/src/tickers.rs (1)
93-99:⚠️ Potential issue | 🟠 MajorUse raw-transaction fallback when pre-encoding fails.
Line 93–97 returns early if
with_encoded(...)fails, which skips scheduled-commit acceptance entirely. Treat pre-encoding as best-effort and continue with the unencoded transaction.Suggested patch
- let Ok(tx) = with_encoded(tx) else { - // Unreachable case, all schedule commit txns are smaller than 64KB by construction - error!("Failed to bincode intent transaction"); - return; - }; - if let Err(err) = transaction_scheduler.execute(tx).await { + let exec_result = match with_encoded(tx.clone()) { + Ok(tx) => transaction_scheduler.execute(tx).await, + Err(err) => { + error!( + error = ?err, + "Failed to pre-encode scheduled commits transaction; falling back to unencoded transaction" + ); + transaction_scheduler.execute(tx).await + } + }; + if let Err(err) = exec_result {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-api/src/tickers.rs` around lines 93 - 99, The current code returns early when with_encoded(tx) fails, skipping scheduled-commit acceptance; instead treat pre-encoding as best‑effort: when with_encoded(tx) returns Err, log a warning and continue using the original unencoded transaction value for the call to transaction_scheduler.execute(tx). Update the branch around with_encoded and the call site (symbols: with_encoded and transaction_scheduler.execute) so the execute path accepts the raw tx on encoding failure (ensuring the tx variable is the unencoded transaction in that branch) and only aborts on execute errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 204-209: The current early return when
with_encoded(intent_sent_transaction) fails aborts the sent-commit signaling;
instead, if with_encoded(...) returns Err, fall back to executing the raw
intent_sent_transaction payload via internal_transaction_scheduler.execute so
the scheduling path still runs. Update the block around
with_encoded(intent_sent_transaction) to try using the encoded txn when Ok(txn)
and on Err call internal_transaction_scheduler.execute with the original
intent_sent_transaction (or its unencoded representation), ensuring error
logging remains but no early return prevents scheduling.
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 55-56: The call to .simulate(...) is passing transaction.txn which
discards the encoded payload prepared by prepare_transaction; instead pass the
full wrapper (the transaction struct/value returned by prepare_transaction) so
sanitize_with_encoded can access and forward the original encoded bytes
consistently—locate the simulate invocation (the .simulate(...) chain) and
replace the argument transaction.txn with the transaction wrapper variable (the
value returned from prepare_transaction) so downstream sanitize_with_encoded
receives the wrapper rather than raw/decoded bytes.
In `@magicblock-api/src/tickers.rs`:
- Around line 93-99: The current code returns early when with_encoded(tx) fails,
skipping scheduled-commit acceptance; instead treat pre-encoding as best‑effort:
when with_encoded(tx) returns Err, log a warning and continue using the original
unencoded transaction value for the call to transaction_scheduler.execute(tx).
Update the branch around with_encoded and the call site (symbols: with_encoded
and transaction_scheduler.execute) so the execute path accepts the raw tx on
encoding failure (ensuring the tx variable is the unencoded transaction in that
branch) and only aborts on execute errors.
In `@magicblock-core/src/link/transactions.rs`:
- Around line 170-171: The bincode::serialize(&txn) call maps any serialization
error to TransactionError::SanitizeFailure, which is confusing; either add an
inline comment next to the serialization call explaining why a serialization
failure is considered a sanitize failure for txn (e.g., serialization indicates
invalid transaction structure after sanitization), or change the error mapping
to a more precise variant (create/return TransactionError::SerializationFailure
or wrap the original bincode error) and preserve the underlying error info;
update the map_err usage around the encoded = bincode::serialize(&txn)
expression and reference TransactionError::SanitizeFailure and the txn variable
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e11b47af-4a6c-4547-ad08-b26ba9699aca
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
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/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-api/src/tickers.rsmagicblock-core/Cargo.tomlmagicblock-core/src/link/transactions.rsmagicblock-processor/src/scheduler/tests.rs
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
from
2a14e70 to
93c53dc
Compare
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
from
1d366ad to
fad4f15
Compare
keep original bincoded transaction along with the sanitized version, this can will be used by future ledger and replicator implementations
bmuddha
force-pushed
the
bmuddha/transaction/original-bincode
branch
from
fad4f15 to
924a009
Compare
bmuddha
force-pushed
the
bmuddha/accountsdb/checksum
branch
from
93c53dc to
6e4f626
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
magicblock-aperture/src/requests/http/simulate_transaction.rs (1)
53-56:⚠️ Potential issue | 🟠 MajorPass the wrapper to
simulate.Line 55 strips the original bincode captured by
prepare_transaction, so the simulation path no longer preserves the encoded payload.🐛 Suggested fix
let result = self .transactions_scheduler - .simulate(transaction.txn) + .simulate(transaction) .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 - 56, The simulate call is currently passing transaction.txn which strips the original bincode prepared by prepare_transaction; change the call to pass the full wrapper produced by prepare_transaction (the variable holding the wrapped transaction used earlier in this function) into transactions_scheduler.simulate(...) so the encoded payload is preserved by simulate and the simulation path receives the original bincode wrapper.magicblock-api/src/tickers.rs (1)
93-99:⚠️ Potential issue | 🟠 MajorFall back to the raw transaction when pre-encoding fails.
Returning here skips
accept_scheduled_commitsentirely, even though the scheduler can still process the transaction withoutencoded.♻️ Suggested fix
- let Ok(tx) = with_encoded(tx) else { - // Unreachable case, all schedule commit txns are smaller than 64KB by construction - error!("Failed to bincode intent transaction"); - return; - }; - if let Err(err) = transaction_scheduler.execute(tx).await { + let exec_result = match with_encoded(tx.clone()) { + Ok(encoded_tx) => transaction_scheduler.execute(encoded_tx).await, + Err(err) => { + warn!( + error = ?err, + "Failed to pre-encode scheduled commits transaction; falling back to raw transaction" + ); + transaction_scheduler.execute(tx).await + } + }; + if let Err(err) = exec_result { error!(error = ?err, "Failed to accept scheduled commits"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-api/src/tickers.rs` around lines 93 - 99, The code currently returns early when with_encoded(tx) fails, skipping the scheduler; instead, change the flow so that if with_encoded(tx) errors you log the failure and fall back to using the original raw transaction for scheduling. Concretely, update the match around with_encoded(tx) (the call to with_encoded and the error log "Failed to bincode intent transaction") to not return on Err but to proceed using the original tx value when invoking transaction_scheduler.execute(tx). Ensure the error case uses the original transaction variable and still calls transaction_scheduler.execute(tx).magicblock-accounts/src/scheduled_commits_processor.rs (1)
204-209:⚠️ Potential issue | 🟠 MajorKeep sent-commit signaling alive on encode failure.
Early-returning here drops the signal transaction after
register_scheduled_commit_sent(sent_commit), even though the raw transaction is still usable.♻️ Suggested fix
- let Ok(txn) = with_encoded(intent_sent_transaction) else { - // Unreachable case, all intent transactions are smaller than 64KB by construction - error!("Failed to bincode intent transaction"); - return; - }; - match internal_transaction_scheduler.execute(txn).await { + let exec_result = match with_encoded(intent_sent_transaction.clone()) { + Ok(txn) => internal_transaction_scheduler.execute(txn).await, + Err(err) => { + warn!( + error = ?err, + "Failed to pre-encode sent-commit signal transaction; falling back to raw transaction" + ); + internal_transaction_scheduler + .execute(intent_sent_transaction) + .await + } + }; + match exec_result { Ok(()) => { debug!("Sent commit signaled") }🤖 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 - 209, The early return on bincode encode failure (the with_encoded(intent_sent_transaction) else branch) drops the previously-registered sent-commit signal; instead, log the encode error but do not return—fall back to executing the original raw intent_sent_transaction so register_scheduled_commit_sent(sent_commit) remains effective. Concretely, replace the else { error!(...); return; } with code that logs the error and sets txn to a fallback representation (e.g., use the original intent_sent_transaction or call the scheduler with the raw transaction) before calling internal_transaction_scheduler.execute(txn).await, keeping the sent-commit signaling alive.
🤖 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-account-cloner/src/lib.rs`:
- Around line 105-108: send_tx currently forces serialization by calling
with_encoded(tx) before scheduling, which turns an optional/missing encoded into
a hard failure; change send_tx to avoid unconditional with_encoded: extract the
signature as before, and call tx_scheduler.execute with the already-encoded
payload only when Transaction has its encoded field present (or when
with_encoded would succeed), otherwise pass the raw Transaction (or a variant
the scheduler accepts) so a missing encoded does not cause a clone failure;
update references to with_encoded, send_tx, Transaction, and tx_scheduler to
perform a conditional/optional encoding path.
In `@magicblock-aperture/src/requests/http/request_airdrop.rs`:
- Around line 42-44: In requestAirdrop, avoid failing when the optional
pre-encoding (with_encoded(txn)) fails; instead try to pre-encode but fall back
to passing the original Transaction to transactions_scheduler.execute. Update
the call site that currently does
self.transactions_scheduler.execute(with_encoded(txn)?).await? to attempt
with_encoded(txn) and, on Err, call execute with the original txn (i.e., use the
encoded value when Ok or the raw Transaction when Err) so the scheduler still
receives a valid input without turning the optimization into an RPC failure.
---
Duplicate comments:
In `@magicblock-accounts/src/scheduled_commits_processor.rs`:
- Around line 204-209: The early return on bincode encode failure (the
with_encoded(intent_sent_transaction) else branch) drops the
previously-registered sent-commit signal; instead, log the encode error but do
not return—fall back to executing the original raw intent_sent_transaction so
register_scheduled_commit_sent(sent_commit) remains effective. Concretely,
replace the else { error!(...); return; } with code that logs the error and sets
txn to a fallback representation (e.g., use the original intent_sent_transaction
or call the scheduler with the raw transaction) before calling
internal_transaction_scheduler.execute(txn).await, keeping the sent-commit
signaling alive.
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 53-56: The simulate call is currently passing transaction.txn
which strips the original bincode prepared by prepare_transaction; change the
call to pass the full wrapper produced by prepare_transaction (the variable
holding the wrapped transaction used earlier in this function) into
transactions_scheduler.simulate(...) so the encoded payload is preserved by
simulate and the simulation path receives the original bincode wrapper.
In `@magicblock-api/src/tickers.rs`:
- Around line 93-99: The code currently returns early when with_encoded(tx)
fails, skipping the scheduler; instead, change the flow so that if
with_encoded(tx) errors you log the failure and fall back to using the original
raw transaction for scheduling. Concretely, update the match around
with_encoded(tx) (the call to with_encoded and the error log "Failed to bincode
intent transaction") to not return on Err but to proceed using the original tx
value when invoking transaction_scheduler.execute(tx). Ensure the error case
uses the original transaction variable and still calls
transaction_scheduler.execute(tx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b73a5b98-1218-4e73-af7c-f608d1d15cb7
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
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/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-api/src/tickers.rsmagicblock-core/Cargo.tomlmagicblock-core/src/link/transactions.rsmagicblock-processor/src/scheduler/tests.rs
thlorenz
approved these changes
Mar 6, 2026
Collaborator
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM aside from the naming nit which we probably should just clarify in a comment in the right place.
| ) -> RpcResult<SanitizedTransaction> { | ||
| let decoded = match encoding { | ||
| ) -> RpcResult<WithEncoded<SanitizedTransaction>> { | ||
| let encoded = match encoding { |
Collaborator
There was a problem hiding this comment.
Confused by the naming here, it's calling decode but then naming the var encoded
|
|
||
| Ok(transaction.sanitize(sigverify)?) | ||
| let txn = transaction.sanitize(sigverify)?; | ||
| Ok(WithEncoded { txn, encoded }) |
Collaborator
There was a problem hiding this comment.
Again very confusing naming.
It's a bs58 decoded and bincode serialized value.
So maybe calling it serialized may make more sense?
|
|
||
| /// Wraps a transaction with its pre-encoded bincode representation. | ||
| /// Use for internally-constructed transactions that need encoded bytes. | ||
| pub struct WithEncoded<T> { |
Collaborator
There was a problem hiding this comment.
It's not pre-encoded, but a bincode serialized value (slight difference).
We don't use bincode::encode/decode, only bincode:de/serialize.
Even though from the docs it looks like there is actually an en/decode.
Very confusing, maybe you can explain (as part of a comment added to the above where the value was named confusingly).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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