don't return zero lamport accounts from 'load' with feature #27793

jeffwashington · 2022-09-15T00:09:48Z

Problem

Seems like a better api to not return zero lamport accounts as Some(zero_lamport_account).

Summary of Changes

AccountsDb::load() returns None if account has zero lamports.

#27800

Fixes #

ryoqun · 2022-09-15T14:19:01Z

runtime/src/accounts_db.rs

@@ -4586,8 +4597,15 @@ impl AccountsDb {
        &self,
        ancestors: &Ancestors,
        pubkey: &Pubkey,
+        ignore_zero_lamports: LoadZeroLamports,


nit: load_zero_lamports.

behzadnouri · 2022-09-15T14:26:07Z

runtime/src/accounts_db.rs

        self.load(ancestors, pubkey, LoadHint::FixedMaxRoot)
+            .filter(|(account, _)| {
+                matches!(
+                    ignore_zero_lamports,
+                    LoadZeroLamports::SomeWithZeroLamportAccount
+                ) || !account.is_zero_lamport()
+            })


Why filter here and not deeper in call-stack in AccountsDb::load or in AccountsDb::do_load, or AccountsDb::do_load_with_populate_read_cache?

AccountsDb::load at least is a public function and this is potentially is adding a backdoor to bypass this filtering.

Also even though this function is returning None, seems like they are still wastefully stored in cache.

Also even though this function is returning None, seems like they are still wastefully stored in cache.

If someone wrote a zero lamport account, it is stored in the write cache. Then it is flushed to an append vec.

Later, if someone uses the index to load the account, load goes to the append vec and reads the account data. Whether it is zero lamport or not, this hits the disk. Currently, anything we load is put in the read cache, under the observation that it will likely be read again. Getting the answer that the account DOES exist in accounts db, but is zero lamport could be a useful answer. Possibly getting it more quickly by having the zero lamport account stored in the read cache is also potentially useful for performance.

Whatever the case, it seems to me that choosing to not put zero lamport accounts in the read cache is a tuning/optimization concern. It is likely of minimal impact and is correct either way.

Incidentally, I think the accounts index also stores a bit that indicates that an entry is zero lamport. We could use that on load, but we are not today. I'll make a note of that. Again, likely minimal impact.

The main issue here is

Why filter here and not deeper in call-stack in AccountsDb::load or in AccountsDb::do_load, or AccountsDb::do_load_with_populate_read_cache?
AccountsDb::load at least is a public function and this is potentially is adding a backdoor to bypass this filtering.

But also I believe what is stored in the cache should be consistent with what api returns. That inconsistently can potentially manifest somewhere else as a bug. So it is not a purely performance issue.

this is potentially is adding a backdoor to bypass this filtering.

This parameterization is required until the feature activates, which is a required parameter by the bank. So, I think the backdoor has to exist.

The parametrization is not the issue.
The issue are the other api which by pass the filtering.
In particular AccountsDb::load which is public.

and I don't see why AccountsDb::do_load, or AccountsDb::do_load_with_populate_read_cache should not do the filtering. Either of these can be used to bypass the filtering.

But also I believe what is stored in the cache should be consistent with what api returns. That inconsistently can potentially manifest somewhere else as a bug. So it is not a purely performance issue.

Another perspective is what is stored in the cache matches what is stored in the underlying thing being cached. In this case, the cache matches the appendvec it is caching. As a result, I have higher confidence we'll get the right answer, whether the item is in the read cache, the write cache, or the append vec - all contain the zero lamport account today. However, as usual, you are very insightful. I had noted in my personal notes the same question - of why we would read cache zero lamport accounts. It is a grey area with some tradeoffs, but I landed on the side of keeping the cache consistent with what it is caching.

I am attempting to keep this change as localized, well understood, and simple as possible.

Please stop focusing on the cache issue.
As I mentioned the issue here is that you are adding the filtering at a wrong place in the call stack.
do_load_with_populate_read_cache is invoked from load_account_into_read_cache which is public function.
So that is another possible route which will bypass the filtering.

The main issue here is

Why filter here and not deeper in call-stack in AccountsDb::load or in AccountsDb::do_load, or AccountsDb::do_load_with_populate_read_cache?

I was getting to replying to this main issue ;-), I was just responding to your other items first.

First, I am attempting to keep this change as localized, well understood, and simple as possible in order to limit unknown unknowns.
Each time we go deeper in the callstack for load, we likely have more fanout client calls to consider whether they would tolerate Some/None in all cases in a well understood way. I have not gone deeper yet in my investigation. I'm not philosophically opposed. I practically drew a line at load_with_fixed_root. I can accept that further investigation and pushing the api change deeper could have value in consistency. I can investigate that. If it does not require a feature change, I'd practically prefer to move this pr along as it is to minimize the unknowns.

behzadnouri · 2022-09-15T14:31:48Z

runtime/src/accounts_db.rs

+    /// return None if loaded account has zero lamports
+    None,


None is in rust prelude. Redefining it to mean something else can be confusing.

This whole enum LoadZeroLamports imo is unnecessary and more confusing than just passing around:

return_none_for_zero_lamport_accounts: bool,

across the callstack.

None is in rust prelude.

I acknowledge your concern about None. But it will always be prefixed like LoadZeroLamports::None. Feel free to suggest an alternative.

This whole enum LoadZeroLamports imo is unnecessary

Passing an additional true/false I consider to be potentially confusing in this case. The intention here is to make this explicit until the feature activation occurs. Then, this enum and the parameters will all disappear.

jstarry

Looks good, nit picks can be hashed out later IMO

sakridge

lgtm

* don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) # Conflicts: # runtime/src/accounts.rs # sdk/src/feature_set.rs

…labs#27793) * don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) manually merged

…27793) (#27803) * don't return zero lamport accounts from 'load' with feature (#27793) * don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) # Conflicts: # runtime/src/accounts.rs # sdk/src/feature_set.rs * fix merge errors Co-authored-by: Jeff Washington (jwash) <jeff.washington@solana.com>

…27793) (#27803) * don't return zero lamport accounts from 'load' with feature (#27793) * don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) # Conflicts: # runtime/src/accounts.rs # sdk/src/feature_set.rs * fix merge errors Co-authored-by: Jeff Washington (jwash) <jeff.washington@solana.com> (cherry picked from commit 0dda25f) # Conflicts: # runtime/src/accounts.rs # runtime/src/accounts_db.rs # sdk/src/feature_set.rs

…#27793) (#27811) don't return zero lamport accounts from 'load' with feature (#27793) * don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) manually merged

…27793) (backport #27803) (#27818) * don't return zero lamport accounts from 'load' with feature (backport #27793) (#27803) * don't return zero lamport accounts from 'load' with feature (#27793) * don't return zero lamport accounts from 'load' * add feature * rename (cherry picked from commit 138d5ed) # Conflicts: # runtime/src/accounts.rs # sdk/src/feature_set.rs * fix merge errors Co-authored-by: Jeff Washington (jwash) <jeff.washington@solana.com> (cherry picked from commit 0dda25f) # Conflicts: # runtime/src/accounts.rs # runtime/src/accounts_db.rs # sdk/src/feature_set.rs * fix merge errors Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: jeff washington <jeff.washington@solana.com>

…olana-labs#27793)" This reverts commit 138d5ed.

jeffwashington changed the title ~~Sep0 6 61~~ don't return zero lamport accounts from 'load' with feature Sep 15, 2022

don't return zero lamport accounts from 'load'

8679098

jeffwashington marked this pull request as ready for review September 15, 2022 13:56

jeffwashington force-pushed the sep0_6_61 branch from 872deb0 to 0049c32 Compare September 15, 2022 13:56

jeffwashington requested a review from jstarry September 15, 2022 13:56

add feature

59fc888

jeffwashington force-pushed the sep0_6_61 branch from 0049c32 to 59fc888 Compare September 15, 2022 13:58

ryoqun reviewed Sep 15, 2022

View reviewed changes

rename

e603905

jeffwashington requested a review from ryoqun September 15, 2022 14:22

behzadnouri reviewed Sep 15, 2022

View reviewed changes

jstarry approved these changes Sep 15, 2022

View reviewed changes

solana-labs deleted a comment from behzadnouri Sep 15, 2022

jeffwashington requested a review from sakridge September 15, 2022 15:19

sakridge approved these changes Sep 15, 2022

View reviewed changes

jeffwashington added the v1.14 label Sep 15, 2022

jeffwashington merged commit 138d5ed into solana-labs:master Sep 15, 2022

mergify bot mentioned this pull request Sep 15, 2022

don't return zero lamport accounts from 'load' with feature (backport #27793) #27803

Merged

behzadnouri mentioned this pull request Sep 15, 2022

filters out zero lamport accounts deeper in call stack #27807

Closed

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Sep 20, 2022

Revert "don't return zero lamport accounts from 'load' with feature (s…

0843141

…olana-labs#27793)" This reverts commit 138d5ed.

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Sep 20, 2022

Revert "don't return zero lamport accounts from 'load' with feature (s…

e8fc8b9

…olana-labs#27793)" This reverts commit 138d5ed.

jeffwashington mentioned this pull request Sep 21, 2022

LoadZeroLamports at deeper level for load #27943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't return zero lamport accounts from 'load' with feature #27793

don't return zero lamport accounts from 'load' with feature #27793

jeffwashington commented Sep 15, 2022 •

edited

ryoqun Sep 15, 2022

jeffwashington Sep 15, 2022

behzadnouri Sep 15, 2022

jeffwashington Sep 15, 2022

behzadnouri Sep 15, 2022

jeffwashington Sep 15, 2022

behzadnouri Sep 15, 2022

jeffwashington Sep 15, 2022

behzadnouri Sep 15, 2022

jeffwashington Sep 15, 2022

behzadnouri Sep 15, 2022

jeffwashington Sep 15, 2022

jeffwashington Sep 15, 2022

jstarry left a comment

sakridge left a comment

don't return zero lamport accounts from 'load' with feature #27793

don't return zero lamport accounts from 'load' with feature #27793

Conversation

jeffwashington commented Sep 15, 2022 • edited

Problem

Summary of Changes

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

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

jstarry left a comment

Choose a reason for hiding this comment

sakridge left a comment

Choose a reason for hiding this comment

jeffwashington commented Sep 15, 2022 •

edited