In #5809, we attempted to fix the assumption that on startup, the
wallet's best chain was at the same position as the node's chain tip.
This did not account for a condition where the node's block index
might not contain the block corresponding to the wallet's best chain,
because the node had crashed before the block index containing that
block could be written to disk.
This commit adds handling so that the `ThreadNotifyWallets` thread
will not start until the wallet's best block has been restored
to the node's block index and we're able to obtain a pointer to
that state.
This adds tests to wallet_z_sendmany that demonstrates conditions
where pool-crossing transfers were not being caught by the privacy
policy checks, which were validating only against the transfer
request rather than the actual transfer selected. This missed
cases where sending funds between unified addresses would reveal
amounts via the turnstile.
Making all privacy policy checks after note selection guards against
this error.
We canonicalize git URLs when linting Cargo patches, because Cargo
treats `path/to/repo` and `path/to/repo.git` identically (and similarly
it strips a trailing slash), so we allow `.cargo/config.offline` and
`Cargo.toml` to mismatch in this way to minimise lints.
The previous code assumed that we would only see wallet addresses
that we had explicitly generated, and crashed if we detected a note
sent to a different Orchard receiver within a known account.
Fixeszcash/zcash#5827.
Co-authored-by: Jack Grigg <jack@electriccoin.co>
`Wallet::append_bundle_commitments` should never be called twice on the
same bundle, as that breaks sequentiality requirements (which we already
check for), so it is safe to assert that the inserted values do not
overwrite any existing data.
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.
str4d
Jack Grigg
Kris Nuttycombe
Kris Nuttycombe
therealyingtong
Sean Bowe
Steven