Make tests deterministic - #96
Conversation
9e5b968 to
4d16b24
Compare
`common::setup` generated the program ID with `Pubkey::new_unique` and the payer with `Keypair::new`, and `litesvm_token`'s builders generate mint and token-account addresses with `Keypair::new` internally. Every PDA the tests derive therefore sat at a different address on every run. That matters for compute: `find_program_address` walks bumps down from 255 and burns ~1500 CU on each rejected candidate, so an instruction that creates a PDA costs a different number of compute units each time it runs. Measured on `max_buffers_in_one_instruction`, which creates 30 buffers in one transaction, four consecutive runs of an unchanged binary reported 159,722 / 170,222 / 179,222 / 185,222 CU. That 25,500-CU spread is what "Fix flaky test by increasing default CU limit" (#94) worked around; the same test now reports 174,722 CU every run. Replace both generators with `common::unique_pubkey` / `common::unique_keypair`, drawing from a counter that `setup` resets, so a test's addresses depend only on the order it allocates them in and not on how libtest interleaves its neighbours. The counter occupies the leading bytes big-endian to preserve the one property of `new_unique` that tests rely on: addresses sort in allocation order, which `BeginSettle`'s sorted order PDAs need. `litesvm_token::CreateMint` takes no mint keypair, so `token::create_mint` now open-codes the create-account plus `initialize_mint2` pair that builder issues. `CreateAccount` does take one, via `account_kp`. A `clippy.toml` entry bans both generators from here on. Unit tests that never derive a PDA against a runtime have nothing to perturb and opt out locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
also remove some unnecessary files and comments
4d16b24 to
04b5889
Compare
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Very good change, great job on the research and implementation! I like the current design, comments are just nits.
| // The disallowed methods at the crate level are only intended for the integration tests | ||
| #![allow(clippy::disallowed_methods)] | ||
|
|
There was a problem hiding this comment.
I see why this is done: the lint is only really helpful in the integration tests and we don't have access to the helper here.
On the other hand, determinism is a nice property and I'd be in favor of either using hardcoded addresses with new_from_array or copy the helper to the settlement lib just to remove the two allows.
There was a problem hiding this comment.
agreed in the unit tests could also benefit. In the past I suggested creating addresses taht start with cool strings in the pubkey case, like Solver... would be interesting and help with readability. But another time right?
There was a problem hiding this comment.
There was a problem hiding this comment.
Linear isn't accessible outside of this repo, let's keep this kind of issues in public GitHub. Let's also write about #![allow(clippy::disallowed_methods)], so we hopefully remember to remove this blanket approval.
Also, for this PR, let's apply the allow to only tests. (I know it's pointless right now, but I'm worried we'll be use this more and we'll forget about this global lint ignore.)
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
| // The disallowed methods at the crate level are only intended for the integration tests | ||
| #![allow(clippy::disallowed_methods)] | ||
|
|
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Nice work, looks good to merge!
I'd still try to decrease the scope of the allow(..) but that's not merge blocking.
it seems its not doing anything (?)
…github.com:cowprotocol/solana-programs into kaze/sc-256-pin-test-keypairs-for-deterministic-cu
|
I ended up finding out that there really are only a couple places that needed changing for the |
2 participants
Description
Make tests deterministic by replacing
Pubkey::new_unique()orKeypair::new()with deterministic randomness from a seed.Motivation
Enables #67 because right now the random key generation can cause CU to shift randomly from test run to test run due to bump searching. As seen in #94
Strategy
Replace all usages of
Pubkey::new_unique()orKeypair::new()with newcommonmethods which draw from a random source from a seed, reset prior to every test insetup(). Utilize aclippy.tomlrule to prevent usage of the non-deterministic functions in the integration tests only.It is also necessary to replace the
CreateMinthelper provided bylitesvmbecause it uses those functions under the hood, and there doesn't seem to be away around.Here is what the clippy rule firing looks like:
How to test
Verify the test results.
Run the tests multiple times to verify the CU is not changing from run to run in #67 . I did so manually by running
just testa couple times and comparing the outputtarget/cu-report.jsonagainst previous runs.If you would like, confirm that the clippy rule is applying as expected in the integration tests, and the clippy rule is NOT applied elsewhere (ex. in the program unit tests, which don't really need to be deterministic)