adds system instruction to upgrade legacy nonce versions #25789

behzadnouri · 2022-06-05T22:45:50Z

Problem

#25788
permanently disables durable transactions with legacy nonce versions
which are within chain blockhash domain.

Summary of Changes

This commit adds a new system instruction for a one-time idempotent
upgrade of legacy nonce accounts in order to bump them out of chain
blockhash domain.

t-nelson

this is looking good, thanks for plumbing through cli and web3.js! just need to add the account owner check, i think

t-nelson · 2022-06-06T00:43:04Z

runtime/src/system_instruction_processor.rs

+                return Err(InstructionError::InvalidInstructionData);
+            }
+            instruction_context.check_number_of_instruction_accounts(1)?;
+            let mut nonce_account =


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

codecov · 2022-06-06T01:54:44Z

Codecov Report

Merging #25789 (169a7e3) into master (8caced6) will decrease coverage by 6.7%.
The diff coverage is n/a.

❗ Current head 169a7e3 differs from pull request most recent head 5cd72ba. Consider uploading reports for the commit 5cd72ba to get more accurate results

@@             Coverage Diff             @@
##           master   #25789       +/-   ##
===========================================
- Coverage    82.1%    75.4%     -6.8%     
===========================================
  Files         628       40      -588     
  Lines      171471     2345   -169126     
  Branches        0      338      +338     
===========================================
- Hits       140878     1769   -139109     
+ Misses      30593      459    -30134     
- Partials        0      117      +117

CriesofCarrots · 2022-06-06T23:20:17Z

cli/src/nonce.rs

+) -> ProcessResult {
+    let latest_blockhash = rpc_client.get_latest_blockhash()?;
+    let ixs = vec![upgrade_nonce_account(nonce_account)].with_memo(memo);
+    // TODO what signers?


Is this comment still needed?

Yeah, this code is still in WIP until #25788 is merged.

jstarry

lgtm after the todo comment cleanup

jstarry · 2022-06-07T00:11:39Z

cli/src/nonce.rs

+        // TODO: need signers.
+        signers: CliSigners::default(),


This is fine because if CliSigners is empty, we fall back to the wallet keypair here:

solana/cli/src/main.rs

Line 183 in cbb0f07

if signers.is_empty() {

removed todo

ryoqun · 2022-06-07T03:46:34Z

sdk/program/src/system_instruction.rs

@@ -307,6 +307,13 @@ pub enum SystemInstruction {
        /// Owner to use to derive the funding account address
        from_owner: Pubkey,
    },
+
+    /// One-time idempotent upgrade of legacy nonce versions in order to bump


what does this idempotent refers to? i mean, this instruction impl. errors InstructionError::InvalidArgument if Current nonce account (i.e. already upgraded) is given. So, you cannot invoke UpgradeNonceAccount multiple times successfully, given the same nonce account.

It refers to the nonce account value.
If the instruction is applied multiple times on the same nonce account, the nonce account will not change beyond the first time.

runtime/src/accounts.rs

behzadnouri · 2022-06-09T18:23:44Z

runtime/src/system_instruction_processor.rs

+            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),
+            }


🚨 🚨 🚨
The assumptions here are that:

Any extant system account that has non-zero data and is not a nonce account is invalid.

Writing to these invalid accounts is fine and won't break anything.

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

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

Pull request has been modified.

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts

…#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts

…5789) (#25890) * feat(nonce): adds system instruction to upgrade legacy nonce versions (#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts * removes mergify merge conflicts * backport UpgradeNonceAccount Co-authored-by: behzad nouri <behzadnouri@gmail.com>

…5789) (#25891) * feat(nonce): adds system instruction to upgrade legacy nonce versions (#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts * removes mergify merge conflicts * backport UpgradeNonceAccount Co-authored-by: behzad nouri <behzadnouri@gmail.com>

Lichtso · 2022-06-10T19:32:42Z

runtime/src/system_instruction_processor.rs

+            if !separate_nonce_from_blockhash {
+                return Err(InstructionError::InvalidInstructionData);
+            }
+            instruction_context.check_number_of_instruction_accounts(1)?;


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.

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.

behzadnouri force-pushed the upgrade-nonce-instruction branch from 7d8e88a to 077e8e2 Compare June 5, 2022 22:46

behzadnouri changed the title ~~adds system instruction to upgrade legacy durable nonce accounts~~ adds system instruction to upgrade legacy nonce versions Jun 5, 2022

behzadnouri force-pushed the upgrade-nonce-instruction branch 3 times, most recently from 14419a4 to 1f652f1 Compare June 6, 2022 00:07

t-nelson reviewed Jun 6, 2022

View reviewed changes

behzadnouri force-pushed the upgrade-nonce-instruction branch 3 times, most recently from 3ea2f13 to 9c7a7fc Compare June 6, 2022 16:04

jstarry self-requested a review June 6, 2022 21:00

behzadnouri force-pushed the upgrade-nonce-instruction branch from 9c7a7fc to 9d46737 Compare June 6, 2022 21:05

CriesofCarrots reviewed Jun 6, 2022

View reviewed changes

jstarry reviewed Jun 7, 2022

View reviewed changes

ryoqun reviewed Jun 7, 2022

View reviewed changes

jstarry mentioned this pull request Jun 8, 2022

Nonce account migration and re-enabling durable nonce transactions #25787

Closed

30 tasks

behzadnouri force-pushed the upgrade-nonce-instruction branch 2 times, most recently from e9f5265 to 2562de1 Compare June 9, 2022 13:43

jackcmay reviewed Jun 9, 2022

View reviewed changes

runtime/src/accounts.rs Outdated Show resolved Hide resolved

behzadnouri force-pushed the upgrade-nonce-instruction branch from 2562de1 to b1b5da1 Compare June 9, 2022 16:21

behzadnouri requested review from t-nelson, jstarry and CriesofCarrots June 9, 2022 16:22

behzadnouri added v1.9 labels Jun 9, 2022

behzadnouri commented Jun 9, 2022

View reviewed changes

behzadnouri requested review from sakridge, mvines, jackcmay and ryoqun June 9, 2022 18:25

behzadnouri force-pushed the upgrade-nonce-instruction branch from b1b5da1 to ec261e1 Compare June 9, 2022 21:42

behzadnouri force-pushed the upgrade-nonce-instruction branch from ec261e1 to 5cd72ba Compare June 9, 2022 22:00

jstarry approved these changes Jun 9, 2022

View reviewed changes

behzadnouri merged commit b419031 into solana-labs:master Jun 10, 2022

behzadnouri deleted the upgrade-nonce-instruction branch June 10, 2022 00:04

mergify bot mentioned this pull request Jun 10, 2022

adds system instruction to upgrade legacy nonce versions (backport #25789) #25890

Merged

mergify bot mentioned this pull request Jun 10, 2022

adds system instruction to upgrade legacy nonce versions (backport #25789) #25891

Merged

Lichtso reviewed Jun 10, 2022

View reviewed changes

snyk-bot mentioned this pull request Jul 1, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.44.0 Balantion2020/Balantion#153

Open

This was referenced Jul 9, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.44.1 Balantion2020/Balantion#178

Open

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.44.2 Balantion2020/Balantion#187

Open

snyk-bot mentioned this pull request Jul 18, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.44.3 Balantion2020/Balantion#198

Open

This was referenced Jul 21, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.46.0 Balantion2020/Balantion#207

Open

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.47.0 Balantion2020/Balantion#212

Open

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.47.2 Balantion2020/Balantion#215

Open

snyk-bot mentioned this pull request Jul 24, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.47.3 Balantion2020/Balantion#216

Open

This was referenced Aug 3, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.47.4 Balantion2020/Balantion#236

Open

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.48.0 Balantion2020/Balantion#238

Open

Balantion2020 mentioned this pull request Aug 5, 2022

[Snyk] Upgrade @solana/web3.js from 1.31.0 to 1.48.0 Balantion2020/Balantion#242

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds system instruction to upgrade legacy nonce versions #25789

adds system instruction to upgrade legacy nonce versions #25789

behzadnouri commented Jun 5, 2022 •

edited

Loading

t-nelson left a comment

t-nelson Jun 6, 2022

behzadnouri Jun 6, 2022

codecov bot commented Jun 6, 2022 •

edited

Loading

CriesofCarrots Jun 6, 2022

behzadnouri Jun 7, 2022

behzadnouri Jun 9, 2022

jstarry left a comment

jstarry Jun 7, 2022

behzadnouri Jun 9, 2022

ryoqun Jun 7, 2022

behzadnouri Jun 7, 2022

behzadnouri Jun 9, 2022 •

edited

Loading

jackcmay Jun 9, 2022

behzadnouri Jun 9, 2022 •

edited

Loading

Lichtso Jun 10, 2022

jackcmay Jun 10, 2022

adds system instruction to upgrade legacy nonce versions #25789

adds system instruction to upgrade legacy nonce versions #25789

Conversation

behzadnouri commented Jun 5, 2022 • edited Loading

Problem

Summary of Changes

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri commented Jun 5, 2022 •

edited

Loading

codecov bot commented Jun 6, 2022 •

edited

Loading

behzadnouri Jun 9, 2022 •

edited

Loading

behzadnouri Jun 9, 2022 •

edited

Loading