Tiny add_native_program bug fixes with cleanups#14042
Tiny add_native_program bug fixes with cleanups#14042ryoqun merged 2 commits intosolana-labs:masterfrom
Conversation
| } | ||
|
|
||
| fn inherit_sysvar_account_balance(&self, old_account: &Option<Account>) -> u64 { | ||
| fn inherit_specially_retained_account_balance(&self, old_account: &Option<Account>) -> u64 { |
| // flush the Stakes cache | ||
| account.lamports = 0; | ||
| self.store_account(&program_id, &account); | ||
| None |
There was a problem hiding this comment.
wrote a new test case specifically for this: (test_add_native_program_squatted_while_not_replacing)
There was a problem hiding this comment.
also improved existing test here: https://github.com/solana-labs/solana/pull/14042/files#r540367566
There was a problem hiding this comment.
bug description (re-post from https://github.com/solana-labs/solana/pull/13403/files#r535953548):
well this logic changes introduced a regression where we could be tricked into skipping to create native program because
existing_genuine_programcan be set to a bad account and not reset and we enters thismatcharm returning from thisfn: https://github.com/solana-labs/solana/pull/13403/files#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cR1760
There was a problem hiding this comment.
in other words, we shouldn't keep around fake account as Some(existing_genuine_program). It would make us skip native account creation (= must_replace = false) So, return None here.
Previously, it only cleared the bogus account...
| let account = native_loader::create_loadable_account(name); | ||
| let account = native_loader::create_loadable_account( | ||
| name, | ||
| self.inherit_specially_retained_account_balance(&existing_genuine_program), |
There was a problem hiding this comment.
this new inherit_specially_retained_account_balance call is justified by this: (test_add_native_program_inherited_cap_while_replacing)
There was a problem hiding this comment.
bug description (re-post from #13403 (comment)):
Also, when replacing, we must consider the balance of existing program account otherwise we would cause capitalization mismatch if the replaced native program isn't 1 lamports.
There was a problem hiding this comment.
Also, I later noticed that we can meaninglessly send to sysvars, but not to executable accounts. So, this can't happen on the wild. But, addressing and adding tests should never hurt us not much (judging from the needed code changes in this pr). :)
| assert!(bank.stakes.read().unwrap().vote_accounts().is_empty()); | ||
| assert!(bank.stakes.read().unwrap().stake_delegations().is_empty()); | ||
| assert_eq!(bank.calculate_capitalization(), bank.capitalization()); | ||
| assert_eq!( |
| let bank1 = Arc::new(Bank::new(&genesis_config)); | ||
| bank1.update_sysvar_account(&dummy_clock_id, |optional_account| { | ||
| assert!(optional_account.is_none()); | ||
| assert_capitalization_diff( |
There was a problem hiding this comment.
best viewed with ignore whitespace mode in github
| }, | ||
| |old, new| { | ||
| // creating new sysvar twice in a slot shouldn't increment capitalization twice | ||
| assert_eq!(old, new); |
There was a problem hiding this comment.
for example, this and that (https://github.com/solana-labs/solana/pull/14042/files#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cR10071) writing style differences are somewhat intentional for easier diff of upcoming #13884 .
| let existing_genuine_program = if let Some(mut account) = self.get_account(&program_id) { | ||
| // it's very unlikely to be squatted at program_id as non-system account because of burden to | ||
| // find victim's pubkey/hash. So, when account.owner is indeed native_loader's, it's | ||
| // safe to assume it's a genuine program. |
There was a problem hiding this comment.
added some explanation.
|
@CriesofCarrots Could you review this? thanks! |
Codecov Report
@@ Coverage Diff @@
## master #14042 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 381 381
Lines 94076 94167 +91
=======================================
+ Hits 77291 77379 +88
- Misses 16785 16788 +3 |
* Tiny add_native_program bug fixes with cleanups * Fix typo (cherry picked from commit 164b789)
2 participants
Problem
the simpler capitalization pr got too big... #13884.
Especially, this is extractable.
Summary of Changes
So, extract it! And bunch of tiny renames and preparation changes for this.
There is no need for gating because add_native_program is seldom touched on the live cluster.