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 flaky test_transaction_result_does_not_affect_bankhash #3916

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

apfitzge
Copy link

@apfitzge apfitzge commented Dec 4, 2024

Problem

  • test_transaction_result_does_not_affect_bankhash is flaky due to random keypairs/pubkeys
  • The flakiness comes down to rent collection (rather updating rent epoch)
  • In successful transactions, we collect rent during transaction processing for writable accounts
    • in this case, that will be the mint_keypair's pubkey
  • In bank.freeze, we collect rent on partition(s)
    • in this case, that will be the mint_keypair IF the randomly generated key is in the partition
  • IF the mint pubkey falls in the bank's rent collection partitions, the bankhash is unaffected since the rent is collected in both cases

Summary of Changes

  • Use deterministic keys in the test, and in other tests

Fixes #

@apfitzge apfitzge marked this pull request as ready for review December 4, 2024 15:46
@apfitzge
Copy link
Author

apfitzge commented Dec 4, 2024

Hoping the change of keypairs/pubkeys in genesis config doens't break other tests, but we'll see. Killing randomness is good overall.

@apfitzge apfitzge force-pushed the fix_flaky_bankhash_test branch from f03e533 to 3093b9b Compare December 4, 2024 15:56
@apfitzge
Copy link
Author

apfitzge commented Dec 4, 2024

See @steviez recently saw this test failing as well: #3549

@steviez
Copy link

steviez commented Dec 4, 2024

See @steviez recently saw this test failing as well: #3549

Do you have any buildkite links handy that failed on this test ? My PR that you mentioned included a bunch of logging that would help identify the offending account; that would help verify the theory you mentioned above (altho I guess I could also craft a Pubkey and re-run the unit test locally if I want to play around with it).

Either way - nice find and will get around to reviewing this shortly

@apfitzge
Copy link
Author

apfitzge commented Dec 4, 2024

See @steviez recently saw this test failing as well: #3549

Do you have any buildkite links handy that failed on this test ? My PR that you mentioned included a bunch of logging that would help identify the offending account; that would help verify the theory you mentioned above (altho I guess I could also craft a Pubkey and re-run the unit test locally if I want to play around with it).

Either way - nice find and will get around to reviewing this shortly

The output is pretty massive - I ended up adding a bunch of print statements and finding the issue that way while talking with brooks.

link: https://buildkite.com/organizations/anza/analytics/suites/agave/tests/c0ac6a06-88b4-8e28-834e-e232a122f763?branch=master#flaky

Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I like removing the randomness. Almost feels like this could be two PRs: one for the changes to genesis utils, and another one for the changes to test_transaction_result_does_not_affect_bankhash.

ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
@apfitzge
Copy link
Author

apfitzge commented Dec 4, 2024

I like removing the randomness. Almost feels like this could be two PRs: one for the changes to genesis utils, and another one for the changes to test_transaction_result_does_not_affect_bankhash.

The changes to genesis_utils change the result of the test **in most cases - or the equality at least. I'd rather just keep it all grouped as a single PR to keep it simple.

@brooksprumo brooksprumo self-requested a review December 5, 2024 16:54
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM

Did a scan through and looks like the changes in genesis_utils.rs are only affecting test code; can possibly throw some #[cfg(test)] in there in a separate PR

ledger/src/blockstore_processor.rs Show resolved Hide resolved
@apfitzge apfitzge merged commit c5473e4 into anza-xyz:master Dec 5, 2024
40 checks passed
@steviez steviez added the v2.1 Backport to v2.1 branch label Jan 22, 2025
Copy link

mergify bot commented Jan 22, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Jan 22, 2025
(cherry picked from commit c5473e4)

# Conflicts:
#	ledger/src/blockstore_processor.rs
t-nelson pushed a commit that referenced this pull request Jan 23, 2025
…kport of #3916) (#4578)

* Fix flaky test_transaction_result_does_not_affect_bankhash (#3916)

(cherry picked from commit c5473e4)

# Conflicts:
#	ledger/src/blockstore_processor.rs

* resolve merge conflicts

---------

Co-authored-by: Andrew Fitzgerald <[email protected]>
Co-authored-by: steviez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.1 Backport to v2.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants