Skip to content

Sync zcash v5.2.0 fixes - #165

Merged
dmidem merged 8 commits into
sync-zcash-v5.2.0from
sync-zcash-v5.2.0-fixes
Sep 4, 2026
Merged

Sync zcash v5.2.0 fixes#165
dmidem merged 8 commits into
sync-zcash-v5.2.0from
sync-zcash-v5.2.0-fixes

Conversation

@PaulLaux

@PaulLaux PaulLaux commented Sep 1, 2026

Copy link
Copy Markdown

Upstream v5.2.0 added a circuit-era axis for Orchard verifier selection (pre/post-NU6.2, GHSA-jfw5-j458-pfv6), keyed on the block's network upgrade. The fork already had an independent flavor axis (OrchardVanilla/OrchardZSA), keyed on the bundle. The merge kept only the era axis, as Nu7 => &VERIFIER_ZSA.

V5 transactions stay valid at NU7 and carry vanilla bundles, so they were verified against the OrchardZSA key, which cannot verify a vanilla proof. Every NU7 block containing a V5 Orchard transaction would have been rejected. Both zcash_unstable="nu7" and feature="tx_v6" are on by default via .cargo/config.toml, so the default build was affected.

  • fix(consensus): verify NU7 vanilla Orchard bundles with the vanilla key. Routes on the bundle again, and restores verifier_for to upstream's era-only mapping so it merges cleanly next time.
  • test(consensus): guard vanilla Orchard routing at NU7. The only vanilla fixtures are pre-NU6.2 proofs, which both the right and the wrong key reject, so the test asserts the verifier choice rather than the outcome. Making that observable moved the flavor match into verifier_for_bundle, composing flavor over verifier_for. Verified non-vacuous: reintroducing Nu7 => &VERIFIER_ZSA fails this test and the pre-existing routing test.
  • fix(deps): allow-list every patched git source for cargo-deny. Three git sources in Cargo.lock were missing from allow-git, which cargo deny check sources denies.
  • fix(chain): require an NU7 branch ID in V6 transactions. The V6 arm reused the V5 guard verbatim, rejecting only pre-NU5 branch IDs for a version valid from NU7. verify_v6_transaction_network_upgrade already rejects everything before NU7, so nothing invalid was accepted; the parse-time floor just did not match the rule it was copied to enforce.
  • fix(chain): count distinct Assets in the ZIP-317 issuance fee. nReferenceNotes stands in for nAssetCreations, which the transaction alone cannot give. Counting Issue Actions overcounts when two actions carry a reference note for the same Asset, since an Asset is created at most once. Distinct Asset bases is still a floor, so the fee cannot be underpaid, but a tighter one.
  • build(deps): restore upstream's incrementalmerkletree 0.8.2 requirement. Both resolve to 0.8.2 in Cargo.lock, so this is drift, not a behaviour change.
  • ci: restore retries for sync_large_checkpoints_mempool_mainnet, then park the block — the nextest rework dropped this #[ignore]d test when retry overrides moved to profile.all-tests; it is also grouped with the other live-Mainnet sync tests so they stop competing for peers. The sync-zcash-v5.2.0 merge then commented out the whole acceptance-scheduling block, so the override is preserved but inert — tests-unit.yml runs those tests unretried and unserialized until it is re-enabled.

Known gaps, neither closed here:

  • A true acceptance test for a V5 Orchard transaction at NU7 needs a post-NU6.2 vanilla proof. No fixture exists and there is no proof-building helper in the workspace, so it is worth requesting with the next ZSA workflow block regeneration.
  • The V6 branch-ID guard's rejection path is untested; no fixture carries a pre-NU7 branch ID, so it would need hand-crafted bytes.

`deny.toml` has `unknown-git = "deny"` and `lint.yml` runs
`cargo deny check sources`, so every git source in `Cargo.lock` must appear in
`allow-git`. Three did not:

- `QED-it/halo2`, newly introduced by the v5.2.0 merge (halo2_gadgets,
  halo2_proofs, halo2_poseidon), replacing the `zcash/halo2` entry;
- `zcash/sinsemilla` and `zcash/zcash_note_encryption`, both still in the patch
  table but dropped from the list while resolving the merge conflict.

Also drop the now-unused `zcash/librustzcash.git` entry: the patch table points
every librustzcash crate at `QED-it/librustzcash`.
The v5.2.0 merge kept the fork's older `0.8.1`. Both resolve to 0.8.2 in
`Cargo.lock`, so this is drift, not a behaviour change.
The nextest rework dropped this test when moving the retry overrides from
`profile.default` to `profile.all-tests`. It is `#[ignore]`d, so only the
`--run-ignored=all` run is affected. Also grouped with the other live-Mainnet
sync tests so they don't compete for peers.
The v5.2.0 merge collapsed two independent axes of verifier selection into one:
upstream's circuit era (pre/post-NU6.2, GHSA-jfw5-j458-pfv6), keyed on the
network upgrade, and the fork's flavor, keyed on the bundle. Only the era
survived, as `Nu7 => &VERIFIER_ZSA`.

V5 transactions stay valid at NU7 and carry vanilla bundles, so they were
verified against the OrchardZSA key, which cannot verify a vanilla proof: any
NU7 block with a V5 Orchard transaction would be rejected. Both cfg flags are
on by default, so the default build was affected.

Route on the bundle at the call site, as before the merge, and restore
`verifier_for` to upstream's era-only mapping. The routing test asserted the
NU7-to-ZSA mapping rather than catching it, so it is fixed too.
Add a regression test for the previous commit. Both the correct and the
incorrect key reject the only vanilla fixture available — a pre-NU6.2 proof from
the mainnet test blocks — so the outcome cannot distinguish them and the test
asserts the verifier choice instead.

To make that choice observable, move the flavor match out of the call site into
`verifier_for_bundle`, which composes flavor over `verifier_for`. This also
leaves `verifier_for` identical to upstream, so it merges cleanly next time.

Verified by reintroducing `Nu7 => &VERIFIER_ZSA`: the new test and the existing
routing test both fail.

The ZSA direction stays covered end to end by `check_orchard_zsa_workflow`. A
true acceptance test for a V5 Orchard transaction at NU7 still needs a
post-NU6.2 vanilla fixture, which the test vectors do not have.
`nReferenceNotes` stands in for `nAssetCreations`, which cannot be computed from
the transaction alone. Counting Issue Actions overcounts whenever two actions in
one transaction carry a reference note for the same Asset: an Asset is created at
most once, so those cannot each imply a creation.

Count distinct Asset bases instead. Still a floor on `nAssetCreations`, so the
fee cannot be underpaid, but a tighter one.
The V6 arm reused the V5 guard verbatim during the v5.2.0 merge, so it rejected
only pre-NU5 branch IDs while claiming to reject pre-NU5 ones for a version that
is only valid from NU7. `verify_v6_transaction_network_upgrade` already rejects
everything before NU7, so nothing invalid was accepted, but the parse-time floor
did not match the rule it was copied to enforce.

`NetworkUpgrade` derives `Ord` over variants declared in activation-height order,
so `< Nu7` admits exactly NU7 and later.
@PaulLaux
PaulLaux marked this pull request as ready for review September 1, 2026 08:43
@dmidem

dmidem commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

@PaulLaux, thanks for going through #155 and sending these fixes. Let me group them and comment on each group: the problem you found, and my opinion.

1. halo2.rs — wrong verifier choice at NU7

Problem you found: the verifier was chosen by network upgrade only, so for NU7 it was always VERIFIER_ZSA. This is wrong: after NU7 activation V5 transactions are still valid, they carry vanilla Orchard bundles, and those must be verified with VERIFIER_POST_NU6_2. So the verifier must be chosen by the Orchard bundle type as well, not by the network upgrade alone.

My opinion: agree with your fix. This is my mistake in the v5.2.0 merge.

2. serialize.rs — V6 was checked against the NU5 branch ID

Problem you found: the deserializer rejected a V6 transaction when the branch ID in the transaction bytes mapped to an upgrade below NU5. It should be checked against NU7, not NU5.

My opinion: agree with your fix. This came from upstream v5.2.0: ZcashFoundation#10426 added the V5 and V6 checks in one commit and copied the V5 condition into the V6 arm. It caused no merge conflict, so I never looked at that code. Upstream main no longer has this code at all — they deserialize transactions with librustzcash now — so there is nothing to report upstream.

3. zip317/zsa.rs — count reference notes per distinct asset

Problem you found: we decided to charge CREATION_COST per issue action that carries a reference note, instead of per newly created asset (at least for now). If a bundle has several actions with a reference note for the same asset, the current code charges each of them, so the same asset is paid for several times. This is an artificial case — there is no practical reason to repeat a reference note for the same asset in one bundle — but it is legal. Your change charges it once per asset.

My opinion: agree with your change. In principle both rules can be explained, but charging once per distinct asset is better. One remark: reference notes for the same asset in different transactions will still be charged separately, and that cannot be avoided at transaction level. So if we propose the ZIP change from "new asset" to "reference note", the wording has to say this explicitly: inside one bundle the cost is charged once per asset, between transactions the uniqueness rule does not apply. Could you also add a unit test for logical_actions with two issue actions for the same asset? There are no tests for this function yet.

4. deny.toml and incrementalmerkletree 0.8.2

Problem you found: the allow-git list did not match Cargo.lock, and our incrementalmerkletree requirement was behind upstream.

My opinion: agree with your changes.

5. .config/nextest.toml — the QED-it acceptance-test block removed

Problem you found: the acceptance-test ignoring block added by QED-it has been removed in #$155 (i.e. the block we added for fine-tuning acceptance tests). So you seggest to restore it commented out.

My opinion: I would leave this block out. I removed it in #155 after Ironwood activated on mainnet and testnet: more and more acceptance tests failed, I could not keep up with fixing them one by one, so I excluded the whole acceptance binary in ci-basic.yml. After that the block does nothing, and upstream does not have it. ci-basic.yml now says why the tests are skipped and that we come back to this after the Ironwood sync. Your sync_large_checkpoints_mempool_mainnet override goes away with the block, but it is commented out and the test is #[ignore]d, so nothing changes in practice — we can restore the whole block from git history later. But it is up to you.


So: I merge the PR, and then will remove the QED-it block from .config/nextest.toml in a separate commit in sync-zcash-v5.2.0, which is easy to revert if you disagree.

@dmidem
dmidem merged commit df9c71b into sync-zcash-v5.2.0 Sep 4, 2026
20 checks passed
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.

2 participants