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.

Sign up for GitHub

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

Jump to bottom

[bug] Remaining bitcoin-core test fixes #247

Closed
b-j-roberts opened this issue Oct 4, 2024 · 7 comments · Fixed by #286
Closed

[bug] Remaining bitcoin-core test fixes #247

b-j-roberts opened this issue Oct 4, 2024 · 7 comments · Fixed by #286
Assignees
@SoarinSkySagar @bloomingpeach
Labels
bug Something isn't working large/difficult issue

Comments

@b-j-roberts
Copy link
Contributor

There are a lot of bitcoin-core tests that are still failing. I created this file with all the failing tests.

The file contains all the failing tests from this file in bitcoin-core.

I threw together this bash script, which allows you to run all the failing tests.

It will go through them one by one, and print something like this for each failed test :

  ScriptSig: '0x27 0x3024021077777777777777777777777777777777020a7777777777777777777777777777777701' -- ScriptPubKey: '0 CHECKSIG NOT' -- Flags: '' -- Expected: OK
  Expected : OK -- Result   : PANIC
  FAIL
scarb cairo-run --package shinigami_cmds '[[85638222473728638769160410210893464261263086061422684165462972715402475319,97557673223841770041305211021412912560163447137005097938746121282641147703],5288612062596008609236648564283419927782315402045370417,23,[],976111785041138328458856412106580,14,[],0,0]'
     Running shinigami_cmds
Running Bitcoin Script with ScriptSig: '0x27 0x3024021077777777777777777777777777777777020a7777777777777777777777777777777701', ScriptPubKey: '0 CHECKSIG NOT' and Flags: ''
Run panicked with [1637570914057682275393755530660268060279989363 ('Index out of bounds'), ].

showing the script that executed, the expected result and the result. The following lines give the scarb command needed to run the failing test manually. Most of the tests have a small bit of info inside the script_tests_failing.json file that explains what it is trying to test, which is usually a good hint for where to look.

The task would be to go through these and fix them. Thank you!

Using btcd in general is very useful for seeing bitcoin spec/code to understand how things may differ in our implementation, since our design is heavily inspired by the work there. But in particular this function will be super useful in seeing how they map errors to results from these bitcoin-core tests.
@b-j-roberts b-j-roberts added bug Something isn't working large/difficult issue labels Oct 4, 2024
@b-j-roberts
Copy link
Contributor Author

@bloomingpeach Could you comment here so I can mark you as working on this? Thank you!

@bloomingpeach bloomingpeach mentioned this issue Oct 5, 2024
Merged
3 tasks
@bloomingpeach
Copy link
Contributor

@b-j-roberts can I work on this?

b-j-roberts pushed a commit that referenced this issue Oct 8, 2024
@bloomingpeach
wip: fix sig_der test (#251)
9e7692d 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->

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`
@bloomingpeach bloomingpeach mentioned this issue Oct 23, 2024
Merged
3 tasks
b-j-roberts added a commit that referenced this issue Oct 31, 2024
@bloomingpeach @b-j-roberts
update (#264)
d509dfe 
<!-- enter the gh issue after hash -->
- [ ] More progress on #247 
- [ ] follows contribution
[guide](https://github.com/keep-starknet-strange/shinigami/blob/main/CONTRIBUTING.md)
- [ ] code change includes tests

<!-- PR description below -->
Fix more issues.
```
Script tests complete!
Passed: 1182    Failed: 25    Total: 1207
```

---------

Co-authored-by: Brandon Roberts <brandonjroberts22@gmail.com>
@SoarinSkySagar
Copy link
Contributor

@b-j-roberts can i work on this?

@b-j-roberts
Copy link
Contributor Author

For sure @SoarinSkySagar !

@SoarinSkySagar SoarinSkySagar mentioned this issue Nov 11, 2024
Merged
3 tasks
b-j-roberts pushed a commit that referenced this issue Nov 13, 2024
@SoarinSkySagar
fix: resolves first failing bitcoin-core test (#281)
64361d1 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->
Earlier, the first test in the `.tests/script_tests_failing.json` was
failing due to it receiving the wrong type of error message. the
expected error was `SCRIPT_ERR_SIG_DER` but it got `SIG_HASHTYPE` error
instead. This PR fixes that issue.
@SoarinSkySagar
Copy link
Contributor

Hi @b-j-roberts, I think you closed this issue unknowingly because there are still some failing tests, which I'm currently working on.

@SoarinSkySagar SoarinSkySagar mentioned this issue Nov 13, 2024
Merged
3 tasks
@bloomingpeach
Copy link
Contributor

Hi @b-j-roberts, I think you closed this issue unknowingly because there are still some failing tests, which I'm currently working on.

@SoarinSkySagar because you are writing the PR description "resolves #xxx", which will auto close the issue when the PR is merged. Best practice should be "This PR addresses #..." if that PR doesn't fully resolve the issue.

@SoarinSkySagar
Copy link
Contributor

@SoarinSkySagar because you are writing the PR description "resolves #xxx", which will auto close the issue when the PR is merged. Best practice should be "This PR addresses #..." if that PR doesn't fully resolve the issue.

I see, will remember that from next time 😅
but can this issue be opened again? I've fixed one more test

@b-j-roberts b-j-roberts reopened this Nov 18, 2024
b-j-roberts added a commit that referenced this issue Nov 18, 2024
@SoarinSkySagar @b-j-roberts
fix: more tests passing (#282)
739a292 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->
### New tests passing:
 - `SIG_HIGH_S`:
 ```
["0x48
0x304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001","0x21
0x03363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640
CHECKSIG","LOW_S","SIG_HIGH_S","P2PK with high S"]
```

- `DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM`:
```

[["304402205ae57ae0534c05ca9981c8a6cdf353b505eaacb7375f96681a2d1a4ba6f02f84022056248e68643b7d8ce7c7d128c9f1f348bcab8be15d094ad5cadd24251a28df8001","0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8",0E-8],"","1
0x14
0x91b24bf9f5288532960ac687abb035127b1d28a5","DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM,P2SH,WITNESS","DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM","P2WPKH
with future witness version"],
```

---------

Co-authored-by: Brandon Roberts <brandonjroberts22@gmail.com>
This was referenced Nov 20, 2024
Merged
Merged
b-j-roberts pushed a commit that referenced this issue Nov 22, 2024
@SoarinSkySagar
fix: more tests (#285)
7a388ec 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->
Fixes 6 more tests, all of which are dependent on the
`WITNESS_PUBKEYTYPE` error
b-j-roberts added a commit that referenced this issue Nov 25, 2024
@SoarinSkySagar @b-j-roberts
fix: all core tests passing! (#286)
1963116 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->
Two testcases were failing, both related to the `UNSATISFIED_LOCKTIME`
error. This PR fixes both.

---------

Co-authored-by: Brandon R <54774639+b-j-roberts@users.noreply.github.com>
Co-authored-by: Brandon Roberts <brandonjroberts22@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SoarinSkySagar SoarinSkySagar

@bloomingpeach bloomingpeach

Labels
bug Something isn't working large/difficult issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: all core tests passing! SoarinSkySagar/shinigami
3 participants
@b-j-roberts @SoarinSkySagar @bloomingpeach