Enable ZIP 216 for blocks prior to NU5 activation #6000

ebfull · 2022-06-08T14:18:31Z

I also take the opportunity in this PR to remove the legacy Sapling verification FFI stuff and replace it with the new cxx approach.

I've tested that ZIP 216 can be applied to blocks prior to activation on both mainnet and testnet.

src/main.cpp

daira · 2022-06-23T18:08:53Z

src/chainparams.cpp

@@ -135,7 +135,8 @@ class CMainParams : public CChainParams {
            uint256S("00000000002038016f976744c369dce7419fca30e7171dfac703af5e5f7ad1d4");
        consensus.vUpgrades[Consensus::UPGRADE_NU5].nProtocolVersion = 170100;
        consensus.vUpgrades[Consensus::UPGRADE_NU5].nActivationHeight = 1687104;
-
+        consensus.vUpgrades[Consensus::UPGRADE_NU5].hashActivationBlock =
+            uint256S("0000000000d723156d9b65ffcf4984da7a19675ed7e2f06d9e5d5188af087bf8");


I checked this hash.

I also checked this hash.

I didn't check this hash.

daira · 2022-06-23T18:09:13Z

src/chainparams.cpp

@@ -239,7 +240,7 @@ class CMainParams : public CChainParams {
        }

        // The best chain should have at least this much work.
-        consensus.nMinimumChainWork = uint256S("00000000000000000000000000000000000000000000000004a90edff47bbdc6");
+        consensus.nMinimumChainWork = uint256S("000000000000000000000000000000000000000000000000098e5c63248dcb28");


I did not check this.

I confirmed that this minimum chain work is greater than the chain work at the NU5 activation block, as required.

I didn't check this either.

src/rust/src/sapling.rs

daira · 2022-06-23T18:30:04Z

src/rust/src/sapling.rs

+        };
+
+        self.0
+            .final_check(value_balance, sighash_value, binding_sig)


These APIs (final_check and SaplingVerificationContext::final_check) also have arguments in an inconsistent order, but here there's no argument that we're preserving the field order in the v4 transaction encoding, because sighash_value is not a field.

daira

utACK with comments.

daira

utACK

daira

utACK

defuse

utACK

…ules to use the new API.

The chain work is taken from mainnet block 1710000.

str4d

utACK af2b3d3

src/rust/src/sapling.rs

daira · 2022-07-04T13:48:44Z

src/rust/src/sapling.rs

+// This is added because `check_spend` takes several arguments over FFI. This
+// annotation gets removed by the cxx procedural macro so it needs to be enabled
+// on the entire module.
+#![allow(clippy::too_many_arguments)]


Filed dtolnay/cxx#1061

This was fixed in dtolnay/cxx#1062, merged in cxx 1.0.70.

daira · 2022-07-04T13:59:13Z

src/rust/src/sapling.rs

+        self.0.check_output(
+            cv,
+            cm,
+            ephemeral_key,


RedDSA.Validate takes an abstract public key and reserializes it for input to $\mathsf{H}^⊛$, so this is correct.
As @str4d says, ephemeral_key should be renamed to epk here.

Suggested change

ephemeral_key,

epk,

daira · 2022-07-04T14:14:02Z

src/uint256.h

+    std::array<uint8_t, WIDTH> GetRawBytes() const
+    {
+        std::array<uint8_t, WIDTH> buf = {};
+        memcpy(buf.data(), this->begin(), WIDTH);


Suggested change

memcpy(buf.data(), this->begin(), WIDTH);

std::memcpy(buf.data(), this->begin(), WIDTH);

It probably works without the std:: (and without using std::memcpy or using namespace std;) by coincidence of the included headers implicitly declaring memcpy in the global namespace. Strictly speaking that's not portable (similarly for the uses of memset in this file).

Yup, definitely not portable.

daira

utACK with suggestions.

therealyingtong

utACK

This completes the work started in zcash#6000. Closes zcash#6396.

As a side-effect, we remove the ability to verify individual transactions with pre-ZIP 216 rules (which we already removed from `zcashd` consensus nodes in zcash/zcash#6000 and zcash/zcash#6399, as all pre-ZIP 216 transactions on mainnet are also valid under ZIP 216).

str4d reviewed Jun 21, 2022

View reviewed changes

src/main.cpp Show resolved Hide resolved

ebfull added the safe-to-build Used to send PR to prod CI environment label Jun 21, 2022

daira reviewed Jun 23, 2022

View reviewed changes

src/rust/src/sapling.rs Show resolved Hide resolved

daira reviewed Jun 23, 2022

View reviewed changes

daira previously approved these changes Jun 23, 2022

View reviewed changes

ebfull dismissed daira’s stale review via 2a6c77a June 29, 2022 16:34

ebfull force-pushed the enablezip216forall branch 2 times, most recently from 2a6c77a to 8b0f27e Compare June 29, 2022 18:01

daira force-pushed the enablezip216forall branch from 8b0f27e to 6767c5c Compare July 1, 2022 14:37

daira previously approved these changes Jul 1, 2022

View reviewed changes

str4d added the safe-to-build Used to send PR to prod CI environment label Jul 1, 2022

str4d added this to the Release 5.1.0 milestone Jul 1, 2022

ebfull dismissed daira’s stale review via 41ddf87 July 1, 2022 16:55

ebfull force-pushed the enablezip216forall branch from 6767c5c to 41ddf87 Compare July 1, 2022 16:55

nuttycom added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jul 1, 2022

nuttycom force-pushed the enablezip216forall branch from 41ddf87 to 079e87c Compare July 1, 2022 18:08

nuttycom added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jul 1, 2022

daira previously approved these changes Jul 1, 2022

View reviewed changes

nuttycom added safe-to-build Used to send PR to prod CI environment and removed safe-to-build-dev Used to send PR to dev CI environment labels Jul 4, 2022

defuse previously approved these changes Jul 4, 2022

View reviewed changes

ebfull added 6 commits July 3, 2022 22:38

Introduce new Sapling verification API via cxx and switch consensus r…

6c51df3

…ules to use the new API.

Enable ZIP 216 for blocks prior to NU5 activation

458e773

Remove the old Sapling verification FFI APIs.

1677ab6

cargo fmt

cc78bf7

Address clippy lints.

ec3d2e5

Update minimum chain work and set NU5 activation block hash for mainnet

af2b3d3

The chain work is taken from mainnet block 1710000.

nuttycom dismissed defuse’s stale review via af2b3d3 July 4, 2022 04:38

nuttycom force-pushed the enablezip216forall branch from b3f33b3 to af2b3d3 Compare July 4, 2022 04:38

nuttycom mentioned this pull request Jul 4, 2022

Use batch validation for Sapling proofs and signatures #6048

Merged

str4d approved these changes Jul 4, 2022

View reviewed changes

src/rust/src/sapling.rs Show resolved Hide resolved

str4d added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Jul 4, 2022

daira reviewed Jul 4, 2022

View reviewed changes

daira approved these changes Jul 4, 2022

View reviewed changes

therealyingtong approved these changes Jul 4, 2022

View reviewed changes

str4d merged commit 466ea88 into zcash:master Jul 4, 2022

str4d mentioned this pull request Feb 2, 2023

Finish retroactively enabling ZIP 216 before NU5 #6396

Closed

str4d mentioned this pull request Feb 2, 2023

Retroactively enable ZIP 216 before NU5 activation #6399

Merged

str4d added a commit to str4d/zcash that referenced this pull request Feb 2, 2023

Retroactively enable ZIP 216 before NU5 activation

9ce6753

This completes the work started in zcash#6000. Closes zcash#6396.

str4d mentioned this pull request Feb 8, 2023

[ZIP 216] Add "Retroactive applicability" section and clarify that the zero point is not valid for pk_d zcash/zips#672

Draft

str4d mentioned this pull request Nov 30, 2023

zcash_primitives: Replace sapling::redjubjub with redjubjub crate zcash/librustzcash#1056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ZIP 216 for blocks prior to NU5 activation #6000

Enable ZIP 216 for blocks prior to NU5 activation #6000

ebfull commented Jun 8, 2022 •

edited

daira Jun 23, 2022

str4d Jul 1, 2022

defuse Jul 4, 2022

daira Jun 23, 2022

str4d Jul 1, 2022

defuse Jul 4, 2022

daira Jun 23, 2022

daira left a comment

daira left a comment

daira left a comment

defuse left a comment

str4d left a comment

daira Jul 4, 2022

daira Jul 25, 2022

daira Jul 4, 2022

daira Jul 4, 2022 •

edited

daira Jul 4, 2022

daira left a comment

therealyingtong left a comment

	memcpy(buf.data(), this->begin(), WIDTH);
	std::memcpy(buf.data(), this->begin(), WIDTH);

Enable ZIP 216 for blocks prior to NU5 activation #6000

Enable ZIP 216 for blocks prior to NU5 activation #6000

Conversation

ebfull commented Jun 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

defuse left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Jul 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

therealyingtong left a comment

Choose a reason for hiding this comment

ebfull commented Jun 8, 2022 •

edited

daira Jul 4, 2022 •

edited