-
Notifications
You must be signed in to change notification settings - Fork 38
feat: forward original bincoded transaction #991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: bmuddha/accountsdb/checksum
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,7 +9,8 @@ use std::{ | |||||||||||||||||||||
| use magicblock_accounts::ScheduledCommitsProcessor; | ||||||||||||||||||||||
| use magicblock_accounts_db::{traits::AccountsBank, AccountsDb}; | ||||||||||||||||||||||
| use magicblock_core::link::{ | ||||||||||||||||||||||
| blocks::BlockUpdateTx, transactions::TransactionSchedulerHandle, | ||||||||||||||||||||||
| blocks::BlockUpdateTx, | ||||||||||||||||||||||
| transactions::{with_encoded, TransactionSchedulerHandle}, | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| use magicblock_ledger::{LatestBlock, Ledger}; | ||||||||||||||||||||||
| use magicblock_magic_program_api as magic_program; | ||||||||||||||||||||||
|
|
@@ -89,6 +90,11 @@ async fn handle_scheduled_commits<C: ScheduledCommitsProcessor>( | |||||||||||||||||||||
| let tx = InstructionUtils::accept_scheduled_commits( | ||||||||||||||||||||||
| latest_block.load().blockhash, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| 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; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
Comment on lines
+93
to
+97
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Comment incorrectly attributes infallibility to transaction size
📝 Suggested comment correction- // Unreachable case, all schedule commit txns are smaller than 64KB by construction
+ // Unreachable case: Transaction always implements Serialize correctly by construction📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| if let Err(err) = transaction_scheduler.execute(tx).await { | ||||||||||||||||||||||
| error!(error = ?err, "Failed to accept scheduled commits"); | ||||||||||||||||||||||
| return; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||
| use flume::{Receiver as MpmcReceiver, Sender as MpmcSender}; | ||||||||||||||||||||
| use magicblock_magic_program_api::args::TaskRequest; | ||||||||||||||||||||
| use serde::Serialize; | ||||||||||||||||||||
| use solana_program::message::{ | ||||||||||||||||||||
| inner_instruction::InnerInstructionsList, SimpleAddressLoader, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
@@ -69,6 +70,9 @@ pub struct TransactionStatus { | |||||||||||||||||||
| pub struct ProcessableTransaction { | ||||||||||||||||||||
| pub transaction: SanitizedTransaction, | ||||||||||||||||||||
| pub mode: TransactionProcessingMode, | ||||||||||||||||||||
| /// Pre-encoded bincode bytes for the transaction. | ||||||||||||||||||||
| /// Used by the replicator to avoid redundant serialization. | ||||||||||||||||||||
| pub encoded: Option<Vec<u8>>, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// An enum that specifies how a transaction should be processed by the scheduler. | ||||||||||||||||||||
|
|
@@ -115,6 +119,57 @@ pub trait SanitizeableTransaction { | |||||||||||||||||||
| self, | ||||||||||||||||||||
| verify: bool, | ||||||||||||||||||||
| ) -> Result<SanitizedTransaction, TransactionError>; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// Sanitizes the transaction and optionally provides pre-encoded bincode bytes. | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// Default implementation delegates to `sanitize()` and returns `None` for encoded bytes. | ||||||||||||||||||||
| /// Override this method when you have pre-encoded bytes (e.g., from the wire) to avoid | ||||||||||||||||||||
| /// redundant serialization. | ||||||||||||||||||||
| fn sanitize_with_encoded( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| verify: bool, | ||||||||||||||||||||
| ) -> Result<(SanitizedTransaction, Option<Vec<u8>>), TransactionError> | ||||||||||||||||||||
| where | ||||||||||||||||||||
| Self: Sized, | ||||||||||||||||||||
| { | ||||||||||||||||||||
| let txn = self.sanitize(verify)?; | ||||||||||||||||||||
| Ok((txn, None)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// Wraps a transaction with its pre-encoded bincode representation. | ||||||||||||||||||||
| /// Use for internally-constructed transactions that need encoded bytes. | ||||||||||||||||||||
| pub struct WithEncoded<T> { | ||||||||||||||||||||
| pub txn: T, | ||||||||||||||||||||
| pub encoded: Vec<u8>, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl<T: SanitizeableTransaction> SanitizeableTransaction for WithEncoded<T> { | ||||||||||||||||||||
| fn sanitize( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| verify: bool, | ||||||||||||||||||||
| ) -> Result<SanitizedTransaction, TransactionError> { | ||||||||||||||||||||
| self.txn.sanitize(verify) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fn sanitize_with_encoded( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| verify: bool, | ||||||||||||||||||||
| ) -> Result<(SanitizedTransaction, Option<Vec<u8>>), TransactionError> { | ||||||||||||||||||||
| let txn = self.txn.sanitize(verify)?; | ||||||||||||||||||||
| Ok((txn, Some(self.encoded))) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// 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 }) | ||||||||||||||||||||
|
Comment on lines
+169
to
+172
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A bincode serialization failure in 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+164
to
173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 The function has no call sites in the codebase and is dead code. 🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| impl SanitizeableTransaction for SanitizedTransaction { | ||||||||||||||||||||
|
|
@@ -162,9 +217,13 @@ impl TransactionSchedulerHandle { | |||||||||||||||||||
| &self, | ||||||||||||||||||||
| txn: impl SanitizeableTransaction, | ||||||||||||||||||||
| ) -> TransactionResult { | ||||||||||||||||||||
| let transaction = txn.sanitize(true)?; | ||||||||||||||||||||
| let (transaction, encoded) = txn.sanitize_with_encoded(true)?; | ||||||||||||||||||||
| let mode = TransactionProcessingMode::Execution(None); | ||||||||||||||||||||
| let txn = ProcessableTransaction { transaction, mode }; | ||||||||||||||||||||
| let txn = ProcessableTransaction { | ||||||||||||||||||||
| transaction, | ||||||||||||||||||||
| mode, | ||||||||||||||||||||
| encoded, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| let r = self.0.send(txn).await; | ||||||||||||||||||||
| r.map_err(|_| TransactionError::ClusterMaintenance) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
@@ -208,10 +267,14 @@ impl TransactionSchedulerHandle { | |||||||||||||||||||
| txn: impl SanitizeableTransaction, | ||||||||||||||||||||
| mode: fn(oneshot::Sender<R>) -> TransactionProcessingMode, | ||||||||||||||||||||
| ) -> Result<R, TransactionError> { | ||||||||||||||||||||
| let transaction = txn.sanitize(true)?; | ||||||||||||||||||||
| let (transaction, encoded) = txn.sanitize_with_encoded(true)?; | ||||||||||||||||||||
| let (tx, rx) = oneshot::channel(); | ||||||||||||||||||||
| let mode = mode(tx); | ||||||||||||||||||||
| let txn = ProcessableTransaction { transaction, mode }; | ||||||||||||||||||||
| let txn = ProcessableTransaction { | ||||||||||||||||||||
| transaction, | ||||||||||||||||||||
| mode, | ||||||||||||||||||||
| encoded, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| self.0 | ||||||||||||||||||||
| .send(txn) | ||||||||||||||||||||
| .await | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_default()silently returns an all-zero signature on invariant violationIf
txn.signaturesis unexpectedly empty,SerdeSignature(Signature::default())(all zeros) is returned as a success response. The client receives a zero signature it can never look up, masking the failure entirely. Since the invariant can be verified, prefer a hard error:🛡️ Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents