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

[FEAT] implement p2pkh #197

Merged

Conversation

prasincs
Copy link
Contributor

@prasincs prasincs commented Sep 2, 2024

I'm trying to do a few naive things first to make sure I understand where the code is right now. I'm trying to replicate some existing tests and it seems to be failing on CHECKSIG and it's unclear if it's some test setup diff or I'm missing something because test_checksig_valid seems to work.

Copy link

vercel bot commented Sep 2, 2024

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

A member of the Team first needs to authorize it.

@prasincs prasincs changed the title [FEAT] implement p2 pkh [FEAT] implement p2pkh Sep 2, 2024
@j1mbo64
Copy link
Contributor

j1mbo64 commented Sep 3, 2024

Hi @prasincs , I think it's because OP_CHECKSIG use the whole transaction to generate signature. You use mock_transaction which mock the transaction according to the signature in test_checksig_valid.
You can try first to make a Transaction who match the desired.
Maybe you can write down the txid you are trying to simulate ?

@prasincs prasincs force-pushed the feat/179-implement-P2PKH branch from d06b65c to eee2de5 Compare September 6, 2024 06:08
@prasincs
Copy link
Contributor Author

prasincs commented Sep 8, 2024

Hi @prasincs , I think it's because OP_CHECKSIG use the whole transaction to generate signature. You use mock_transaction which mock the transaction according to the signature in test_checksig_valid. You can try first to make a Transaction who match the desired. Maybe you can write down the txid you are trying to simulate ?

I wasn't picking a specific transaction at the time, I was merely trying to pattern match some existing tests to get more familiar with the code. @b-j-roberts pointed me to the right direction. I'm now trying to replicate the first p2pkh tx https://learnmeabitcoin.com/explorer/tx/6f7cf9580f1c2dfb3c4d5d043cdbb128c640e3f20161245aa7372e9666168516#output-0
it's failing on verifying signature, which seems appropriate. Is this verification the missing part?

@prasincs
Copy link
Contributor Author

I haven't had much time to look into this but I'd suspected something was wrong with signature validation. So I compared the values from the tx that spent the first P2PKH tx and the intermediate values for message and while the values from test_validate_transaction pass, what I have with the P2PKH doesn't. I've pushed it over here if you want to check and run for yourself https://gist.github.com/prasincs/3c133f3fc75931c1f6c4408a6231ae54

@b-j-roberts
Copy link
Contributor

It seems the test was failing due to a bug mentioned and fixed here. #209 (comment)

Thank you for diving into this, was tricky to find the bug.

@b-j-roberts b-j-roberts marked this pull request as ready for review September 17, 2024 05:59
@b-j-roberts b-j-roberts merged commit 8681f6b into keep-starknet-strange:main Sep 17, 2024
3 of 4 checks passed
j1mbo64 pushed a commit to j1mbo64/shinigami that referenced this pull request Sep 18, 2024
<!-- enter the gh issue after hash -->

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

<!-- PR description below -->

I'm trying to do a few naive things first to make sure I understand
where the code is right now. I'm trying to replicate some existing tests
and it seems to be failing on CHECKSIG and it's unclear if it's some
test setup diff or I'm missing something because `test_checksig_valid`
seems to work.

---------

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants