Check bank capitalization #11927
Merged
Check bank capitalization #11927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
btw, because the testnet ledger was restarted, it's now safe to start to enforce this everytime validator starts. (thanks @mvines). |
ryoqun
commented
Sep 1, 2020
| @@ -2843,22 +2845,7 @@ impl Bank { | |||
| } | |||
|
|
|||
| pub fn calculate_capitalization(&self) -> u64 { | |||
| self.get_program_accounts(None) | |||
There was a problem hiding this comment.
This has been solely used for the ledger-tool capitalization subcommand.
Move to AccountsDB:: and use this from ledger-tool and snapshot codepaths
Codecov Report
@@ Coverage Diff @@
## master #11927 +/- ##
========================================
Coverage 82.0% 82.0%
========================================
Files 337 337
Lines 79220 79369 +149
========================================
+ Hits 65015 65160 +145
- Misses 14205 14209 +4 |
ryoqun
commented
Sep 1, 2020
ryoqun
commented
Sep 1, 2020
|
@ryoqun Let's say we had an inflation bug, like an overflow in the contract, would this catch it? I want to make sure that we can compute the max possible supply in the snapshot and fail if it exceeds it. |
ryoqun
commented
Sep 1, 2020
ryoqun
commented
Sep 1, 2020
ryoqun
commented
Sep 1, 2020
@aeyakovenko Yeah, I've tested this locally. These catches them as the last protection mechanism. For both inflation and overflow in contracts, these will be protected by their specific protections, btw. So, I've manually disabled them while testing locally. I've added some sanity assertion: Lines 932 to 940 in 11ac4eb solana/runtime/src/message_processor.rs Lines 559 to 578 in 11ac4eb |
|
@sakridge Could you review this? I have still something to do (add test for overflow; benchmark checked u64 vs u128 for summing) but mostly this should be done. What I have added to your original pr:
|
ryoqun
force-pushed
the
bank-capitalization
branch
from
0a64fb0 to
d4d1831
Compare
All of these are now done. For benchmarking, I saw no noticeable difference and doing the cap calculation should be no harm because it's so small compared to hashing: slower: faster: For completeness and pure curiosity, I've went into details and found that https://docs.google.com/spreadsheets/d/1nsnNwkaTQ4pjpQKPwEhYcxhuhU0YCnvYm81Nmvf9zjA/edit#gid=0 |
ryoqun
force-pushed
the
bank-capitalization
branch
from
f9b58ac to
4ba5a8c
Compare
sakridge
reviewed
Sep 8, 2020
sakridge
reviewed
Sep 8, 2020
sakridge
reviewed
Sep 8, 2020
sakridge
reviewed
Sep 8, 2020
|
@sakridge Thanks for reviewing. I think I've addressed all your review comments. |
mergify bot
pushed a commit
that referenced
this pull request
Sep 11, 2020
* Check bank capitalization * Simplify and unify capitalization calculation * Improve and add tests * Avoid overflow and inhibit automatic restart * Fix test * Tweak checked sum for cap. and add tests * Fix broken build after merge conflicts.. * Rename to ClusterType * Rename confusing method * Clarify comment * Verify cap. in rent and inflation tests Co-authored-by: Stephen Akridge <[email protected]> (cherry picked from commit de4a613) # Conflicts: # Cargo.lock # accounts-bench/Cargo.toml
mergify bot
added a commit
that referenced
this pull request
Sep 11, 2020
* Check bank capitalization (#11927) * Check bank capitalization * Simplify and unify capitalization calculation * Improve and add tests * Avoid overflow and inhibit automatic restart * Fix test * Tweak checked sum for cap. and add tests * Fix broken build after merge conflicts.. * Rename to ClusterType * Rename confusing method * Clarify comment * Verify cap. in rent and inflation tests Co-authored-by: Stephen Akridge <[email protected]> (cherry picked from commit de4a613) # Conflicts: # Cargo.lock # accounts-bench/Cargo.toml * Fix conflict 1/2 * Fix conflict 2/2 Co-authored-by: Ryo Onodera <[email protected]>
This was referenced Sep 14, 2020
This was referenced Sep 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
No check for bank capitalization.
Also, the current offline bank capitalization check doesn't catch overflows.
Summary of Changes
On accounts hashing, load the total accounts capitalization and compare with bank.capitalization.
Fixes #
context
reboot of #10937