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

Do not store Merkle branches in the wallet. #6550

Merged
merged 2 commits into from Sep 23, 2015
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/release-notes.md
Expand Up @@ -105,6 +105,15 @@ In this version, it is only enforced for peers that send protocol versions
removed. It is recommended to update SPV clients to check for the `NODE_BLOOM`
service bit for nodes that report versions newer than 70011.

Merkle branches removed from wallet
-----------------------------------

Previously, every wallet transaction stored a Merkle branch to prove its
presence in blocks. This wasn't being used for more than an expensive
sanity check. Since 0.12, these are no longer stored. When loading a
0.12 wallet into an older version, it will automatically rescan to avoid
failed checks.

0.12.0 Change log
=================

Expand Down
2 changes: 1 addition & 1 deletion src/chainparams.cpp
Expand Up @@ -31,7 +31,7 @@ static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesi
genesis.nVersion = nVersion;
genesis.vtx.push_back(txNew);
genesis.hashPrevBlock.SetNull();
genesis.hashMerkleRoot = genesis.BuildMerkleTree();
genesis.hashMerkleRoot = genesis.ComputeMerkleRoot();
return genesis;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core_memusage.h
Expand Up @@ -48,7 +48,7 @@ static inline size_t RecursiveDynamicUsage(const CMutableTransaction& tx) {
}

static inline size_t RecursiveDynamicUsage(const CBlock& block) {
size_t mem = memusage::DynamicUsage(block.vtx) + memusage::DynamicUsage(block.vMerkleTree);
size_t mem = memusage::DynamicUsage(block.vtx);
for (std::vector<CTransaction>::const_iterator it = block.vtx.begin(); it != block.vtx.end(); it++) {
mem += RecursiveDynamicUsage(*it);
}
Expand Down
8 changes: 7 additions & 1 deletion src/main.cpp
Expand Up @@ -2587,6 +2587,9 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
{
// These are checks that are independent of context.

if (block.fChecked)
return true;

// Check that the header is valid (particularly PoW). This is mostly
// redundant with the call in AcceptBlockHeader.
if (!CheckBlockHeader(block, state, fCheckPOW))
Expand All @@ -2595,7 +2598,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
// Check the merkle root.
if (fCheckMerkleRoot) {
bool mutated;
uint256 hashMerkleRoot2 = block.BuildMerkleTree(&mutated);
uint256 hashMerkleRoot2 = block.ComputeMerkleRoot(&mutated);
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Member Author

Choose a reason for hiding this comment

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

const what?

if (block.hashMerkleRoot != hashMerkleRoot2)
return state.DoS(100, error("CheckBlock(): hashMerkleRoot mismatch"),
REJECT_INVALID, "bad-txnmrklroot", true);
Expand Down Expand Up @@ -2642,6 +2645,9 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
return state.DoS(100, error("CheckBlock(): out-of-bounds SigOpCount"),
REJECT_INVALID, "bad-blk-sigops", true);

if (fCheckPOW && fCheckMerkleRoot)
block.fChecked = true;

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/miner.cpp
Expand Up @@ -368,7 +368,7 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned
assert(txCoinbase.vin[0].scriptSig.size() <= 100);

pblock->vtx[0] = txCoinbase;
pblock->hashMerkleRoot = pblock->BuildMerkleTree();
pblock->hashMerkleRoot = pblock->ComputeMerkleRoot();
}

//////////////////////////////////////////////////////////////////////////////
Expand Down
39 changes: 2 additions & 37 deletions src/primitives/block.cpp
Expand Up @@ -15,7 +15,7 @@ uint256 CBlockHeader::GetHash() const
return SerializeHash(*this);
}

uint256 CBlock::BuildMerkleTree(bool* fMutated) const
uint256 CBlock::ComputeMerkleRoot(bool* fMutated) const
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but is there any reason we prefer the optional arguments compared to a tuple return value?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is so much "preferred", it's how it happens to be written between two more-or-less equivalent ways to formulate it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're relying on an optional optimization allowed by the C++ standard (copy elision) to avoid constructing a tuple as a temporary and copying it to the return location. In C++11 it's better because you can explicitly indicate move semantics.

{
/* WARNING! If you're reading this because you're learning about crypto
and/or designing a new system that will use merkle trees, keep in mind
Expand Down Expand Up @@ -52,7 +52,7 @@ uint256 CBlock::BuildMerkleTree(bool* fMutated) const
known ways of changing the transactions without affecting the merkle
root.
*/
vMerkleTree.clear();
std::vector<uint256> vMerkleTree;
vMerkleTree.reserve(vtx.size() * 2 + 16); // Safe upper bound for the number of total nodes.
for (std::vector<CTransaction>::const_iterator it(vtx.begin()); it != vtx.end(); ++it)
vMerkleTree.push_back(it->GetHash());
Expand All @@ -78,37 +78,6 @@ uint256 CBlock::BuildMerkleTree(bool* fMutated) const
return (vMerkleTree.empty() ? uint256() : vMerkleTree.back());
}

std::vector<uint256> CBlock::GetMerkleBranch(int nIndex) const
{
if (vMerkleTree.empty())
BuildMerkleTree();
std::vector<uint256> vMerkleBranch;
int j = 0;
for (int nSize = vtx.size(); nSize > 1; nSize = (nSize + 1) / 2)
{
int i = std::min(nIndex^1, nSize-1);
vMerkleBranch.push_back(vMerkleTree[j+i]);
nIndex >>= 1;
j += nSize;
}
return vMerkleBranch;
}

uint256 CBlock::CheckMerkleBranch(uint256 hash, const std::vector<uint256>& vMerkleBranch, int nIndex)
{
if (nIndex == -1)
return uint256();
for (std::vector<uint256>::const_iterator it(vMerkleBranch.begin()); it != vMerkleBranch.end(); ++it)
{
if (nIndex & 1)
hash = Hash(BEGIN(*it), END(*it), BEGIN(hash), END(hash));
else
hash = Hash(BEGIN(hash), END(hash), BEGIN(*it), END(*it));
nIndex >>= 1;
}
return hash;
}

std::string CBlock::ToString() const
{
std::stringstream s;
Expand All @@ -123,9 +92,5 @@ std::string CBlock::ToString() const
{
s << " " << vtx[i].ToString() << "\n";
}
s << " vMerkleTree: ";
for (unsigned int i = 0; i < vMerkleTree.size(); i++)
s << " " << vMerkleTree[i].ToString();
s << "\n";
return s.str();
}
10 changes: 4 additions & 6 deletions src/primitives/block.h
Expand Up @@ -78,7 +78,7 @@ class CBlock : public CBlockHeader
std::vector<CTransaction> vtx;

// memory only
mutable std::vector<uint256> vMerkleTree;
mutable bool fChecked;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd have this fChecked status (which isn't used in CBlock itself) on an administrative object that wraps a CBlock, instead of on CBlock itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fully agree, I actually started implementing that as a follow-up, but didn't want to interfere with other refactorings.

Copy link
Member

Choose a reason for hiding this comment

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

OK, yes, I'm fine with keeping it like this for this pull


CBlock()
{
Expand All @@ -103,7 +103,7 @@ class CBlock : public CBlockHeader
{
CBlockHeader::SetNull();
vtx.clear();
vMerkleTree.clear();
fChecked = false;
}

CBlockHeader GetBlockHeader() const
Expand All @@ -118,14 +118,12 @@ class CBlock : public CBlockHeader
return block;
}

// Build the in-memory merkle tree for this block and return the merkle root.
// Build the merkle tree for this block and return the merkle root.
// If non-NULL, *mutated is set to whether mutation was detected in the merkle
// tree (a duplication of transactions in the block leading to an identical
// merkle root).
uint256 BuildMerkleTree(bool* mutated = NULL) const;
uint256 ComputeMerkleRoot(bool* mutated = NULL) const;

std::vector<uint256> GetMerkleBranch(int nIndex) const;
static uint256 CheckMerkleBranch(uint256 hash, const std::vector<uint256>& vMerkleBranch, int nIndex);
std::string ToString() const;
};

Expand Down
2 changes: 1 addition & 1 deletion src/test/miner_tests.cpp
Expand Up @@ -87,7 +87,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
pblock->vtx[0] = CTransaction(txCoinbase);
if (txFirst.size() < 2)
txFirst.push_back(new CTransaction(pblock->vtx[0]));
pblock->hashMerkleRoot = pblock->BuildMerkleTree();
pblock->hashMerkleRoot = pblock->ComputeMerkleRoot();
pblock->nNonce = blockinfo[i].nonce;
CValidationState state;
BOOST_CHECK(ProcessNewBlock(state, NULL, pblock, true, NULL));
Expand Down
2 changes: 1 addition & 1 deletion src/test/pmt_tests.cpp
Expand Up @@ -48,7 +48,7 @@ BOOST_AUTO_TEST_CASE(pmt_test1)
}

// calculate actual merkle root and height
uint256 merkleRoot1 = block.BuildMerkleTree();
uint256 merkleRoot1 = block.ComputeMerkleRoot();
std::vector<uint256> vTxid(nTx, uint256());
for (unsigned int j=0; j<nTx; j++)
vTxid[j] = block.vtx[j].GetHash();
Expand Down
15 changes: 1 addition & 14 deletions src/wallet/wallet.cpp
Expand Up @@ -702,9 +702,8 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD
wtx.hashBlock = wtxIn.hashBlock;
fUpdated = true;
}
if (wtxIn.nIndex != -1 && (wtxIn.vMerkleBranch != wtx.vMerkleBranch || wtxIn.nIndex != wtx.nIndex))
if (wtxIn.nIndex != -1 && (wtxIn.nIndex != wtx.nIndex))
{
wtx.vMerkleBranch = wtxIn.vMerkleBranch;
wtx.nIndex = wtxIn.nIndex;
fUpdated = true;
}
Expand Down Expand Up @@ -2812,15 +2811,11 @@ int CMerkleTx::SetMerkleBranch(const CBlock& block)
break;
if (nIndex == (int)block.vtx.size())
{
vMerkleBranch.clear();
nIndex = -1;
LogPrintf("ERROR: SetMerkleBranch(): couldn't find tx in block\n");
return 0;
}

// Fill in merkle branch
vMerkleBranch = block.GetMerkleBranch(nIndex);

// Is the tx in a block that's in the main chain
BlockMap::iterator mi = mapBlockIndex.find(hashBlock);
if (mi == mapBlockIndex.end())
Expand All @@ -2846,14 +2841,6 @@ int CMerkleTx::GetDepthInMainChainINTERNAL(const CBlockIndex* &pindexRet) const
if (!pindex || !chainActive.Contains(pindex))
return 0;

// Make sure the merkle branch connects to this block
if (!fMerkleVerified)
{
if (CBlock::CheckMerkleBranch(GetHash(), vMerkleBranch, nIndex) != pindex->hashMerkleRoot)
return 0;
fMerkleVerified = true;
}

pindexRet = pindex;
return chainActive.Height() - pindex->nHeight + 1;
}
Expand Down
7 changes: 1 addition & 6 deletions src/wallet/wallet.h
Expand Up @@ -151,13 +151,8 @@ class CMerkleTx : public CTransaction

public:
uint256 hashBlock;
std::vector<uint256> vMerkleBranch;
int nIndex;

// memory only
mutable bool fMerkleVerified;


CMerkleTx()
{
Init();
Expand All @@ -172,13 +167,13 @@ class CMerkleTx : public CTransaction
{
hashBlock = uint256();
nIndex = -1;
fMerkleVerified = false;
}

ADD_SERIALIZE_METHODS;

template <typename Stream, typename Operation>
inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
std::vector<uint256> vMerkleBranch; // For compatibility with older versions.
READWRITE(*(CTransaction*)this);
nVersion = this->nVersion;
READWRITE(hashBlock);
Expand Down
6 changes: 4 additions & 2 deletions src/wallet/walletdb.cpp
Expand Up @@ -131,12 +131,14 @@ bool CWalletDB::EraseWatchOnly(const CScript &dest)
bool CWalletDB::WriteBestBlock(const CBlockLocator& locator)
{
nWalletDBUpdated++;
return Write(std::string("bestblock"), locator);
Write(std::string("bestblock"), CBlockLocator()); // Write empty block locator so versions that require a merkle branch automatically rescan
return Write(std::string("bestblock_nomerkle"), locator);
}

bool CWalletDB::ReadBestBlock(CBlockLocator& locator)
{
return Read(std::string("bestblock"), locator);
if (Read(std::string("bestblock"), locator) && !locator.vHave.empty()) return true;
return Read(std::string("bestblock_nomerkle"), locator);
}

bool CWalletDB::WriteOrderPosNext(int64_t nOrderPosNext)
Expand Down