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

[DRAFT] Rust interpreter #158

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ebfull
Copy link
Contributor

@ebfull ebfull commented May 24, 2024

No description provided.

@nuttycom nuttycom marked this pull request as draft May 24, 2024 19:34
Copy link
Collaborator

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

Flushing initial review comments from my pairing with @daira

@@ -47,6 +47,7 @@ path = "src/lib.rs"

[features]
external-secp = []
rust-interpreter = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rust-interpreter = []

Since features can be enabled transitively and/or accidentally, it would be better to use an explicit compilation flag for now.

@@ -70,6 +70,9 @@ mod orchard_ffi;
mod streams_ffi;
mod transaction_ffi;

#[cfg(feature = "rust-interpreter")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "rust-interpreter")]
#[cfg(zcash_unstable = "rust-interpreter")]

This follows the convention that we use in librustzcash, e.g. https://github.com/zcash/librustzcash/blob/main/zcash_primitives/src/lib.rs#L29-L30


Ok(match leading_byte {
OP_PUSHDATA1 | OP_PUSHDATA2 | OP_PUSHDATA4 => {
let read_le = |script: &mut &[u8], needed_bytes: usize| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unlike in the C++ code, require_minimal_pushes is not checked as part of parsing.


pub fn parse_opcode(
script: &mut &[u8],
mut buffer: Option<&mut Vec<u8>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't need to pass a mutable buffer here, since we clear it out; let's just allocate a new one? This may be a microoptimization, but it also introduces the potential for error.

size <<= 8;
size |= script[i] as usize;
}
*script = &script[needed_bytes..];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of mutating *script, we could return the slice of the underlying buffer that remains after parsing the first opcode.

buffer.map(|buffer| {
buffer.extend(&script[0..size]);
*script = &script[size..];
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

With sufficiently recent Rust, we'll want to use https://doc.rust-lang.org/std/primitive.slice.html#method.split_first_chunk here.

return Err(ScriptError::ScriptSize);
}

let mut script = (*self).clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just self.0.clone()?

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.

None yet

2 participants