When a `zcash-cli` command fails, it attempts to print the help message for the command. However,
making the `help` call can also fail, and there was a bug in this check, so that we tried to display
the help message when the `help` call failed, and tried to display the error when the `help` call
succeeded – both leading to an assertion failure.
This also makes some minor changes to the output formatting.
Fixes#6561
Some of these are newly-introduced names in this PR, others are ones that I had
renamed from param -> arg in an earlier commit.
Also restores `strMethod` where it had been changed to `method`.
Without `AllowRevealedRecipients`, we can’t send transparent change, but previously we asserted if
we couldn’t get a transparent change address. Now it returns a new `TransparentChangeNotAllowed`
failure, which is just a more specific `TransparentRecipientNotAllowed` to avoid confusion when
there are no explicit unshielded recipients.
The other changes in this PR push more errors to the client side, where error messages have been
minimal. Also if a request made it to the server _then_ errored, it would return the help text, but
not specific failure information.
Now, client-side errors make a help RPC call for the method, resulting in a message with both the
specific failure plus the help text, whereas previously we only ever received one or the other.
Instead of storing the indices of args to convert from string, store two
`vector<bool>` (per operation), the first containing an entry for each required
parameter (`true` if we should convert it), and the second containing an entry
for each optional parameter.
This allows us to check a few more things on the client side:
- does the operation exist
- have enough arguments been passed
- have too many arguments been passed
This is ostensibly a fix for `zcash-cli` to be able to use `asOfHeight` where
available, but it also caught a few bugs in the old implementation:
- `submitblock` didn’t convert its optional (but ignored) second arg;
- `z_getpaymentdisclosure` docs claimed all the args were strings, but two are
actually ints;`
- `listreceivedbyaddress` didn’t convert the optional `includeImmatureCodebase`;
- `listsinceblock` didn’t convert the optional `includeRemoved` and
`includeChange`;`
- `gettransaction` didn’t convert `verbose`;
- `listunspent` didn’t convert `includeUnsafe` or `queryOptions`;
- `z_getbalanceforviewingkey` didn’t convert minconf; and
- a minor non-bug – `z_getbalanceforaddress` had a handler even though the
operation has been removed.
`getblockdeltas` also incorrectly tries to convert its required string argument,
but correcting that would be a breaking API change. Instead, it is deferred to
Fixes#6429.
There was a lot of indirection to build a `set<pair<string, int>>` that should
have been a `map<string, set<int>>` from a `[pair<string, int>]`, when it could
just be initialized directly.
This now determines the meaning of “LegacyCompat” in advance, then always has a
known `TransactionStrategy` when we create the `ZTXOSelector`. With the addition
of `TransparentCoinbasePolicy`, there were two different ways that the selector
depended on the strategy, so it became more complicated to manage the cycle. And
the coinbase policy was overly conservative when there were no transparent
recipients or UAs in the tx. So this resolves that as well.
Fixes#6541.
- still support explicit fixed fees everywhere – if a caller provides a fee, we
don’t adjust it
- treat negative fees as signifier for use of ZIP 317 fees when a fee needs to
be provided because of additional positional arguments
When deciding whether we can pay Orchard receivers, rather than checking
whether we have “sufficient non-Sprout funds”, we can just check whether the
selector selects Sprout – if it doesn’t then we can select Orchard receivers
regardless, and we’ll catch insufficient funds later.
This is also helpful for ZIP 317, because in that case we don’t even know the
total non-Sprout funds necessary until after we decide which receivers to pay.