Add orchard bundle creation to `zcash_primitives::transaction::builder::Builder`

[AloeareV]

I've marked this as draft, due to an issue with the selection of the orchard anchor. In Sapling, the anchor is only used in the creation of the spends. For a bundle with only dummy spends, no anchor is needed, so the anchor field of the SaplingBuilder is an optional value derived from the first spend added to the builder.

In Orchard, the anchor is encoded into the bundle. With the code as I've currently written it, the anchor is derived from the first spend added to the orchard builder, with the orchard builder being an optional value initialized the first time a spend is added. This will not work in cases where all spends are dummies. Some possible solutions I would appreciate feedback on:

1. Add an anchor parameter (or orchard_builder parameter) to transaction::builder::Builder::new(). This would be a breaking change to the api.
2. Add a with_orchard function to transaction builder, which will enable the builder to create orchard bundles. This is likely the most feasible solution, but I'm not sold on it yet, as it's a potentially confusing interface.
3. Add a default value for the anchor in instances where no spends are added. This would be possibly the cleanest interface, but I don't know what the anchor is used for in the case where all spends are dummies, so I don't know that it's possible to create a default value here that will work for all use-cases. This also doesn't allow for orchard builder flags to be provided.
4. Add a special case function for building a transaction with orchard actions containing only dummy spends.

Also a draft because I should probably add a test or two for this new functionality, there's probably a changelog that needs to be updated, and I haven't added the orchard_builder.build call to the transaction build yet.

[codecov[bot]]

Codecov Report

Patch coverage has no change and project coverage change: -20.48 :warning:

Comparison is base (d37e6ad) 71.60% compared to head (68b84c8) 51.12%.

:exclamation: Current head 68b84c8 differs from pull request most recent head 426a64e. Consider uploading reports for the commit 426a64e to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #583       +/-   ##
===========================================
- Coverage   71.60%   51.12%   -20.48%     
===========================================
  Files         125       94       -31     
  Lines       11843     8858     -2985     
===========================================
- Hits         8480     4529     -3951     
- Misses       3363     4329      +966     

Impacted Files	Coverage Δ
zcash_primitives/src/transaction/builder.rs	46.66% <0.00%> (-28.18%)	:arrow_down:

... and 128 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[str4d]

I had started working on this change (which is #406) a month and a half ago, but had to put it aside while doing other NU5 work, and forgot to open a draft PR (which I've now done: #584). The complexity was indeed in figuring out the areas you have run into 😄

Add a with_orchard function to transaction builder, which will enable the builder to create orchard bundles. This is likely the most feasible solution, but I'm not sold on it yet, as it's a potentially confusing interface.

This is the direction I was taking, specifically to add a with_orchard_anchor API.

[AloeareV]

Great! If I were to do some work atop that, would you prefer I open a PR into your feature, or push it to here? Pushing here for now.

[AloeareV]

I've marked this as ready for review. It needs tests added, but I'm finding myself at a bit of a loss on how to test this in a meaningful way, as adding an orchard spend needs a note, and currently (I've opened this PR which might also help here) there seems to be no way to get a note aside from decrypting one, and that requires creating one. All the tests are currently using mock tx provers, which can't actually bind signatures.

So do I need to either access sapling parameters, or feature-gate behind transparent inputs, in order to create a transparent-to-orchard or sapling-to-orchard transaction, in order to have a spendable orchard note?

[AloeareV]

Zingo is currently relying on a cargo patch in order to use this builder (it's also patching librustzcash to use https://github.com/zcash/orchard/pull/344). This means all downstream libraries need to copy our patch into their Cargo.tomls in order to use the library. Is there something more I can/should be doing to get this merged in? Is this blocked on https://github.com/zcash/orchard/pull/344 (in order to make some mock notes for testing)?

Edit: I don't think this is ready to merge. Right now it doesn't account for the orchard value balance, and so can give false negative-change errors, for example. Once https://github.com/zcash/orchard/pull/352 lands, I can add that and revisit.

[AloeareV]

Re-marked as draft, as this is blocked on zcash/orchard#352

[AloeareV]

Some of my code is squashed into str4d's initial commit, as doing so made rebasing a lot easier.

[AloeareV]

Alright! Librustzcash has moved far enough since I opened this that a rebase proved really difficult, so I reimplemented instead.

Ideally, we should get the number of orchard actions from the orchard builder, instead of the builder keeping its own count of inputs and outputs. This (and the later work of adding orchard to change handling in zcash_client_backend) will require adding access to spends and outputs of the orchard builder (and for change, note of SpendInfo and value of RecipientInfo).

[AloeareV]

There is also a change to test_only_new_with_rng so that it returns an existential type Builder<'a, P, impl RngCore + CryptoRng> instead of a Builder<'a, P, R>. This allows new_internal to require a CryptoRng bound, which is faked by the test fn. This is technically a breaking change to a public interface. It makes some things easier but isn't strictly necessary for the rest of the changes, so it could be reverted if it needs to be, but it makes calls to new_internal safer.

[AloeareV]

Testing this locally with zingolib (branch https://github.com/zingolabs/zingolib/pull/185), I'm getting error_code: -26, error_message: \"16: bad-sapling-bundle-authorization\". I haven't touched the sapling authorization code. Have I done something wrong, or is this a bug that exists on main? I can't trivially test that, as it would involve digging up a version of zingolib that can work with a transaction builder that doesn't support orchard.

I plan to test that, but I won't get a change until monday.

Edit: I don't seem to get that error with librustzcash main. I haven't touched the sapling code at all, so I'm not sure why it's happening.

[AloeareV]

@str4d, @daira, @sellout and I had a look at this in PR gardening and determined that the Orchard integration process would be more straightforward if we first changed the Sapling builder to match the builder pattern that we introduced for the orchard crate, and then updated the existing transaction builder to make use of this without introducing the Orchard builder, at which point it should be easier for you to handle the Orchard builder integration.

Sounds good! I'll see if I can contrib towards this in the next few days.

[AloeareV]

I pushed to this last week, there are still a couple changes suggested that I need to get to before this should be merged (plus I should probably get it so it passes CI), but as far as I can tell it's once again functional. It turns out the unauthed_tx needed the orchard bundle.

[AloeareV]

@nuttycom Alright, I've gotten this rebased and working, and addressed the review comments!

[nuttycom]

@AloeareV just to give you a heads-up, we likely won't get to a full review of this for a few days because the team is trying to get zcashd 5.5.0 out the door.

[zancas]

WOOOT! On the 5.5.0 release!!!

[AloeareV]

@nuttycom Pinging you now that 5.5.0 is out, it'd be great to get zingo depending on upstream librustzcash

[zancas]

Hi! Thanks in advance!! I notice we're approaching our 1 year anniversary on this PR. zinglib has to patch because this is not landed. We have had to rebase our feature several times over the months.

[AloeareV]

@nuttycom @str4d

[zancas]

Do we need to do more work to update this? I am concerned that it may have been superseded, again.

[str4d]

It was pushed back by the necessary 5.6.0 timeline. Now that's out, this is near the top of my review list.

[daira]

@AloeareV The first comment in a PR goes into the merge commit, so it should reflect the current state of the PR. Can you update it please?

[nuttycom]

Closing in favor of #863