From ee2c2ef6c765d0782c9eb9a4cc0bcbbaad7be904 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 7 Jun 2023 17:12:41 +0200 Subject: [PATCH] Cleanup - require_static_program_ids_in_transaction (#31767) require_static_program_ids_in_transaction --- banks-server/src/banks_server.rs | 2 - core/src/banking_stage/consumer.rs | 1 - core/src/read_write_account_set.rs | 1 - entry/benches/entry_sigverify.rs | 2 - entry/src/entry.rs | 1 - ledger-tool/src/main.rs | 1 - ledger/src/blockstore.rs | 11 +-- rpc/src/rpc.rs | 10 +-- rpc/src/transaction_status_service.rs | 1 - runtime/src/bank.rs | 20 +---- runtime/src/cost_tracker.rs | 1 - sdk/program/src/message/versions/mod.rs | 4 +- sdk/program/src/message/versions/sanitized.rs | 2 +- sdk/program/src/message/versions/v0/mod.rs | 78 +++++-------------- sdk/src/transaction/sanitized.rs | 3 +- sdk/src/transaction/versioned/mod.rs | 7 +- transaction-status/src/lib.rs | 8 +- 17 files changed, 33 insertions(+), 120 deletions(-) diff --git a/banks-server/src/banks_server.rs b/banks-server/src/banks_server.rs index 21491038f6..d9c59bdc5c 100644 --- a/banks-server/src/banks_server.rs +++ b/banks-server/src/banks_server.rs @@ -178,7 +178,6 @@ fn simulate_transaction( MessageHash::Compute, Some(false), // is_simple_vote_tx bank, - true, // require_static_program_ids ) { Err(err) => { return BanksTransactionResultWithSimulation { @@ -330,7 +329,6 @@ impl Banks for BanksServer { MessageHash::Compute, Some(false), // is_simple_vote_tx bank.as_ref(), - true, // require_static_program_ids ) { Ok(tx) => tx, Err(err) => return Some(Err(err)), diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index ee073fc771..9db5790415 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -1697,7 +1697,6 @@ mod tests { MessageHash::Compute, Some(false), bank.as_ref(), - true, // require_static_program_ids ) .unwrap(); diff --git a/core/src/read_write_account_set.rs b/core/src/read_write_account_set.rs index a0a8b41ca3..455ae4804b 100644 --- a/core/src/read_write_account_set.rs +++ b/core/src/read_write_account_set.rs @@ -157,7 +157,6 @@ mod tests { MessageHash::Compute, Some(false), bank, - true, // require_static_program_ids ) .unwrap() } diff --git a/entry/benches/entry_sigverify.rs b/entry/benches/entry_sigverify.rs index 1f4abf9182..b3a1b7b5cd 100644 --- a/entry/benches/entry_sigverify.rs +++ b/entry/benches/entry_sigverify.rs @@ -40,7 +40,6 @@ fn bench_gpusigverify(bencher: &mut Bencher) { message_hash, None, SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) }?; @@ -82,7 +81,6 @@ fn bench_cpusigverify(bencher: &mut Bencher) { message_hash, None, SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) }?; diff --git a/entry/src/entry.rs b/entry/src/entry.rs index 0551abe02a..7456ad7492 100644 --- a/entry/src/entry.rs +++ b/entry/src/entry.rs @@ -1041,7 +1041,6 @@ mod tests { message_hash, None, SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) }?; diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index d8ba1f6372..fde4b279db 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -922,7 +922,6 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> MessageHash::Compute, None, SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) .map_err(|err| { warn!("Failed to compute cost of transaction: {:?}", err); diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 3e9ac1cfa3..b06198d122 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -1984,12 +1984,7 @@ impl Blockstore { .into_iter() .flat_map(|entry| entry.transactions) .map(|transaction| { - if let Err(err) = transaction.sanitize( - // Don't enable additional sanitization checks until - // all clusters have activated the static program id - // feature gate so that bigtable upload isn't affected - false, // require_static_program_ids - ) { + if let Err(err) = transaction.sanitize() { warn!( "Blockstore::get_block sanitize failed: {:?}, \ slot: {:?}, \ @@ -2376,9 +2371,7 @@ impl Blockstore { .cloned() .flat_map(|entry| entry.transactions) .map(|transaction| { - if let Err(err) = transaction.sanitize( - true, // require_static_program_ids - ) { + if let Err(err) = transaction.sanitize() { warn!( "Blockstore::find_transaction_in_slot sanitize failed: {:?}, \ slot: {:?}, \ diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index b08a6ed7b8..48324ad0c1 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -4516,14 +4516,8 @@ fn sanitize_transaction( transaction: VersionedTransaction, address_loader: impl AddressLoader, ) -> Result { - SanitizedTransaction::try_create( - transaction, - MessageHash::Compute, - None, - address_loader, - true, // require_static_program_ids - ) - .map_err(|err| Error::invalid_params(format!("invalid transaction: {err}"))) + SanitizedTransaction::try_create(transaction, MessageHash::Compute, None, address_loader) + .map_err(|err| Error::invalid_params(format!("invalid transaction: {err}"))) } pub fn create_validator_exit(exit: Arc) -> Arc> { diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index d210365789..6e6a9eb225 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -349,7 +349,6 @@ pub(crate) mod tests { MessageHash::Compute, None, SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) .unwrap(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f929a16203..fdb98b23bb 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3763,16 +3763,7 @@ impl Bank { pub fn prepare_entry_batch(&self, txs: Vec) -> Result { let sanitized_txs = txs .into_iter() - .map(|tx| { - SanitizedTransaction::try_create( - tx, - MessageHash::Compute, - None, - self, - self.feature_set - .is_active(&feature_set::require_static_program_ids_in_transaction::ID), - ) - }) + .map(|tx| SanitizedTransaction::try_create(tx, MessageHash::Compute, None, self)) .collect::>>()?; let tx_account_lock_limit = self.get_transaction_account_lock_limit(); let lock_results = self @@ -6852,14 +6843,7 @@ impl Bank { tx.message.hash() }; - SanitizedTransaction::try_create( - tx, - message_hash, - None, - self, - self.feature_set - .is_active(&feature_set::require_static_program_ids_in_transaction::ID), - ) + SanitizedTransaction::try_create(tx, message_hash, None, self) }?; if verification_mode == TransactionVerificationMode::HashAndVerifyPrecompiles diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index df2421704b..a07afa3367 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -367,7 +367,6 @@ mod tests { MessageHash::Compute, Some(true), SimpleAddressLoader::Disabled, - true, // require_static_program_ids ) .unwrap(); let mut tx_cost = TransactionCost::new_with_capacity(1); diff --git a/sdk/program/src/message/versions/mod.rs b/sdk/program/src/message/versions/mod.rs index fd95bd85f8..ccef565fd5 100644 --- a/sdk/program/src/message/versions/mod.rs +++ b/sdk/program/src/message/versions/mod.rs @@ -39,10 +39,10 @@ pub enum VersionedMessage { } impl VersionedMessage { - pub fn sanitize(&self, require_static_program_ids: bool) -> Result<(), SanitizeError> { + pub fn sanitize(&self) -> Result<(), SanitizeError> { match self { Self::Legacy(message) => message.sanitize(), - Self::V0(message) => message.sanitize(require_static_program_ids), + Self::V0(message) => message.sanitize(), } } diff --git a/sdk/program/src/message/versions/sanitized.rs b/sdk/program/src/message/versions/sanitized.rs index 38264fc307..5d44f7ed48 100644 --- a/sdk/program/src/message/versions/sanitized.rs +++ b/sdk/program/src/message/versions/sanitized.rs @@ -18,7 +18,7 @@ impl TryFrom for SanitizedVersionedMessage { impl SanitizedVersionedMessage { pub fn try_new(message: VersionedMessage) -> Result { - message.sanitize(true /* require_static_program_ids */)?; + message.sanitize()?; Ok(Self { message }) } diff --git a/sdk/program/src/message/versions/v0/mod.rs b/sdk/program/src/message/versions/v0/mod.rs index f264e286ad..c2f87cf508 100644 --- a/sdk/program/src/message/versions/v0/mod.rs +++ b/sdk/program/src/message/versions/v0/mod.rs @@ -89,7 +89,7 @@ pub struct Message { impl Message { /// Sanitize message fields and compiled instruction indexes - pub fn sanitize(&self, reject_dynamic_program_ids: bool) -> Result<(), SanitizeError> { + pub fn sanitize(&self) -> Result<(), SanitizeError> { let num_static_account_keys = self.account_keys.len(); if usize::from(self.header.num_required_signatures) .saturating_add(usize::from(self.header.num_readonly_unsigned_accounts)) @@ -143,18 +143,15 @@ impl Message { .checked_sub(1) .expect("message doesn't contain any account keys"); - // switch to rejecting program ids loaded from lookup tables so that - // static analysis on program instructions can be performed without - // loading on-chain data from a bank - let max_program_id_ix = if reject_dynamic_program_ids { + // reject program ids loaded from lookup tables so that + // static analysis on program instructions can be performed + // without loading on-chain data from a bank + let max_program_id_ix = // `expect` is safe because of earlier check that // `num_static_account_keys` is non-zero num_static_account_keys .checked_sub(1) - .expect("message doesn't contain any static account keys") - } else { - max_account_ix - }; + .expect("message doesn't contain any static account keys"); for ci in &self.instructions { if usize::from(ci.program_id_index) > max_program_id_ix { @@ -375,9 +372,7 @@ mod tests { account_keys: vec![Pubkey::new_unique()], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -396,9 +391,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -417,9 +410,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -444,13 +435,7 @@ mod tests { ..Message::default() }; - assert!(message.sanitize( - false, // require_static_program_ids - ).is_ok()); - - assert!(message.sanitize( - true, // require_static_program_ids - ).is_err()); + assert!(message.sanitize().is_err()); } #[test] @@ -473,9 +458,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -486,9 +469,7 @@ mod tests { account_keys: vec![Pubkey::new_unique()], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } @@ -503,9 +484,7 @@ mod tests { account_keys: vec![Pubkey::new_unique()], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } @@ -524,9 +503,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } @@ -540,9 +517,7 @@ mod tests { account_keys: (0..=u8::MAX).map(|_| Pubkey::new_unique()).collect(), ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -556,9 +531,7 @@ mod tests { account_keys: (0..=256).map(|_| Pubkey::new_unique()).collect(), ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } @@ -577,9 +550,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_ok()); } @@ -598,9 +569,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } @@ -625,12 +594,7 @@ mod tests { ..Message::default() }; - assert!(message - .sanitize(true /* require_static_program_ids */) - .is_err()); - assert!(message - .sanitize(false /* require_static_program_ids */) - .is_err()); + assert!(message.sanitize().is_err()); } #[test] @@ -653,9 +617,7 @@ mod tests { }], ..Message::default() } - .sanitize( - true, // require_static_program_ids - ) + .sanitize() .is_err()); } diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index 6f598eed0e..ca5871bcd4 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -95,9 +95,8 @@ impl SanitizedTransaction { message_hash: impl Into, is_simple_vote_tx: Option, address_loader: impl AddressLoader, - require_static_program_ids: bool, ) -> Result { - tx.sanitize(require_static_program_ids)?; + tx.sanitize()?; let message_hash = match message_hash.into() { MessageHash::Compute => tx.message.hash(), diff --git a/sdk/src/transaction/versioned/mod.rs b/sdk/src/transaction/versioned/mod.rs index 29e385e627..9faecf2dce 100644 --- a/sdk/src/transaction/versioned/mod.rs +++ b/sdk/src/transaction/versioned/mod.rs @@ -115,11 +115,8 @@ impl VersionedTransaction { }) } - pub fn sanitize( - &self, - require_static_program_ids: bool, - ) -> std::result::Result<(), SanitizeError> { - self.message.sanitize(require_static_program_ids)?; + pub fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + self.message.sanitize()?; self.sanitize_signatures()?; Ok(()) } diff --git a/transaction-status/src/lib.rs b/transaction-status/src/lib.rs index 7c9877a26b..bb1ddd42d0 100644 --- a/transaction-status/src/lib.rs +++ b/transaction-status/src/lib.rs @@ -1131,13 +1131,7 @@ impl EncodedTransaction { .and_then(|bytes| bincode::deserialize(&bytes).ok()), }; - transaction.filter(|transaction| { - transaction - .sanitize( - true, // require_static_program_ids - ) - .is_ok() - }) + transaction.filter(|transaction| transaction.sanitize().is_ok()) } }