-
Notifications
You must be signed in to change notification settings - Fork 29
framework: ability to use test or prod signature scheme in tests #230
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
framework: ability to use test or prod signature scheme in tests #230
Conversation
…t_shared_key_manager()
|
@fselmo @tcoratger Some updates to the PR:
I havn't broken down to smaller PRs because the approach changes how different components interact. Happy to break down to smaller PRs if the overall approach looks okay. |
tcoratger
left a comment
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.
Sounds super cool, thanks a lot, just two super small nits for the rest, looks good to me.
packages/testing/src/consensus_testing/test_fixtures/fork_choice.py
Outdated
Show resolved
Hide resolved
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 seems great 👌🏼. Very similar to what I was imagining as well. I just left some small things to think about.
I think also it could be nicer for DevEx if we had a flag for fill, something like --scheme that overrides the env var for that run perhaps. If you ran with --scheme=prod it would override any sort of test env var you had just for that run.
I think it would be as simple as adding a scheme flag to the click API for fill and doing something like:
if scheme:
os.environ["LEAN_ENV"] = scheme.lower()So the order of precedence would be:
- CLI flag (sets
os.environ["LEAN_ENV"]which is only used on current run) - If no CLI flag, reads from
LEAN_ENVfrom global user settings - If no
LEAN_ENV, defaults totest
Thoughts? Either way looks good from my end 👍🏼
edit: Ah, it looks like it was in there at one point but then removed in 6eb89e1.
Added --scheme flag to the test filler. This is where the scheme gets defined for usage
Curious on your thoughts on keeping it vs removing 👀
I also liked to use But since you suggested + also my preference, let's add it back! |
|
@fselmo On your suggestions to remove the env default from conftest.py and adding The xmss subspecs are assigning these env-dependent config on module init: e.g. I think the only way I can do your suggestions is to have the spec modules loaded after the filler parses the CLI args, but I'm not sure if there's a way to do that. Btw. the |
tcoratger
left a comment
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.
Thanks a lot @unnawut good to merge on my side.
895257c to
b816515
Compare
|
@unnawut I think the flag is actually quite fine. I did some testing around it. If we trace it, we start at Let me know what you think. I didn't want to rebase your branch bc it would require a force push and I didn't want to clean any of your history but we have a new typechecker, Another thing is I noticed we still don't have |
Doing via
Because the test keys are over 200 MB, I didn't add them into the PR/repo and expected it to be downloaded via |
Co-authored-by: Thomas Coratger <[email protected]>
Co-authored-by: felipe <[email protected]>
1ea06f6 to
d20b21f
Compare
|
@tcoratger @fselmo I think the PR features are quite stable now and remaining comments are quite small enough that if there're more changes needed I can do follow up PRs so I'll merge now |
|
Sounds good, yeah I approve :) |
🗒️ Description
In this PR, there are no more independent references to
TEST_SIGNATURE_SCHEMEorPROD_SIGNATURE_SCHEMEin the test vector framework nor the test cases. All usage go throughlean_spec.config.LEAN_ENV. This should help ensure that the signature scheme setting is consistent across all tests.Defaults around key managers and signature operations are removed so the configured scheme is always trickled down from the starting point than trying to default mid-way. The exception is in the client subspecs like
signed_block_with_attestation.verify_signatures()andstore.on_block()where they default toPROD_SIGNATURE_SCHEMEfor specs clarity.Key changes:
_get_shared_key_manager()from each fixture intoconsensus_testing/keys.pyso there's only one place to set the signature scheme for the key managerLEAN_ENVenv var that takes in either "test" or "prod". I avoid usingSIGNATURE_SCHEMEor similar because it gets really ambiguous when it's used in the xmss subspecs.LEAN_ENVparsing:LEAN_ENVis parsed only once byxmss.constants. This ensures the entire subspec uses a consistent value.LEAN_ENVis defaulted toLEAN_ENV="test"intests/conftest.py. It's then parsed and consumed mainly byBaseFixture.lean_env()as a computed field. There's an additional parsing bytest_types.genesis.generate_pre_state()because the function is independent from the fixtures.LEAN_ENVis not set:TARGET_CONFIG=PROD_CONFIGandTARGET_SIGNATURE_SCHEME=PROD_SIGNATURE_SCHEME, so specs are consumed with prod config/scheme by default.LEAN_ENV="test", so tests are run in test config/scheme by default, which in turn would also configure subspecs to use test config.test-keys.jsoninto individual key files. This helps with managing prod keys that would've been ~200MB for 12 validators in a single file.consensus.keys --downloadoption in addition to the existing keygen so pre-generated prod keys can be downloaded.✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox