Make address serialization respect encoding order. #5450

nuttycom · 2022-01-04T15:48:39Z

This builds atop #5395 to address #5395 (review)

Correct typo in docker run's volume argument.

This test-only change allows python (rpc) tests to specify, for example, that NU5 should be activated at height X, without having to specify all the previous network upgrades. Previous upgrades can (and must) still be specified if they activate at different block heights (than, in this example, NU5). This makes tests easier to write (and read), especially as the number of network upgrades increases over time. Note that this change only affects zcashd behavior in regtest mode. For the other network modes (testnet and mainnet), the activation heights are hard-coded in chainparams.cpp.

Thanks to metrics-rs/metrics#231 being merged, our dependency tree is now almost entirely de-duplicated!

Fixes zcash#5316

In the three days since the last update, `futures 0.3.18` was yanked due to panics: rust-lang/futures-rs#2529

Batch-verify Orchard transactions at the block level.

Also adds a developer script to make updating the hashes easier.

…ogging Dont log ERROR: spent index not enabled

Update dependencies for 4.6.0

This reverts commit fb38cf0. The lint fix caused a problem on macOS, where the escaped double quote was interpreted as part of an argument, and not as defining an argument. We will need to find another way to address the lint. Closes zcash#5379.

We use the `zcash_address` crate to parse Unified Addresses (which we currently do nothing with). That crate returns an `UnsupportedAddress` error for unhandled address kinds, which we were previously logging. However, we _do_ support the other address kinds; we just parse them in the C++ code. Closes zcash#5321.

test: automatically add missing nuparams (network upgrade heights)

…essage rust: Remove misleading log message

- merkleroot - authdataroot - chainhistoryroot - blockcommitmentshash

`ed25519-zebra` 3

zcash/zcash: The `getmininginfo` RPC now omits `currentblockize` and `currentblocktx` when a block was never assembled via RPC on this node during its current process instantiation. The relevant RPCs are `generate` (regtest only) and `getblocktemplate`. Blocks are also assembled when running the internal miner (`zcashd -gen=1`), after the node mines its first block. (cherry picked from commit bitcoin/bitcoin@fa178a6)

[rpc] mining: Omit uninitialized currentblockweight, currentblocktx

Add defaultroots field to getblocktemplate

Revert lint: Fix false positive

Pre-4.6.0-rc1 changes

This test currently fails with submitblock returning the error "bad-heartwood-root-in-block". Added authdigest to GBT coinbasetxn field because we can't obtain this via getrawtransaction. Co-authored-by: Jack Grigg <jack@z.cash>

Script codes must be serialized in their field encoding, which includes the CompactSize length prefix. Co-authored-by: Jack Grigg <jack@z.cash>

We added support for the NU5 consensus rules in v4.5.0, which alters the block header to contain a `hashBlockCommitments` value instead of the chain history root. However, the output of `getblocktemplate` wasn't returning this value; once NU5 activated, the `blockcommitmentshash` field was being set to "null" (all-zeroes). In v4.6.0 we added full NU5 support to `getblocktemplate`, by adding a `defaultroots` field that gave default values for `hashBlockCommitments` and the components required to derive it. However, in doing so we introduced a regression in the (now-deprecated) legacy fields, where prior to NU5 activation they contained nonsense. This commit fixes the output of `getblocktemplate` to have the intended semantics for all fields: - The `blockcommitmentshash` and `authdataroot` fields in `defaultroots` are now omitted from block templates for heights before NU5 activation. - The legacy fields now always contain the default value to be placed into the block header (regaining their previous semantics). Co-authored-by: Larry Ruane <larry@z.cash>

…ression-fix rpc: Fix regression in getblocktemplate output

Release v4.6.0-1

Hotfix v4.6.0-1

…ified_addresses

daira

utACK fe777c5

str4d

utACK. I verified that the diff between this PR and #5419 is just the last commit.

str4d · 2022-01-06T16:56:57Z

Cargo.toml

-zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "5622b060b1f57de7afc3d0b4e425b9b4b22482a0" }
-zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "5622b060b1f57de7afc3d0b4e425b9b4b22482a0" }
-zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "5622b060b1f57de7afc3d0b4e425b9b4b22482a0" }
-zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "5622b060b1f57de7afc3d0b4e425b9b4b22482a0" }
-zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "5622b060b1f57de7afc3d0b4e425b9b4b22482a0" }
+zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" }
+zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" }
+zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" }
+zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" }
+zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "7801dddf35ca247345cf4f5c5e48791297cad531" }


I checked that the change this corresponds to is just the introduction of the ZIP 316 changes.

str4d · 2022-01-06T17:04:02Z

src/rust/src/address_ffi.rs

    };

-    let ua = match unified::Address::try_from_items_preserving_order(receivers) {
+    let ua = match unified::Address::try_from_items(receivers) {


This change keeps unit tests passing, but only because in #5455 the merge conflict with master was resolved in a way that discarded a bugfix (to the previous ZIP 316 implementation). The problem now is:

On the C++ side, UnifiedAddress stores its internals in the exact order provided by UnifiedAddress::AddReceiver.

This API now resorts the receivers into canonical order (as we want for ZIP 316).

On subsequent parsing (where the merge conflict was), we are resorting from canonical order to preference order.

This is only round-trip compatible if UnifiedAddress::AddReceiver is always called in preference order. It just so happens that we do so, because in the previous ZIP 316 version we intended zcashd to produce UAs encoded in preference order, but that's insufficient to ensure API correctness.

What we need to do is refactor the C++ UnifiedAddress so that it no longer depends on any particular internal order, which the new canonical ZIP 316 encoding allows:

Known receiver types should be stored separately in std::optionals, while all unknown receivers remain in the std::vec.

After this, the iteration APIs can be completely removed, because there's no longer any need to search for the known receivers within the preserved encoding order.

This greatly simplifies the API of UnifiedAddress, to the point that we might just be able to make it a wrapper around a Rust type (like has been done for UFVK etc).

None of this needs to happen in this PR, but the first point MUST be completed before we merge the feature branch.

Opened #5456 to track this.

str4d · 2022-01-06T17:17:46Z

src/test/data/unified_addrs.json

@@ -1,14 +1,14 @@
 [


I verified these changes matched the test vectors generated by zcash/zcash-test-vectors@61894e7.

str4d · 2022-01-06T17:17:52Z

src/test/data/unified_full_viewing_keys.json

@@ -1,14 +1,14 @@
 [
    ["From https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/unified_full_viewing_keys.py"],


I verified these changes matched the test vectors generated by zcash/zcash-test-vectors@61894e7.

softminus and others added 30 commits November 18, 2021 10:39

fix typo in docker run's volume argument

2d25e08

Merge pull request zcash#5389 from superbaud/docker-volume-readme-patch

7aaefab

Correct typo in docker run's volume argument.

cargo update

5ae516f

ed25519-zebra 3

6c0bd90

cargo update

df0534e

Thanks to metrics-rs/metrics#231 being merged, our dependency tree is now almost entirely de-duplicated!

Don't log 'ERROR: spent index not enabled'

d0e836a

Batch-verify Orchard transactions at the block level.

99b2098

Fixes zcash#5316

cargo update

e257844

In the three days since the last update, `futures 0.3.18` was yanked due to panics: rust-lang/futures-rs#2529

Merge pull request zcash#5423 from nuttycom/fix/orchard_batch_verifydb

fc1a16f

Batch-verify Orchard transactions at the block level.

depends: Update Boost to 1.78.0

0d310a7

depends Update Rust to 1.57.0

c37a69a

Also adds a developer script to make updating the hashes easier.

qa: Postpone recent CCache releases

407a0d9

Merge pull request zcash#5422 from LarryRuane/2021-12-getspentindex-l…

ceeede3

…ogging Dont log ERROR: spent index not enabled

Merge pull request zcash#5424 from str4d/update-deps-4.6.0

6a57389

Update dependencies for 4.6.0

Revert "lint: Fix false positive"

09e0af8

This reverts commit fb38cf0. The lint fix caused a problem on macOS, where the escaped double quote was interpreted as part of an argument, and not as defining an argument. We will need to find another way to address the lint. Closes zcash#5379.

Merge pull request zcash#5403 from LarryRuane/2021-12-nuparams

34a8677

test: automatically add missing nuparams (network upgrade heights)

Merge pull request zcash#5427 from str4d/5321-remove-misleading-log-m…

5c2d915

…essage rust: Remove misleading log message

getblocktemplate: add NU5 commitments to new defaultroots section

c937ba5

- merkleroot - authdataroot - chainhistoryroot - blockcommitmentshash

Merge pull request zcash#5398 from str4d/ed25519-zebra-3

9e4b6f6

`ed25519-zebra` 3

Merge pull request zcash#5404 from LarryRuane/2021-12-05-getmininginfo

72aad89

[rpc] mining: Omit uninitialized currentblockweight, currentblocktx

Merge pull request zcash#5402 from LarryRuane/2021-12-getblocktemplate

8b093a7

Add defaultroots field to getblocktemplate

Merge pull request zcash#5426 from str4d/5379-revert-lint-fix

eb175dd

Revert lint: Fix false positive

Migrate to latest revisions of Zcash Rust crates

d85bfb4

Update release notes

8387aa4

Merge pull request zcash#5429 from str4d/pre-4.6.0-rc1-changes

75b5394

Pre-4.6.0-rc1 changes

make-release.py: Versioning changes for 4.6.0-rc1.

d4b850d

make-release.py: Updated manpages for 4.6.0-rc1.

1c910cf

LarryRuane and others added 3 commits January 5, 2022 12:07

test: check getblocktemplate output before and after NU5

dfefab2

This test currently fails with submitblock returning the error "bad-heartwood-root-in-block". Added authdigest to GBT coinbasetxn field because we can't obtain this via getrawtransaction. Co-authored-by: Jack Grigg <jack@z.cash>

test: Fix ZIP 244 implementation

97a76f1

Script codes must be serialized in their field encoding, which includes the CompactSize length prefix. Co-authored-by: Jack Grigg <jack@z.cash>

nuttycom added this to the Core Sprint 2021-52 milestone Jan 5, 2022

nuttycom added the S-committed Status: Planned work in a sprint label Jan 5, 2022

nuttycom self-assigned this Jan 5, 2022

nuttycom force-pushed the feature/wallet_unified_addresses-fix_receiver_order branch from a167abc to 0245bab Compare January 5, 2022 16:59

str4d and others added 9 commits January 5, 2022 20:07

Merge pull request zcash#5451 from zcash/2022-01-getblocktemplate-reg…

7b820cf

…ression-fix rpc: Fix regression in getblocktemplate output

make-release.py: Versioning changes for 4.6.0-1.

5bf937b

make-release.py: Updated manpages for 4.6.0-1.

b97133d

make-release.py: Updated release notes and changelog for 4.6.0-1.

cf22fa7

Add release notes for v4.6.0-1

daef004

Merge pull request zcash#5452 from str4d/release-v4.6.0-1

fe7b87a

Release v4.6.0-1

Merge pull request zcash#5454 from zcash/hotfix-v4.6.0-1

0403799

Hotfix v4.6.0-1

Merge remote-tracking branch 'upstream/master' into feature/wallet_un…

98967e2

…ified_addresses

Fix encoding order of unified addresses.

fe777c5

nuttycom force-pushed the feature/wallet_unified_addresses-fix_receiver_order branch from 0245bab to fe777c5 Compare January 6, 2022 01:33

str4d mentioned this pull request Jan 6, 2022

Update feature/wallet_unified_addresses with latest master #5455

Merged

nuttycom requested review from daira and str4d January 6, 2022 16:24

daira approved these changes Jan 6, 2022

View reviewed changes

str4d approved these changes Jan 6, 2022

View reviewed changes

str4d added NU5 Network upgrade: NU5-specific tasks A-rust-ffi Area: The Rust FFI in the librustzcash library. labels Jan 6, 2022

str4d mentioned this pull request Jan 6, 2022

Refactor UnifiedAddress class to not assume preference order internally #5456

Open

nuttycom removed their assignment Jan 6, 2022

nuttycom merged commit 89755a1 into zcash:feature/wallet_unified_addresses Jan 6, 2022

nuttycom deleted the feature/wallet_unified_addresses-fix_receiver_order branch January 6, 2022 19:17

nuttycom mentioned this pull request Jan 6, 2022

Add Unified Address key generation and storage to the wallet #5179

Closed

str4d mentioned this pull request Feb 14, 2022

Add Unified Address support to the zcashd wallet #5419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make address serialization respect encoding order. #5450

Make address serialization respect encoding order. #5450

nuttycom commented Jan 4, 2022

Uh oh!

daira left a comment

Uh oh!

str4d left a comment

Uh oh!

str4d Jan 6, 2022

Uh oh!

str4d Jan 6, 2022

Uh oh!

str4d Jan 6, 2022

Uh oh!

str4d Jan 6, 2022

Uh oh!

str4d Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -1,14 +1,14 @@
		[
		["From https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/unified_full_viewing_keys.py"],

Make address serialization respect encoding order. #5450

Make address serialization respect encoding order. #5450

Conversation

nuttycom commented Jan 4, 2022

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

str4d Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

str4d Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

str4d Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

str4d Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

str4d Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants