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

wip: fix sig_der test #251

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

bloomingpeach
Copy link
Contributor

@bloomingpeach bloomingpeach commented Oct 5, 2024

currently I am aiming on fixing related issue to SIG_DER error, first thing I've done is update the existing scripts accordingly.

And also I create a new helper script for only run the SIG_DER tests : tests/run-sig-der-tests.sh, and the tests is on tests/sig_der_failing_tests.json

Copy link

vercel bot commented Oct 5, 2024

@bloomingpeach is attempting to deploy a commit to the keep-starknet-strange Team on Vercel.

A member of the Team first needs to authorize it.

@bloomingpeach bloomingpeach marked this pull request as ready for review October 5, 2024 06:41
@b-j-roberts
Copy link
Contributor

Hey, thanks. Seems there are a lot of SIG_DER tests this could potentially fix. I can take a deeper look soon, but I noticed a couple tests that were passing before are now giving SIG_DER error results.

Might be worth looking into.

  ScriptSig: '0 0' -- ScriptPubKey: '1 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 1 CHECKMULTISIG NOT' -- Flags: 'STRICTENC' -- Expected: OK
  Expected : OK -- Result   : SIG_DER
  ^[[0;31mFAIL^[[0m
scarb cairo-run --package shinigami_cmds '[[],3153968,3,[86797668792800902634759660184717057194939896983113578517425642626872259120,178833796868394003489311767857203710869216261298125424937602268936145286756,88709531411426821719016062832706955173577765959701167948755959603171631182],20308,2,[],1537155757518694469187,9]'
     Running shinigami_cmds
Running Bitcoin Script with ScriptSig: '0 0', ScriptPubKey: '1 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 1 CHECKMULTISIG NOT' and Flags: 'STRICTENC'
Execution failed: Signature DER error
Run completed successfully, returning [0]


  ScriptSig: '0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501' -- ScriptPubKey: '2 0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT' -- Flags: 'STRICTENC' -- Expected: OK
  Expected : OK -- Result   : SIG_DER
  ^[[0;31mFAIL^[[0m
scarb cairo-run --package shinigami_cmds '[[85030821728854912212103639134078053702964006258768454302692087137220899126,87254525100547216138737704407463574591211451173251962822336614354210926689,85203574242364798966473637736391103342906527145179793776979491736415188791,89033935833619398992168525023862704412162053266433865976743650390894524216,171718209788098576382437812964891619024125541979237380967585648309498312756,97398751181881103446764428367695131467343699862134368258622011906073966135,173822024168810095159582463443975364878490129286553452164396506234081523558,94030885787836686287116010850159398514782618971742482788832359674922940769,99311934435561283860364279238061322910324663746412015148896282079434008121],20281842709938480414733099515293384809826359958646833,22,[88564506589389073457714855955858846363583940029576408545492106665543689008,88676365850748824338134126615774255052137093683900969439013902277619573045,96101269732739450356016305886093115921589303891563216294708782741507492167],542003028,4,[],1537155757518694469187,9]'
     Running shinigami_cmds
Running Bitcoin Script with ScriptSig: '0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501', ScriptPubKey: '2 0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT' and Flags: 'STRICTENC'
Execution failed: Signature DER error
Run completed successfully, returning [0]

@bloomingpeach
Copy link
Contributor Author

@b-j-roberts I am just confused the flag a little bit, do you think this is correct let strict_encoding = vm.has_flag(ScriptFlags::ScriptVerifyStrictEncoding) || vm.has_flag(ScriptFlags::ScriptVerifyDERSignatures);(in parse_base_sig_and_pk)

@bloomingpeach
Copy link
Contributor Author

bloomingpeach commented Oct 6, 2024

Also do you have an idea to prevent such regression in the future? @b-j-roberts, maybe a nice thing to have when working on fixing these remaining tests.

@b-j-roberts
Copy link
Contributor

b-j-roberts commented Oct 8, 2024

Also do you have an idea to prevent such regression in the future? @b-j-roberts, maybe a nice thing to have when working on fixing these remaining tests.

Hmm, not certain. One way could be to keep track of the passing tests with another run-passing-tests.sh script. It'll be super slow to run though.
Could probably speed it up quite a bit by executing tests in parallel, but might be a little tricky to get right.

Copy link
Contributor

@b-j-roberts b-j-roberts left a comment

Choose a reason for hiding this comment

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

Thank you for fixing these tests! I think this is good for now based on the signature parsing we have implemented.

@b-j-roberts b-j-roberts merged commit 9e7692d into keep-starknet-strange:main Oct 8, 2024
3 of 4 checks passed
@bloomingpeach bloomingpeach mentioned this pull request Oct 8, 2024
3 tasks
b-j-roberts pushed a commit that referenced this pull request Oct 8, 2024
<!-- enter the gh issue after hash -->

- [ ] issue #
- [ ] follows contribution
[guide](https://github.com/keep-starknet-strange/shinigami/blob/main/CONTRIBUTING.md)
- [ ] code change includes tests

<!-- PR description below -->

This fixes all tests in `sig_der_failing_tests.json`. fix the bug in the
last PR #251, where I seperate `strict_encoding` and `der_sig` flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants