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] [bitcoin-core test] Script sig push only with NOP #215

Closed
b-j-roberts opened this issue Sep 12, 2024 · 10 comments
Closed

[bug] [bitcoin-core test] Script sig push only with NOP #215

b-j-roberts opened this issue Sep 12, 2024 · 10 comments
Assignees
@Kaminar-i
Labels
bug Something isn't working

Comments

@b-j-roberts
Copy link
Contributor

Issue

The following bitcoin-core test is failing to give the correct result :

ScriptSig: '0x47 0x3044022018a2a81a93add5cb5f5da76305718e4ea66045ec4888b28d84cb22fae7f4645b02201e6daa5ed5d2e4b2b2027cf7ffd43d8d9844dd49f74ef86899ec8e669dfd39aa01 NOP8 0x23 0x2103363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640ac', ScriptPubKey: 'HASH160 0x14 0x215640c2f72f0d16b4eced26762035a42ffed39a EQUAL'

This code should be failing with a p2sh not pushonly error within the engine, but is passing. This is most likely due to the NOP8 opcode execution in the Script sig not giving the p2sh not pushonly error like it should.

Replicate

To replicate the issue you can use the latest main branch with the following command:

scarb cairo-run '[[85638276393621973070440747833762038827135161182803658775478804299459683170,94348312009021740139961859936069915624944684415242580173280269784951435832,177072616656797796472887515523897033852567714435353765129584358724158710837,177032522266426335070459730455281860734772707315710790496976700294919633254,97538213092435571229197708352016018136236349306739941081708643270112852000,85638222052479472070506766003547666308441769408253546983376385338131440485,172068737871228978941526372597074399983714305295778164763682290855266562614],510820426138955130342517808661815651,15,[127663847454887939587139309636239821030768701899862108006692212793516765494],677783736642793086246593772050010351370588991303528376563755713536803148,30,[],100686891288717405067299929,11]'

This comment might help some with how to debug : #192 (comment)
@b-j-roberts b-j-roberts added the bug Something isn't working label Sep 12, 2024
@Kaminar-i
Copy link

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

@onlydustapp OnlyDustApp
Copy link

onlydustapp bot commented Sep 12, 2024

Hey @Kaminar-i!
Thanks for showing interest.
We've created an application for you to contribute to shinigami.
Go check it out on OnlyDust!

@onlydustapp OnlyDustApp
Copy link

onlydustapp bot commented Sep 12, 2024

Hey @ShantelPeters!
Thanks for showing interest.
We've created an application for you to contribute to shinigami.
Go check it out on OnlyDust!

@PavitraAgarwal21
Copy link

Hey @b-j-roberts I want to work on this

@onlydustapp OnlyDustApp
Copy link

onlydustapp bot commented Sep 12, 2024

Hey @PavitraAgarwal21!
Thanks for showing interest.
We've created an application for you to contribute to shinigami.
Go check it out on OnlyDust!

@mubarak23
Copy link
Contributor

This bug is happening due to the way PUSHONLY OPCODE was implemented

A quick log along the execution path should show what is happening

@b-j-roberts kindly assigned

@b-j-roberts
Copy link
Contributor Author

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

@Kaminar-i Yes, I can assign you. Welcome to the project.
Please let me know if you have any questions, I will reach out in a couple days to check on progress 🚀

@b-j-roberts
Copy link
Contributor Author

@Kaminar-i Any luck debugging this script yet? Did you have any questions?

@jsandinoDev jsandinoDev mentioned this issue Sep 17, 2024
Merged
@jsandinoDev
Copy link
Contributor

Hi @Kaminar-i @b-j-roberts I think that the problem of this issue might be in the pull_data function, you could check it after #221 it is integrated

b-j-roberts added a commit that referenced this issue Sep 18, 2024
@jsandinoDev @b-j-roberts
Fix Script sig push only with OP_DATA_X [Bug] (#221)
4e5aec6 
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->


Refactor the implementation of the **is_push_only** function.

Closes #213  
Closes #216 


For the #214 && #215 I think that the problem might be in the 

<img width="702" alt="Screenshot 2024-09-16 at 10 58 59 PM"
src="https://github.com/user-attachments/assets/73ef5d77-5965-478c-9543-3826c22d41b0">

---------

Co-authored-by: [email protected] <jsandinoDevjsandinoDevjsandinoDev>
Co-authored-by: Brandon Roberts <[email protected]>
@b-j-roberts
Copy link
Contributor Author

This was resolved as part of this PR, thank you for looking into it.

j1mbo64 pushed a commit to j1mbo64/shinigami that referenced this issue Sep 18, 2024
@jsandinoDev @b-j-roberts
Fix Script sig push only with OP_DATA_X [Bug] (keep-starknet-strange#221
7c52bdf 
)

<!-- enter the gh issue after hash -->

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

<!-- PR description below -->


Refactor the implementation of the **is_push_only** function.

Closes keep-starknet-strange#213  
Closes keep-starknet-strange#216 


For the keep-starknet-strange#214 && keep-starknet-strange#215 I think that the problem might be in the 

<img width="702" alt="Screenshot 2024-09-16 at 10 58 59 PM"
src="https://github.com/user-attachments/assets/73ef5d77-5965-478c-9543-3826c22d41b0">

---------

Co-authored-by: [email protected] <jsandinoDevjsandinoDevjsandinoDev>
Co-authored-by: Brandon Roberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Kaminar-i Kaminar-i

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mubarak23 @jsandinoDev @b-j-roberts @PavitraAgarwal21 @Kaminar-i