- Notifications
You must be signed in to change notification settings - Fork 399
Simplicity #1219
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
Simplicity #1219
Conversation
480f8e6 to acf8f90 Compare 0de6797 to 4d633f6 Compare 2bf20d6 to ea318a4 Compare ea318a4 to d177d2d Compare 7cfc6f3 to e144fa0 Compare 20d536d to 80c5d58 Compare 80c5d58 to 16fd80c Compare 86223f8 to 0c562ea Compare 2485f7a to cff3732 Compare | changed the base branch to 23.x since this was rebased @roconnor-blockstream |
37c2311 to 4194517 Compare | In 10ff078 you make simplicity a subtree (it appears not to be before then). You should also update |
6b3035d to a61faf5 Compare | CI failures look real. e.g. on the ASan build it originates in in feature_taproot.py |
cb98f9c to 0aeafeb Compare | code review ACK 0aeafeb |
| 4 CI failures -- the windows one might be real ( |
Simplicity needs this fix.
git-subtree-dir: src/simplicity git-subtree-split: 86ac0f92c4b2b6d694dca85dce3b6909aed25894
also filters src/simplicity/secp256k1 from the coverage results
This will be used by Simplicity.
Unactivated.
Take the existing function to parse script errors and extend it to parse Elements errors. SCRIPT_ERR_ERROR_COUNT is not included because it is a pseudo error.
Compare the expected script error, parsed from JSON, with the actual error that VerifyScript returns. If the JSON does not include an expected error, then skip this check.
0aeafeb to 5088360 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.
utACK 5088360
pending CI at https://cirrus-ci.com/build/5064884356907008
apoelstra 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.
utACK 5088360
| -p "src/crypto/ctaes" \ | ||
| -p "src/minisketch" \ | ||
| -p "src/secp256k1" \ | ||
| -p "src/simplicity/secp256k1" \ |
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.
In 50916cf:
Probably we didn't want to filter this. (I'm kinda surprised that the upstream libsecp is filtered. But I guess they have their own coverage analysis.)
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.
While we're at it, we should stick /nix/store into this list and src/include/boost. I'm not sure why we need to do this and Bitcoin doesn't (well, the answer is "NixOS") but it's harmless and helpful for Russell and me.
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'm a little hesitant to add nix-specific fixes here, and rather just patch the file in https://github.com/roconnor-blockstream/elements-nix/
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've removed src/simplicity/secp256k1 from the filter as part of #1390.
Tasks needed to pass CI:
Tasks needed for public signet:
Tasks needed for testing.
libelementssimplicity.Before merging: