simplify bank drop calls by HaoranYi · Pull Request #24142 · solana-labs/solana

HaoranYi · 2022-04-06T15:33:10Z

Problem

The extra parameter, is_from_abs in purge_slot function is only used for assert. It seems unnecessary. Furthermore, it is also a bit confusing since caller from block_store also set it to true.

is_bank_drop_callback_enabled flag seems also unnecessary since the call back is an Option. Besides, this flag is set in a callback create function, which seems not to be the right place. The right place should be at the caller, which installed the drop callback to the root bank.

Summary of Changes

simplify the bank drop calls.

Fixes #

brooksprumo · 2022-04-06T16:30:25Z

Some quick comments; I still need to review more carefully.

Furthermore, it is also a bit confusing since caller from block_store also set it to true.

Yes, it's confusingly named currently. This code path was just recently changed to fix #23976 in PR #23999. So is_abs is the legacy name, and could use an update. The concept is still correct as far as I can tell though.

Besides, this flag is set in a callback create function, which seems not to be the right place. The right place should be at the caller, which installed the drop callback to the root bank.

I agree that setting the is_bank_drop_callback_enabled is not correct. I made this PR up to fix it: #23970. There are conflicts because PR #23999 got merged and I intended to resolve & update my PR.

HaoranYi · 2022-04-06T17:28:32Z

My reading of the code is that - it seems that is_from_abs and is_bank_drop_callback_enabled is only used to panic.
The only place that is_from_abs is set to false is here https://github.com/solana-labs/solana/pull/24142/files#r844202353, which we already checked for the existence of the callback.
Therefore, we don't need is_from_abs and is_bank_drop_callback_enabled any more. Likely, this is from the legacy code and with recent changes, now these are no longer needed.

brooksprumo

Ok, I'm on board with this change now. I think it's now safe since #23999 sets the bank drop callback on load, before any new banks are created or any bank refs are passed out.

I had some nits below, but otherwise looks good. Once the changes are in and the CI passes coverage, please re-request a review. Thanks!

brooksprumo · 2022-04-07T15:31:03Z

    /// Purge a slot if it is not a root
    /// Root slots cannot be purged
-    /// `is_from_abs` is true if the caller is the AccountsBackgroundService
-    pub fn purge_slot(&self, slot: Slot, bank_id: BankId, is_from_abs: bool) {
-        self.accounts_db.purge_slot(slot, bank_id, is_from_abs);
+    pub fn purge_slot(&self, slot: Slot, bank_id: BankId) {
+        self.accounts_db.purge_slot(slot, bank_id);
    }


Just remove this whole function. I'll mark the caller next.

brooksprumo · 2022-04-07T15:33:27Z


    /// This should only be called after the `Bank::drop()` runs in bank.rs, See BANK_DROP_SAFETY
    /// comment below for more explanation.
    /// `is_from_abs` is true if the caller is the AccountsBackgroundService


nit: Please remove this comment now that is_from_abs has been removed.

Co-authored-by: Brooks Prumo <brooks@prumo.org>

brooksprumo

This looks good to me. I think it'd probably be worthwhile to have another person approve as well; to make sure I didn't miss anything.

codecov · 2022-04-07T21:42:25Z

Codecov Report

Merging #24142 (aa6a105) into master (2da4e3e) will decrease coverage by 0.1%.
The diff coverage is 80.5%.

@@            Coverage Diff            @@
##           master   #24142     +/-   ##
=========================================
- Coverage    81.8%    81.6%   -0.2%     
=========================================
  Files         581      590      +9     
  Lines      158312   160999   +2687     
=========================================
+ Hits       129518   131469   +1951     
- Misses      28794    29530    +736

HaoranYi · 2022-04-07T21:52:51Z

@carllin @jeffwashington Can you help to review this? Thanks!

jeffwashington · 2022-04-07T22:03:34Z

@carllin I think this needs to be you.

carllin · 2022-04-07T22:44:31Z

The flag serves a few purposes I think which are still useful:

Documenting that the function needs to be serialized with all the other background account processes
Forcing anybody calling this code to reason about whether their function is always serialized with said background processes
If in step 2, the developer makes a mistake in their reasoning, the code will panic instead of silently corrupting the state. For instance, this was useful to catch that this code:

solana/runtime/src/bank.rs

Line 6809 in 781094e

.purge_slot(self.slot(), self.bank_id(), false);

could run concurrently with ABS on startup

That being said, the is_abs flag could maybe be more clearly renamed to is_serialized_with_abs to better convey its purpose

HaoranYi · 2022-04-07T23:03:20Z

OK. Close the pull request. Let's leave the code as it is.

HaoranYi · 2022-04-08T16:02:44Z

Reopen the pull request for more discussion.

Ideally, the check shouldn't be on the purge_slot fn. It should be inside the drop_callback.
I can imagine that we add a bool flag into the bank to indicates that we are 'dropping' then have the drop_callback to assert that flag. However, this is difficult because we can't change the bank struct. I wonder if there is any unused fields in the bank that can be repurposed for using as the "dropping" flag.

Or maybe we could use the msb the bank_id to indicates that we are dropping (659af56)? This will limit that the range of bank_id will only be 63bit.

Pull request has been modified.

brooksprumo · 2022-04-08T18:51:49Z

However, this is difficult because we can't change the bank struct.

Why can't we change the Bank struct?

HaoranYi · 2022-04-08T19:26:47Z

solana/runtime/src/bank.rs

Line 1085 in 2e5042d

    
           /// Manager for the state of all accounts and programs after processing its entries.

In the comments, it is saying that serialization/deserialization are implemented elsewhere. I assume that Bank is embedded in some other serialized struct?

carllin · 2022-04-08T19:34:15Z

Ideally, the check shouldn't be on the purge_slot fn. It should be inside the drop_callback.
I can imagine that we add a bool flag into the bank to indicates that we are 'dropping' then have the drop_callback to assert that flag.

I think it makes sense to leave the flag in the purge_slot() function because that's the exact function that needs to be serialized with ABS

From my comment above: #24142 (comment), one of the purposes is Forcing anybody calling this code to reason about whether their function is always serialized with said background processes, which can only be enforced if the flag is in purge_slot(). It may not be guaranteed that the only caller of purge_slot() is Bank::drop() in the future

HaoranYi · 2022-04-08T20:10:50Z

what do you mean by always serialized with said background processes?

carllin · 2022-04-08T23:55:30Z

It means that the purge_slot() function and all the clean+shrink+flush cache functions in AccountsBackgroundService cannot run concurrently, they must run in a serial fashion

HaoranYi · 2022-04-11T17:13:57Z

Ok. Added back the bool param for fn purge_slot. Rename is to is_serialized_with_abs for clarity.

Pull request has been modified.

* simplify bank drop calls * clippy: import * Update ledger/src/blockstore_processor.rs Co-authored-by: Brooks Prumo <brooks@prumo.org> * Update runtime/src/accounts_background_service.rs Co-authored-by: Brooks Prumo <brooks@prumo.org> * Update runtime/src/bank.rs Co-authored-by: Brooks Prumo <brooks@prumo.org> * cleanup * format * use msb of bank_id to indicates that we are dropping * clippy * restore bank id * clippy * revert is-serialized_with_abs flag * assert * clippy * whitespace * fix bank drop callback check * more fix * remove msb dropping implementation * fix Co-authored-by: Brooks Prumo <brooks@prumo.org>

HaoranYi added 2 commits April 6, 2022 10:31

simplify bank drop calls

628d47b

clippy: import

d3ffa56

HaoranYi requested review from brooksprumo, carllin and jeffwashington April 6, 2022 15:53

HaoranYi commented Apr 6, 2022

View reviewed changes

Comment thread runtime/src/bank.rs

brooksprumo reviewed Apr 7, 2022

View reviewed changes

This was referenced Apr 7, 2022

Use Release/Acquire instead of SeqCst for is_bank_drop_callback_enabled #24134

Merged

Fix race: Enable the bank-drop-callback *after* setting the callback #23970

Closed

HaoranYi and others added 5 commits April 7, 2022 11:42

Update ledger/src/blockstore_processor.rs

36fae2e

Co-authored-by: Brooks Prumo <brooks@prumo.org>

Update runtime/src/accounts_background_service.rs

f4aa913

Co-authored-by: Brooks Prumo <brooks@prumo.org>

Update runtime/src/bank.rs

b51de0b

Co-authored-by: Brooks Prumo <brooks@prumo.org>

cleanup

b5b4d14

format

aa6a105

HaoranYi requested a review from brooksprumo April 7, 2022 19:38

brooksprumo previously approved these changes Apr 7, 2022

View reviewed changes

HaoranYi closed this Apr 7, 2022

HaoranYi reopened this Apr 8, 2022

use msb of bank_id to indicates that we are dropping

659af56

HaoranYi added 2 commits April 8, 2022 11:25

clippy

17168ea

restore bank id

023e3c2

clippy

a0633b8

revert is-serialized_with_abs flag

ca420c1

HaoranYi added 3 commits April 11, 2022 12:17

assert

856e893

clippy

63b264a

Merge remote-tracking branch 'upstream/master' into simplify_drop

9e6caa8

carllin previously approved these changes Apr 13, 2022

View reviewed changes

HaoranYi added 2 commits April 13, 2022 13:28

merge

9cc45b2

whitespace

d2d4a19

HaoranYi added 4 commits April 13, 2022 13:51

fix bank drop callback check

d17dd72

more fix

22b74b3

remove msb dropping implementation

d61d957

fix

fd55d0e

HaoranYi merged commit e3ef074 into solana-labs:master Apr 14, 2022

Conversation

HaoranYi commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

brooksprumo commented Apr 6, 2022

Uh oh!

Uh oh!

HaoranYi commented Apr 6, 2022

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

brooksprumo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brooksprumo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 7, 2022

Codecov Report

Uh oh!

HaoranYi commented Apr 7, 2022

Uh oh!

jeffwashington commented Apr 7, 2022

Uh oh!

carllin commented Apr 7, 2022

Uh oh!

HaoranYi commented Apr 7, 2022

Uh oh!

HaoranYi commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brooksprumo commented Apr 8, 2022

Uh oh!

HaoranYi commented Apr 8, 2022

Uh oh!

carllin commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoranYi commented Apr 8, 2022

Uh oh!

carllin commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoranYi commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HaoranYi commented Apr 6, 2022 •

edited

Loading

HaoranYi commented Apr 8, 2022 •

edited

Loading

carllin commented Apr 8, 2022 •

edited

Loading

carllin commented Apr 8, 2022 •

edited

Loading