Add NSM transaction test vectors

[tomekpiotrowski]

This adds test data generation for NSM transactions. Simply adds zip233_amount to tx id and sighash calculations.

For now NSM is developed under the ZFuture Network Update and ZFuture transaction version as the final NU and tx versions under which it'll be deployed are not decided yet.

The librustzcash PR that uses the generated data is zcash/librustzcash#1567 .

[giddie]

I've updated this PR following the naming change (ZSF -> NSM).

[aphelionz]

Thanks @mariopil

[aphelionz]

@str4d @daira Can this be merged?

[aphelionz]

@daira or somebody - can I have permissions to run this workflow or can somebody run it?

[nuttycom]

Review not yet complete, but marking with changes requested to prevent inadvertent merge.

[daira]

Looks good modulo comments.

[mariopil]

Thank you @daira for the suggestions, I've updated the PR.

[daira]

This intentionally isn't defined yet because ZIP 230 is not yet stable.

[mariopil]

I've added this to match the versions in librustzcash, ensuring the generated vectors used in the librustzcash tests are correct.

[daira]

utACK, except that the checked-in test vectors need to be updated.

[mariopil]

@daira I've updated zip_0244 test vectors and added zip_0233 test vectors.

[aphelionz]

How does it look now, @nuttycom?

[nuttycom]

How does it look now, @nuttycom?

CI will validate the generated test vectors against those that have been checked in, beyond that I'm fine with @daira's ACK.

[aphelionz]

@mariopil looks like there's a failure

[mariopil]

@mariopil looks like there's a failure

There was a slight change in the TransactionV5 logic after @daira suggestions. It could affect the zip_0244 test vectors.

Problems have been resolved.

[aphelionz]

@mariopil @daira what are the next steps?

[daira]

The fact that the CI is failing indicates that some of the test vectors (the "-t zcash" ones) have not been regenerated.

[mariopil]

Thanks, I've regenerated the "zcash" vectors.

[nuttycom]

This looks like it now needs to be rebased on main following #109

[aphelionz]

Hi @daira may we merge this?

[daira]

Sorry it's taken me a while to get to this. I will try to review it this week.

[oxarbitrage]

Sorry it's taken me a while to get to this. I will try to review it this week.

I have some (minor and optional) suggestions on this PR. It is ok to make a review or do you prefer to review as it is ?

[oxarbitrage]

Optional suggestions and some questions/comments. All minor stuff, feel free to ignore.

[oxarbitrage]

FYI: In #108, the TransactionV5 is renamed to TransactionBase:

zcash-test-vectors/zcash_test_vectors/transaction.py

Line 420 in 3e6f9eb

class TransactionBase(object):

Then TransactionV5(TransactionBase) and TransactionV6(TransactionBase) are defined. I think that makes sense.

Given that this PR is probably going to be merged first, should we apply that change here ?

[daira]

utACK

[daira]

utACK