From 475805044436bf5989906e0b8a61154ef54aca1e Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Wed, 6 Jul 2016 10:59:51 -0700 Subject: [PATCH] SHAMap fixups: * Change state_ to Invalid if inner node types mismatch * flushDirty before unsharing to ensure modified nodes get written * Remove unnecessary code * Fix updateHash for V2 inner nodes * In descend, call isInconsistentNode only if node is found * getMissingNodes could request duplicates in some cases * An invalid node with the right hash is useful, it proves the map invalid --- src/ripple/shamap/SHAMap.h | 3 +- src/ripple/shamap/SHAMapTreeNode.h | 1 + src/ripple/shamap/impl/SHAMap.cpp | 65 ++++++++------------- src/ripple/shamap/impl/SHAMapSync.cpp | 52 ++++++++--------- src/ripple/shamap/impl/SHAMapTreeNode.cpp | 24 +++++++- src/ripple/shamap/tests/SHAMap.test.cpp | 50 ++++++++-------- src/ripple/shamap/tests/SHAMapSync.test.cpp | 36 ++++++------ 7 files changed, 117 insertions(+), 114 deletions(-) diff --git a/src/ripple/shamap/SHAMap.h b/src/ripple/shamap/SHAMap.h index e8578c1f7c..48e8afe8a6 100644 --- a/src/ripple/shamap/SHAMap.h +++ b/src/ripple/shamap/SHAMap.h @@ -84,7 +84,7 @@ private: std::uint32_t seq_; std::uint32_t ledgerSeq_ = 0; // sequence number of ledger this is part of std::shared_ptr root_; - SHAMapState state_; + mutable SHAMapState state_; SHAMapType type_; bool backed_ = true; // Map is backed by the database @@ -372,6 +372,7 @@ inline bool SHAMap::is_v2() const { + assert (root_); return std::dynamic_pointer_cast(root_) != nullptr; } diff --git a/src/ripple/shamap/SHAMapTreeNode.h b/src/ripple/shamap/SHAMapTreeNode.h index ad31ca8393..b8ec01ecaf 100644 --- a/src/ripple/shamap/SHAMapTreeNode.h +++ b/src/ripple/shamap/SHAMapTreeNode.h @@ -208,6 +208,7 @@ public: bool has_common_prefix(uint256 const& key) const; int get_common_prefix(uint256 const& key) const; void set_common(int depth, uint256 const& key); + bool updateHash () override; void addRaw(Serializer& s, SHANodeFormat format) const override; uint256 const& key() const override; void setChildren(std::shared_ptr const& child1, diff --git a/src/ripple/shamap/impl/SHAMap.cpp b/src/ripple/shamap/impl/SHAMap.cpp index 4a50bccc94..a2678e92a1 100644 --- a/src/ripple/shamap/impl/SHAMap.cpp +++ b/src/ripple/shamap/impl/SHAMap.cpp @@ -111,8 +111,8 @@ SHAMap::make_v2() const t = hotUNKNOWN; break; } - ret->unshare(); ret->flushDirty(t, ret->seq_); + ret->unshare(); return ret; } @@ -144,8 +144,8 @@ SHAMap::make_v1() const t = hotUNKNOWN; break; } - ret->unshare(); ret->flushDirty(t, ret->seq_); + ret->unshare(); return ret; } @@ -184,12 +184,13 @@ SHAMap::walkTowardsKey(uint256 const& id, SharedPtrNodeStack* stack) const assert(stack == nullptr || stack->empty()); auto inNode = root_; SHAMapNodeID nodeID; - if (stack != nullptr) - stack->push({inNode, nodeID}); auto const isv2 = is_v2(); while (inNode->isInner()) { + if (stack != nullptr) + stack->push({inNode, nodeID}); + auto const inner = std::static_pointer_cast(std::move(inNode)); if (isv2) { @@ -208,7 +209,10 @@ SHAMap::walkTowardsKey(uint256 const& id, SharedPtrNodeStack* stack) const { auto n = std::dynamic_pointer_cast(inNode); if (n == nullptr) + { + assert (false); return nullptr; + } nodeID = SHAMapNodeID{n->depth(), n->common()}; } else @@ -220,9 +224,10 @@ SHAMap::walkTowardsKey(uint256 const& id, SharedPtrNodeStack* stack) const { nodeID = nodeID.getChildNodeID (branch); } - if (stack != nullptr) - stack->push({inNode, nodeID}); } + + if (stack != nullptr) + stack->push({inNode, nodeID}); return static_cast(inNode.get()); } @@ -305,26 +310,6 @@ SHAMap::checkFilter(SHAMapHash const& hash, { node = SHAMapAbstractNode::make( nodeData, 0, snfPREFIX, hash, true, f_.journal ()); - if (node && node->isInner()) - { - bool isv2 = std::dynamic_pointer_cast(node) != nullptr; - if (isv2 != is_v2()) - { - auto root = std::dynamic_pointer_cast(root_); - assert(root); - assert(root->isEmpty()); - if (isv2) - { - auto temp = make_v2(); - swap(temp->root_, const_cast&>(root_)); - } - else - { - auto temp = make_v1(); - swap(temp->root_, const_cast&>(root_)); - } - } - } if (node) { filter->gotNode (true, hash, @@ -458,14 +443,15 @@ SHAMap::descend (SHAMapInnerNode * parent, SHAMapNodeID const& parentID, if (!child) { std::shared_ptr childNode = fetchNodeNT (childHash, filter); - if (isInconsistentNode(childNode)) - childNode = nullptr; if (childNode) { childNode = parent->canonicalizeChild (branch, std::move(childNode)); child = childNode.get (); } + + if (child && isInconsistentNode(childNode)) + child = nullptr; } if (child && is_v2()) @@ -512,21 +498,12 @@ SHAMap::descendAsync (SHAMapInnerNode* parent, int branch, ptr = SHAMapAbstractNode::make(obj->getData(), 0, snfPREFIX, hash, true, f_.journal()); - - if (ptr && ptr->isInner()) - { - bool isv2 = std::dynamic_pointer_cast(ptr) != nullptr; - if (isv2 != is_v2()) - { - assert(false); - } - } if (ptr && backed_) canonicalize (hash, ptr); } } - if (isInconsistentNode(ptr)) + if (ptr && isInconsistentNode(ptr)) ptr = nullptr; if (ptr) ptr = parent->canonicalizeChild (branch, std::move(ptr)); @@ -955,7 +932,7 @@ SHAMap::addGiveItem (std::shared_ptr const& item, auto parent_depth = parent->depth(); auto depth = inner->get_common_prefix(tag); auto new_inner = std::make_shared(seq_); - auto nodeID = SHAMapNodeID{depth, prefix(depth, inner->common())}; + nodeID = SHAMapNodeID{depth, prefix(depth, inner->common())}; new_inner->setChild(nodeID.selectBranch(inner->common()), inner); nodeID = SHAMapNodeID{depth, prefix(depth, tag)}; new_inner->setChild(nodeID.selectBranch(tag), @@ -1394,10 +1371,18 @@ SHAMap::invariants() const bool SHAMap::isInconsistentNode(std::shared_ptr const& node) const { + assert(root_); + assert(node); if (std::dynamic_pointer_cast(node) != nullptr) return false; bool is_node_v2 = std::dynamic_pointer_cast(node) != nullptr; - return is_v2() != is_node_v2; + assert (! is_node_v2 || (std::dynamic_pointer_cast(node)->depth() != 0)); + + if (is_v2() == is_node_v2) + return false; + + state_ = SHAMapState::Invalid; + return true; } } // ripple diff --git a/src/ripple/shamap/impl/SHAMapSync.cpp b/src/ripple/shamap/impl/SHAMapSync.cpp index b7c73d813c..95c01867e5 100644 --- a/src/ripple/shamap/impl/SHAMapSync.cpp +++ b/src/ripple/shamap/impl/SHAMapSync.cpp @@ -183,6 +183,7 @@ SHAMap::getMissingNodes(std::size_t max, SHAMapSyncFilter* filter) { if (!pending) { // node is not in the database + missingHashes.insert (childHash); ret.emplace_back ( childID, childHash.as_uint256()); @@ -254,15 +255,15 @@ SHAMap::getMissingNodes(std::size_t max, SHAMapSyncFilter* filter) // Process all deferred reads int hits = 0; - for (auto const& node : deferredReads) + for (auto const& deferredNode : deferredReads) { - auto parent = std::get<0>(node); - auto branch = std::get<1>(node); - auto const& nodeID = std::get<2>(node); + auto parent = std::get<0>(deferredNode); + auto branch = std::get<1>(deferredNode); + auto const& deferredNodeID = std::get<2>(deferredNode); auto const& nodeHash = parent->getChildHash (branch); auto nodePtr = fetchNodeNT(nodeHash, filter); - if (nodePtr && !isInconsistentNode(nodePtr)) + if (nodePtr) { ++hits; if (backed_) @@ -273,7 +274,7 @@ SHAMap::getMissingNodes(std::size_t max, SHAMapSyncFilter* filter) { ret.push_back ( std::make_pair ( - nodeID, + deferredNodeID, nodeHash.as_uint256())); --max; @@ -401,10 +402,10 @@ bool SHAMap::getNodeFat (SHAMapNodeID wanted, else if (childNode->isInner() || fatLeaves) { // Just include this node - Serializer s; - childNode->addRaw (s, snfWIRE); + Serializer ns; + childNode->addRaw (ns, snfWIRE); nodeIDs.push_back (childID); - rawNodes.push_back (std::move (s.peekData ())); + rawNodes.push_back (std::move (ns.peekData ())); } } } @@ -498,18 +499,6 @@ SHAMap::addKnownNode (const SHAMapNodeID& node, Blob const& rawNode, if (iNode == nullptr) { - if ((std::dynamic_pointer_cast(newNode) && !iNodeID.has_common_prefix(node)) || - (!std::dynamic_pointer_cast(newNode) && iNodeID != node)) - { - // Either this node is broken or we didn't request it (yet) - JLOG(journal_.warn()) << "unable to hook node " << node; - JLOG(journal_.info()) << " stuck at " << iNodeID; - JLOG(journal_.info()) << - "got depth=" << node.getDepth () << - ", walked to= " << iNodeID.getDepth (); - return SHAMapAddNode::invalid (); - } - if (!newNode || !newNode->isValid() || childHash != newNode->getNodeHash ()) { JLOG(journal_.warn()) << "Corrupt node received"; @@ -525,16 +514,25 @@ SHAMap::addKnownNode (const SHAMapNodeID& node, Blob const& rawNode, if (newNode && isInconsistentNode(newNode)) { - return SHAMapAddNode::invalid(); + state_ = SHAMapState::Invalid; + return SHAMapAddNode::useful(); + } + + if ((std::dynamic_pointer_cast(newNode) && !iNodeID.has_common_prefix(node)) || + (!std::dynamic_pointer_cast(newNode) && iNodeID != node)) + { + // Either this node is broken or we didn't request it (yet) + JLOG(journal_.warn()) << "unable to hook node " << node; + JLOG(journal_.info()) << " stuck at " << iNodeID; + JLOG(journal_.info()) << + "got depth=" << node.getDepth () << + ", walked to= " << iNodeID.getDepth (); + return SHAMapAddNode::useful (); } if (backed_) - { - auto temp = newNode; canonicalize (childHash, newNode); - if (isInconsistentNode(newNode)) - newNode = temp; - } + newNode = prevNode->canonicalizeChild (branch, std::move(newNode)); if (filter) diff --git a/src/ripple/shamap/impl/SHAMapTreeNode.cpp b/src/ripple/shamap/impl/SHAMapTreeNode.cpp index be3e4fdf69..7cf64a49d9 100644 --- a/src/ripple/shamap/impl/SHAMapTreeNode.cpp +++ b/src/ripple/shamap/impl/SHAMapTreeNode.cpp @@ -202,7 +202,7 @@ SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat f } else if (type == 5) { - // full inner + // full v2 inner if (len != 512) Throw ("invalid FI node"); @@ -224,7 +224,7 @@ SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat f else if (type == 6) { auto ret = std::make_shared(seq); - // compressed inner + // compressed v2 inner for (int i = 0; i < (len / 33); ++i) { int pos; @@ -296,7 +296,7 @@ SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat f auto len = s.getLength(); bool isV2 = (prefix == HashPrefix::innerNodeV2); - if ((len < 512) || (!isV2 && (len != 512))) + if ((len < 512) || (!isV2 && (len != 512)) || (isV2 && (len == 512))) Throw ("invalid PIN node"); std::shared_ptr ret; @@ -498,6 +498,24 @@ SHAMapInnerNodeV2::addRaw(Serializer& s, SHANodeFormat format) const } } +bool +SHAMapInnerNodeV2::updateHash() +{ + uint256 nh; + + if (mIsBranch != 0) + { + Serializer s(580); + addRaw (s, snfPREFIX); + nh = s.getSHA512Half(); + } + + if (nh == mHash.as_uint256()) + return false; + mHash = SHAMapHash{nh}; + return true; +} + void SHAMapTreeNode::addRaw(Serializer& s, SHANodeFormat format) const { diff --git a/src/ripple/shamap/tests/SHAMap.test.cpp b/src/ripple/shamap/tests/SHAMap.test.cpp index ac5601fe00..0dedd65755 100644 --- a/src/ripple/shamap/tests/SHAMap.test.cpp +++ b/src/ripple/shamap/tests/SHAMap.test.cpp @@ -224,14 +224,14 @@ public: std::vector hashes(8); if (is_v2) { - hashes[0].SetHex ("B7387CFEA0465759ADC718E8C42B52D2309D179B326E239EB5075C64B6281F7F"); - hashes[1].SetHex ("6A70885D21024F9F4F50E688B365D0E017266F53AE3E77B52AAEF84E167FE942"); - hashes[2].SetHex ("BA635322BDF510CCFC0578C194C1F87DA2D97C1A55A469F6E7A463BE963663D7"); - hashes[3].SetHex ("5F751361AC7A161DED4D6EAAC4B587C7C01E50E1F9EC3DD61207BD6B196E7DB1"); - hashes[4].SetHex ("FC1C57DD4BF15E37961E0B3064C43E60A9DC26EA332D7A6178FE2284901DB49F"); - hashes[5].SetHex ("4FCDFE944E8E19E35FF60E7BDA7DB21B1CD99670BCF158FEF8F0F8B49BF5C9AD"); - hashes[6].SetHex ("31F6DE8152FBE77EAD59805631FCDDB71F1BC6A5E9BD8FA3948D82D1CE1F93D6"); - hashes[7].SetHex ("00895AD3B161D483C4EF7B5469B0D305685222B5102C2C3F614FD84AD50C4B14"); + hashes[0].SetHex ("90F77DA53895E34042DC8048518CC98AD24276D0A96CCA2C515A83FDAF9F9FC9"); + hashes[1].SetHex ("425A3B6A68FAD9CB43B9981C7D0D39B942FE62110B437201057EE703F5E76390"); + hashes[2].SetHex ("1B4BE72DD18F90F367D64C0147D2414329149724339F79958D6470E7C99E3F4A"); + hashes[3].SetHex ("CCC18ED9B0C353278F02465E2E2F3A8A07427B458CF74C51D87ABE9C1B2ECAD8"); + hashes[4].SetHex ("24AF98675227F387CE0E4932B71B099FE8BC66E5F07BE2DA70D7E7D98E16C8BC"); + hashes[5].SetHex ("EAA373271474A9BF18F1CC240B40C7B5C83C7017977F1388771E56D5943F2B9B"); + hashes[6].SetHex ("C7968A323A06BD46769B402B2A85A7FE7F37FCE99C0004A6197AD8E5D76F200D"); + hashes[7].SetHex ("0A2412DBB16308706211E5FA5B0160817D54757B4DDC0CB105391A79D06B47BA"); } else { @@ -250,11 +250,11 @@ public: map.setUnbacked (); BEAST_EXPECT(map.getHash() == zero); - for (int i = 0; i < keys.size(); ++i) + for (int k = 0; k < keys.size(); ++k) { - SHAMapItem item (keys[i], IntToVUC (i)); + SHAMapItem item (keys[k], IntToVUC (k)); BEAST_EXPECT(map.addItem (std::move(item), true, false)); - BEAST_EXPECT(map.getHash().as_uint256() == hashes[i]); + BEAST_EXPECT(map.getHash().as_uint256() == hashes[k]); map.invariants(); } if (v == SHAMap::version{1}) @@ -264,21 +264,21 @@ public: BEAST_EXPECT(map_v2 != nullptr); BEAST_EXPECT(map_v2->is_v2()); map_v2->invariants(); - auto i1 = map.begin(); + auto m1 = map.begin(); auto e1 = map.end(); - auto i2 = map_v2->begin(); + auto m2 = map_v2->begin(); auto e2 = map_v2->end(); - for (; i1 != e1; ++i1, ++i2) + for (; m1 != e1; ++m1, ++m2) { - BEAST_EXPECT(i2 != e2); - BEAST_EXPECT(*i1 == *i2); + BEAST_EXPECT(m2 != e2); + BEAST_EXPECT(*m1 == *m2); } - BEAST_EXPECT(i2 == e2); + BEAST_EXPECT(m2 == e2); } - for (int i = keys.size() - 1; i >= 0; --i) + for (int k = keys.size() - 1; k >= 0; --k) { - BEAST_EXPECT(map.getHash().as_uint256() == hashes[i]); - BEAST_EXPECT(map.delItem (keys[i])); + BEAST_EXPECT(map.getHash().as_uint256() == hashes[k]); + BEAST_EXPECT(map.delItem (keys[k])); map.invariants(); } BEAST_EXPECT(map.getHash() == zero); @@ -300,8 +300,8 @@ public: keys[6].SetHex ("b91891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); keys[7].SetHex ("292891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); - tests::TestFamily f{beast::Journal{}}; - SHAMap map{SHAMapType::FREE, f, v}; + tests::TestFamily tf{beast::Journal{}}; + SHAMap map{SHAMapType::FREE, tf, v}; if (! backed) map.setUnbacked (); for (auto const& k : keys) @@ -310,11 +310,11 @@ public: map.invariants(); } - int i = 7; + int h = 7; for (auto const& k : map) { - BEAST_EXPECT(k.key() == keys[i]); - --i; + BEAST_EXPECT(k.key() == keys[h]); + --h; } } } diff --git a/src/ripple/shamap/tests/SHAMapSync.test.cpp b/src/ripple/shamap/tests/SHAMapSync.test.cpp index c500d2f2ff..f936fe6634 100644 --- a/src/ripple/shamap/tests/SHAMapSync.test.cpp +++ b/src/ripple/shamap/tests/SHAMapSync.test.cpp @@ -94,9 +94,9 @@ public: void run(SHAMap::version v) { beast::Journal const j; // debug journal - TestFamily f(j); + TestFamily f(j), f2(j); SHAMap source (SHAMapType::FREE, f, v); - SHAMap destination (SHAMapType::FREE, f, v); + SHAMap destination (SHAMapType::FREE, f2, v); int items = 10000; for (int i = 0; i < items; ++i) @@ -133,21 +133,21 @@ public: destination.setSynching (); { - std::vector gotNodeIDs; - std::vector gotNodes; + std::vector gotNodeIDs_a; + std::vector gotNodes_a; BEAST_EXPECT(source.getNodeFat ( SHAMapNodeID (), - gotNodeIDs, - gotNodes, + gotNodeIDs_a, + gotNodes_a, rand_bool(), rand_int(2))); - unexpected (gotNodes.size () < 1, "NodeSize"); + unexpected (gotNodes_a.size () < 1, "NodeSize"); BEAST_EXPECT(destination.addRootNode ( source.getHash(), - *gotNodes.begin (), + *gotNodes_a.begin (), snfWIRE, nullptr).isGood()); } @@ -163,29 +163,29 @@ public: break; // get as many nodes as possible based on this information - std::vector gotNodeIDs; - std::vector gotNodes; + std::vector gotNodeIDs_b; + std::vector gotNodes_b; for (auto& it : nodesMissing) { BEAST_EXPECT(source.getNodeFat ( it.first, - gotNodeIDs, - gotNodes, + gotNodeIDs_b, + gotNodes_b, rand_bool(), rand_int(2))); } - BEAST_EXPECT(gotNodeIDs.size () == gotNodes.size ()); - BEAST_EXPECT(!gotNodeIDs.empty ()); + BEAST_EXPECT(gotNodeIDs_b.size () == gotNodes_b.size ()); + BEAST_EXPECT(!gotNodeIDs_b.empty ()); - for (std::size_t i = 0; i < gotNodeIDs.size(); ++i) + for (std::size_t i = 0; i < gotNodeIDs_b.size(); ++i) { BEAST_EXPECT( destination.addKnownNode ( - gotNodeIDs[i], - gotNodes[i], - nullptr).isGood ()); + gotNodeIDs_b[i], + gotNodes_b[i], + nullptr).isUseful ()); } } while (true);