-
Notifications
You must be signed in to change notification settings - Fork 23
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
Praos headers validation properties and generators #1285
Praos headers validation properties and generators #1285
Conversation
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/Headers.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/Headers.hs
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/Headers.hs
Show resolved
Hide resolved
So far, headers are very much specific to protocols more so than era. Byron (RealPBFT), Shelley (TPraos), and Babbage (proper Praos) were the only eras that altered the protocol. |
Thanks a lot @nfrisby for taking the time to review this draft, it comforts me this can be useful and gives useful directions to improve it. The reason for this comment about eras is that the code pretty much pins down the block header's era to |
2249b54
to
2f64542
Compare
Noticed while looking at IntersectMBO#1285
fbfdb6e
to
6bcbaab
Compare
6bcbaab
to
eff65bf
Compare
Took me a while to have CI passing and fix some issues in the generators' logic but I think it's now ready for a proper review. |
59cc1a4
to
4a37b00
Compare
Any plans to merge this? It's CI green and I think I have addressed all reviewer's comments |
ouroboros-consensus-cardano/changelog.d/20241029_062000_abailly_header_validation_test.md
Outdated
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/Headers.hs
Outdated
Show resolved
Hide resolved
4a37b00
to
fce0ef5
Compare
@jasagredo I have removed changelog entry, but how do I prevent the CI from failing? |
I mentioned it in the suggestion above. I have set the |
I understand, point is: I cannot do this. I would suggest finding another way, perhaps looking at commit messages, otherwise this will annoy external contributors. |
Ah I didn't realize you couldn't do it. Indeed that is annoying for contributors hmmm. We will try to come up with a better solution. |
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.
Just some minor comments.
Also yesterday we discussed it in our meeting and you can actually just push an empty changelog fragment in your PR to go around the CI check. I think it makes sense as that says "I am declaring that this PR has no relevant changes worth to be included in the changelog".
...os-consensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-protocol/changelog.d/20241029_062000_abailly_header_validation_test.md
Outdated
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Outdated
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
Show resolved
Hide resolved
...sus-protocol/src/unstable-protocol-testlib/Test/Ouroboros/Consensus/Protocol/Praos/Header.hs
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.
You are required to sign your commits before I the PR can be merged.
The error in CI is a flaky tests that has not been flaky for a very long time and we are recently seeing more instances of it. Let's pray that the signed force-push makes it go away... |
Ok, I will sign the commits |
* Provide base generator and mutations for headers, covering some parts related to KES header signature * Extract testable method from Protocol.Praos module * Add a property testing the consistency of validation logic with both valid and mutated headers * Add a command-line tool to generate JSON-formatted test vectors and validate them
some mutations are not possible for some content of the header, eg. if ocertN = 0 then it's not possible to generate a smaller expected value
also remove hardcoded maxKESEvo parameter from test run
* remove changelog entry * add explicit export list * use generic JSON derivation
0cd6683
to
0653bac
Compare
Description
This PR introduces generators, properties, and an executable to be able to QuickCheck Praos headers validation in a somewhat exhaustive and explicit way. Here is a brief summary:
GeneratorContext
with various keys and parameters used in the forging of headers, and valid PraosHeader StandardCrypto
from such a contextHeader
one can apply aMutation
that invalidates one particular rule for headers validation, usingGen
erators tooHeader
andGeneratorContext
have JSON serialisers, with keys and bytes being output as base64-encoded stringsgen-header
program can be used to generate aSample
of headers and context to be stored as part of a regression test suite, and to provide reference test vectors for other projects interested in validating headersThe JSON output looks like the following:
TODO:
Mutation
type to cover all kind of potential validation errorsGeneratorContext
(eg. stake distribution, KES period...)generate
andvalidate
commands togen-header
Cover more Eras (?)