Adds StartingSnapshotStorages to AccountsHashVerifier #58

brooksprumo · 2024-03-04T17:42:14Z

Problem

When starting up from fastboot, if shrink runs before taking the next bank snapshot, then snapshot storages can change. And then if the node restarts before taking the next bank snapshot, it may fail because the snapshot storages are now wrong.

Please see solana-labs#35376 for more information.

Summary of Changes

At startup, get the storages that were loaded from, for fastboot. Pass them into AccountsHashVerifier, because AHV is in charge of holding fastboot storages to prevent early cleanup.

I tested this with a node on mnb.

I started a node and let it run for a while, so it took a bunch of snapshots
I removed the code to purge all bank snapshots at startup with fastboot
I restarted the node, and it started with fastboot correctly
I inserted code to crash the node before the bank snapshot POST was reserialized (this ensures shrink runs)
the node crashed, as expected
I restarted the node, and it used the same snapshot for fastboot as in step (3)

And it worked! Previously, this would crash.

Fixes solana-labs#35376

jeffwashington · 2024-03-04T18:48:20Z

I haven't taken the time to look deeply at every code change. I did skim it and I don't think this is addressed.
Is the issue with shrink due to recycling?
Recycling should only be possible if Arc::strong_count is =0.
Assuming recycling is the issue, could we:

get rid of it?
hold an Arc::strong_count on boot from fast boot and drop it when we create the first new fast_boot folder?

brooksprumo · 2024-03-04T18:53:52Z

Is the issue with shrink due to recycling? Recycling should only be possible if Arc::strong_count is =0.

Yeah, it's from recycling. This was an issue for all of fastboot initially. The fix is with fastboot_storages:

agave/core/src/accounts_hash_verifier.rs

Lines 54 to 57 in 3f9a7a5

    
           // To support fastboot, we must ensure the storages used in the latest POST snapshot are 
        
           // not recycled nor removed early.  Hold an Arc of their AppendVecs to prevent them from 
        
           // expiring. 
        
           let mut fastboot_storages = None;

Assuming recycling is the issue, could we:

get rid of it?

hold an Arc::strong_count on boot from fast boot and drop it when we create the first new fast_boot folder?

I don't know enough about recycling to do (1). Is it safe? Why was it added initially? Code spelunking sees to indicate that recreating the append vecs was an issue (maybe the underlying mmap?). Is that not an issue anymore?

And for (2), that's what this PR does. (Except you cannot drop the storages until the subsequent bank snapshot POST is made.)

jeffwashington · 2024-03-04T18:57:39Z

I don't know enough about recycling to do (1). Is it safe? Why was it added initially? Code spelunking sees to indicate that recreating the append vecs was an issue (maybe the underlying mmap?). Is that not an issue anymore?

it is a mysterious area of the code. We could probably use some more expertise in it. Originally append vecs were the way to store written accounts from tx processing. This model isn't used anymore. We would also use multiple append vecs per slot. Also not used anymore. Maybe there is still a place for recycling. I do know it is under suspicion so often and sometimes IS the reason some operation has a hole (like this one). It definitely adds complexity.

codecov-commenter · 2024-03-04T19:10:44Z

Codecov Report

Attention: Patch coverage is 42.85714% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (3f9a7a5) to head (ee760bc).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         837      838    +1     
  Lines      225922   225949   +27     
=======================================
+ Hits       184955   184980   +25     
- Misses      40967    40969    +2

brooksprumo · 2024-03-04T19:19:51Z

it is a mysterious area of the code. We could probably use some more expertise in it. Originally append vecs were the way to store written accounts from tx processing. This model isn't used anymore. We would also use multiple append vecs per slot. Also not used anymore. Maybe there is still a place for recycling. I do know it is under suspicion so often and sometimes IS the reason some operation has a hole (like this one). It definitely adds complexity.

Ok, I'll take a look into this. And this PR may be the recycling genesis: solana-labs#12885

jeffwashington · 2024-03-04T19:54:07Z

core/src/accounts_hash_verifier.rs

@@ -54,7 +56,11 @@ impl AccountsHashVerifier {
                // To support fastboot, we must ensure the storages used in the latest POST snapshot are
                // not recycled nor removed early.  Hold an Arc of their AppendVecs to prevent them from
                // expiring.
-                let mut fastboot_storages = None;
+                let mut fastboot_storages = match starting_snapshot_storages {


ok, fastboot_storages already existed and is overwritten later in the existing code.

jeffwashington

lgtm

apfitzge

Changes seem reasonable to me, we cache an Arc of the storages we used to load our bank from. By doing so it prevents them from being shrunk so long as we hold the Arc. We had already done this for snapshots we were creating, and this PR makes startup more similar to other code.

We have some hidden argument for skipping (or not) shrinking/cleaning. It seems we will now skip shrinking unless we are loading from archives - do we need to add any sort of conflict with these args & fastboot?

brooksprumo · 2024-03-04T21:22:02Z

We have some hidden argument for skipping (or not) shrinking/cleaning. It seems we will now skip shrinking unless we are loading from archives - do we need to add any sort of conflict with these args & fastboot?

I think we'll be OK. By holding these Arcs, we prevent them from getting recycled. When/if shrink runs (startup or otherwise), I believe it'll create a new append vec for the shrunk results, instead of recycling one of these append vecs. @jeffwashington does that sound right?

jeffwashington · 2024-03-04T21:28:53Z

it'll create a new append vec for the shrunk results, instead of recycling one of these append vecs.

this is correct. This change prevents recycling the append vecs used in fast boot.

jeffwashington · 2024-03-04T21:30:01Z

We have some hidden argument for skipping (or not) shrinking/cleaning.

this cli arg causes the validator to start first, then do the initial clean and then shrink in the background. The previous behavior was to wait to start the validator (replay, turbine) until the initial clean and shrink completed.

Adds StartingSnapshotStorages to AccountsHashVerifier

ee760bc

brooksprumo self-assigned this Mar 4, 2024

brooksprumo marked this pull request as ready for review March 4, 2024 18:43

brooksprumo requested review from apfitzge and jeffwashington March 4, 2024 18:43

jeffwashington reviewed Mar 4, 2024

View reviewed changes

jeffwashington approved these changes Mar 4, 2024

View reviewed changes

apfitzge approved these changes Mar 4, 2024

View reviewed changes

brooksprumo merged commit 93f5b51 into anza-xyz:master Mar 4, 2024
35 checks passed

brooksprumo deleted the fastboot/starting-snapshot-storages branch March 4, 2024 21:32

brooksprumo mentioned this pull request Mar 11, 2024

Removes holding storages in AccountsHashVerifier for fastboot #120

Merged

codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024

Adds StartingSnapshotStorages to AccountsHashVerifier (anza-xyz#58)

f94752d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds StartingSnapshotStorages to AccountsHashVerifier #58

Adds StartingSnapshotStorages to AccountsHashVerifier #58

brooksprumo commented Mar 4, 2024 •

edited

Loading

jeffwashington commented Mar 4, 2024

brooksprumo commented Mar 4, 2024 •

edited

Loading

jeffwashington commented Mar 4, 2024

codecov-commenter commented Mar 4, 2024

brooksprumo commented Mar 4, 2024

jeffwashington Mar 4, 2024

jeffwashington left a comment

apfitzge left a comment

brooksprumo commented Mar 4, 2024

jeffwashington commented Mar 4, 2024

jeffwashington commented Mar 4, 2024

Adds StartingSnapshotStorages to AccountsHashVerifier #58

Adds StartingSnapshotStorages to AccountsHashVerifier #58

Conversation

brooksprumo commented Mar 4, 2024 • edited Loading

Problem

Summary of Changes

jeffwashington commented Mar 4, 2024

brooksprumo commented Mar 4, 2024 • edited Loading

jeffwashington commented Mar 4, 2024

codecov-commenter commented Mar 4, 2024

Codecov Report

brooksprumo commented Mar 4, 2024

jeffwashington Mar 4, 2024

Choose a reason for hiding this comment

jeffwashington left a comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 4, 2024

jeffwashington commented Mar 4, 2024

jeffwashington commented Mar 4, 2024

brooksprumo commented Mar 4, 2024 •

edited

Loading

brooksprumo commented Mar 4, 2024 •

edited

Loading