Move nonce into system program #7645

t-nelson · 2019-12-31T04:03:56Z

Problem

The durable nonce feature being its own program adds complexity leading to issues like #7443 and complicating their resolution.

Summary of Changes

This will probably be easier to review by commit...

Preemptive renaming where possible (First commit. Purely cosmetic)
Move durable nonce to system program (Second commit. Bulk of change set. Relocations and deletions only. Logically inert)
Add checks for account/instruction compatibility (Commits 3-4)
Add checks to maintain nonce rent-exempt status (Commits 5-7)

Toward #7443

sdk/src/system_instruction.rs

codecov · 2019-12-31T04:58:30Z

Codecov Report

Merging #7645 into master will increase coverage by <.1%.
The diff coverage is 98.1%.

@@           Coverage Diff            @@
##           master   #7645     +/-   ##
========================================
+ Coverage    81.6%   81.7%   +<.1%     
========================================
  Files         245     243      -2     
  Lines       50230   50383    +153     
========================================
+ Hits        41021   41176    +155     
+ Misses       9209    9207      -2

runtime/src/system_instruction_processor.rs

rob-solana · 2019-12-31T19:47:04Z

runtime/src/system_instruction_processor.rs

@@ -621,6 +771,31 @@ mod tests {
        );
    }

+    #[test]
+    fn test_assign_from_nonce_account_fail() {


glad you're checking, but I think that verify_instruction also guards against this...

Mind pointing to where you're talking about? git grep is failing me

ah, was renamed to verify_account_changes()

Looks like it. The !is_zeroed() check gets us. So I guess what we're buying here is a bit of accuracy for the return code, InvalidArgument vs. ModifiedProgramId

verify_account_changes() can be really explicit, have a ton of errors, all part of the base set. the rules apply to all programs, not just system. lean on it and make a followup PR?

legal under the current rules: the stake program can change account.owner if it first zeros the state memory. that's pretty cool and correct, right?

sdk/src/account_utils.rs

runtime/src/accounts.rs

rob-solana · 2019-12-31T19:55:37Z

runtime/src/accounts.rs

        let res = accounts.load_accounts(
            &ancestors,
            &[tx],
            None,
            vec![(Ok(()), Some(HashAgeKind::Extant))],
            &hash_queue,
            error_counters,
-            &rent_collector,
+            rent_collector,


whoa! CC @ParthDesai

nevermind, is part of tests ;)

rob-solana

looking good, some important questions outstanding (see above)

also: where's the "refund the fee if the Nonce instruction fails" code? in here yet?

t-nelson · 2019-12-31T20:36:56Z

looking good, some important questions outstanding (see above)

🙏

also: where's the "refund the fee if the Nonce instruction fails" code? in here yet?

Next changeset. I wanted to pump the brakes on this one since the relocation blew up the diff so badly

runtime/src/bank.rs

runtime/src/system_instruction_processor.rs

rob-solana

nits, but otherwise lgtm

Pull request has been modified.

move to system program prereq

t-nelson · 2020-01-03T19:20:05Z

Rebased and ready to go by my eye

t-nelson · 2020-01-03T20:30:19Z

@rob-solana 5053407?w=1 look ok?

automerge (cherry picked from commit 7e94cc2) # Conflicts: # cli/src/nonce.rs # runtime/src/accounts.rs # runtime/src/system_instruction_processor.rs

automerge

t-nelson requested a review from rob-solana December 31, 2019 04:03

t-nelson commented Dec 31, 2019

View reviewed changes

sdk/src/system_instruction.rs Outdated Show resolved Hide resolved

mvines added the v0.22 label Dec 31, 2019