Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix another unstable test after eager rent #10120

Merged

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented May 19, 2020

Problem

test_bank_get_program_accounts is flaky since eager rent was implemented.

thread 'bank::tests::test_bank_get_program_accounts' panicked at 'assertion failed: `(left == right)`
left: `[(2ENdwbDcvnewdQZwY4mAJ9sApwCMBo3AmXoQnUzG3m8p, Account { lamports: 1 data.len: 0 owner: 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR executable: false rent_epoch: 0 })]`,
right: `[]`', runtime/src/bank.rs:5868:9

Summary of Changes

Preserve lazy rent for this test, seems to resolve the flakiness (about 40 local runs succeeded, compared with failures every 3-6).

However, @ryoqun, I would like your insight as to whether (a) it is correct that this account should show that it was modified since parent, even though the balance hasn't changes; (b) this is the right solution.
Turning off rent seems like another option, since this test is meant to detect non-rent account changes. (It does fix the flakiness)

I'm actually a bit surprised there aren't more flaky tests like this, but I guess there aren't any other tests using a chain of banks and expecting accounts to stay unchanged in this way. I did a quick scan through bank.rs and didn't see any.

Fyi @mvines

@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #10120 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #10120   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         281      281           
  Lines       65971    65972    +1     
=======================================
+ Hits        53819    53825    +6     
+ Misses      12152    12147    -5     

@ryoqun
Copy link
Member

ryoqun commented May 20, 2020

@CriesofCarrots Thanks for spotting on this!

I'm actually a bit surprised there aren't more flaky tests like this

Well, there was and is some.... ;) (Past: #10031, waiting for my love: https://buildkite.com/solana-labs/solana/builds/24442#d1c0a942-4d7b-491e-b6b6-71653ffeab06/159-287). Sorry for ongoing trouble.

I'll work on these once devnet bloat snapshot issue is settled down asap.

@ryoqun
Copy link
Member

ryoqun commented May 20, 2020

(a) it is correct that this account should show that it was modified since parent, even though the balance hasn't changes

Yeah, it's correct. undocumented (#10133) and unintuitive. :( But eager rent collection always updates account even if the balance hasn't changed. That's intended behavior to purge too old account states for each epoch #8931 .

By the way, I've noticed all touched accounts by eager rent collection would be exposed to get_program_accounts_modified_since_parent and the like, eventually RPC. Would that be problem? I mean, those RPCs will start to return those accounts as noise.

(b) this is the right solution.

Yeah, that flag is exactly designed for that. Once eager rent collection is enabled across the board, I'll rewrite those tests.

Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Really thanks for working on my bug..!

@CriesofCarrots
Copy link
Contributor Author

By the way, I've noticed all touched accounts by eager rent collection would be exposed to get_program_accounts_modified_since_parent and the like, eventually RPC. Would that be problem? I mean, those RPCs will start to return those accounts as noise.

Yes, I do think it is a slight UX problem, as it will be confusing for users for accounts to appear as modified_since_parent but show no changes. We might want to investigate a way to prevent that. Or I suppose we could toggle a bit in the account to indicate that rent was checked, so there is actually a change in the account state. I'll create an issue for it.

@CriesofCarrots CriesofCarrots merged commit 9d89fb5 into solana-labs:master May 20, 2020
mergify bot pushed a commit that referenced this pull request May 20, 2020
solana-grimes pushed a commit that referenced this pull request May 20, 2020
@CriesofCarrots CriesofCarrots deleted the flaky-runtime-test branch May 26, 2020 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants