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

adds system instruction to upgrade legacy nonce versions #25789

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
10 changes: 10 additions & 0 deletions cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ pub enum CliCommand {
destination_account_pubkey: Pubkey,
lamports: u64,
},
UpgradeNonceAccount {
nonce_account: Pubkey,
memo: Option<String>,
},
// Program Deployment
Deploy {
program_location: String,
Expand Down Expand Up @@ -633,6 +637,7 @@ pub fn parse_command(
("withdraw-from-nonce-account", Some(matches)) => {
parse_withdraw_from_nonce_account(matches, default_signer, wallet_manager)
}
("upgrade-nonce-account", Some(matches)) => parse_upgrade_nonce_account(matches),
// Program Deployment
("deploy", Some(matches)) => {
let (address_signer, _address) = signer_of(matches, "address_signer", wallet_manager)?;
Expand Down Expand Up @@ -1019,6 +1024,11 @@ pub fn process_command(config: &CliConfig) -> ProcessResult {
destination_account_pubkey,
*lamports,
),
// Upgrade nonce account out of blockhash domain.
CliCommand::UpgradeNonceAccount {
nonce_account,
memo,
} => process_upgrade_nonce_account(&rpc_client, config, *nonce_account, memo.as_ref()),

// Program Deployment

Expand Down
83 changes: 80 additions & 3 deletions cli/src/nonce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use {
solana_clap_utils::{
input_parsers::*,
input_validators::*,
keypair::{DefaultSigner, SignerIndex},
keypair::{CliSigners, DefaultSigner, SignerIndex},
memo::{memo_arg, MEMO_ARG},
nonce::*,
},
Expand All @@ -30,8 +30,8 @@ use {
pubkey::Pubkey,
system_instruction::{
advance_nonce_account, authorize_nonce_account, create_nonce_account,
create_nonce_account_with_seed, instruction_to_nonce_error, withdraw_nonce_account,
NonceError, SystemError,
create_nonce_account_with_seed, instruction_to_nonce_error, upgrade_nonce_account,
withdraw_nonce_account, NonceError, SystemError,
},
system_program,
transaction::{Transaction, TransactionError},
Expand Down Expand Up @@ -173,6 +173,19 @@ impl NonceSubCommands for App<'_, '_> {
.arg(nonce_authority_arg())
.arg(memo_arg()),
)
.subcommand(
SubCommand::with_name("upgrade-nonce-account")
.about("One-time idempotent upgrade of legacy nonce versions \
in order to bump them out of chain blockhash domain.")
.arg(
pubkey!(Arg::with_name("nonce_account_pubkey")
.index(1)
.value_name("NONCE_ACCOUNT_ADDRESS")
.required(true),
"Nonce account to upgrade. "),
)
.arg(memo_arg()),
)
}
}

Expand Down Expand Up @@ -325,6 +338,20 @@ pub fn parse_withdraw_from_nonce_account(
})
}

pub(crate) fn parse_upgrade_nonce_account(
matches: &ArgMatches<'_>,
) -> Result<CliCommandInfo, CliError> {
let nonce_account = pubkey_of(matches, "nonce_account_pubkey").unwrap();
let memo = matches.value_of(MEMO_ARG.name).map(String::from);
Ok(CliCommandInfo {
command: CliCommand::UpgradeNonceAccount {
nonce_account,
memo,
},
signers: CliSigners::default(),
})
}

/// Check if a nonce account is initialized with the given authority and hash
pub fn check_nonce_account(
nonce_account: &Account,
Expand Down Expand Up @@ -656,6 +683,39 @@ pub fn process_withdraw_from_nonce_account(
}
}

pub(crate) fn process_upgrade_nonce_account(
rpc_client: &RpcClient,
config: &CliConfig,
nonce_account: Pubkey,
memo: Option<&String>,
) -> ProcessResult {
let latest_blockhash = rpc_client.get_latest_blockhash()?;
let ixs = vec![upgrade_nonce_account(nonce_account)].with_memo(memo);
let message = Message::new(&ixs, Some(&config.signers[0].pubkey()));
let mut tx = Transaction::new_unsigned(message);
tx.try_sign(&config.signers, latest_blockhash)?;
check_account_for_fee_with_commitment(
rpc_client,
&config.signers[0].pubkey(),
&tx.message,
config.commitment,
)?;
let merge_errors =
get_feature_is_active(rpc_client, &merge_nonce_error_into_system_error::id())?;
let result = rpc_client.send_and_confirm_transaction_with_spinner(&tx);
if merge_errors {
log_instruction_custom_error::<SystemError>(result, config)
} else {
log_instruction_custom_error_ex::<NonceError, _>(result, config, |ix_error| {
if let InstructionError::Custom(_) = ix_error {
instruction_to_nonce_error(ix_error, merge_errors)
} else {
None
}
})
}
}

#[cfg(test)]
mod tests {
jackcmay marked this conversation as resolved.
Show resolved Hide resolved
use {
Expand Down Expand Up @@ -925,6 +985,23 @@ mod tests {
],
}
);

// Test UpgradeNonceAccount Subcommand.
let test_upgrade_nonce_account = test_commands.clone().get_matches_from(vec![
"test",
"upgrade-nonce-account",
&nonce_account_string,
]);
assert_eq!(
parse_command(&test_upgrade_nonce_account, &default_signer, &mut None).unwrap(),
CliCommandInfo {
command: CliCommand::UpgradeNonceAccount {
nonce_account: nonce_account_pubkey,
memo: None,
},
signers: CliSigners::default(),
}
);
}

#[test]
Expand Down
1 change: 1 addition & 0 deletions docs/src/developing/clients/javascript-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ pub enum SystemInstruction {
/** 9 **/AllocateWithSeed {/**/},
/** 10 **/AssignWithSeed {/**/},
/** 11 **/TransferWithSeed {/**/},
/** 12 **/UpgradeNonceAccount,
}
```

Expand Down
172 changes: 171 additions & 1 deletion runtime/src/system_instruction_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,28 @@ pub fn process_instruction(
instruction_context.try_borrow_instruction_account(transaction_context, 0)?;
authorize_nonce_account(&mut me, &nonce_authority, &signers, invoke_context)
}
SystemInstruction::UpgradeNonceAccount => {
let separate_nonce_from_blockhash = invoke_context
.feature_set
.is_active(&feature_set::separate_nonce_from_blockhash::id());
if !separate_nonce_from_blockhash {
return Err(InstructionError::InvalidInstructionData);
}
instruction_context.check_number_of_instruction_accounts(1)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

check_number_of_instruction_accounts is not necessary here as UpgradeNonceAccount is added as a whole. It was only used for older instructions to keep the error priorities the same without a feature gate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though keeping it is consistent with the rest of the instruction processing and might make folks wonder why other places and not here. I would vote to keep it here for consistency with the other instructions and remove them all at once when deemed appropriate.

let mut nonce_account =
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to verify that the account owner is the system program here to avoid account confusion vulnerabilities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

instruction_context.try_borrow_instruction_account(transaction_context, 0)?;
if !system_program::check_id(nonce_account.get_owner()) {
return Err(InstructionError::InvalidAccountOwner);
}
if !nonce_account.is_writable() {
return Err(InstructionError::InvalidArgument);
}
let nonce_versions: nonce::state::Versions = nonce_account.get_state()?;
match nonce_versions.upgrade() {
None => Err(InstructionError::InvalidArgument),
Some(nonce_versions) => nonce_account.set_state(&nonce_versions),
}
Comment on lines +502 to +506
Copy link
Contributor Author

@behzadnouri behzadnouri Jun 9, 2022

Choose a reason for hiding this comment

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

🚨 🚨 🚨
The assumptions here are that:

  1. Any extant system account that has non-zero data and is not a nonce account is invalid.
  2. Writing to these invalid accounts is fine and won't break anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I didn't see them but would be good to have a test to exercise these assumptions

Copy link
Contributor Author

@behzadnouri behzadnouri Jun 9, 2022

Choose a reason for hiding this comment

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

I believe these assumptions come from get_system_account_kind:
https:/solana-labs/solana/blob/b4bdbb27d/runtime/src/system_instruction_processor.rs#L546-L563

}
SystemInstruction::Allocate { space } => {
instruction_context.check_number_of_instruction_accounts(1)?;
let mut account = instruction_context
Expand Down Expand Up @@ -568,11 +590,18 @@ mod tests {
use solana_sdk::{
account::{self, Account, AccountSharedData, ReadableAccount},
client::SyncClient,
fee_calculator::FeeCalculator,
genesis_config::create_genesis_config,
hash::{hash, Hash},
instruction::{AccountMeta, Instruction, InstructionError},
message::Message,
nonce, nonce_account, recent_blockhashes_account,
nonce::{
self,
state::{
Data as NonceData, DurableNonce, State as NonceState, Versions as NonceVersions,
},
},
nonce_account, recent_blockhashes_account,
signature::{Keypair, Signer},
system_instruction, system_program,
sysvar::{self, recent_blockhashes::IterItem, rent::Rent},
Expand Down Expand Up @@ -2202,4 +2231,145 @@ mod tests {
},
);
}

#[test]
fn test_nonce_account_upgrade_check_owner() {
let nonce_address = Pubkey::new_unique();
let versions = NonceVersions::Legacy(Box::new(NonceState::Uninitialized));
let nonce_account = AccountSharedData::new_data(
1_000_000, // lamports
&versions, // state
&Pubkey::new_unique(), // owner
)
.unwrap();
let accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account.clone())],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: true,
}],
Err(InstructionError::InvalidAccountOwner),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
assert_eq!(accounts[0], nonce_account);
}

fn new_nonce_account(versions: NonceVersions) -> AccountSharedData {
let nonce_account = AccountSharedData::new_data(
1_000_000, // lamports
&versions, // state
&system_program::id(), // owner
)
.unwrap();
assert_eq!(
nonce_account.deserialize_data::<NonceVersions>().unwrap(),
versions
);
nonce_account
}

#[test]
fn test_nonce_account_upgrade() {
let nonce_address = Pubkey::new_unique();
let versions = NonceVersions::Legacy(Box::new(NonceState::Uninitialized));
let nonce_account = new_nonce_account(versions);
let accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account.clone())],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: true,
}],
Err(InstructionError::InvalidArgument),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
assert_eq!(accounts[0], nonce_account);
let versions = NonceVersions::Current(Box::new(NonceState::Uninitialized));
let nonce_account = new_nonce_account(versions);
let accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account.clone())],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: true,
}],
Err(InstructionError::InvalidArgument),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
assert_eq!(accounts[0], nonce_account);
let blockhash = Hash::from([171; 32]);
let durable_nonce =
DurableNonce::from_blockhash(&blockhash, /*separate_domains:*/ false);
let data = NonceData {
authority: Pubkey::new_unique(),
durable_nonce,
fee_calculator: FeeCalculator {
lamports_per_signature: 2718,
},
};
let versions = NonceVersions::Legacy(Box::new(NonceState::Initialized(data.clone())));
let nonce_account = new_nonce_account(versions);
let accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account.clone())],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: false, // Should fail!
}],
Err(InstructionError::InvalidArgument),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
assert_eq!(accounts[0], nonce_account);
let mut accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account)],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: true,
}],
Ok(()),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
let nonce_account = accounts.remove(0);
let durable_nonce =
DurableNonce::from_blockhash(&blockhash, /*separate_domains:*/ true);
assert_ne!(data.durable_nonce, durable_nonce);
let data = NonceData {
durable_nonce,
..data
};
let upgraded_nonce_account =
NonceVersions::Current(Box::new(NonceState::Initialized(data)));
assert_eq!(
nonce_account.deserialize_data::<NonceVersions>().unwrap(),
upgraded_nonce_account
);
let accounts = process_instruction(
&serialize(&SystemInstruction::UpgradeNonceAccount).unwrap(),
vec![(nonce_address, nonce_account)],
vec![AccountMeta {
pubkey: nonce_address,
is_signer: false,
is_writable: true,
}],
Err(InstructionError::InvalidArgument),
super::process_instruction,
);
assert_eq!(accounts.len(), 1);
assert_eq!(
accounts[0].deserialize_data::<NonceVersions>().unwrap(),
upgraded_nonce_account
);
}
}
Loading