Initial Rust implementation #174

sellout · 2024-09-26T18:31:32Z

This provides two more implementations of the ZcashScript trait. One is a pure Rust implementation and the other runs both the C++ and Rust implementations, returning the C++ result and logging a warning if they differ.

This follows the C++ implementation extremely closely for ease of initial review. Future changes will move from this to a Rustier approach.

E.g.,

early returns
asserts
bounds checking followed by accesses¹
if instead of match
mutable refs
etc.

It differs from the C++ in some places, as well. Numeric types and casting/From are changed in some places to avoid partiality. Also, the Opcodes are partitioned into multiple types because we eventually want to be able to enforce push_only at the type level, and PushValue operations carry data (although not yet in the structure), which makes them distinct from operations (PushdataBytelength, in particular).

accesses generally return in Result, so ? ensures they’re safe, but we keep the bounds checking so that the stack state matches the C++ version (e.g., without the bounds checking, an op that pops two elements off the stack could fail after popping one, which would leave the stack in a different state than the C++ impl which checks the length of the stack before popping either). ↩

sellout · 2024-09-26T18:34:47Z

The files are intended to be compared side-by-side with the C++ implementation. E.g., script.rs first contains definitions from script.h, followed by definitions from script.cpp.

sellout · 2024-09-30T19:02:48Z

@daira, @str4d, @ebfull – can’t officially add you as reviewers, but here it is.

The only changes here other than moving chunks of code around are - moving `evaluate` out of `impl Script`, which required changing `&self` to `script: &Script`; and - unifying `ExecutionOptions` with `VerificationFlags`.

For easier side-by-side comparison.

The C++ impl uses exceptions for `ScriptNum`, but catches them.

These tests run both the C++ and Rust impls. One uses completely arbitrary inputs, and the other limits script_sig to data pushes.

For now, the underlying errors are discarded when comparing against the C++ results, but there are corresponding changes on the C++ side in a separate branch.

sellout · 2024-10-21T21:18:21Z

I don’t know why the test suite is failing on the C++ side now.

There are no changes to the C++ in this PR.
“It works for me” – locally running cargo test behaves as expected, running both the C++ and Rust tests.

My only guess is that it is somehow caused by the secp256k1(_sys) dependency maybe forcing a different version of the underlying libsecp256k1 to be linked.

Since there’s a lot of noise in the logs, here’s the actual error:

  = note: /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/target/debug/build/zcash_script-13bd32f862a45584/out/libzcash_script.a(484cc929e4b13849-pubkey.o): in function `(anonymous namespace)::Secp256k1SelfTester::Secp256k1SelfTester()':
          /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:17: undefined reference to `secp256k1_selftest'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/target/debug/build/zcash_script-13bd32f862a45584/out/libzcash_script.a(484cc929e4b13849-pubkey.o): in function `CPubKey::Verify(uint256 const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) const':
          /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:28: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:28: undefined reference to `secp256k1_ec_pubkey_parse'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:35: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:35: undefined reference to `secp256k1_ecdsa_signature_parse_der'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:40: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:40: undefined reference to `secp256k1_ecdsa_signature_normalize'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:41: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:41: undefined reference to `secp256k1_ecdsa_verify'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/target/debug/build/zcash_script-13bd32f862a45584/out/libzcash_script.a(484cc929e4b13849-pubkey.o): in function `CPubKey::CheckLowS(std::vector<unsigned char, std::allocator<unsigned char> > const&)':
          /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:149: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:149: undefined reference to `secp256k1_ecdsa_signature_parse_der'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:152: undefined reference to `secp256k1_context_static'
          /usr/bin/ld: /home/runner/work/zcash_script/zcash_script/depend/zcash/src/pubkey.cpp:152: undefined reference to `secp256k1_ecdsa_signature_normalize'
          collect2: error: ld returned 1 exit status

nuttycom

Flushing comments; reviewed up to 336d0fe

nuttycom · 2024-10-23T18:50:46Z

src/interpreter.rs

+                    // Data is being pushed to the stack here; we may need to check
+                    // that the minimal script size was used to do so if our caller
+                    // requires it.
+                    if options.contains(VerificationFlags::MinimalData)


nonblocking: This is not a pure move; ideally this change would be split out into a separate commit.

Do you mean because it’s not a const with bitwise ops?

I can push this to a later PR and swing VerificationFlags back to a bunch of consts. I think I just wrote this bit before I decided to stick as close to the C++ as possible.

Ah, I see. This is because of the commit it’s in, which is intended to just rearrange @ebfull’s implementation a bit.

nuttycom · 2024-10-23T18:52:52Z

src/interpreter.rs

+                            // of a standard tx rule, for example) they can
+                            // enable `discourage_upgradable_nops` to turn
+                            // these opcodes into errors.
+                            if options.contains(VerificationFlags::DiscourageUpgradableNOPs) {


nuttycom · 2024-10-23T18:53:00Z

src/interpreter.rs

+                            if !options.contains(VerificationFlags::CHECKLOCKTIMEVERIFY) {
+                                if options.contains(VerificationFlags::DiscourageUpgradableNOPs) {


src/external/mod.rs

src/external/uint256.rs

src/lib.rs

nuttycom · 2024-10-23T19:42:00Z

src/script_error.rs

 pub enum ScriptError {
+    Ok = 0,


I'm not sure how big of a deal this is, but it might be better to be explicit about the exact binding to the C++ enum reprs for all of the cases; a reordering could accidentally cause these to get out of sync.

The only reason for having the repr is for the C++/Rust comparisons, where the C++ errors get mapped to the Rust ones. If they get out of sync, the tests should fail.

src/script_error.rs

src/zcash_script.rs

src/interpreter.rs

conradoplg · 2024-10-29T21:09:48Z

“It works for me” – locally running cargo test behaves as expected, running both the C++ and Rust tests.

It works on Rust 1.81 but it doesn't on 1.82. Which is super weird, but should give you a direction where to investigate

- remove `uint256` module - create a specific type for the C++/Rust comparison implementation - rename some identifiers - rephrase some comments

mpguerra · 2024-12-03T14:39:25Z

Thank you for your work on this so far @sellout! Are you planning to continue working on this one? We were hoping to tag a new release of zcash_script including this work once it is ready.

- Switch from `log` to `tracing` for `warn!`, - Export `SignedOutputs`, which is needed to create a `HashType`, and - Upgrade zcash_primitives to the same version used by zebrad.

sellout · 2024-12-06T01:35:12Z

Yes, sorry – until I saw your comment, I didn’t realize this branch wasn’t up-to-date with my current work (I had accidentally been pushing to my fork).

I’ve updated it, fixing the merge conflicts, etc., but it still seems to have the test-building issue on Ubuntu. I’ll dig into that tomorrow.

Also, this PR still needs some review. If anyone on the ZF side (@conradoplg, @arya2, whoever) has time to give it a go, I’m happy to facilitate that however I can.

Matches the one in Zebra, although I think pinning to a specific release (e.g., `channel = "1.82"`) would help with consistency, perhaps avoiding issues like ZcashFoundation#174 (comment)

sellout · 2024-12-06T22:47:45Z

Ok, the current failing checks are failing on master (because of Rust stable toolchain changes that happened since those commits were merged), so #176 is trying to address those independently of this massive PR.

* Add a basic rust-toolchain.toml Matches the one in Zebra, although I think pinning to a specific release (e.g., `channel = "1.82"`) would help with consistency, perhaps avoiding issues like #174 (comment) * Remove redundant toolchain info from GH workflow [actions-rs/toolchain doesn’t support TOML-formatted rust-toolchain files](actions-rs/toolchain#126), but it’s unnecessary anyway. - actions-rs/cargo will pick up the rust-toolchain.toml, so we usually don’t need to mention the toolchain at all; - the Windows build just runs `rustup target add x86_64-pc-windows-msvc` directly; and - where we want to build with multiple toolchains (in a matrix), there are some slightly-awkward conditionals. This also makes some other changes: - `fail-fast` is disabled because it hides useful & distinct build results; and - `rustup component add` for clippy and rustfmt are removed because they’re in the rust-toolchain.toml toolchain, and we want to make sure they are, so that they’re available to developers. * Pin rustup channel to "1.81" Newer versions until 1.85 (current nightly) have some breakage wrt C++ linking. * Have bindgen target correct rustc version It should match the version in rust-toolchain.toml. Unfortunately, it’s not possible to reference that directly, so this adds comments to remind contributors to update them together.

ebfull and others added 2 commits May 24, 2024 13:25

[DRAFT]

5b6bc86

Merge remote-tracking branch 'upstream/master' into 'rust-interpreter'

5906175

This was referenced Sep 26, 2024

Initial Rust implementation #173

Closed

[DRAFT] Rust interpreter #158

Closed

sellout marked this pull request as draft September 26, 2024 18:33

sellout force-pushed the sellout/rust-interpreter branch from 629eed7 to e95972b Compare September 29, 2024 20:04

sellout marked this pull request as ready for review September 29, 2024 20:26

sellout requested a review from nuttycom September 30, 2024 19:02

sellout mentioned this pull request Sep 30, 2024

Address Str4d’s comments on #171 #175

Merged

sellout added 2 commits October 15, 2024 14:10

Rearrange Rust impl to match C++ more closely

d2763f9

The only changes here other than moving chunks of code around are - moving `evaluate` out of `impl Script`, which required changing `&self` to `script: &Script`; and - unifying `ExecutionOptions` with `VerificationFlags`.

Rename Rust identifiers to match C++

3d71aef

For easier side-by-side comparison.

sellout force-pushed the sellout/rust-interpreter branch 3 times, most recently from 848ebaa to 09ac96b Compare October 17, 2024 19:54

sellout added 9 commits October 21, 2024 13:23

Connected the new API, but fails

336d0fe

Existing unit tests succeed

bd168e1

The rest of the owl

571836d

Reverting to C++ style, and some other changes

1b57460

Appease Clippy

0329117

Replace ScriptNum panics with error case

ebca981

The C++ impl uses exceptions for `ScriptNum`, but catches them.

Add some shallow property tests

d9d46cd

These tests run both the C++ and Rust impls. One uses completely arbitrary inputs, and the other limits script_sig to data pushes.

Add shallow fuzz testing

a16d6ad

Preserve richer errors on the Rust side

4c34b3e

For now, the underlying errors are discarded when comparing against the C++ results, but there are corresponding changes on the C++ side in a separate branch.

sellout force-pushed the sellout/rust-interpreter branch from 09ac96b to 4c34b3e Compare October 21, 2024 19:35

nuttycom reviewed Oct 23, 2024

View reviewed changes

Address @nuttycom’s review comments

81fdb8e

- remove `uint256` module - create a specific type for the C++/Rust comparison implementation - rename some identifiers - rephrase some comments

Merge remote-tracking branch 'upstream/master' into rust-interpreter

e0fabfa

mpguerra mentioned this pull request Nov 18, 2024

Publish a new zcash_script release #172

Open

sellout force-pushed the sellout/rust-interpreter branch from 7e16a5f to 3b81cf5 Compare December 5, 2024 23:32

sellout added 2 commits December 5, 2024 16:47

Some changes to ease zebrad integration

db4be8b

- Switch from `log` to `tracing` for `warn!`, - Export `SignedOutputs`, which is needed to create a `HashType`, and - Upgrade zcash_primitives to the same version used by zebrad.

Appease Clippy

f215757

sellout force-pushed the sellout/rust-interpreter branch from 3b81cf5 to f215757 Compare December 6, 2024 01:26

sellout mentioned this pull request Dec 6, 2024

Add a basic rust-toolchain.toml #176

Merged

mpguerra requested a review from conradoplg December 12, 2024 09:27

Merge remote-tracking branch 'upstream/master' into rust-interpreter

a902385

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Rust implementation #174

Initial Rust implementation #174

sellout commented Sep 26, 2024

sellout commented Sep 26, 2024

sellout commented Sep 30, 2024

sellout commented Oct 21, 2024

nuttycom left a comment

nuttycom Oct 23, 2024

sellout Oct 28, 2024

sellout Oct 29, 2024

nuttycom Oct 23, 2024

nuttycom Oct 23, 2024

nuttycom Oct 23, 2024

sellout Oct 28, 2024

conradoplg commented Oct 29, 2024

mpguerra commented Dec 3, 2024

sellout commented Dec 6, 2024

sellout commented Dec 6, 2024

		if !options.contains(VerificationFlags::CHECKLOCKTIMEVERIFY) {
		if options.contains(VerificationFlags::DiscourageUpgradableNOPs) {

Initial Rust implementation #174

Are you sure you want to change the base?

Initial Rust implementation #174

Conversation

sellout commented Sep 26, 2024

Footnotes

sellout commented Sep 26, 2024

sellout commented Sep 30, 2024

sellout commented Oct 21, 2024

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom Oct 23, 2024

Choose a reason for hiding this comment

sellout Oct 28, 2024

Choose a reason for hiding this comment

sellout Oct 29, 2024

Choose a reason for hiding this comment

nuttycom Oct 23, 2024

Choose a reason for hiding this comment

nuttycom Oct 23, 2024

Choose a reason for hiding this comment

nuttycom Oct 23, 2024

Choose a reason for hiding this comment

sellout Oct 28, 2024

Choose a reason for hiding this comment

conradoplg commented Oct 29, 2024

mpguerra commented Dec 3, 2024

sellout commented Dec 6, 2024

sellout commented Dec 6, 2024