Skip to content
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

Add Message::is_maybe_writable #35340

Merged
merged 3 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rpc-client/src/nonblocking/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ impl RpcClient {
'sending: for _ in 0..SEND_RETRIES {
let signature = self.send_transaction(transaction).await?;

let recent_blockhash = if transaction.uses_durable_nonce() {
let recent_blockhash = if transaction.maybe_uses_durable_nonce() {
let (recent_blockhash, ..) = self
.get_latest_blockhash_with_commitment(CommitmentConfig::processed())
.await?;
Expand Down Expand Up @@ -753,7 +753,7 @@ impl RpcClient {
commitment: CommitmentConfig,
config: RpcSendTransactionConfig,
) -> ClientResult<Signature> {
let recent_blockhash = if transaction.uses_durable_nonce() {
let recent_blockhash = if transaction.maybe_uses_durable_nonce() {
self.get_latest_blockhash_with_commitment(CommitmentConfig::processed())
.await?
.0
Expand Down
12 changes: 6 additions & 6 deletions rpc-client/src/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use {
message::{v0, Message as LegacyMessage},
pubkey::Pubkey,
signature::Signature,
transaction::{self, uses_durable_nonce, Transaction, VersionedTransaction},
transaction::{self, maybe_uses_durable_nonce, Transaction, VersionedTransaction},
},
solana_transaction_status::{
EncodedConfirmedBlock, EncodedConfirmedTransactionWithStatusMeta, TransactionStatus,
Expand Down Expand Up @@ -80,7 +80,7 @@ impl SerializableMessage for v0::Message {}
pub trait SerializableTransaction: Serialize {
fn get_signature(&self) -> &Signature;
fn get_recent_blockhash(&self) -> &Hash;
fn uses_durable_nonce(&self) -> bool;
fn maybe_uses_durable_nonce(&self) -> bool;
}
impl SerializableTransaction for Transaction {
fn get_signature(&self) -> &Signature {
Expand All @@ -89,8 +89,8 @@ impl SerializableTransaction for Transaction {
fn get_recent_blockhash(&self) -> &Hash {
&self.message.recent_blockhash
}
fn uses_durable_nonce(&self) -> bool {
uses_durable_nonce(self).is_some()
fn maybe_uses_durable_nonce(&self) -> bool {
maybe_uses_durable_nonce(self).is_some()
}
}
impl SerializableTransaction for VersionedTransaction {
Expand All @@ -100,8 +100,8 @@ impl SerializableTransaction for VersionedTransaction {
fn get_recent_blockhash(&self) -> &Hash {
self.message.recent_blockhash()
}
fn uses_durable_nonce(&self) -> bool {
self.uses_durable_nonce()
fn maybe_uses_durable_nonce(&self) -> bool {
self.maybe_uses_durable_nonce()
}
}

Expand Down
22 changes: 19 additions & 3 deletions sdk/program/src/message/legacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,12 +548,28 @@ 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)
}

pub fn is_writable(&self, i: usize) -> bool {
jstarry marked this conversation as resolved.
Show resolved Hide resolved
(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)
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/program/src/message/versions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
Expand Down
7 changes: 4 additions & 3 deletions sdk/program/src/message/versions/v0/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
&& !{
Expand Down
22 changes: 11 additions & 11 deletions sdk/src/transaction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ impl Transaction {
}
}

pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> {
pub fn maybe_uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this one more, I wonder if we can come up with a more descriptive fn name; the returned ix definitely does use durable nonce but the nonce account may not be writable.
Also, I quibble with "uses" in the first place, since this is a method that returns the instruction, not a bool.
Super wordy, but what about try_get_durable_nonce_instruction? Sort of ignores the "maybe writable" part

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it definitely looks like it's using a durable nonce but in the context of the Solana runtime, a tx is only considered to be using a durable nonce if it's a "valid" durable nonce which requires the nonce account to be writable. Maybe a name with "valid" in it would be more clear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair. I'm open to adding "valid" somehow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think I was focusing too much on the validity... a nonce is also only valid if the transaction's recent blockhash is correct. This method should probably not be doing the writable check at all.

I'm inclined to go with your suggestion of try_get_durable_nonce_instruction but I will add it as an alias and deprecate the original function because I really shouldn't have introduced that breaking change in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok actually I think I'm going to just leave everything as is but take out the is-writable check from this function

let message = tx.message();
message
.instructions
Expand All @@ -1093,7 +1093,7 @@ pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> {
// Nonce account is writable
&& matches!(
instruction.accounts.first(),
Some(index) if message.is_writable(*index as usize)
Some(index) if message.is_maybe_writable(*index as usize)
)
})
}
Expand Down Expand Up @@ -1553,19 +1553,19 @@ mod tests {
#[test]
fn tx_uses_nonce_ok() {
let (_, _, tx) = nonced_transfer_tx();
assert!(uses_durable_nonce(&tx).is_some());
assert!(maybe_uses_durable_nonce(&tx).is_some());
}

#[test]
fn tx_uses_nonce_empty_ix_fail() {
assert!(uses_durable_nonce(&Transaction::default()).is_none());
assert!(maybe_uses_durable_nonce(&Transaction::default()).is_none());
}

#[test]
fn tx_uses_nonce_bad_prog_id_idx_fail() {
let (_, _, mut tx) = nonced_transfer_tx();
tx.message.instructions.get_mut(0).unwrap().program_id_index = 255u8;
assert!(uses_durable_nonce(&tx).is_none());
assert!(maybe_uses_durable_nonce(&tx).is_none());
}

#[test]
Expand All @@ -1580,7 +1580,7 @@ mod tests {
];
let message = Message::new(&instructions, Some(&from_pubkey));
let tx = Transaction::new(&[&from_keypair, &nonce_keypair], message, Hash::default());
assert!(uses_durable_nonce(&tx).is_none());
assert!(maybe_uses_durable_nonce(&tx).is_none());
}

#[test]
Expand All @@ -1606,7 +1606,7 @@ mod tests {
&[&from_keypair, &nonce_keypair],
Hash::default(),
);
assert!(uses_durable_nonce(&tx).is_none());
assert!(maybe_uses_durable_nonce(&tx).is_none());
}

#[test]
Expand All @@ -1626,13 +1626,13 @@ mod tests {
];
let message = Message::new(&instructions, Some(&nonce_pubkey));
let tx = Transaction::new(&[&from_keypair, &nonce_keypair], message, Hash::default());
assert!(uses_durable_nonce(&tx).is_none());
assert!(maybe_uses_durable_nonce(&tx).is_none());
}

#[test]
fn get_nonce_pub_from_ix_ok() {
let (_, nonce_pubkey, tx) = nonced_transfer_tx();
let nonce_ix = uses_durable_nonce(&tx).unwrap();
let nonce_ix = maybe_uses_durable_nonce(&tx).unwrap();
assert_eq!(
get_nonce_pubkey_from_instruction(nonce_ix, &tx),
Some(&nonce_pubkey),
Expand All @@ -1642,7 +1642,7 @@ mod tests {
#[test]
fn get_nonce_pub_from_ix_no_accounts_fail() {
let (_, _, tx) = nonced_transfer_tx();
let nonce_ix = uses_durable_nonce(&tx).unwrap();
let nonce_ix = maybe_uses_durable_nonce(&tx).unwrap();
let mut nonce_ix = nonce_ix.clone();
nonce_ix.accounts.clear();
assert_eq!(get_nonce_pubkey_from_instruction(&nonce_ix, &tx), None,);
Expand All @@ -1651,7 +1651,7 @@ mod tests {
#[test]
fn get_nonce_pub_from_ix_bad_acc_idx_fail() {
let (_, _, tx) = nonced_transfer_tx();
let nonce_ix = uses_durable_nonce(&tx).unwrap();
let nonce_ix = maybe_uses_durable_nonce(&tx).unwrap();
let mut nonce_ix = nonce_ix.clone();
nonce_ix.accounts[0] = 255u8;
assert_eq!(get_nonce_pubkey_from_instruction(&nonce_ix, &tx), None,);
Expand Down
14 changes: 7 additions & 7 deletions sdk/src/transaction/versioned/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ impl VersionedTransaction {
/// instruction. Since dynamically loaded addresses can't have write locks
/// demoted without loading addresses, this shouldn't be used in the
/// runtime.
pub fn uses_durable_nonce(&self) -> bool {
pub fn maybe_uses_durable_nonce(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about the fact that it definitely uses durable nonce, just is maybe writable. I haven't thought of a better option for this one, though.

let message = &self.message;
message
.instructions()
Expand Down Expand Up @@ -291,12 +291,12 @@ mod tests {
#[test]
fn tx_uses_nonce_ok() {
let (_, _, tx) = nonced_transfer_tx();
assert!(tx.uses_durable_nonce());
assert!(tx.maybe_uses_durable_nonce());
}

#[test]
fn tx_uses_nonce_empty_ix_fail() {
assert!(!VersionedTransaction::default().uses_durable_nonce());
assert!(!VersionedTransaction::default().maybe_uses_durable_nonce());
}

#[test]
Expand All @@ -308,7 +308,7 @@ mod tests {
}
VersionedMessage::V0(_) => unreachable!(),
};
assert!(!tx.uses_durable_nonce());
assert!(!tx.maybe_uses_durable_nonce());
}

#[test]
Expand All @@ -324,7 +324,7 @@ mod tests {
let message = LegacyMessage::new(&instructions, Some(&from_pubkey));
let tx = Transaction::new(&[&from_keypair, &nonce_keypair], message, Hash::default());
let tx = VersionedTransaction::from(tx);
assert!(!tx.uses_durable_nonce());
assert!(!tx.maybe_uses_durable_nonce());
}

#[test]
Expand All @@ -351,7 +351,7 @@ mod tests {
Hash::default(),
);
let tx = VersionedTransaction::from(tx);
assert!(!tx.uses_durable_nonce());
assert!(!tx.maybe_uses_durable_nonce());
}

#[test]
Expand All @@ -372,6 +372,6 @@ mod tests {
let message = LegacyMessage::new(&instructions, Some(&nonce_pubkey));
let tx = Transaction::new(&[&from_keypair, &nonce_keypair], message, Hash::default());
let tx = VersionedTransaction::from(tx);
assert!(!tx.uses_durable_nonce());
assert!(!tx.maybe_uses_durable_nonce());
}
}
2 changes: 1 addition & 1 deletion transaction-status/src/parse_accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub fn parse_legacy_message_accounts(message: &Message) -> Vec<ParsedAccount> {
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),
});
Expand Down
Loading