From c9c2fbbdd6e481c9e0c2552ff74fd272bc40499e Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 29 Feb 2024 10:27:33 +0800 Subject: [PATCH] Add `Message::is_maybe_writable` (#35340) --- sdk/program/src/message/legacy.rs | 26 +++++++++++++-- sdk/program/src/message/versions/mod.rs | 2 +- sdk/program/src/message/versions/v0/mod.rs | 7 ++-- sdk/src/transaction/mod.rs | 34 ++----------------- sdk/src/transaction/versioned/mod.rs | 39 ++-------------------- transaction-status/src/parse_accounts.rs | 2 +- 6 files changed, 33 insertions(+), 77 deletions(-) diff --git a/sdk/program/src/message/legacy.rs b/sdk/program/src/message/legacy.rs index 32d7411ea..780259cd0 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -548,12 +548,32 @@ impl Message { self.is_key_called_as_program(i) && !self.is_upgradeable_loader_present() } - pub fn is_writable(&self, i: usize) -> bool { - (i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) + /// Returns true if the account at the specified index was requested to be + /// writable. This method should not be used directly. + fn is_writable_index(&self, i: usize) -> bool { + i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) as usize || (i >= self.header.num_required_signatures as usize && i < self.account_keys.len() - - self.header.num_readonly_unsigned_accounts as usize)) + - self.header.num_readonly_unsigned_accounts as usize) + } + + /// Returns true if the account at the specified index should be write + /// locked when loaded for transaction processing in the runtime. This + /// method differs from `is_maybe_writable` because it is aware of the + /// latest reserved accounts which are not allowed to be write locked. + pub fn is_writable(&self, i: usize) -> bool { + (self.is_writable_index(i)) + && !is_builtin_key_or_sysvar(&self.account_keys[i]) + && !self.demote_program_id(i) + } + + /// Returns true if the account at the specified index is writable by the + /// instructions in this message. Since the dynamic set of reserved accounts + /// isn't used here to demote write locks, this shouldn't be used in the + /// runtime. + pub fn is_maybe_writable(&self, i: usize) -> bool { + (self.is_writable_index(i)) && !is_builtin_key_or_sysvar(&self.account_keys[i]) && !self.demote_program_id(i) } diff --git a/sdk/program/src/message/versions/mod.rs b/sdk/program/src/message/versions/mod.rs index 70a1091ae..f1481bfcb 100644 --- a/sdk/program/src/message/versions/mod.rs +++ b/sdk/program/src/message/versions/mod.rs @@ -79,7 +79,7 @@ impl VersionedMessage { /// used in the runtime. pub fn is_maybe_writable(&self, index: usize) -> bool { match self { - Self::Legacy(message) => message.is_writable(index), + Self::Legacy(message) => message.is_maybe_writable(index), Self::V0(message) => message.is_maybe_writable(index), } } diff --git a/sdk/program/src/message/versions/v0/mod.rs b/sdk/program/src/message/versions/v0/mod.rs index df001bb19..57f82c270 100644 --- a/sdk/program/src/message/versions/v0/mod.rs +++ b/sdk/program/src/message/versions/v0/mod.rs @@ -334,9 +334,10 @@ impl Message { .any(|&key| key == bpf_loader_upgradeable::id()) } - /// Returns true if the account at the specified index was requested as writable. - /// Before loading addresses, we can't demote write locks for dynamically loaded - /// addresses so this should not be used by the runtime. + /// Returns true if the account at the specified index was requested as + /// writable. Before loading addresses and without the reserved account keys + /// set, we can't demote write locks properly so this should not be used by + /// the runtime. pub fn is_maybe_writable(&self, key_index: usize) -> bool { self.is_writable_index(key_index) && !{ diff --git a/sdk/src/transaction/mod.rs b/sdk/src/transaction/mod.rs index 4173a93b6..000955108 100644 --- a/sdk/src/transaction/mod.rs +++ b/sdk/src/transaction/mod.rs @@ -1074,6 +1074,7 @@ impl Transaction { } } +/// Returns true if transaction begins with an advance nonce instruction. pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> { let message = tx.message(); message @@ -1090,11 +1091,6 @@ pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> { limited_deserialize(&instruction.data), Ok(SystemInstruction::AdvanceNonceAccount) ) - // Nonce account is writable - && matches!( - instruction.accounts.first(), - Some(index) if message.is_writable(*index as usize) - ) }) } @@ -1119,7 +1115,7 @@ mod tests { hash::hash, instruction::AccountMeta, signature::{Keypair, Presigner, Signer}, - system_instruction, sysvar, + system_instruction, }, bincode::{deserialize, serialize, serialized_size}, std::mem::size_of, @@ -1583,32 +1579,6 @@ mod tests { assert!(uses_durable_nonce(&tx).is_none()); } - #[test] - fn tx_uses_ro_nonce_account() { - let from_keypair = Keypair::new(); - let from_pubkey = from_keypair.pubkey(); - let nonce_keypair = Keypair::new(); - let nonce_pubkey = nonce_keypair.pubkey(); - let account_metas = vec![ - AccountMeta::new_readonly(nonce_pubkey, false), - #[allow(deprecated)] - AccountMeta::new_readonly(sysvar::recent_blockhashes::id(), false), - AccountMeta::new_readonly(nonce_pubkey, true), - ]; - let nonce_instruction = Instruction::new_with_bincode( - system_program::id(), - &system_instruction::SystemInstruction::AdvanceNonceAccount, - account_metas, - ); - let tx = Transaction::new_signed_with_payer( - &[nonce_instruction], - Some(&from_pubkey), - &[&from_keypair, &nonce_keypair], - Hash::default(), - ); - assert!(uses_durable_nonce(&tx).is_none()); - } - #[test] fn tx_uses_nonce_wrong_first_nonce_ix_fail() { let from_keypair = Keypair::new(); diff --git a/sdk/src/transaction/versioned/mod.rs b/sdk/src/transaction/versioned/mod.rs index 9faecf2dc..ea06037d0 100644 --- a/sdk/src/transaction/versioned/mod.rs +++ b/sdk/src/transaction/versioned/mod.rs @@ -185,10 +185,7 @@ impl VersionedTransaction { .collect() } - /// Returns true if transaction begins with a valid advance nonce - /// instruction. Since dynamically loaded addresses can't have write locks - /// demoted without loading addresses, this shouldn't be used in the - /// runtime. + /// Returns true if transaction begins with an advance nonce instruction. pub fn uses_durable_nonce(&self) -> bool { let message = &self.message; message @@ -205,11 +202,6 @@ impl VersionedTransaction { limited_deserialize(&instruction.data), Ok(SystemInstruction::AdvanceNonceAccount) ) - // Nonce account is writable - && matches!( - instruction.accounts.first(), - Some(index) if message.is_maybe_writable(*index as usize) - ) }) .is_some() } @@ -222,7 +214,7 @@ mod tests { crate::{ message::Message as LegacyMessage, signer::{keypair::Keypair, Signer}, - system_instruction, sysvar, + system_instruction, }, solana_program::{ instruction::{AccountMeta, Instruction}, @@ -327,33 +319,6 @@ mod tests { assert!(!tx.uses_durable_nonce()); } - #[test] - fn tx_uses_ro_nonce_account() { - let from_keypair = Keypair::new(); - let from_pubkey = from_keypair.pubkey(); - let nonce_keypair = Keypair::new(); - let nonce_pubkey = nonce_keypair.pubkey(); - let account_metas = vec![ - AccountMeta::new_readonly(nonce_pubkey, false), - #[allow(deprecated)] - AccountMeta::new_readonly(sysvar::recent_blockhashes::id(), false), - AccountMeta::new_readonly(nonce_pubkey, true), - ]; - let nonce_instruction = Instruction::new_with_bincode( - system_program::id(), - &system_instruction::SystemInstruction::AdvanceNonceAccount, - account_metas, - ); - let tx = Transaction::new_signed_with_payer( - &[nonce_instruction], - Some(&from_pubkey), - &[&from_keypair, &nonce_keypair], - Hash::default(), - ); - let tx = VersionedTransaction::from(tx); - assert!(!tx.uses_durable_nonce()); - } - #[test] fn tx_uses_nonce_wrong_first_nonce_ix_fail() { let from_keypair = Keypair::new(); diff --git a/transaction-status/src/parse_accounts.rs b/transaction-status/src/parse_accounts.rs index 6ad0ec82a..5388c15ec 100644 --- a/transaction-status/src/parse_accounts.rs +++ b/transaction-status/src/parse_accounts.rs @@ -21,7 +21,7 @@ pub fn parse_legacy_message_accounts(message: &Message) -> Vec { for (i, account_key) in message.account_keys.iter().enumerate() { accounts.push(ParsedAccount { pubkey: account_key.to_string(), - writable: message.is_writable(i), + writable: message.is_maybe_writable(i), signer: message.is_signer(i), source: Some(ParsedAccountSource::Transaction), });