fixes#6518
In a ZIP sync meeting we decided that:
* The minimum cost should be changed to 10000, in order to avoid
penalizing Orchard-using transactions too much relative to other
transactions.
* `low_fee_penalty` should be changed to 40000. This preserves the
property that a transaction paying less than the ZIP 317 conventional
fee is deprioritized relative to a min-cost, conventional-fee
transaction by a factor of 5, as in the original design.
* The recommended default for `mempooltxcostlimit` should remain at
80000000. Rationale: 80000000 was chosen so that the worst-case size
of the mempool would be equal to the worst-case size of 40 blocks,
which is the current default transaction expiry delta. That reasoning
still holds even with the above changes.
* `eviction_memory_entries` remains at 40000. It could have been lowered
given that there will now be at most 80000000/10000 = 8000 transactions
"in-flight", but it doesn't need to be because the rationale that
"40000 [RecentlyEvicted queue] entries can be stored in ~1.6 MB,
which is small compared to other node memory usage" still holds.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
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.
zcash/zcash#5987 added a bridge to `orchard::Bundle<Authorized, Amount>`
for `getrawtransaction`. This commit expands it to also cover the
consensus rules, by migrating over missing functionality from the
hand-written FFI methods, and exposing the Orchard `BatchValidator` type
directly (as with Sapling) instead of via the C++ `AuthValidator`
intermediary.
Part of zcash/zcash#6397.
In its existing usage, `CppStream` was only used in a context where the
C++ `READWRITE` macro was being called with a `CDataStream`. However, in
other contexts the macro can be called with various other types with a
stream-like interface. Since we can't expose C++ templates across the
`cxx` bridge (or FFI generally), we instead turn `CppStream` into an
enum that covers all of the stream-like types we may want to use.
`std::visit(match {...}, specimen)`, improving code readability.
For ease of review, this commit includes only obviously correct
transformations that all follow the same pattern.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
InsertBlockIndex should take a const reference to a uint256 instead of
just a uint256.
LoadBlockIndexGuts also assume std::function<CBlockIndex*(const uint256&)> insertBlockIndex
as an argument:
2d456afebe/src/txdb.h (L150)
This fixes an RPC test failure that tests specifically for this with
z_shieldcoinbase. This also exposed an issue where an overly-high fee resulted
in a negative Payment causing an exception too deep. Added an assert when
creating a Payment and guarded against it for z_shieldcoinbase.
Fixes#2621 and #5654 (but does not handle Orchard locking, which is tracked in
a separate issue).
Extracted from z_sendmany to be used across multiple transaction
operations. Previously, it also checked a `bool` to decide what “LegacyCompat”
means, but bools are too easy to concoct, so it now expects the sender and
recipients to be checked internally. Also, z_sendmany was generating the bool
only from the sender, illustrating how easy it is to miss something when you try
to precompute.
Do the check deeper, preventing test_bitcoin from being able to bypass it. This
also moves it out of z_sendmany-specific code, which will be helpful when we add
other operations, like sendfromaccount.
This resolves a conflict where most usage is `const`, but some modifies the
wallet. Previously it held a const member and then used `pwalletMain` directly
for the mutating calls. This now passes `CWallet` explicitly where necessary,
using `const` when possible.
This also benefits a follow-up PR (#6408) that introduces locking, which also
mutates the wallet.
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 inverts the structure, dispatching on the selector pattern first, then only checking
`SpendableInputs` if the selector is multi-pool. This shortens the code, eliminates some failure
cases, and caught a bug where `SaplingExtendedFullViewingKey` selectors were not
supported (previously hidden by `match { ..., [](const auto&) … }`).
This also removes the code that stops adding spends if they ever go
`>= targetAmount`. The included note limiting and change calculation should
ensure that it’s always `==` at the end, and we don’t want paper over a mistake
in those earlier calculations.
There are existing tests that fail if either
- the newly-added Orchard increment is missing or
- the assertion is applied when there’s Sprout change.
The `Payment` type had an `isInternal` field, but it is (and should always be) `false`.
`ResolvedPayment` is the corresponding type internal to the transaction builder that can be either
internal (for change) or external (for user-requested payments).
Some of the more significant changes are
- remove release note entry for already-released feature;
- rephrase some error messages and comments;
- add a missing case to `EstimateTxSize`;
- don’t return a selector when we don’t have a UFVK for a UA, which allows some
simplifications (and elimination of a failure case) to happen; and
- remove a redundant `InsufficientFundsError`.
`CWalletTx::DecryptSaplingNote` and `CWalletTx::RecoverSaplingNote` were
previously unused, and the variants that skipped the plaintext version
check were only used by `z_viewtransaction` to inspect details about
transactions that had already been added to the wallet.
We now merge the two back together, and use the activation height of the
Canopy NU as the checked height. This re-checks the plaintext version in
`z_viewtransaction` but permits either valid version.
The `zcashd` impl of `consensus::Parameters` is moved into a new
`params` module. It still uses the `cxx::bridge` in `wallet_scanner`
because `cxx` doesn't support Rust type aliases yet.
This was breaking with `--disable-wallet`, likely due to some PR
removing a `using namespace std;` line from a header file that this code
was implicitly depending on.
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
-----BEGIN PGP SIGNATURE-----
iQGzBAABCgAdFiEEX8Nd8pnYcf0pobEL9FXpuSAjoYsFAmP1DKQACgkQ9FXpuSAj
oYt2jQv+IurleLqEbtBW2ajDYDIkcMu+asEtHCk3B6GELykKaFMgHYTffFBmuyJ+
5GdoX685Rn6r8BxK6K1u129A9ztY7K0JpVeDA75tN5WLBj9twLdNaODfCLg5EjlZ
UtxTNnynQ0MX5Uv7pt1DAM+++OKYujUHypaajgZ9ttqpSHZl3Z2ye0/HFEc4023p
VH13CvU/3R4JLkFENi49rbS49LFfVuQrhAQoOPlCf3xoWbUYIdmWWZa/HOJV3g0e
3mqC+rhz97GVylLI4LJrm3v0tLeEUIuu+fdAziWfuWrBlB4jQ5p4L5trDHiQoqWB
5Qt5tjJKHHLnHHSyLcFFaes12tjPfrn9PBxYDPyfFAHIGf0WEiy36+6G5P7jpjYj
OXLAmBPBIRBgZf9LJIHrgvqQynfGe9vaWgCArWWgSC8wZ2hWlM7pZRhCe+uw35R1
e5AfZjvbXj6gR+1631Mhl84e6xkGXe24szzuZDrUCqLBJpTb1JNLRh1OtT8zDN0t
MfKtnE3W
=AXAQ
-----END PGP SIGNATURE-----
Merge tag 'v5.4.2' into hotfix-v5.4.2
zcashd release 5.4.2
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
-----BEGIN PGP SIGNATURE-----
iQGzBAABCgAdFiEEX8Nd8pnYcf0pobEL9FXpuSAjoYsFAmP1CD8ACgkQ9FXpuSAj
oYvGXQwAqaa1l5qC3VY/60jkH4xmWoSUEzeCOPUG7lYJrIEyzgXj8Ko0Cjr308jm
ISXDDTOxKb2hfnCbeqbZqRyFbzGzG5L6AkjHAvmQMiZwx2JlbH2k+jd5fggOZSMv
shL5KbxWN2YRftc8r+fDraJMbGULbKBWQooKaFyQmupT+bVsRf1Nh+lFIVG4FUwA
oWZot36wB6Y99Y57wlyN2m22+j1glyk5mKv2ttXYbdwSRTFB5W5L1US6Z8uxXQPb
Qa7sVO90QtzkHr+GPtMiTn513VuLFr+KArGn+qidU/PvblJI/vXuBf54g7JZw4Ot
gkKziatgaN3pO30I4rTij78LCgKJZ/WImLE7nwTl/bG2Ki1WfyolNLjS+1pXpcPL
xlXso5ioKlSIGhnlPouXwoxlaqTpDwRKLp0azNJl5hG/tXEHupToK2M61woi9LlP
4RB+a75OptFi9NMp/Sx0T8zNRn7OB8iP+3BgRP0+mzcLC1AHfjJk/IRo9CQh5jRZ
MMe98OrC
=6QYn
-----END PGP SIGNATURE-----
Merge tag 'v5.3.3' into hotfix-v5.4.2
zcashd release v5.3.3
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
Greg Pfeil
Daira Emma Hopwood
Kris Nuttycombe
Jack Grigg
str4d
ANISH M
DeckerSU
Greg Pfeil
Dimitris Apostolou
Greg Pfeil
str4d
Evan Klitzke