allow test feature to skip rewrites #33851

jeffwashington · 2023-10-24T22:21:55Z

Problem

When the feature to disable rewrites during rent collection is enabled, rent exempt accounts will not be rewritten during rent collection. And, consensus will not expect those accounts to be rewritten.
However, prior to that feature being activated, it is now possible for a validator to avoid rewrites (and improve performance), while still creating compatible bank hashes.

Summary of Changes

With a test option, during rent collection, do not do rewrites on rent exempt accounts. But, include the hashes into the bank hash's accounts delta hash so the validator remains compatible with consensus.
This allows us to test the long term effects of eliminating rewrites.

Fixes #

codecov · 2023-10-25T15:21:20Z

Codecov Report

Merging #33851 (48c875d) into master (9ffbe2a) will decrease coverage by 0.1%.
Report is 13 commits behind head on master.
The diff coverage is 81.6%.

@@            Coverage Diff            @@
##           master   #33851     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         809      809             
  Lines      217717   217761     +44     
=========================================
- Hits       178345   178329     -16     
- Misses      39372    39432     +60

HaoranYi · 2023-10-25T15:52:00Z

@jeffwashington can you take a look? thanks!

jeffwashington · 2023-10-25T16:32:09Z

runtime/src/serde_snapshot/tests.rs

@@ -764,7 +768,7 @@ mod serde_snapshot_tests {
        // F: Finally, make Step A cleanable
        current_slot += 1;
        accounts.store_for_tests(current_slot, &[(&pubkey2, &account)]);
-        accounts.calculate_accounts_delta_hash(current_slot);
+        accounts.calculate_accounts_delta_hash(current_slot, Vec::default());


seems like we should probably make a calculate_accounts_delta_hash_with_skips fn that takes the Vec and leave the signature of calculate_accounts_delta_hash the same.
I should've done that initially. That would remove all this test noise. Once all features are activated, we'll be removing this arg and all these tests would just have to go back.

yeah. updated.

jeffwashington · 2023-10-25T16:32:56Z

validator/src/cli.rs

+        .arg(
+            Arg::with_name("accounts_db_test_skip_rewrites_but_include_in_bank_hash")
+                .long("accounts-db-test-skip-rewrites-but-include-in-bank-hash")
+                .help("Debug option to skip rewrites for rent exempted accounts but still added them in bank delta hash calculation")


rent exempt. Remove ed
add them. Remove ed

yes updated.

jeffwashington · 2023-10-25T16:33:26Z

validator/src/cli.rs

@@ -1194,6 +1194,12 @@ pub fn app<'a>(version: &'a str, default_args: &'a DefaultArgs) -> App<'a, 'a> {
                .help("Debug option to scan all append vecs and verify account index refcounts prior to clean")
                .hidden(hidden_unless_forced())
        )
+        .arg(
+            Arg::with_name("accounts_db_test_skip_rewrites_but_include_in_bank_hash")
+                .long("accounts-db-test-skip-rewrites-but-include-in-bank-hash")


my words here are so verbose. @brooksprumo or @HaoranYi do you have any better ideas?

How about just accounts-db-test-skip-rewrites? Since it's testing-only and hidden, it really only needs to make sense to us.

yes. updated.

jeffwashington · 2023-10-25T16:34:41Z

accounts-db/src/accounts_hash.rs

@@ -1243,7 +1243,7 @@ pub enum ZeroLamportAccounts {

 /// Hash of an account
 #[repr(transparent)]
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable)]
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable, AbiExample)]


I guess this is caused by the addition of the rewrites on Bank?

ledger-tool/src/main.rs

runtime/src/bank.rs

accounts-db/src/accounts_db.rs

runtime/src/bank.rs

pubkey set

jeffwashington · 2023-10-26T02:45:01Z

I think a read is going to run every slot, right?Sounds like a mutex is better performance?Sent from my phone.On Oct 25, 2023, at 8:57 PM, Brooks ***@***.***> wrote: @brooksprumo commented on this pull request. In runtime/src/bank.rs:

@@ -816,6 +819,10 @@ pub struct Bank {

/// The change to accounts data size in this Bank, due to off-chain events (i.e. rent collection) accounts_data_size_delta_off_chain: AtomicI64, + /// until the skipped rewrites feature is activated, it is possible to skip rewrites and still include + /// the account hash of the accounts that would have been rewritten as bank hash expects. + skipped_rewrites: RwLock<Vec<(Pubkey, AccountHash)>>, This confuses me. I don't understand why we would use RwLock over Mutex. Without a read-heavy workload, RwLock will be strictly worse than a Mutex. I understand this is test code, but the change to a Mutex is very minimal. Any I missing something? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

brooksprumo · 2023-10-26T11:17:07Z

I think a read is going to run every slot, right?

There are no read locks in the current impl, only write locks.

HaoranYi · 2023-10-26T14:10:11Z

There seems to be a bug in fn calculate_accounts_delta_hash_internal.

When calling this fn, it has the side effect of clearing skipped_rewrites. If the function is called again, i.e. rehash() by snapshot, it will yield a different hash.

A fix could be as follows - do not clear skipped_rewrites. And to be safe, we clear skipped_rewrites before rent collection.

WDYT?

diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs
index 6313a57904..bda8da0b51 100644
--- a/runtime/src/bank.rs
+++ b/runtime/src/bank.rs
@@ -5901,6 +5901,7 @@ impl Bank {
         let partitions = self.rent_collection_partitions();
         let count = partitions.len();
         let rent_metrics = RentMetrics::default();
+        self.skipped_rewrites.write().unwrap().clear();
         // partitions will usually be 1, but could be more if we skip slots
         let mut parallel = count > 1;
         if parallel {
@@ -7103,7 +7104,7 @@ impl Bank {
             .calculate_accounts_delta_hash_internal(
                 slot,
                 ignore,
-                std::mem::take(&mut self.skipped_rewrites.write().unwrap()),
+                self.skipped_rewrites.write().unwrap().clone(),
             );

         let mut signature_count_buf = [0u8; 8];

jeffwashington · 2023-10-26T14:37:29Z

i.e. rehash() by

The two places that call rehash() have this comment:
/// This is only called from ledger-tool or tests. Warping is a special case as well.

I think we can ignore these use cases. There will only be interesting info in skipped_rewrites when this debug option is turned on.

jeffwashington · 2023-10-26T14:38:45Z

I think a read is going to run every slot, right?

There are no read locks in the current impl, only write locks.

I was considering this a read, but you are right, it is technically a write:
std::mem::take(&mut self.skipped_rewrites.write().unwrap()),

runtime/src/bank.rs

brooksprumo

lgtm!

jeffwashington and others added 4 commits October 24, 2023 17:18

allow test feature to skip rewrites

c2e406b

hook up cli arg for test skip rewrites, update tests

d2868a5

fix sanity checker

fa36141

add account hash to abi to fix a test

b6ebeb7

HaoranYi self-assigned this Oct 25, 2023

HaoranYi requested a review from brooksprumo October 25, 2023 15:50

HaoranYi marked this pull request as ready for review October 25, 2023 15:51

jeffwashington commented Oct 25, 2023

View reviewed changes

brooksprumo reviewed Oct 25, 2023

View reviewed changes

ledger-tool/src/main.rs Outdated Show resolved Hide resolved

runtime/src/bank.rs Outdated Show resolved Hide resolved

accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved

brooksprumo reviewed Oct 25, 2023

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

runtime/src/bank.rs Outdated Show resolved Hide resolved

reviews

238dda6

HaoranYi force-pushed the oc17 branch from ee32f24 to 238dda6 Compare October 25, 2023 17:57

HaoranYi added 2 commits October 25, 2023 21:07

use hashmap to collect skip_rewrites. exclude skip_rewrites from dirty

d596446

pubkey set

accumulate skipped_rewrite in reduce

1ae5737

mutex

561b9a4

HaoranYi requested a review from brooksprumo October 26, 2023 15:16

HaoranYi added 3 commits October 26, 2023 15:27

fmt

d986234

skip hash verify for this test flag

41c5fd6

add skipped rewrites num stat

b6bf790

brooksprumo reviewed Oct 26, 2023

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

skip bank hash verify not account hash verify

70c1c01

brooksprumo reviewed Oct 26, 2023

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

reviews

48c875d

brooksprumo approved these changes Oct 26, 2023

View reviewed changes

HaoranYi merged commit a18debc into solana-labs:master Oct 27, 2023
22 of 32 checks passed

HaoranYi mentioned this pull request Dec 4, 2023

Rebuilds skipped rewrites when loading from a snapshot #34280

Merged

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow test feature to skip rewrites #33851

allow test feature to skip rewrites #33851

jeffwashington commented Oct 24, 2023

codecov bot commented Oct 25, 2023 •

edited

Loading

HaoranYi commented Oct 25, 2023

jeffwashington Oct 25, 2023

HaoranYi Oct 25, 2023

jeffwashington Oct 25, 2023

HaoranYi Oct 25, 2023

jeffwashington Oct 25, 2023

brooksprumo Oct 25, 2023

HaoranYi Oct 25, 2023

jeffwashington Oct 25, 2023

HaoranYi Oct 25, 2023

jeffwashington commented Oct 26, 2023 via email

brooksprumo commented Oct 26, 2023

HaoranYi commented Oct 26, 2023 •

edited

Loading

jeffwashington commented Oct 26, 2023

jeffwashington commented Oct 26, 2023

brooksprumo left a comment

allow test feature to skip rewrites #33851

allow test feature to skip rewrites #33851

Conversation

jeffwashington commented Oct 24, 2023

Problem

Summary of Changes

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

HaoranYi commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwashington commented Oct 26, 2023 via email

brooksprumo commented Oct 26, 2023

HaoranYi commented Oct 26, 2023 • edited Loading

jeffwashington commented Oct 26, 2023

jeffwashington commented Oct 26, 2023

brooksprumo left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2023 •

edited

Loading

HaoranYi commented Oct 26, 2023 •

edited

Loading