Apply `HaveShieldedRequirements` to coinbase transactions

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.
This commit is contained in:
Jack Grigg 2022-04-01 19:11:18 +00:00
parent f7b11f6da1
commit f1cda64602
6 changed files with 125 additions and 52 deletions

View File

@ -545,18 +545,20 @@ TEST(ContextualCheckShieldedInputsTest, BadTxnsInvalidJoinsplitSignature) {
const PrecomputedTransactionData txdata(tx, allPrevOutputs);
MockCValidationState state;
AssumeShieldedInputsExistAndAreSpendable baseView;
CCoinsViewCache view(&baseView);
// during initial block download, for transactions being accepted into the
// mempool (and thus not mined), DoS ban score should be zero, else 10
EXPECT_CALL(state, DoS(0, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, 0, false, false, [](const Consensus::Params&) { return true; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, 0, false, false, [](const Consensus::Params&) { return true; });
EXPECT_CALL(state, DoS(10, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, 0, false, false, [](const Consensus::Params&) { return false; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, 0, false, false, [](const Consensus::Params&) { return false; });
// for transactions that have been mined in a block, DoS ban score should
// always be 100.
EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, 0, false, true, [](const Consensus::Params&) { return true; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, 0, false, true, [](const Consensus::Params&) { return true; });
EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, 0, false, true, [](const Consensus::Params&) { return false; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, 0, false, true, [](const Consensus::Params&) { return false; });
}
TEST(ContextualCheckShieldedInputsTest, JoinsplitSignatureDetectsOldBranchId) {
@ -578,9 +580,11 @@ TEST(ContextualCheckShieldedInputsTest, JoinsplitSignatureDetectsOldBranchId) {
const PrecomputedTransactionData txdata(tx, allPrevOutputs);
MockCValidationState state;
AssumeShieldedInputsExistAndAreSpendable baseView;
CCoinsViewCache view(&baseView);
// Ensure that the transaction validates against Sapling.
EXPECT_TRUE(ContextualCheckShieldedInputs(
tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, false,
tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, false,
[](const Consensus::Params&) { return false; }));
// Attempt to validate the inputs against Blossom. We should be notified
@ -592,7 +596,7 @@ TEST(ContextualCheckShieldedInputsTest, JoinsplitSignatureDetectsOldBranchId) {
HexInt(saplingBranchId)),
false, "")).Times(1);
EXPECT_FALSE(ContextualCheckShieldedInputs(
tx, txdata, state, orchardAuth, consensus, blossomBranchId, false, false,
tx, txdata, state, view, orchardAuth, consensus, blossomBranchId, false, false,
[](const Consensus::Params&) { return false; }));
// Attempt to validate the inputs against Heartwood. All we should learn is
@ -602,7 +606,7 @@ TEST(ContextualCheckShieldedInputsTest, JoinsplitSignatureDetectsOldBranchId) {
10, false, REJECT_INVALID,
"bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
EXPECT_FALSE(ContextualCheckShieldedInputs(
tx, txdata, state, orchardAuth, consensus, heartwoodBranchId, false, false,
tx, txdata, state, view, orchardAuth, consensus, heartwoodBranchId, false, false,
[](const Consensus::Params&) { return false; }));
}
@ -611,6 +615,9 @@ TEST(ContextualCheckShieldedInputsTest, NonCanonicalEd25519Signature) {
auto consensus = Params().GetConsensus();
auto orchardAuth = orchard::AuthValidator::Disabled();
AssumeShieldedInputsExistAndAreSpendable baseView;
CCoinsViewCache view(&baseView);
auto saplingBranchId = NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId;
CMutableTransaction mtx = GetValidTransaction(saplingBranchId);
@ -623,7 +630,7 @@ TEST(ContextualCheckShieldedInputsTest, NonCanonicalEd25519Signature) {
CTransaction tx(mtx);
const PrecomputedTransactionData txdata(tx, allPrevOutputs);
MockCValidationState state;
EXPECT_TRUE(ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, true));
EXPECT_TRUE(ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, true));
}
// Copied from libsodium/crypto_sign/ed25519/ref10/open.c
@ -647,15 +654,15 @@ TEST(ContextualCheckShieldedInputsTest, NonCanonicalEd25519Signature) {
// during initial block download, for transactions being accepted into the
// mempool (and thus not mined), DoS ban score should be zero, else 10
EXPECT_CALL(state, DoS(0, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, false, [](const Consensus::Params&) { return true; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, false, [](const Consensus::Params&) { return true; });
EXPECT_CALL(state, DoS(10, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, false, [](const Consensus::Params&) { return false; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, false, [](const Consensus::Params&) { return false; });
// for transactions that have been mined in a block, DoS ban score should
// always be 100.
EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, true, [](const Consensus::Params&) { return true; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, true, [](const Consensus::Params&) { return true; });
EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false, "")).Times(1);
ContextualCheckShieldedInputs(tx, txdata, state, orchardAuth, consensus, saplingBranchId, false, true, [](const Consensus::Params&) { return false; });
ContextualCheckShieldedInputs(tx, txdata, state, view, orchardAuth, consensus, saplingBranchId, false, true, [](const Consensus::Params&) { return false; });
}
TEST(ChecktransactionTests, OverwinterConstructors) {
@ -1322,9 +1329,11 @@ TEST(ChecktransactionTests, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) {
// their transparent input; ZIP 244 handles this by making the coinbase
// sighash the txid.
PrecomputedTransactionData txdata(tx, {});
AssumeShieldedInputsExistAndAreSpendable baseView;
CCoinsViewCache view(&baseView);
EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-sapling-binding-signature-invalid", false, "")).Times(1);
EXPECT_FALSE(ContextualCheckShieldedInputs(
tx, txdata, state, orchardAuth, chainparams.GetConsensus(), heartwoodBranchId, false, true));
tx, txdata, state, view, orchardAuth, chainparams.GetConsensus(), heartwoodBranchId, false, true));
RegtestDeactivateHeartwood();
}

View File

@ -1233,6 +1233,7 @@ bool ContextualCheckShieldedInputs(
const CTransaction& tx,
const PrecomputedTransactionData& txdata,
CValidationState &state,
const CCoinsViewCache &view,
orchard::AuthValidator& orchardAuth,
const Consensus::Params& consensus,
uint32_t consensusBranchId,
@ -1240,6 +1241,12 @@ bool ContextualCheckShieldedInputs(
bool isMined,
bool (*isInitBlockDownload)(const Consensus::Params&))
{
// This doesn't trigger the DoS code on purpose; if it did, it would make it easier
// for an attacker to attempt to split the network.
if (!Consensus::CheckTxShieldedInputs(tx, state, view, 0)) {
return false;
}
const int DOS_LEVEL_BLOCK = 100;
// DoS level set to 10 to be more forgiving.
const int DOS_LEVEL_MEMPOOL = 10;
@ -1874,16 +1881,10 @@ bool AcceptToMemoryPool(
return state.Invalid(false, REJECT_DUPLICATE, "bad-txns-inputs-spent");
// Are the shielded spends' requirements met?
auto unmetShieldedReq = view.HaveShieldedRequirements(tx);
if (unmetShieldedReq) {
auto txid = tx.GetHash().ToString();
auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq);
auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq);
TracingError(
"main", "AcceptToMemoryPool(): shielded requirements not met",
"txid", txid.c_str(),
"reason", rejectReason.c_str());
return state.Invalid(false, rejectCode, rejectReason);
// This doesn't trigger the DoS code on purpose; if it did, it would make it easier
// for an attacker to attempt to split the network.
if (!Consensus::CheckTxShieldedInputs(tx, state, view, 0)) {
return false;
}
// Bring the best block into scope
@ -2010,6 +2011,7 @@ bool AcceptToMemoryPool(
tx,
txdata,
state,
view,
orchardAuth,
chainparams.GetConsensus(),
consensusBranchId,
@ -2557,26 +2559,39 @@ int GetSpendHeight(const CCoinsViewCache& inputs)
}
namespace Consensus {
bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, const Consensus::Params& consensusParams)
bool CheckTxShieldedInputs(
const CTransaction& tx,
CValidationState& state,
const CCoinsViewCache& view,
int dosLevel)
{
// This doesn't trigger the DoS code on purpose; if it did, it would make it easier
// for an attacker to attempt to split the network.
if (!inputs.HaveInputs(tx))
return state.Invalid(false, 0, "", "Inputs unavailable");
// Are the shielded spends' requirements met?
auto unmetShieldedReq = inputs.HaveShieldedRequirements(tx);
auto unmetShieldedReq = view.HaveShieldedRequirements(tx);
if (unmetShieldedReq) {
auto txid = tx.GetHash().ToString();
auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq);
auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq);
TracingError(
"main", "CheckInputs(): shielded requirements not met",
"main", "CheckTxShieldedInputs(): shielded requirements not met",
"txid", txid.c_str(),
"reason", rejectReason.c_str());
return state.Invalid(false, rejectCode, rejectReason);
return state.DoS(dosLevel, false, rejectCode, rejectReason);
}
return true;
}
bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, const Consensus::Params& consensusParams)
{
assert(!tx.IsCoinBase());
// Indented to reduce conflicts with upstream.
{
// This doesn't trigger the DoS code on purpose; if it did, it would make it easier
// for an attacker to attempt to split the network.
if (!inputs.HaveInputs(tx))
return state.Invalid(false, 0, "", "Inputs unavailable");
CAmount nValueIn = 0;
CAmount nFees = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
@ -2627,6 +2642,7 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins
nFees += nTxFee;
if (!MoneyRange(nFees))
return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange");
}
return true;
}
}// namespace Consensus
@ -3226,25 +3242,17 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
// txid.
std::vector<CTxOut> allPrevOutputs;
// Are the shielded spends' requirements met?
if (!Consensus::CheckTxShieldedInputs(tx, state, view, 100)) {
return false;
}
if (!tx.IsCoinBase())
{
if (!view.HaveInputs(tx))
return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
REJECT_INVALID, "bad-txns-inputs-missingorspent");
// Are the shielded spends' requirements met?
auto unmetShieldedReq = view.HaveShieldedRequirements(tx);
if (unmetShieldedReq) {
auto txid = tx.GetHash().ToString();
auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq);
auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq);
TracingError(
"main", "ConnectBlock(): shielded requirements not met",
"txid", txid.c_str(),
"reason", rejectReason.c_str());
return state.DoS(100, false, rejectCode, rejectReason);
}
for (const auto& input : tx.vin) {
allPrevOutputs.push_back(view.GetOutputFor(input));
}
@ -3309,6 +3317,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
tx,
txdata.back(),
state,
view,
orchardAuth,
chainparams.GetConsensus(),
consensusBranchId,

View File

@ -373,19 +373,29 @@ bool ContextualCheckInputs(const CTransaction& tx, CValidationState &state, cons
std::vector<CScriptCheck> *pvChecks = NULL);
/**
* Checks the signatures for a transaction's shielded components.
* Check whether all shielded inputs of this transaction are valid.
*
* This checks that:
* - The anchors in the transaction exist in the given view.
* - The nullifiers in the transaction do not exist in the given view.
* - The signatures for the transaction's shielded components are valid.
*
* This also currently checks the Sapling proofs, due to the way the Rust verification
* code is written. Sprout and Orchard proofs are currently checked in CheckTransaction().
* Once we have batch proof validation implemented, these will all be accumulated in
* CheckTransaction().
*
* To skip checking signatures, use `Consensus::CheckTxShieldedInputs` instead.
*
* This does not modify the view to add the nullifiers to the spent set.
*
* The `isInitBlockDownload` argument is a function parameter to assist with testing.
*/
bool ContextualCheckShieldedInputs(
const CTransaction& tx,
const PrecomputedTransactionData& txdata,
CValidationState &state,
const CCoinsViewCache &view,
orchard::AuthValidator& orchardAuth,
const Consensus::Params& consensus,
uint32_t consensusBranchId,
@ -410,6 +420,22 @@ bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidatio
namespace Consensus {
/**
* Check whether all shielded inputs of this transaction are valid.
*
* This checks that:
* - The anchors in the transaction exist in the given view.
* - The nullifiers in the transaction do not exist in the given view.
*
* This does not modify the view to add the nullifiers to the spent set.
* This does not check proofs or signatures.
*/
bool CheckTxShieldedInputs(
const CTransaction& tx,
CValidationState& state,
const CCoinsViewCache& view,
int dosLevel);
/**
* Check whether all inputs of this transaction are valid (no double spends and amounts)
* This does not modify the UTXO set. This does not check scripts and sigs.

View File

@ -24,6 +24,7 @@
#include "test/test_util.h"
#include "primitives/transaction.h"
#include "transaction_builder.h"
#include "utiltest.h"
#include <array>
#include <map>
@ -436,6 +437,8 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa
// joinsplits.
CMutableTransaction newTx(tx);
CValidationState state;
AssumeShieldedInputsExistAndAreSpendable baseView;
CCoinsViewCache view(&baseView);
Ed25519SigningKey joinSplitPrivKey;
ed25519_generate_keypair(&joinSplitPrivKey, &newTx.joinSplitPubKey);
@ -462,7 +465,7 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa
BOOST_CHECK(CheckTransactionWithoutProofVerification(newTx, state));
BOOST_CHECK(ContextualCheckTransaction(newTx, state, Params(), 0, true));
BOOST_CHECK(!ContextualCheckShieldedInputs(newTx, txdata, state, orchardAuth, Params().GetConsensus(), consensusBranchId, false, true));
BOOST_CHECK(!ContextualCheckShieldedInputs(newTx, txdata, state, view, orchardAuth, Params().GetConsensus(), consensusBranchId, false, true));
BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-joinsplit-signature");
// Empty output script.
@ -478,7 +481,7 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa
state = CValidationState();
BOOST_CHECK(CheckTransactionWithoutProofVerification(newTx, state));
BOOST_CHECK(ContextualCheckTransaction(newTx, state, Params(), 0, true));
BOOST_CHECK(ContextualCheckShieldedInputs(newTx, txdata, state, orchardAuth, Params().GetConsensus(), consensusBranchId, false, true));
BOOST_CHECK(ContextualCheckShieldedInputs(newTx, txdata, state, view, orchardAuth, Params().GetConsensus(), consensusBranchId, false, true));
BOOST_CHECK_EQUAL(state.GetRejectReason(), "");
}
{

View File

@ -619,6 +619,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
CValidationState state;
bool fCheckResult = tx.IsCoinBase() ||
Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight, Params().GetConsensus());
fCheckResult &= Consensus::CheckTxShieldedInputs(tx, state, mempoolDuplicate, 0);
assert(fCheckResult);
UpdateCoins(tx, mempoolDuplicate, 1000000);
}
@ -635,6 +636,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
} else {
bool fCheckResult = entry->GetTx().IsCoinBase() ||
Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, nSpendHeight, Params().GetConsensus());
fCheckResult &= Consensus::CheckTxShieldedInputs(entry->GetTx(), state, mempoolDuplicate, 0);
assert(fCheckResult);
UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000);
stepsSinceLastRemove = 0;

View File

@ -5,12 +5,36 @@
#ifndef ZCASH_UTILTEST_H
#define ZCASH_UTILTEST_H
#include "coins.h"
#include "key_io.h"
#include "wallet/wallet.h"
#include "zcash/Address.hpp"
#include "zcash/Note.hpp"
#include "zcash/NoteEncryption.hpp"
// A fake chain state view where anchors and nullifiers are assumed to exist.
class AssumeShieldedInputsExistAndAreSpendable : public CCoinsView {
public:
AssumeShieldedInputsExistAndAreSpendable() {}
bool GetSproutAnchorAt(const uint256 &rt, SproutMerkleTree &tree) const {
return true;
}
bool GetSaplingAnchorAt(const uint256 &rt, SaplingMerkleTree &tree) const {
return true;
}
bool GetOrchardAnchorAt(const uint256 &rt, OrchardMerkleFrontier &tree) const {
return true;
}
bool GetNullifier(const uint256 &nf, ShieldedType type) const {
// Always return false so we treat every nullifier as being unspent.
return false;
}
};
// Sprout
CWalletTx GetValidSproutReceive(const libzcash::SproutSpendingKey& sk,
CAmount value,