Simple cap by including sysvars and native programs#13884
Simple cap by including sysvars and native programs#13884ryoqun merged 11 commits intosolana-labs:masterfrom
Conversation
ryoqun
force-pushed
the
simpler-capitalization
branch
from
d7e608b to
f11a8df
Compare
ryoqun
added
CI
| let existing_sysvar_account_count = 9; | ||
| let existing_native_program_account_count = 7; |
There was a problem hiding this comment.
if I killed the cluster, this would mean I even fail the elementary math class, I guess... ;)
| // flush the Stakes cache | ||
| account.lamports = 0; | ||
| self.store_account(&program_id, &account); | ||
| None |
There was a problem hiding this comment.
add a test justifying this new behavior
There was a problem hiding this comment.
Improved existing: https://github.com/solana-labs/solana/pull/13884/files#r535949045
Also wrote a new test case specifically for this: (test_add_native_program_squated_while_not_replacing)
Codecov Report
@@ Coverage Diff @@
## master #13884 +/- ##
========================================
Coverage 82.2% 82.2%
========================================
Files 384 384
Lines 94803 94965 +162
========================================
+ Hits 77948 78103 +155
- Misses 16855 16862 +7 |
mvines
left a comment
mvines
left a comment
There was a problem hiding this comment.
I read through this draft, looks good
| std::process::exit(1); | ||
|
|
||
| #[cfg(test)] | ||
| panic!("process::exit(1) is intercepted for friendly test failure..."); |
There was a problem hiding this comment.
thanks! Ideally, I want to get rid of all process:exit(1)s in lib code... But, I don't have enough bandwidth; this is a tiny 10-ish lines of hacky work-around for the current situation...
| ip_echo_server: solana_net_utils::IpEchoServer, | ||
| } | ||
|
|
||
| fn abort() -> ! { |
There was a problem hiding this comment.
my til: !! (never type)
| 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!( |
| }) | ||
| }, | ||
| |old, new| { | ||
| // creating new sysvar twice in a slot shouldn't increment capitalization twice |
There was a problem hiding this comment.
(repost)
Very subtle bug in the original draft pr was caught here.
Combined with new use of inherit_specially_retained_account_balance in this (now-parameterized) test, this new code block illustrates why it was a bad idea to update bank.capitalization in inherit_specially_retained_account_balance (this was my original draft pr changes)...
|
Status update: well greatly interrupted by other things, but I've finally finished almost all work locally. This pr is just waiting to be rebased on #14042 after its merge. |
ryoqun
force-pushed
the
simpler-capitalization
branch
from
92a31b0 to
2b5a307
Compare
| if let Some(account) = self.get_account(&feature_set::simpler_capitalization::id()) { | ||
| if let Some(feature) = feature::from_account(&account) { | ||
| if feature.activated_at == Some(0) { | ||
| return true; |
There was a problem hiding this comment.
I think this is pretty safe, not having any possibility of falsely activating this feature on testnet/mainnet-beta.
ryoqun
force-pushed
the
simpler-capitalization
branch
from
e069e6f to
b6aa451
Compare
| let mut existing_native_program_account_count = 4; | ||
|
|
||
| if self.get_account(&sysvar::rewards::id()).is_some() { | ||
| existing_sysvar_account_count += 1; |
There was a problem hiding this comment.
Heh, casual looking of coverage revealed I'm missing to test this branch... ;) b6aa451
| } | ||
| // updating sysvars (the fees sysvar in this case) now depends on feature activations in | ||
| // genesis_config.accounts above | ||
| self.update_fees(); |
There was a problem hiding this comment.
this was a hard to debug thing... ;p
|
@mvines hooray! i needed various supllemental prs for this (see referenced links...). But finally this pr is finally ready for serious review. I think this pr is pretty solid now compared from 9 days ago #13884 (review) |
mvines
left a comment
mvines
left a comment
There was a problem hiding this comment.
Looks good to me. I think I'd prefer simpler_capitalization be called simple_capitalization as a global search and replace.
ryoqun
changed the title
Thanks for the review! Yeah, I've renamed: 31a3cd6 |
|
After long and extensive testing, I finally merged this. :) |
* Simpler cap by including sysvars and native programs * Fix tests * Add comment * revert some unrelated code * Update test_bank_update_sysvar_account for cap. * Test cap. for add_native_program using new helper * Improve the cap adjustment with new tests * Fix typo... * Adjust test for improved code coverage * Rename simpler_capitalization => simple_capitalization * More rename and bonus commenting (cherry picked from commit de9ac43)
* Simpler cap by including sysvars and native programs * Fix tests * Add comment * revert some unrelated code * Update test_bank_update_sysvar_account for cap. * Test cap. for add_native_program using new helper * Improve the cap adjustment with new tests * Fix typo... * Adjust test for improved code coverage * Rename simpler_capitalization => simple_capitalization * More rename and bonus commenting (cherry picked from commit de9ac43) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
4 participants
Problem
Simplify capitalization calculation by removing special casing.
Summary of Changes
Adjust cap while adding/updating sysvar and native program accounts correctly to simplify cap calculation.
Also, this fixes a regression bug and a new bug introduced at #13403. In this sense, this pr is a follow up to #13403, #11750.(EDIT: this was done by separate pr)