Remove SHAMapNodeID from SHAMapTreeNode (RIPD-347)

This resolves the "right data, wrong ID" issue in the
tree node cache.
This commit is contained in:
David Schwartz
2014-07-16 16:58:46 -07:00
parent f1bb0afc4e
commit 07db5d497c
6 changed files with 41 additions and 85 deletions

View File

@@ -250,7 +250,6 @@ public:
return;
SHAMapTreeNode newNode(
SHAMapNodeID (node.nodeid().data(), node.nodeid().size()),
Blob (node.nodedata().begin(), node.nodedata().end()),
0, snfWIRE, uZero, false);

View File

@@ -50,7 +50,7 @@ SHAMap::SHAMap (
mTNByID.rehash (STATE_MAP_BUCKETS);
SHAMapNodeID rootID{};
root = std::make_shared<SHAMapTreeNode> (mSeq, rootID);
root = std::make_shared<SHAMapTreeNode> (mSeq);
root->makeInner ();
mTNByID.replace(rootID, root);
}
@@ -74,7 +74,7 @@ SHAMap::SHAMap (
mTNByID.rehash (STATE_MAP_BUCKETS);
SHAMapNodeID rootID{};
root = std::make_shared<SHAMapTreeNode> (mSeq, rootID);
root = std::make_shared<SHAMapTreeNode> (mSeq);
root->makeInner ();
mTNByID.replace(rootID, root);
}
@@ -357,7 +357,7 @@ SHAMapTreeNode* SHAMap::getNodePointerNT (const SHAMapNodeID& id, uint256 const&
if (filter->haveNode (id, hash, nodeData))
{
SHAMapTreeNode::pointer node = std::make_shared<SHAMapTreeNode> (
id, nodeData, 0, snfPREFIX, hash, true);
nodeData, 0, snfPREFIX, hash, true);
canonicalize (hash, node);
// Canonicalize the node with mTNByID to make sure all threads gets the same node
@@ -821,7 +821,7 @@ bool SHAMap::addGiveItem (SHAMapItem::ref item, bool isTransaction, bool hasMeta
assert (node->isEmptyBranch (branch));
SHAMapNodeID newNodeID = nodeID.getChildNodeID (branch);
SHAMapTreeNode::pointer newNode =
std::make_shared<SHAMapTreeNode> (newNodeID, item, type, mSeq);
std::make_shared<SHAMapTreeNode> (item, type, mSeq);
if (!mTNByID.peekMap().emplace (newNodeID, newNode).second)
{
@@ -851,7 +851,7 @@ bool SHAMap::addGiveItem (SHAMapItem::ref item, bool isTransaction, bool hasMeta
// we need a new inner node, since both go on same branch at this level
SHAMapNodeID newNodeID = nodeID.getChildNodeID (b1);
SHAMapTreeNode::pointer newNode =
std::make_shared<SHAMapTreeNode> (mSeq, newNodeID);
std::make_shared<SHAMapTreeNode> (mSeq);
newNode->makeInner ();
if (!mTNByID.peekMap().emplace (newNodeID, newNode).second)
@@ -867,7 +867,7 @@ bool SHAMap::addGiveItem (SHAMapItem::ref item, bool isTransaction, bool hasMeta
assert (node->isInner ());
SHAMapNodeID newNodeID = nodeID.getChildNodeID (b1);
SHAMapTreeNode::pointer newNode =
std::make_shared<SHAMapTreeNode> (newNodeID, item, type, mSeq);
std::make_shared<SHAMapTreeNode> (item, type, mSeq);
assert (newNode->isValid () && newNode->isLeaf ());
if (!mTNByID.peekMap().emplace (newNodeID, newNode).second)
@@ -876,8 +876,7 @@ bool SHAMap::addGiveItem (SHAMapItem::ref item, bool isTransaction, bool hasMeta
node->setChildHash (b1, newNode->getNodeHash ()); // OPTIMIZEME hash op not needed
trackNewNode (newNode, newNodeID);
newNodeID = nodeID.getChildNodeID (b2);
newNode = std::make_shared<SHAMapTreeNode> (newNodeID, otherItem, type,
mSeq);
newNode = std::make_shared<SHAMapTreeNode> (otherItem, type, mSeq);
assert (newNode->isValid () && newNode->isLeaf ());
if (!mTNByID.peekMap().emplace (newNodeID, newNode).second)
@@ -962,7 +961,7 @@ SHAMapTreeNode* SHAMap::getNodeAsync (
return ptr.get ();
// Try the tree node cache
ptr = getCache (hash, id);
ptr = getCache (hash);
if (!ptr)
{
@@ -974,7 +973,7 @@ SHAMapTreeNode* SHAMap::getNodeAsync (
if (filter->haveNode (id, hash, nodeData))
{
ptr = std::make_shared <SHAMapTreeNode> (
id, nodeData, 0, snfPREFIX, hash, true);
nodeData, 0, snfPREFIX, hash, true);
filter->gotNode (true, id, hash, nodeData, ptr->getType ());
}
}
@@ -999,7 +998,7 @@ SHAMapTreeNode* SHAMap::getNodeAsync (
if (!obj)
return nullptr;
ptr = std::make_shared <SHAMapTreeNode> (id, obj->getData(), 0,
ptr = std::make_shared <SHAMapTreeNode> (obj->getData(), 0,
snfPREFIX, hash, true);
}
@@ -1036,7 +1035,7 @@ SHAMap::fetchNodeExternalNT (const SHAMapNodeID& id, uint256 const& hash)
return ret;
// Check the cache of shared, immutable tree nodes
ret = getCache (hash, id);
ret = getCache (hash);
if (ret)
{ // The node was found in the TreeNodeCache
assert (ret->getSeq() == 0);
@@ -1059,7 +1058,7 @@ SHAMap::fetchNodeExternalNT (const SHAMapNodeID& id, uint256 const& hash)
{
// We make this node immutable (seq == 0) so that it can be shared
// CoW is needed if it is modified
ret = std::make_shared<SHAMapTreeNode> (id, obj->getData (), 0, snfPREFIX, hash, true);
ret = std::make_shared<SHAMapTreeNode> (obj->getData (), 0, snfPREFIX, hash, true);
if (ret->getNodeHash () != hash)
{
@@ -1116,7 +1115,7 @@ bool SHAMap::fetchRoot (uint256 const& hash, SHAMapSyncFilter* filter)
if (!filter || !filter->haveNode (SHAMapNodeID (), hash, nodeData))
return false;
root = std::make_shared<SHAMapTreeNode> (SHAMapNodeID (), nodeData,
root = std::make_shared<SHAMapTreeNode> (nodeData,
mSeq - 1, snfPREFIX, hash, true);
mTNByID.replace(SHAMapNodeID (), root);
filter->gotNode (true, SHAMapNodeID (), hash, nodeData, root->getType ());
@@ -1316,34 +1315,16 @@ void SHAMap::dump (bool hash)
for (ripple::unordered_map<SHAMapNodeID, SHAMapTreeNode::pointer, SHAMapNode_hash>::iterator it = mTNByID.peekMap().begin ();
it != mTNByID.peekMap().end (); ++it)
{
WriteLog (lsINFO, SHAMap) << it->second->getString ();
WriteLog (lsINFO, SHAMap) << it->second->getString (it->first);
CondLog (hash, lsINFO, SHAMap) << it->second->getNodeHash ();
}
}
SHAMapTreeNode::pointer SHAMap::getCache (uint256 const& hash, SHAMapNodeID const& id)
SHAMapTreeNode::pointer SHAMap::getCache (uint256 const& hash)
{
SHAMapTreeNode::pointer ret = mTreeNodeCache.fetch (hash);
assert (!ret || !ret->getSeq());
if (ret != nullptr)
{
SHAMapNodeID retID = ret->getID();
if (retID != id)
{
// We have the data, but with a different node ID
WriteLog (lsTRACE, SHAMap) << "ID mismatch: " << id << " != " << retID;
ret = std::make_shared <SHAMapTreeNode> (*ret, 0);
ret->setID(id);
// Future fetches are likely to use the "new" ID
mTreeNodeCache.canonicalize (hash, ret, true);
// HH I see no way to assure this at the moment
assert (ret->getID() == id);
assert (ret->getNodeHash() == hash);
}
}
return ret;
}
@@ -1351,22 +1332,7 @@ void SHAMap::canonicalize (uint256 const& hash, SHAMapTreeNode::pointer& node)
{
assert (node->getSeq() == 0);
SHAMapNodeID id = node->getID();
mTreeNodeCache.canonicalize (hash, node);
// HH I don't see how to ensure that id has not changed under mTreeNodeCache.canonicalize
if (id != node->getID())
{
// The cache has the node with a different ID
node = std::make_shared <SHAMapTreeNode> (*node, 0);
node->setID(id);
// Future fetches are likely to use the newer ID
mTreeNodeCache.canonicalize (hash, node, true);
// HH I don't see how to ensure that id has not changed under mTreeNodeCache.canonicalize
assert (id == node->getID());
}
}
//------------------------------------------------------------------------------

View File

@@ -114,7 +114,7 @@ public:
typedef std::pair<SHAMapItem::pointer, SHAMapItem::pointer> DeltaItem;
typedef std::pair<SHAMapItem::ref, SHAMapItem::ref> DeltaRef;
typedef std::map<uint256, DeltaItem> Delta;
typedef ripple::unordered_map<SHAMapNodeID, SHAMapTreeNode::pointer, SHAMapNode_hash> NodeMap;
typedef ripple::unordered_map<SHAMapNodeID, SHAMapTreeNode::pointer> NodeMap;
typedef std::unordered_set<SHAMapNodeID, SHAMapNode_hash> DirtySet;
typedef boost::shared_mutex LockType;
@@ -257,7 +257,7 @@ private:
void dump (bool withHashes = false);
// tree node cache operations
SHAMapTreeNode::pointer getCache (uint256 const& hash, SHAMapNodeID const& id);
SHAMapTreeNode::pointer getCache (uint256 const& hash);
void canonicalize (uint256 const& hash, SHAMapTreeNode::pointer&);
void dirtyUp (std::stack<std::pair<SHAMapTreeNode::pointer, SHAMapNodeID>>& stack,

View File

@@ -353,14 +353,14 @@ SHAMapAddNode SHAMap::addRootNode (Blob const& rootNode, SHANodeFormat format,
assert (mSeq >= 1);
SHAMapTreeNode::pointer node =
std::make_shared<SHAMapTreeNode> (SHAMapNodeID (), rootNode, mSeq - 1,
std::make_shared<SHAMapTreeNode> (rootNode, mSeq - 1,
format, uZero, false);
if (!node)
return SHAMapAddNode::invalid ();
#ifdef BEAST_DEBUG
node->dump ();
node->dump (SHAMapNodeID ());
#endif
root = node;
@@ -395,7 +395,7 @@ SHAMapAddNode SHAMap::addRootNode (uint256 const& hash, Blob const& rootNode, SH
assert (mSeq >= 1);
SHAMapTreeNode::pointer node =
std::make_shared<SHAMapTreeNode> (SHAMapNodeID (), rootNode, mSeq - 1,
std::make_shared<SHAMapTreeNode> (rootNode, mSeq - 1,
format, uZero, false);
if (!node || node->getNodeHash () != hash)
@@ -477,7 +477,7 @@ SHAMap::addKnownNode (const SHAMapNodeID& node, Blob const& rawNode,
}
SHAMapTreeNode::pointer newNode =
std::make_shared<SHAMapTreeNode> (node, rawNode, 0, snfWIRE,
std::make_shared<SHAMapTreeNode> (rawNode, 0, snfWIRE,
uZero, false);
if (childHash != newNode->getNodeHash ())
@@ -488,7 +488,7 @@ SHAMap::addKnownNode (const SHAMapNodeID& node, Blob const& rawNode,
canonicalize (childHash, newNode);
if (!iNode->isInBounds ())
if (!iNode->isInBounds (iNodeID))
{
// Map is provably invalid
mState = smsInvalid;

View File

@@ -19,9 +19,8 @@
namespace ripple {
SHAMapTreeNode::SHAMapTreeNode (std::uint32_t seq, const SHAMapNodeID& nodeID)
: mID (nodeID)
, mHash (std::uint64_t(0))
SHAMapTreeNode::SHAMapTreeNode (std::uint32_t seq)
: mHash (std::uint64_t(0))
, mSeq (seq)
, mAccessSeq (seq)
, mType (tnERROR)
@@ -31,8 +30,7 @@ SHAMapTreeNode::SHAMapTreeNode (std::uint32_t seq, const SHAMapNodeID& nodeID)
}
SHAMapTreeNode::SHAMapTreeNode (const SHAMapTreeNode& node, std::uint32_t seq)
: mID (node.mID)
, mHash (node.mHash)
: mHash (node.mHash)
, mSeq (seq)
, mType (node.mType)
, mIsBranch (node.mIsBranch)
@@ -44,10 +42,9 @@ SHAMapTreeNode::SHAMapTreeNode (const SHAMapTreeNode& node, std::uint32_t seq)
memcpy (mHashes, node.mHashes, sizeof (mHashes));
}
SHAMapTreeNode::SHAMapTreeNode (const SHAMapNodeID& id, SHAMapItem::ref item,
SHAMapTreeNode::SHAMapTreeNode (SHAMapItem::ref item,
TNType type, std::uint32_t seq)
: mID (id)
, mItem (item)
: mItem (item)
, mSeq (seq)
, mType (type)
, mIsBranch (0)
@@ -57,11 +54,10 @@ SHAMapTreeNode::SHAMapTreeNode (const SHAMapNodeID& id, SHAMapItem::ref item,
updateHash ();
}
SHAMapTreeNode::SHAMapTreeNode (const SHAMapNodeID& id, Blob const& rawNode,
SHAMapTreeNode::SHAMapTreeNode (Blob const& rawNode,
std::uint32_t seq, SHANodeFormat format,
uint256 const& hash, bool hashValid)
: mID (id)
, mSeq (seq)
: mSeq (seq)
, mType (tnERROR)
, mIsBranch (0)
, mFullBelow (false)
@@ -426,17 +422,17 @@ void SHAMapTreeNode::makeInner ()
mHash.zero ();
}
void SHAMapTreeNode::dump ()
void SHAMapTreeNode::dump (const SHAMapNodeID & id)
{
WriteLog (lsDEBUG, SHAMapNodeID) << "SHAMapTreeNode(" << mID.getNodeID () << ")";
WriteLog (lsDEBUG, SHAMapNodeID) << "SHAMapTreeNode(" << id.getNodeID () << ")";
}
std::string SHAMapTreeNode::getString () const
std::string SHAMapTreeNode::getString (const SHAMapNodeID & id) const
{
std::string ret = "NodeID(";
ret += beast::lexicalCastThrow <std::string> (mID.getDepth ());
ret += beast::lexicalCastThrow <std::string> (id.getDepth ());
ret += ",";
ret += to_string (mID.getNodeID ());
ret += to_string (id.getNodeID ());
ret += ")";
if (isInner ())

View File

@@ -55,13 +55,12 @@ public:
SHAMapTreeNode (const SHAMapTreeNode&) = delete;
SHAMapTreeNode& operator= (const SHAMapTreeNode&) = delete;
SHAMapTreeNode (std::uint32_t seq, const SHAMapNodeID & nodeID); // empty node
SHAMapTreeNode (std::uint32_t seq); // empty node
SHAMapTreeNode (const SHAMapTreeNode & node, std::uint32_t seq); // copy node from older tree
SHAMapTreeNode (const SHAMapNodeID & nodeID, SHAMapItem::ref item, TNType type,
std::uint32_t seq);
SHAMapTreeNode (SHAMapItem::ref item, TNType type, std::uint32_t seq);
// raw node functions
SHAMapTreeNode (const SHAMapNodeID & id, Blob const & data, std::uint32_t seq,
SHAMapTreeNode (Blob const & data, std::uint32_t seq,
SHANodeFormat format, uint256 const & hash, bool hashValid);
void addRaw (Serializer&, SHANodeFormat format);
@@ -103,10 +102,10 @@ public:
{
return mType == tnINNER;
}
bool isInBounds () const
bool isInBounds (SHAMapNodeID const &id) const
{
// Nodes at depth 64 must be leaves
return (!isInner() || (mID.getDepth() < 64));
return (!isInner() || (id.getDepth() < 64));
}
bool isValid () const
{
@@ -173,11 +172,8 @@ public:
mFullBelow = true;
}
virtual void dump ();
virtual std::string getString () const;
SHAMapNodeID const& getID() const {return mID;}
void setID(SHAMapNodeID const& id) {mID = id;}
virtual void dump (SHAMapNodeID const&);
virtual std::string getString (SHAMapNodeID const&) const;
/** Descends along the specified branch
* On invocation, nodeID must be the ID of this node
@@ -196,7 +192,6 @@ private:
// VFALCO TODO remove the use of friend
friend class SHAMap;
SHAMapNodeID mID;
uint256 mHash;
uint256 mHashes[16];
SHAMapItem::pointer mItem;