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

validation: UTXO snapshot activation #19806

Merged
merged 8 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/chain.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,27 @@ class CBlockIndex

//! Number of transactions in this block.
//! Note: in a potential headers-first mode, this number cannot be relied upon
//! Note: this value is faked during UTXO snapshot load to ensure that
//! LoadBlockIndex() will load index entries for blocks that we lack data for.
//! @sa ActivateSnapshot
unsigned int nTx{0};

//! (memory only) Number of transactions in the chain up to and including this block.
//! This value will be non-zero only if and only if transactions for this block and all its parents are available.
//! Change to 64-bit type when necessary; won't happen before 2030
//!
//! Note: this value is faked during use of a UTXO snapshot because we don't
//! have the underlying block data available during snapshot load.
Copy link
Member

Choose a reason for hiding this comment

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

03e0de1

If those values are going to be sanitized during background-IBD maybe you should document it.

//! @sa AssumeutxoData
//! @sa ActivateSnapshot
unsigned int nChainTx{0};

//! Verification status of this block. See enum BlockStatus
//!
//! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
//! load to avoid the block index being spuriously rewound.
//! @sa RewindBlockIndex
//! @sa ActivateSnapshot
uint32_t nStatus{0};

//! block header
Expand Down
26 changes: 25 additions & 1 deletion src/chainparams.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <chainparamsseeds.h>
#include <consensus/merkle.h>
#include <hash.h> // for signet block challenge hash
#include <tinyformat.h>
Copy link
Member

Choose a reason for hiding this comment

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

7a6c46b: why is this removed. This is needed for strprintf

#include <util/system.h>
#include <util/strencodings.h>
#include <versionbitsinfo.h>
Expand Down Expand Up @@ -161,6 +160,10 @@ class CMainParams : public CChainParams {
}
};

m_assumeutxo_data = MapAssumeutxo{
// TODO to be specified in a future patch.
Copy link
Contributor

Choose a reason for hiding this comment

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

in d2d1eb2:
Just a thought: Should a node refuse to start if it has a chain that does not match with the assumeutxo data here? Maybe in the next steps it could make sense to add such a check to init but I haven't thought about it much and maybe you already plan to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Response to d684ecd#r523799037:

@fjahr I'd say no. assume* values are optimizations where we know some computation can be avoided because it's known to be valid. But refusing to start with a mismatching one should just mean you don't get the optimization; doing anything else is very close to making it a checkpoint, with all repercussions (actually affecting which chain the network can accept etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

@sipa Right, I didn't look at it from that perspective. Thanks!

};

chainTxData = ChainTxData{
// Data from RPC: getchaintxstats 4096 0000000000000000000b9d2ec5a352ecba0592946514a92f14319dc2b367fc72
/* nTime */ 1603995752,
Expand Down Expand Up @@ -250,6 +253,10 @@ class CTestNetParams : public CChainParams {
}
};

m_assumeutxo_data = MapAssumeutxo{
// TODO to be specified in a future patch.
};

chainTxData = ChainTxData{
// Data from RPC: getchaintxstats 4096 000000000000006433d1efec504c53ca332b64963c425395515b01977bd7b3b0
/* nTime */ 1603359686,
Expand Down Expand Up @@ -431,6 +438,17 @@ class CRegTestParams : public CChainParams {
}
};

m_assumeutxo_data = MapAssumeutxo{
{
110,
{uint256S("0x76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"), 110},
},
{
210,
{uint256S("0x9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"), 210},
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if this was previously discussed in the earlier, can MapAssumeutxo be simplified to not store the height twice?

Copy link
Member

Choose a reason for hiding this comment

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

Follow-up to d684ecd#r547190362:

@jonatack It's not the height, but the nChainTx value. It just happens to be equal to the height in chains that have never had anything but coinbase transactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I must have been confused here. Will re-review this.


chainTxData = ChainTxData{
0,
0,
Expand Down Expand Up @@ -526,3 +544,9 @@ void SelectParams(const std::string& network)
SelectBaseParams(network);
globalChainParams = CreateChainParams(gArgs, network);
}

std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud)
{
o << strprintf("AssumeutxoData(%s, %s)", aud.hash_serialized.ToString(), aud.nChainTx);
return o;
}
26 changes: 26 additions & 0 deletions src/chainparams.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,26 @@ struct CCheckpointData {
}
};

/**
* Holds configuration for use during UTXO snapshot load and validation. The contents
* here are security critical, since they dictate which UTXO snapshots are recognized
* as valid.
*/
struct AssumeutxoData {
//! The expected hash of the deserialized UTXO set.
const uint256 hash_serialized;

//! Used to populate the nChainTx value, which is used during BlockManager::LoadBlockIndex().
//!
//! We need to hardcode the value here because this is computed cumulatively using block data,
//! which we do not necessarily have at the time of snapshot load.
const unsigned int nChainTx;
};

std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud);

using MapAssumeutxo = std::map<int, const AssumeutxoData>;
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "chainparams: add allowed assumeutxo values" (a1aa474)

It might be good to say in comment that int is a height. Also, it looks like strictly speaking there is no need for this data structure to reference heights. E.g. it could just be a simple map from hash -> nChainTx. Not important, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, you're right about that. I kind of like the constraint that keying by height creates (enforces single entry per height), but that's neither here nor there. I'll leave as-is unless we can think of a good reason to change.


/**
* Holds various statistics on transactions within a chain. Used to estimate
* verification progress during chain sync.
Expand Down Expand Up @@ -90,6 +110,11 @@ class CChainParams
const std::string& Bech32HRP() const { return bech32_hrp; }
const std::vector<SeedSpec6>& FixedSeeds() const { return vFixedSeeds; }
const CCheckpointData& Checkpoints() const { return checkpointData; }

//! Get allowed assumeutxo configuration.
//! @see ChainstateManager
const MapAssumeutxo& Assumeutxo() const { return m_assumeutxo_data; }

const ChainTxData& TxData() const { return chainTxData; }
protected:
CChainParams() {}
Expand All @@ -111,6 +136,7 @@ class CChainParams
bool m_is_test_chain;
bool m_is_mockable_chain;
CCheckpointData checkpointData;
MapAssumeutxo m_assumeutxo_data;
ChainTxData chainTxData;
};

Expand Down
8 changes: 8 additions & 0 deletions src/coins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
}

void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
cachedCoinsUsage += coin.DynamicMemoryUsage();
cacheCoins.emplace(
std::piecewise_construct,
std::forward_as_tuple(std::move(outpoint)),
std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
}

void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
bool fCoinbase = tx.IsCoinBase();
const uint256& txid = tx.GetHash();
Expand Down
12 changes: 12 additions & 0 deletions src/coins.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <functional>
#include <unordered_map>

class ChainstateManager;
Copy link
Member

Choose a reason for hiding this comment

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

f6e2da5: why is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

Removed in #21592


/**
* A UTXO entry.
*
Expand Down Expand Up @@ -125,6 +127,7 @@ struct CCoinsCacheEntry

CCoinsCacheEntry() : flags(0) {}
explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {}
};

typedef std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher> CCoinsMap;
Expand Down Expand Up @@ -262,6 +265,15 @@ class CCoinsViewCache : public CCoinsViewBacked
*/
void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);

/**
* Emplace a coin into cacheCoins without performing any checks, marking
* the emplaced coin as dirty.
*
* NOT FOR GENERAL USE. Used only when loading coins from a UTXO snapshot.
* @sa ChainstateManager::PopulateAndValidateSnapshot()
*/
void EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin);

/**
* Spend a coin. Pass moveto in order to get the deleted data.
* If no unspent output exists for the passed outpoint, this call
Expand Down
12 changes: 12 additions & 0 deletions src/node/coinstats.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ static void ApplyHash(CCoinsStats& stats, MuHash3072& muhash, const uint256& has
muhash.Insert(MakeUCharSpan(ss));
}

//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot
//! validation commitments are reliant on the hash constructed by this
//! function.
Copy link
Member

Choose a reason for hiding this comment

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

f3bfa25

Should you describe more the concerns with any bug slip in in ApplyStats ? A bug breaking validation of already committed assume-valid chains and thus leading to their reject is okay. What is really concerning would be to validate a mischievous utxo set under a committed assume-valid chain, it might lead to double-spend against the assumeutxo user.

//!
//! If the construction of this hash is changed, it will invalidate
//! existing UTXO snapshots. This will not result in any kind of consensus
//! failure, but it will force clients that were expecting to make use of
//! assumeutxo to do traditional IBD instead.
//!
//! It is also possible, though very unlikely, that a change in this
//! construction could cause a previously invalid (and potentially malicious)
//! UTXO snapshot to be considered valid.
Copy link
Member

Choose a reason for hiding this comment

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

7a6c46b: I don't understand this section. Does this assume that the way outputs are applied to the hash_obj is broken? In that case it doesn't require a "previously" invalid snapshot. Likely, any invalid snapshot can be generated/modified, so that it is considered valid.

If it assumes that the underlying hash function is broken, there is nothing we can do anyway and any invalid snapshot can be generated/modified to be valid, regardless of changing this construction.

template <typename T>
static void ApplyStats(CCoinsStats& stats, T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
{
Expand Down
24 changes: 16 additions & 8 deletions src/rpc/blockchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2411,10 +2411,21 @@ static RPCHelpMan dumptxoutset()

FILE* file{fsbridge::fopen(temppath, "wb")};
CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
NodeContext& node = EnsureNodeContext(request.context);
UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), afile);
fs::rename(temppath, path);

result.pushKV("path", path.string());
return result;
},
};
}

UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile)
Copy link
Contributor

@jonatack jonatack Dec 22, 2020

Choose a reason for hiding this comment

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

9a2c888 NodeContext should be passed by reference to const

Suggested change
UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile)
UniValue CreateUTXOSnapshot(const NodeContext& node, CChainState& chainstate, CAutoFile& afile);

{
std::unique_ptr<CCoinsViewCursor> pcursor;
CCoinsStats stats;
CBlockIndex* tip;
NodeContext& node = EnsureNodeContext(request.context);

{
// We need to lock cs_main to ensure that the coinsdb isn't written to
Expand All @@ -2431,13 +2442,13 @@ static RPCHelpMan dumptxoutset()
//
LOCK(::cs_main);

::ChainstateActive().ForceFlushStateToDisk();
chainstate.ForceFlushStateToDisk();

if (!GetUTXOStats(&::ChainstateActive().CoinsDB(), stats, CoinStatsHashType::NONE, node.rpc_interruption_point)) {
if (!GetUTXOStats(&chainstate.CoinsDB(), stats, CoinStatsHashType::NONE, node.rpc_interruption_point)) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
}

pcursor = std::unique_ptr<CCoinsViewCursor>(::ChainstateActive().CoinsDB().Cursor());
pcursor = std::unique_ptr<CCoinsViewCursor>(chainstate.CoinsDB().Cursor());
tip = g_chainman.m_blockman.LookupBlockIndex(stats.hashBlock);
CHECK_NONFATAL(tip);
}
Expand All @@ -2462,16 +2473,13 @@ static RPCHelpMan dumptxoutset()
}

afile.fclose();
fs::rename(temppath, path);

UniValue result(UniValue::VOBJ);
result.pushKV("coins_written", stats.coins_count);
result.pushKV("base_hash", tip->GetBlockHash().ToString());
result.pushKV("base_height", tip->nHeight);
result.pushKV("path", path.string());

return result;
},
};
}

void RegisterBlockchainRPCCommands(CRPCTable &t)
Expand Down
8 changes: 8 additions & 0 deletions src/rpc/blockchain.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define BITCOIN_RPC_BLOCKCHAIN_H

#include <amount.h>
#include <streams.h>
#include <sync.h>

#include <stdint.h>
Expand All @@ -16,6 +17,7 @@ extern RecursiveMutex cs_main;
class CBlock;
class CBlockIndex;
class CBlockPolicyEstimator;
class CChainState;
class CTxMemPool;
class ChainstateManager;
class UniValue;
Expand Down Expand Up @@ -57,4 +59,10 @@ CTxMemPool& EnsureMemPool(const util::Ref& context);
ChainstateManager& EnsureChainman(const util::Ref& context);
CBlockPolicyEstimator& EnsureFeeEstimator(const util::Ref& context);

/**
* Helper to create UTXO snapshots given a chainstate and a file handle.
* @return a UniValue map containing metadata about the snapshot.
*/
UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile);
Copy link
Member

Choose a reason for hiding this comment

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

6bb63e9 : alternatively, it could return path rather than a UniValue, so it could even live outside of RPC land.

Copy link
Contributor

@jonatack jonatack Dec 22, 2020

Choose a reason for hiding this comment

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

9a2c888 agree with returning path

It looks like NodeContext should be passed by reference to const ("in" param), not by reference for an "out" param

Suggested change
UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile);
UniValue CreateUTXOSnapshot(const NodeContext& node, CChainState& chainstate, CAutoFile& afile);


#endif
38 changes: 35 additions & 3 deletions src/test/util/setup_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,43 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
}
}

TestChain100Setup::TestChain100Setup()
TestChain100Setup::TestChain100Setup(bool deterministic)
{
m_deterministic = deterministic;

if (m_deterministic) {
SetMockTime(1598887952);
constexpr std::array<unsigned char, 32> vchKey = {
{
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1
}
};
coinbaseKey.Set(vchKey.begin(), vchKey.end(), false);
} else {
coinbaseKey.MakeNewKey(true);
}

// Generate a 100-block chain:
coinbaseKey.MakeNewKey(true);
this->mineBlocks(COINBASE_MATURITY);

if (m_deterministic) {
LOCK(::cs_main);
assert(
m_node.chainman->ActiveChain().Tip()->GetBlockHash().ToString() ==
"49c95db1e470fed04496d801c9d8fbb78155d2c7f855232c918823d2c17d0cf6");
}
}

void TestChain100Setup::mineBlocks(int num_blocks)
{
CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
for (int i = 0; i < COINBASE_MATURITY; i++) {
for (int i = 0; i < num_blocks; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

414ba87 here and 51f3f97 in a few places in src/test/validation_chainstatemanager_tests.cpp, per developer-notes.md prefer the prefix operator

Suggested change
for (int i = 0; i < num_blocks; i++)
for (int i = 0; i < num_blocks; ++i)

{
std::vector<CMutableTransaction> noTxns;
CBlock b = CreateAndProcessBlock(noTxns, scriptPubKey);
if (m_deterministic) {
SetMockTime(GetTime() + 1);
}
m_coinbase_txns.push_back(b.vtx[0]);
}
}
Expand Down Expand Up @@ -234,6 +263,9 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
TestChain100Setup::~TestChain100Setup()
{
gArgs.ForceSetArg("-segwitheight", "0");
if (m_deterministic) {
SetMockTime(0);
}
}

CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) const
Expand Down
12 changes: 10 additions & 2 deletions src/test/util/setup_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ struct BasicTestingSetup {
explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
~BasicTestingSetup();

private:
Copy link
Member

Choose a reason for hiding this comment

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

e82498b: why are you dropping private here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Could use GetDataDir() instead

const fs::path m_path_root;
};

Expand Down Expand Up @@ -112,7 +111,7 @@ class CScript;
* Testing fixture that pre-creates a 100-block REGTEST-mode block chain
*/
struct TestChain100Setup : public RegTestingSetup {
TestChain100Setup();
TestChain100Setup(bool deterministic = false);

/**
* Create a new block with just given transactions, coinbase paying to
Expand All @@ -121,12 +120,21 @@ struct TestChain100Setup : public RegTestingSetup {
CBlock CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns,
const CScript& scriptPubKey);

//! Mine a series of new blocks on the active chain.
void mineBlocks(int num_blocks);

~TestChain100Setup();

bool m_deterministic;
std::vector<CTransactionRef> m_coinbase_txns; // For convenience, coinbase transactions
CKey coinbaseKey; // private/public key needed to spend coinbase transactions
};


struct TestChain100DeterministicSetup : public TestChain100Setup {
TestChain100DeterministicSetup() : TestChain100Setup(true) { }
};
Copy link
Member

Choose a reason for hiding this comment

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

Any need for this? Seems odd to have an option to make a test non-deterministic

Copy link
Member

Choose a reason for hiding this comment

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


class CTxMemPoolEntry;

struct TestMemPoolEntryHelper
Expand Down
Loading