-
Notifications
You must be signed in to change notification settings - Fork 340
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
v2.1: Fix flaky test_transaction_result_does_not_affect_bankhash (backport of #3916) #4578
Conversation
(cherry picked from commit c5473e4) # Conflicts: # ledger/src/blockstore_processor.rs
Cherry-pick of c5473e4 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
b0d3b6b
to
c2b2d50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on other stuff today so rushed through the merge conflict; should be corrected now + paper-trail explaining why it previously failed
ledger/src/blockstore_processor.rs
Outdated
.last_blockhash | ||
); | ||
// AND should not affect bankhash IF the rent is collected during freeze. | ||
assert_eq!(ok_bank_details == bank_details, fee_payer_in_rent_partition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the assertion we need to do as noted by what we have in master:
agave/ledger/src/blockstore_processor.rs
Line 3660 in 1e721f0
assert_eq!(ok_bank_details == bank_details, fee_payer_in_rent_partition); |
which you can also see is what was added in the initial PR:
https://github.com/anza-xyz/agave/pull/3916/files#diff-281d947e593a76f3c3380804162dc8cf426fcc33e0ae1192589c8193221cd1e2R3626
assert_eq!(blockhash_ok, bank.last_blockhash()); | ||
assert!(bankhash_ok != bank.hash()); | ||
assert_eq!(bankhash_ok == bank.hash(), fee_payer_in_rent_partition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the merge conflict needed to bring this check from c5473e4, but keep the rest from v2.1
head
What's the justification/catalyst to backport this? |
Came up in backport meeting. Motivations is just reducing the amount of times we have to hit the retry button in CI when backporting to 2.1 |
Problem
test_transaction_result_does_not_affect_bankhash
is flaky due to random keypairs/pubkeysmint_keypair
's pubkeymint_keypair
IF the randomly generated key is in the partitionSummary of Changes
Fixes #
This is an automatic backport of pull request #3916 done by [Mergify](https://mergify.com).