Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject blocks that violate turnstile #3885

Merged
merged 12 commits into from Mar 19, 2019
Merged
8 changes: 8 additions & 0 deletions src/chainparams.cpp
Expand Up @@ -353,6 +353,14 @@ class CTestNetParams : public CChainParams {
715 // total number of tx / (checkpoint block height / (24 * 24))
};

// Hardcoded fallback value for the Sprout shielded value pool balance
// for nodes that have not reindexed since the introduction of monitoring
// in #2795.
nSproutValuePoolCheckpointHeight = 440329;
nSproutValuePoolCheckpointBalance = 40000029096803;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified that this balance is accurate.

fZIP209Enabled = true;
hashSproutValuePoolCheckpointBlock = uint256S("000a95d08ba5dcbabe881fc6471d11807bcca7df5f1795c99f3ec4580db4279b");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified that this hash is accurate.


// Founders reward script expects a vector of 2-of-3 multisig addresses
vFoundersRewardAddress = {
"t2UNzUUx8mWBCRYPRezvA363EYXyEpHokyi", "t2N9PH9Wk9xjqYg9iin1Ua3aekJqfAtE543", "t2NGQjYMQhFndDHguvUw4wZdNdsssA6K7x2", "t2ENg7hHVqqs9JwU5cgjvSbxnT2a9USNfhy",
Expand Down
10 changes: 10 additions & 0 deletions src/chainparams.h
Expand Up @@ -70,6 +70,11 @@ class CChainParams
const std::vector<unsigned char>& AlertKey() const { return vAlertPubKey; }
int GetDefaultPort() const { return nDefaultPort; }

CAmount SproutValuePoolCheckpointHeight() const { return nSproutValuePoolCheckpointHeight; }
CAmount SproutValuePoolCheckpointBalance() const { return nSproutValuePoolCheckpointBalance; }
uint256 SproutValuePoolCheckpointBlockHash() const { return hashSproutValuePoolCheckpointBlock; }
bool ZIP209Enabled() const { return fZIP209Enabled; }

const CBlock& GenesisBlock() const { return genesis; }
/** Make miner wait to have peers to avoid wasting work */
bool MiningRequiresPeers() const { return fMiningRequiresPeers; }
Expand Down Expand Up @@ -125,6 +130,11 @@ class CChainParams
bool fTestnetToBeDeprecatedFieldRPC = false;
CCheckpointData checkpointData;
std::vector<std::string> vFoundersRewardAddress;

CAmount nSproutValuePoolCheckpointHeight = 0;
CAmount nSproutValuePoolCheckpointBalance = 0;
uint256 hashSproutValuePoolCheckpointBlock;
bool fZIP209Enabled = false;
};

/**
Expand Down
74 changes: 74 additions & 0 deletions src/main.cpp
Expand Up @@ -2451,6 +2451,35 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
return true;
}

// Reject a block that results in a negative shielded value pool balance.
if (chainparams.ZIP209Enabled()) {
// Sprout
//
// We can expect nChainSproutValue to be valid after the hardcoded
// height, and this will be enforced on all descendant blocks. If
// the node was reindexed then this will be enforced for all blocks.
if (pindex->nChainSproutValue) {
if (*pindex->nChainSproutValue < 0) {
return state.DoS(100, error("ConnectBlock(): turnstile violation in Sprout shielded value pool"),
REJECT_INVALID, "turnstile-violation-sprout-shielded-pool");
}
}

// Sapling
ebfull marked this conversation as resolved.
Show resolved Hide resolved
//
// If we've reached ConnectBlock, we have all transactions of
// parents and can expect nChainSaplingValue not to be boost::none.
// However, the miner and mining RPCs may not have populated this
// value and will call `TestBlockValidity`. So, we act
// conditionally.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that the built-in miner might try to mine a block that violates the turnstile rule. (In this situation the miner is not the attacker who submitted the transaction, and so we would care if they're DoS-penalised; I'm not sure whether they are.) The block will be seen as invalid by other nodes, but this still seems undesirable.

This comment also applies for Sprout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is logic in the miner for avoiding mining a block which violates the turnstile, it's just that the block index fields don't get populated before TestBlockValidity is called so we aren't relying on TestBlockValidity to defend us against wasted mining effort -- we're relying on the mining code to correctly avoid placing transactions that violate the turnstile in the block.

if (pindex->nChainSaplingValue) {
if (*pindex->nChainSaplingValue < 0) {
return state.DoS(100, error("ConnectBlock(): turnstile violation in Sapling shielded value pool"),
REJECT_INVALID, "turnstile-violation-sapling-shielded-pool");
}
}
}

// Do not allow blocks that contain transactions which 'overwrite' older transactions,
// unless those are already completely spent.
BOOST_FOREACH(const CTransaction& tx, block.vtx) {
Expand Down Expand Up @@ -3296,9 +3325,46 @@ CBlockIndex* AddToBlockIndex(const CBlockHeader& block)
return pindexNew;
}

void FallbackSproutValuePoolBalance(
CBlockIndex *pindex,
const CChainParams& chainparams
)
{
// We might not want to enable the checkpointing for mainnet
// yet.
if (!chainparams.ZIP209Enabled()) {
return;
}

// Check if the height of this block matches the checkpoint
if (pindex->nHeight == chainparams.SproutValuePoolCheckpointHeight()) {
if (pindex->GetBlockHash() == chainparams.SproutValuePoolCheckpointBlockHash()) {
// Are we monitoring the Sprout pool?
if (!pindex->nChainSproutValue) {
// Apparently not. Introduce the hardcoded value so we monitor for
// this point onwards (assuming the checkpoint is late enough)
pindex->nChainSproutValue = chainparams.SproutValuePoolCheckpointBalance();
} else {
// Apparently we have been. So, we should expect the current
// value to match the hardcoded one.
assert(*pindex->nChainSproutValue == chainparams.SproutValuePoolCheckpointBalance());
// And we should expect non-none for the delta stored in the block index here,
// or the checkpoint is too early.
assert(pindex->nSproutValue != boost::none);
}
} else {
LogPrintf(
"FallbackSproutValuePoolBalance(): fallback block hash is incorrect, we got %s\n",
pindex->GetBlockHash().ToString()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're at the checkpoint height but on the wrong chain, this will fall through, leaving pindex->nChainSproutValue as none. Then the conditional on line R2461 will cause the turnstile rule not to be enforced.

It's important that we don't just assert that the block hash at the checkpoint height is as expected, because that would introduce a potential DoS during node startup if an attacker created an orphan block at the checkpoint height and sent it to the victim node at just the right time. On the other hand, it seems like we should be able to do better than failing open in this case: in particular if the node reaches 100 blocks past the checkpoint height and is still on the wrong chain, then it's never going to recover. Maybe it's too much complexity to detect that case (if it happens then we have other problems than the turnstile rule).

}
}
}

/** Mark a block as having its data received and checked (up to BLOCK_VALID_TRANSACTIONS). */
bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBlockIndex *pindexNew, const CDiskBlockPos& pos)
{
const CChainParams& chainparams = Params();
pindexNew->nTx = block.vtx.size();
pindexNew->nChainTx = 0;
CAmount sproutValue = 0;
Expand Down Expand Up @@ -3351,6 +3417,10 @@ bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBl
pindex->nChainSproutValue = pindex->nSproutValue;
pindex->nChainSaplingValue = pindex->nSaplingValue;
}

// Fall back to hardcoded Sprout value pool balance
FallbackSproutValuePoolBalance(pindex, chainparams);

{
LOCK(cs_nBlockSequenceId);
pindex->nSequenceId = nBlockSequenceId++;
Expand Down Expand Up @@ -4003,6 +4073,7 @@ CBlockIndex * InsertBlockIndex(uint256 hash)
bool static LoadBlockIndexDB()
{
const CChainParams& chainparams = Params();

if (!pblocktree->LoadBlockIndexGuts())
return false;

Expand Down Expand Up @@ -4048,6 +4119,9 @@ bool static LoadBlockIndexDB()
pindex->nChainSproutValue = pindex->nSproutValue;
pindex->nChainSaplingValue = pindex->nSaplingValue;
}

// Fall back to hardcoded Sprout value pool balance
FallbackSproutValuePoolBalance(pindex, chainparams);
}
// Construct in-memory chain of branch IDs.
// Relies on invariant: a block that does not activate a network upgrade
Expand Down
48 changes: 48 additions & 0 deletions src/miner.cpp
Expand Up @@ -250,6 +250,28 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
TxPriorityCompare comparer(fSortedByFee);
std::make_heap(vecPriority.begin(), vecPriority.end(), comparer);

// We want to track the value pool, but if the miner gets
// invoked on an old block before the hardcoded fallback
// is active we don't want to trip up any assertions. So,
// we only adhere to the turnstile (as a miner) if we
// actually have all of the information necessary to do
// so.
CAmount sproutValue = 0;
CAmount saplingValue = 0;
bool monitoring_pool_balances = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Technically we can still enforce Sapling monitoring even if we don't have full coverage of Sprout. This is standard-rule territory though, so we can alter how we handle this later.

if (chainparams.ZIP209Enabled()) {
if (pindexPrev->nChainSproutValue) {
sproutValue = *pindexPrev->nChainSproutValue;
} else {
monitoring_pool_balances = false;
}
if (pindexPrev->nChainSaplingValue) {
saplingValue = *pindexPrev->nChainSaplingValue;
} else {
monitoring_pool_balances = false;
}
}

while (!vecPriority.empty())
{
// Take highest priority transaction off the priority queue:
Expand Down Expand Up @@ -305,6 +327,32 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
if (!ContextualCheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, txdata, Params().GetConsensus(), consensusBranchId))
continue;

if (chainparams.ZIP209Enabled() && monitoring_pool_balances) {
// Does this transaction lead to a turnstile violation?

CAmount sproutValueDummy = sproutValue;
CAmount saplingValueDummy = saplingValue;

saplingValueDummy += -tx.valueBalance;

for (auto js : tx.vjoinsplit) {
sproutValueDummy += js.vpub_old;
sproutValueDummy -= js.vpub_new;
}

if (sproutValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sprout turnstile\n", tx.GetHash().ToString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: We should log the txids of the transactions that are in the block at this time, because it's the set of transactions that causes the violation, not necessarily this specific transaction.

continue;
}
if (saplingValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sapling turnstile\n", tx.GetHash().ToString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: ditto.

continue;
}

sproutValue = sproutValueDummy;
saplingValue = saplingValueDummy;
}

UpdateCoins(tx, view, nHeight);

BOOST_FOREACH(const OutputDescription &outDescription, tx.vShieldedOutput) {
Expand Down