In `ScanForWalletTransactions` we skip forward from `pindexStart` to a block
near our wallet birthday. But we use this as the basis for deciding the value
of `performOrchardWalletUpdates` later, which is wrong because our wallet
birthday might be beyond the NU5 activation height. The result is that we
won't actually perform the required scanning (mainly witnessing and so forth)
needed to spend notes in our wallet.
If we no longer have any checkpoints in the Orchard wallet, we must
skip the check against the prior note commitment tree root because
we know that the Orchard wallet's state may be for a chain that is
being reorg'ed away. This is safe because we know that we will have
removed all information from the wallet that we need to perform spends
from that state, and we also know that when we start rolling forward
along the new chain that we will overwrite the initial state of the
Orchard note commitment tree.
When initializing a new Orchard wallet after NU5 activation, it is
not valid to start from the empty note commitment tree; instead,
the note commitment tree needs to be initialized from the state of
the global Orchard Merkle frontier.
In addition, this change necessitates a change to how rewinds work,
such that in a rollback scenario with a newly initialized wallet
that does not have sufficient checkpoints to fully satisfy a requested
rewind, the rewind is allowed to proceed so long as it does not
invalidate any persisted witness data.
If a new Orchard wallet is created after the first Orchard spend
post NU5 activation, it causes an assertion failure because the root
of the wallet's empty note commitment tree does not match the global
note commitment tree root.
This more accurately reflects its meaning, as it corresponds to the most
recently persisted best chain (i.e. the chain tip that the wallet will
return to on restart), rather than the chain tip to which the in-memory
wallet state has been synced.
The comment on `view.SetBackend(dummy)` was removed when we backported
upstream locking PRs in zcash/zcash#5017. The upstream commit in
question removed a locking scope but did not remove the reference to
that scope in the comment. Our backport removed the outdated comment,
but should have modified it instead, because otherwise the existence of
`view.SetBackend(dummy)` is very confusing (as it disconnects the cache
from `pcoinsTip`, on the assumption that everything we need from it has
been cached via calls to `CCoinsViewCache::HaveCoins` and
`CCoinsViewCache::HaveShieldedRequirements`).
Now that we call `Consensus::CheckTxShieldedInputs` on all transactions
in `CTxMemPool::check`, we don't need to separately check the nullifiers
and anchors directly. That change also fixed a bug where we weren't
previously making the same assertions about Orchard Actions.
Both transparent and shielded inputs have contextual checks that need to
be enforced in the consensus rules. For shielded inputs, these are that
the anchors in transactions correspond to real commitment tree states
(to ensure that the spent notes existed), and that their nullifiers are
not being double-spent.
When Sprout was first added to the codebase, we added input checks in
the same places that transparent inputs were checked; namely anywhere
`CCoinsViewCache::HaveInputs` is called. These all happened to be gated
on `!tx.IsCoinBase()`, which was fine because we did not allow Sprout
JoinSplits in coinbase transactions (enforced with a non-contextual
check).
When we added Sapling we also allowed coinbase outputs to Sapling
addresses (shielded coinbase). We updated `HaveShieldedRequirements` to
check Sapling anchors and nullifiers, but didn't change the consensus
code to call it on coinbase. This was fine because Sapling Spends and
Outputs are separate, and we did not allow Sapling Spends in coinbase
transactions (meaning that there were no anchors or nullifiers to
enforce the input rules on).
Orchard falls into an interesting middle-ground:
- We allowed coinbase outputs to Orchard addresses, to enable Sapling
shielded coinbase users to migrate to Orchard.
- Orchard uses Actions, which are a hybrid of Sprout JoinSplits and
Sapling Spends/Outputs. That is, an Orchard Action comprises a single
spend and a single output.
To maintain the "no shielded spends in coinbase" rule, we added an
`enableSpends` flag to the Orchard circuit. We force it to be set to
`false` for coinbase, ensuring that all Orchard spends in a coinbase use
dummy (zero-valued) notes. However, this is insufficient: the coinbase
transaction will still contain an Orchard anchor and nullifiers, and
these need to be correctly constrained.
In particular, not constraining the Orchard nullifiers in a coinbase
transaction enables a Faerie Gold attack. We explicitly require that
Orchard nullifiers are unique, so that there is a unique input to the
nullifier derivation. Without the coinbase check, the following attack
is possible:
- An adversary creates an Orchard Action sending some amount of ZEC to a
victim address, with a dummy spent note. The entire transaction can be
fully-shielded by placing the real spent note in a separate Action.
- The adversary uses the exact same dummy note in a coinbase
transaction, creating the exact same output note (same victim address
and amount).
- The victim now has two notes with the same ZEC amount, but can only
spend one of them because they have the same nullifier.
This commit fixes the consensus bug by calling `HaveShieldedRequirements`
outside of `!tx.IsCoinBase()` gates. To simplify its usage, there is now
a `Consensus::CheckTxShieldedInputs` function that handles the logging
and validation state updates. We also move shielded input checks from
`ContextualCheckInputs` to `ContextualCheckShieldedInputs`; these now
mirror each other in that they check contextual rules on transparent and
shielded inputs respectively, followed by checking signatures.
The previous code assumed that the last chain tip notified to the wallet
was equal to the node's chain tip at startup. However, this assumption
fails if the node shuts down uncleanly, or if a wallet file is moved
from one node to another.
We now try to start notifying from the wallet's best block, and if the
node doesn't have that block we fall back to the node's chain tip like
before.
Closeszcash/zcash#5805.
This is a temporary change to ensure that if this record is unreadable,
we immediately shut down the node and don't make any further changes
that could impair our ability to recover from this state later.
Part of zcash/zcash#5806.
(within the mempool only), that are redundant with checks in
HaveShieldedRequirements. The latter take into account nullifiers in the
chain (using CCoinsViewMemPool::GetNullifier) and include Orchard.
There is no meaningful DoS-protection motivation for the checks being
removed here, nor do they simplify reasoning about the code (if anything
the redundancy makes it more confusing).
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Previously we were reconstructing the height on wallet load by looking
up the `blockHash` field of `CMerkleTx` to find the transaction's height
in the main chain. However, this field is updated whenever `AddToWallet`
is called, while the transaction's height and note positions need to be
kept in sync with `SetBestChain`, which is only called once every 10
minutes. In the case that a reorg occurs between `SetBestChain` and the
node shutting down, the resulting height on wallet load would be
inconsistent. As with note positions, any inconsistency should be
resolved by the post-load wallet rescan, which rewinds the Orchard
witness tree and unsets any position information.
Part of zcash/zcash#5784.
We add both C++ client version information (enabling version-specific
changes if necessary, matching the rest of the C++ wallet), and record
version information (enabling local serialization format changes).
Part of zcash/zcash#5784.
We rewrite the entire witness tree each time we update the wallet's best
chain state, and we need the note positions to always be consistent with
the tree state (otherwise mined notes become unspendable after the node
restarts).
The test fails during the final `z_sendmany`, because it is selecting a
note that was detected before restarting the node. Because we force the
wallet to call `SetBestChain` on every block, the wallet doesn't need to
rescan on restart, and thus doesn't repopulate the `position` field of
the in-memory note.
This issue went unnoticed in existing tests that exercise node restarts,
because the RPC tests are fast enough that they never pass the 10-minute
timeout for writing the wallet state. This commit adds a regtest-only
config option that disables the lazy writing.
therealyingtong
str4d
Kris Nuttycombe
Sean Bowe
Jack Grigg
Steven
Daira Hopwood
str4d
Kris Nuttycombe
sasha