Use `RandomInvalidOutputDescription()` everywhere it makes sense

As a pre-check inside `z_sendmany` we estimate the size of the
transaction that would be created, to confirm it won't exceed any
limits. We do this by creating a fake transaction with fake outputs and
measuring its size. In the case of Sapling recipients, we'd push an
empty `OutputDescription`.

In zcash/zcash#6459 we pulled in changes that improved type safety in
the Rust types. One of these changes was that the `cv` field in a
Sapling Output Description is now enforced at parsing time to be not
small order (where previously we enforced this at proof verification
time).

The two above paragraphs collide because when measuring the size of the
fake transaction, we convert a `CMutableTransaction` into a
`CTransaction`; this calls `UpdateHash` to pin its txid, and that causes
the transaction to be serialized and then parsed across the FFI. This
causes the null `OutputDescription` to reach the Rust parser which
treats it as invalid.

There are two solutions to this, which are used in various contexts:

- Avoid pushing a null `OutputDescription` into a `CMutableTransaction`.
  This is the fix implemented in this PR for `z_sendmany`: we now call
  `RandomInvalidOutputDescription()` which gives us a consensus-invalid
  but parser-valid `OutputDescription`, suitable for estimating tx size.

- Use `UNSAFE_CTransaction` to avoid having `UpdateHash` be called on
  construction. This type is used in tests where we explicitly want to
  construct an invalid type in C++, for consensus checking purposes. One
  of the `OutputDescription()` uses was in a test, but didn't trigger
  the issue because the test was checking a different part of the
  transaction being invalid. Technically no change is needed here;
  however we now also call `RandomInvalidOutputDescription()` here for
  uniformity.

Part of zcash/zcash#6509.
This commit is contained in:
Jack Grigg 2023-03-27 22:33:45 +00:00
parent 666a8d1b37
commit 4a94975268
2 changed files with 4 additions and 3 deletions

View File

@ -879,7 +879,7 @@ TEST(ChecktransactionTests, SaplingSproutInputSumsTooLarge) {
mtx.vJoinSplit.push_back(jsdesc);
}
mtx.vShieldedSpend.push_back(SpendDescription());
mtx.vShieldedSpend.push_back(RandomInvalidSpendDescription());
mtx.vJoinSplit[0].vpub_new = (MAX_MONEY / 2) + 10;

View File

@ -34,6 +34,7 @@
#include "zcash/Address.hpp"
#include "zcash/address/zip32.h"
#include "util/test.h"
#include "util/time.h"
#include "asyncrpcoperation.h"
#include "asyncrpcqueue.h"
@ -4660,7 +4661,7 @@ size_t EstimateTxSize(
taddrRecipientCount += 1;
},
[&](const libzcash::SaplingPaymentAddress& addr) {
mtx.vShieldedOutput.push_back(OutputDescription());
mtx.vShieldedOutput.push_back(RandomInvalidOutputDescription());
},
[&](const libzcash::SproutPaymentAddress& addr) {
JSDescription jsdesc;
@ -4671,7 +4672,7 @@ size_t EstimateTxSize(
if (addr.GetOrchardReceiver().has_value()) {
orchardRecipientCount += 1;
} else if (addr.GetSaplingReceiver().has_value()) {
mtx.vShieldedOutput.push_back(OutputDescription());
mtx.vShieldedOutput.push_back(RandomInvalidOutputDescription());
} else if (addr.GetP2PKHReceiver().has_value()
|| addr.GetP2SHReceiver().has_value()) {
taddrRecipientCount += 1;