Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Maintain sysvar balances for consistent market cap. #9936

Merged
merged 2 commits into from
May 8, 2020

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented May 8, 2020

problem

sysvar account's balance is forced to overwrite to the fixed 1-lamport for each update even after someone transfers some lamports to it (btw, this effectively burns the lamports and so it must be mistake, anyway).

That creates discrepancy between bank.capitalization and the actual sum of all alive account balances.

solution

Correctly, inherit the deposited balance when updating sysvars.

Also, add subcommand solana-ledger-tool capitalization which shows the capitalization after verifying there is no discrepancy.

@@ -563,16 +563,20 @@ impl Bank {
self.store_account(pubkey, &new_account);
}

fn inherit_sysvar_account_balance(&self, old_account: &Option<Account>) -> u64 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not Self::? hint: for gating logic of backport prs: self.epoch().

fn fmt(&self, f: &mut Formatter) -> Result {
write!(
f,
"{}.{:09} SOL",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to promote this to the Display across the whole cli. But, that change will be invasive; let's minimize for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this pr absolutely needs this precision over f64 because capitalization tends to be well larger than the significant digits of f64.

computed_capitalization
);
}
println!("Capitalization: {}", Sol(bank.capitalization()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danpaul000 FYI: I've created the rust version of capitalization bash script, which you created in that past iirc. :)

$ RUST_LOG=solana=error solana-ledger-tool --ledger config/ledger/ capitalization
Capitalization: 999999999.998820000 SOL

}
}

let computed_capitalization: u64 = bank
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to use u64 over u128 unlike this (pr #9582) because account.lamports are well trusted data field in this context (We're past the snapshot verification) and the max capitazalition must always be within u64. Also, being u64 caught this.

.get_program_accounts(None)
.into_iter()
.filter_map(|(_pubkey, account)| {
if account.lamports == u64::max_value() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is like this handling (at #9869) CC: @CriesofCarrots:

  • Filter out Storage program RewardsPools that get initialized with u64::MAX lamports

}

let is_specially_retained =
solana_sdk::native_loader::check_id(&account.owner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackcmay I'm not that familiar with loaders... Well, loaders can have similar problem as this pr? What happens to the balance if one of loaders is updated? (or not updated ever, first of all?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it seems that I can't change of account.lamports first of all:

$ solana --url http://localhost:8899 pay -v Stake11111111111111111111111111111111111111 0.82643
RPC URL: http://localhost:8899
Default Signer Path: /home/ryoqun/.config/solana/id.json
Error: TransactionError::InstructionError(0, ExecutableLamportChange)

Copy link
Contributor

Choose a reason for hiding this comment

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

Once marked executable you can't change its lamports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And, iirc the executable bit can't be off after that, too? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct

@ryoqun
Copy link
Contributor Author

ryoqun commented May 8, 2020

@mvines I've created a manual-backport for v1.0: #9937

I'll leave the v1.1 branch to mergify, this shouldn't need gating, I guess.

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #9936 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master   #9936   +/-   ##
======================================
  Coverage    80.4%   80.4%           
======================================
  Files         283     283           
  Lines       65917   65951   +34     
======================================
+ Hits        53027   53073   +46     
+ Misses      12890   12878   -12     

@ryoqun ryoqun removed the v1.0 label May 8, 2020
@ryoqun ryoqun merged commit 00e45ec into solana-labs:master May 8, 2020
@ryoqun ryoqun added the security Pull requests that address a security vulnerability label May 13, 2020
@ryoqun
Copy link
Contributor Author

ryoqun commented May 13, 2020

Well, this was dos, fixed now on the mainnet-beta: https://github.com/solana-labs/solana/pull/9937/files#r424512737

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants