Add legacy sigops counts to the zcash_script library API

[teor2345]

Purpose

This PR adds legacy sigops counts to the zcash_script library API.
This allows Zebra to check the MAX_BLOCK_SIGOPS consensus rule.

This PR also splits src/primitives/transaction.cpp, to reduce zcash_script's dependencies.

[str4d]

Move orchard_ffi.rs into orchard_ffi/mod.rs

Fix an issue where the path that Cargo uses to search for submodules is
incorrect.

@jvff What issue did you encounter that made this change necessary? Rust 2018 (which is the edition we use for the Rust code) explicitly allows both orchard_ffi.rs and orchard_ffi/ to coexist. I am unwilling to merge this kind of change without knowing precisely why it is needed, because it sounds to me more like a bug in some other dependency that is assuming pre-2018 edition logic.

[str4d]

utACK modulo requested changes below, and my question above being addressed.

[jvff]

@jvff What issue did you encounter that made this change necessary? Rust 2018 (which is the edition we use for the Rust code) explicitly allows both orchard_ffi.rs and orchard_ffi/ to coexist. I am unwilling to merge this kind of change without knowing precisely why it is needed, because it sounds to me more like a bug in some other dependency that is assuming pre-2018 edition logic.

The issue is that not using mod.rs causes Cargo to look for sub-modules in the incorrect path. This is the error I get if I move orchard/mod.rs back to orchard.rs:

   Compiling zcash_script v0.1.6-alpha.0 (/project)
error[E0583]: file not found for module `incremental_sinsemilla_tree_ffi`
  --> src/../depend/zcash/src/rust/src/orchard_ffi.rs:22:1
   |
22 | mod incremental_sinsemilla_tree_ffi;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: to create the module `incremental_sinsemilla_tree_ffi`, create file "src/../depend/zcash/src/rust/src/incremental_sinsemilla_tree_ffi.rs" or "src/../depend/zcash/src/rust/src/incremental_sinsemilla_tree_ffi/mod.rs"

For more information about this error, try `rustc --explain E0583`.
error: could not compile `zcash_script` due to previous error

(rustc --version gives me rustc 1.56.0 (09c42c458 2021-10-18))

[str4d]

Hmm, this seems like an issue related to the use of the include! macro to "import" the Rust files from zcash/zcash into the zcash_script crate. My guess is that it is evaluating that include's relative files differently for pre- and post-2018 edition module paths (i.e. it looks relative to the original file for module files in the same folder as itself, but for 2018-edition where it needs to look in a subdir, it looks next to the including file), which seems annoying. Did you try instead adding a zcash_script/src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs file with a similar include?

[jvff]

Hmm, this seems like an issue related to the use of the include! macro to "import" the Rust files from zcash/zcash into the zcash_script crate. My guess is that it is evaluating that include's relative files differently for pre- and post-2018 edition module paths (i.e. it looks relative to the original file for module files in the same folder as itself, but for 2018-edition where it needs to look in a subdir, it looks next to the including file), which seems annoying. Did you try instead adding a zcash_script/src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs file with a similar include?

Yes. I tried both with zcash_script/src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs and zcash_script/src/incremental_sinsemilla_tree_ffi.rs. Unfortunately the error is the same, I think Cargo doesn't even try to look in other directories after the module's path changed through the include! macro.

I'll add some more details below just to make sure there isn't a mistake I made and didn't see:

janito:/project$ cat src/orchard_ffi.rs
include!("../depend/zcash/src/rust/src/orchard_ffi.rs");
janito:/project$ cat src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs
include!("../../depend/zcash/src/rust/src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs");
janito:/project$ cat src/incremental_sinsemilla_tree_ffi.rs
include!("../depend/zcash/src/rust/src/orchard_ffi/incremental_sinsemilla_tree_ffi.rs");
janito:/project$ cargo build
   Compiling zcash_script v0.1.6-alpha.0 (/project)
error[E0583]: file not found for module `incremental_sinsemilla_tree_ffi`
  --> src/../depend/zcash/src/rust/src/orchard_ffi.rs:22:1
   |
22 | mod incremental_sinsemilla_tree_ffi;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: to create the module `incremental_sinsemilla_tree_ffi`, create file "src/../depend/zcash/src/rust/src/incremental_sinsemilla_tree_ffi.rs" or "src/../depend/zcash/src/rust/src/incremental_sinsemilla_tree_ffi/mod.rs"

For more information about this error, try `rustc --explain E0583`.
error: could not compile `zcash_script` due to previous error
janito:/project$

[str4d]

I found a Rust issue that describes this undocumented behaviour of include!. I've added a comment describing the until-now-unnoticed issue with 2018 edition module structures: rust-lang/rust#50132 (comment)

Given that it's unlikely that issue will be resolved any time soon, I'm now -0 on altering our code to use 2015 edition module structure for this specific module. I'd prefer if zcash_script could find some other way to use the necessary files without using include!, but if that isn't possible, I'm not against the compatibility change made in this PR. It's unfortunately something we're likely to run into again in the future though.

[teor2345]

I'd prefer if zcash_script could find some other way to use the necessary files without using include!, but if that isn't possible, I'm not against the compatibility change made in this PR.

Ideally, transparent script verification shouldn't need to pull in a C++ to Orchard FFI module.
Unless it's needed to verify the sighash?

It's unfortunately something we're likely to run into again in the future though.

We'd like to minimise zcash_script's dependencies, so it shouldn't happen too often:
ZcashFoundation/zebra#3041

(But we won't have time to remove any more dependencies for at least a month.)

[str4d]

Unless it's needed to verify the sighash?

We need orchard_ffi.rs for implementing the Orchard component of CTransaction parsing and serialization:

zcash/src/primitives/transaction.h

Lines 329 to 386 in 5646dc7

	/**
	* The Orchard component of a transaction.
	*/
	class OrchardBundle
	{
	private:
	/// An optional Orchard bundle (with `nullptr` corresponding to `None`).
	/// Memory is allocated by Rust.
	std::unique_ptr<OrchardBundlePtr, decltype(&orchard_bundle_free)> inner;
	public:
	OrchardBundle() : inner(nullptr, orchard_bundle_free) {}
	OrchardBundle(OrchardBundle&& bundle) : inner(std::move(bundle.inner)) {}
	OrchardBundle(const OrchardBundle& bundle) :
	inner(orchard_bundle_clone(bundle.inner.get()), orchard_bundle_free) {}
	OrchardBundle& operator=(OrchardBundle&& bundle)
	{
	if (this != &bundle) {
	inner = std::move(bundle.inner);
	}
	return *this;
	}
	OrchardBundle& operator=(const OrchardBundle& bundle)
	{
	if (this != &bundle) {
	inner.reset(orchard_bundle_clone(bundle.inner.get()));
	}
	return *this;
	}
	template<typename Stream>
	void Serialize(Stream& s) const {
	RustStream rs(s);
	if (!orchard_bundle_serialize(inner.get(), &rs, RustStream<Stream>::write_callback)) {
	throw std::ios_base::failure("Failed to serialize v5 Orchard bundle");
	}
	}
	template<typename Stream>
	void Unserialize(Stream& s) {
	RustStream rs(s);
	OrchardBundlePtr* bundle;
	if (!orchard_bundle_parse(&rs, RustStream<Stream>::read_callback, &bundle)) {
	throw std::ios_base::failure("Failed to parse v5 Orchard bundle");
	}
	inner.reset(bundle);
	}
	/// Queues this bundle's signatures for validation.
	///
	/// `txid` must be for the transaction this bundle is within.
	void QueueSignatureValidation(
	orchard::AuthValidator& batch, const uint256& txid) const
	{
	batch.Queue(inner.get(), txid.begin());
	}
	};

We need transaction_ffi.rs for verifying the sighash:

zcash/src/script/interpreter.cpp

Lines 1201 to 1224 in c4b2ef7

	if (cache) {
	uint256 hash;
	zcash_transaction_transparent_signature_digest(
	cache->preTx.get(),
	nHashType,
	nIn,
	reinterpret_cast<const unsigned char*>(sScriptCode.data()),
	sScriptCode.size(),
	amount,
	hash.begin());
	return hash;
	} else {
	PrecomputedTransactionData local(txTo);
	uint256 hash;
	zcash_transaction_transparent_signature_digest(
	local.preTx.get(),
	nHashType,
	nIn,
	reinterpret_cast<const unsigned char*>(sScriptCode.data()),
	sScriptCode.size(),
	amount,
	hash.begin());
	return hash;
	}

[teor2345]

We need orchard_ffi.rs for implementing the Orchard component of CTransaction parsing and serialization.

We need transaction_ffi.rs for verifying the sighash:

What about:

mod ed25519;

mod history_ffi;
mod zip339_ffi;

mod test_harness_ffi;

[str4d]

Hmm, actually it appears that the unnecessary structure is crate::orchard_ffi::incremental_sinsemilla_tree_ffi. We don't use the tree logic for anything that orchard_ffi is used for AFAICT. So the correct fix here is to flatten the structure by instead moving incremental_sinsemilla_tree_ffi.rs into src/rust/src.

I think @nuttycom created this structure because it conceptually matches the orchard::tree module structure, but here we are only using these FFI modules for horizontal separation to make the FFI files smaller, rather than creating nested crate structures for APIs. If we did want to still have a nested structure, I think we'd need to ensure that FFI-containing files are leaves in the module tree.

[str4d]

What about:

mod ed25519;

Exposes ed25519-zebra for our consensus layer. Unnecessary here as Ed25519 is not usable from Bitcoin scripts, and in any Rust code we use the crate directly.

mod history_ffi;

Only used by consensus code, not transaction parsing or Bitcoin script validation.

mod zip339_ffi;

Only (going to be) used by wallet code.

mod test_harness_ffi;

Only used by zcashd unit tests.

[teor2345]

Thanks, I've edited our dependency ticket.

[str4d]

utACK

[teor2345]

Just confirming that these changes work for Zebra:
ZcashFoundation/zebra#3080
ZcashFoundation/zcash_script#27