-
Notifications
You must be signed in to change notification settings - Fork 1k
SIMD-0242: Static Nonce Account Only #5555
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
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 |
|---|---|---|
|
|
@@ -237,7 +237,10 @@ impl Bank { | |
| &self, | ||
| message: &impl SVMMessage, | ||
| ) -> Option<(Pubkey, AccountSharedData, NonceData)> { | ||
| let nonce_address = message.get_durable_nonce()?; | ||
| let require_static_nonce_account = self | ||
|
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. Should we pass this in instead of lookup in feature-set per transaction?
Author
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. I did consider that. I didn't love that it would technically allow calling a bank method with a feature gate enabled which the bank didn't have enabled. Also this will only get called for transactions which don't have a recent blockhash, so not too bad I think. Would be nice to do some refactoring in this area tho, wdyt? 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. sg |
||
| .feature_set | ||
| .is_active(&agave_feature_set::require_static_nonce_account::id()); | ||
| let nonce_address = message.get_durable_nonce(require_static_nonce_account)?; | ||
| let nonce_account = self.get_account_with_fixed_root(nonce_address)?; | ||
| let nonce_data = | ||
| nonce_account::verify_nonce_account(&nonce_account, message.recent_blockhash())?; | ||
|
|
@@ -300,8 +303,20 @@ mod tests { | |
| setup_nonce_with_bank, | ||
| }, | ||
| solana_sdk::{ | ||
| hash::Hash, message::Message, signature::Keypair, signer::Signer, system_instruction, | ||
| hash::Hash, | ||
| instruction::CompiledInstruction, | ||
| message::{ | ||
| v0::{self, LoadedAddresses, MessageAddressTableLookup}, | ||
| Message, MessageHeader, SanitizedMessage, SanitizedVersionedMessage, | ||
| SimpleAddressLoader, VersionedMessage, | ||
| }, | ||
| signature::Keypair, | ||
| signer::Signer, | ||
| system_instruction::{self, SystemInstruction}, | ||
| system_program, | ||
| }, | ||
| std::collections::HashSet, | ||
| test_case::test_case, | ||
| }; | ||
|
|
||
| #[test] | ||
|
|
@@ -491,8 +506,79 @@ mod tests { | |
| .check_load_and_advance_message_nonce_account( | ||
| &message, | ||
| &bank.next_durable_nonce(), | ||
| lamports_per_signature | ||
| lamports_per_signature, | ||
| ) | ||
| .is_none()); | ||
| } | ||
|
|
||
| #[test_case(true; "test_check_and_load_message_nonce_account_nonce_is_alt_disallowed")] | ||
| #[test_case(false; "test_check_and_load_message_nonce_account_nonce_is_alt_allowed")] | ||
| fn test_check_and_load_message_nonce_account_nonce_is_alt(require_static_nonce_account: bool) { | ||
| let feature_set = if require_static_nonce_account { | ||
| FeatureSet::all_enabled() | ||
| } else { | ||
| FeatureSet::default() | ||
| }; | ||
| let nonce_authority = Pubkey::new_unique(); | ||
| let (bank, _mint_keypair, _custodian_keypair, nonce_keypair, _) = setup_nonce_with_bank( | ||
| 10_000_000, | ||
| |_| {}, | ||
| 5_000_000, | ||
| 250_000, | ||
| Some(nonce_authority), | ||
| feature_set, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let nonce_pubkey = nonce_keypair.pubkey(); | ||
| let nonce_hash = get_nonce_blockhash(&bank, &nonce_pubkey).unwrap(); | ||
| let loaded_addresses = LoadedAddresses { | ||
| writable: vec![nonce_pubkey], | ||
| readonly: vec![], | ||
| }; | ||
|
|
||
| let message = SanitizedMessage::try_new( | ||
| SanitizedVersionedMessage::try_new(VersionedMessage::V0(v0::Message { | ||
| header: MessageHeader { | ||
| num_required_signatures: 1, | ||
| num_readonly_signed_accounts: 0, | ||
| num_readonly_unsigned_accounts: 1, | ||
| }, | ||
| account_keys: vec![nonce_authority, system_program::id()], | ||
| recent_blockhash: nonce_hash, | ||
| instructions: vec![CompiledInstruction::new( | ||
| 1, // index of system program | ||
| &SystemInstruction::AdvanceNonceAccount, | ||
| vec![ | ||
| 2, // index of alt nonce account | ||
| 0, // index of nonce_authority | ||
| ], | ||
| )], | ||
| address_table_lookups: vec![MessageAddressTableLookup { | ||
| account_key: Pubkey::new_unique(), | ||
| writable_indexes: (0..loaded_addresses.writable.len()) | ||
| .map(|x| x as u8) | ||
| .collect(), | ||
| readonly_indexes: (0..loaded_addresses.readonly.len()) | ||
| .map(|x| (loaded_addresses.writable.len() + x) as u8) | ||
| .collect(), | ||
| }], | ||
| })) | ||
| .unwrap(), | ||
| SimpleAddressLoader::Enabled(loaded_addresses), | ||
| &HashSet::new(), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); | ||
| assert_eq!( | ||
| bank.check_load_and_advance_message_nonce_account( | ||
| &message, | ||
| &bank.next_durable_nonce(), | ||
| lamports_per_signature | ||
| ) | ||
| .is_none(), | ||
| require_static_nonce_account, | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,9 @@ pub trait SVMMessage: Debug { | |
| /// the pubkey of the program. | ||
| fn program_instructions_iter(&self) -> impl Iterator<Item = (&Pubkey, SVMInstruction)> + Clone; | ||
|
|
||
| /// Return the list of static account keys. | ||
| fn static_account_keys(&self) -> &[Pubkey]; | ||
|
|
||
| /// Return the account keys. | ||
| fn account_keys(&self) -> AccountKeys; | ||
|
|
||
|
|
@@ -81,7 +84,7 @@ pub trait SVMMessage: Debug { | |
| } | ||
|
|
||
| /// If the message uses a durable nonce, return the pubkey of the nonce account | ||
| fn get_durable_nonce(&self) -> Option<&Pubkey> { | ||
| fn get_durable_nonce(&self, require_static_nonce_account: bool) -> Option<&Pubkey> { | ||
| let account_keys = self.account_keys(); | ||
| self.instructions_iter() | ||
| .nth(usize::from(NONCED_TX_MARKER_IX_INDEX)) | ||
|
|
@@ -104,7 +107,9 @@ pub trait SVMMessage: Debug { | |
| .and_then(|ix| { | ||
| ix.accounts.first().and_then(|idx| { | ||
| let index = usize::from(*idx); | ||
| if !self.is_writable(index) { | ||
| if (require_static_nonce_account && index >= self.static_account_keys().len()) | ||
|
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. It looks like we really only require the number of static account keys; I think exposing static account keys is reasonable. Can you think of any reason we'd want to limit the interface to only the number of static account keys?
Author
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.
Yeah, just figured we will be using
Nope, we always have the full list of static account keys available, seems fine to expose it |
||
| || !self.is_writable(index) | ||
| { | ||
| None | ||
| } else { | ||
| account_keys.get(index) | ||
|
|
||
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.
any reason to not put this at the end?
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.
to avoid git conflicts