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

completed txscript tests and fixed some opcodes #242

Merged
merged 5 commits into from
Oct 2, 2023

Conversation

tmrlvi
Copy link
Contributor

@tmrlvi tmrlvi commented Aug 19, 2023

No description provided.

Copy link
Contributor

@someone235 someone235 left a comment

Choose a reason for hiding this comment

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

crypto/txscript/src/opcodes/mod.rs Outdated Show resolved Hide resolved
@tmrlvi tmrlvi force-pushed the more_txscript_tests branch from 044f445 to 73a8727 Compare September 17, 2023 20:20
@tmrlvi
Copy link
Contributor Author

tmrlvi commented Sep 17, 2023

Can you also run the tests on https://github.com/kaspanet/kaspad/blob/master/domain/consensus/utils/txscript/data/script_tests.json ? Can be done on another PR

I wrote a test that run script_tests.json.

What about the rest of the tests in https://github.com/kaspanet/kaspad/blob/master/domain/consensus/utils/txscript/data/?
I have found a ticket about them (kaspanet/kaspad#1037)
Should I sync them as well, and open a parallel ticket?

crypto/txscript/src/lib.rs Show resolved Hide resolved
crypto/txscript/src/lib.rs Outdated Show resolved Hide resolved
crypto/txscript/src/lib.rs Outdated Show resolved Hide resolved

#[cfg(test)]
#[allow(unused_comparisons)]
pub(crate) fn parse_short_form(test: String) -> ScriptBuilderResult<Vec<u8>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why call it test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you dropped the version arg? When we'll have multiple version the result of this function can be affected by the script pub key version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't changing the version require change in the JSON file as well, and therefore some cascading changes anyway (parsing, functions calls etc)? I don't think leaving the version (that is just hard coded in calls) saves work in the long term

Copy link
Contributor Author

@tmrlvi tmrlvi Oct 2, 2023

Choose a reason for hiding this comment

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

Why call it test?

This function uses only to parse test scripts. I can change it to test_script

crypto/txscript/src/opcodes/macros.rs Outdated Show resolved Hide resolved
crypto/txscript/src/opcodes/macros.rs Outdated Show resolved Hide resolved
}

fn always_illegal(&self) -> bool {
matches!(CODE, codes::OpVerIf | codes::OpVerNotIf)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about OpReserved,OpReserved1 and OpReserved2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only ones checked pre-execution in go-kaspad are OpVerIf and OpVerNotIf.
It follows from here:
https://github.com/kaspanet/kaspad/blob/92574e623f0741d57d6a097498866dd21c5ce6da/domain/consensus/utils/txscript/opcode.go#L656

@tmrlvi tmrlvi force-pushed the more_txscript_tests branch from ec97d68 to be7011c Compare October 2, 2023 14:24
@michaelsutton michaelsutton merged commit 222ae83 into kaspanet:master Oct 2, 2023
6 checks passed
smartgoo pushed a commit to smartgoo/rusty-kaspa that referenced this pull request Jun 18, 2024
* completed txscript tests and fixed some opcodes

* implemented bitcoind tests and fixed errors discovered in testing process

* changed to go-kaspad

* fixed clippy issue

* fixed from PR
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