diff --git a/src/ripple/consensus/LedgerTrie.h b/src/ripple/consensus/LedgerTrie.h index 44faa863a..1368bf09e 100644 --- a/src/ripple/consensus/LedgerTrie.h +++ b/src/ripple/consensus/LedgerTrie.h @@ -395,6 +395,28 @@ class LedgerTrie return std::make_pair(curr, pos); } + /** Find the node in the trie with an exact match to the given ledger ID + + @return the found node or nullptr if an exact match was not found. + + @note O(n) since this searches all nodes until a match is found + */ + Node* + findByLedgerID(Ledger const& ledger, Node* parent = nullptr) const + { + if (!parent) + parent = root.get(); + if (ledger.id() == parent->span.tip().id) + return parent; + for (auto const& child : parent->children) + { + auto cl = findByLedgerID(ledger, child.get()); + if (cl) + return cl; + } + return nullptr; + } + void dumpImpl(std::ostream& o, std::unique_ptr const& curr, int offset) const @@ -513,58 +535,51 @@ public: bool remove(Ledger const& ledger, std::uint32_t count = 1) { - Node* loc; - Seq diffSeq; - std::tie(loc, diffSeq) = find(ledger); + Node* loc = findByLedgerID(ledger); + // Must be exact match with tip support + if (!loc || loc->tipSupport == 0) + return false; - if (loc) + // found our node, remove it + count = std::min(count, loc->tipSupport); + loc->tipSupport -= count; + + auto const it = seqSupport.find(ledger.seq()); + assert(it != seqSupport.end() && it->second >= count); + it->second -= count; + if (it->second == 0) + seqSupport.erase(it->first); + + Node* decNode = loc; + while (decNode) { - // Must be exact match with tip support - if (diffSeq == loc->span.end() && diffSeq > ledger.seq() && - loc->tipSupport > 0) - { - count = std::min(count, loc->tipSupport); - loc->tipSupport -= count; - - auto const it = seqSupport.find(ledger.seq()); - assert(it != seqSupport.end() && it->second >= count); - it->second -= count; - if (it->second == 0) - seqSupport.erase(it->first); - - Node* decNode = loc; - while (decNode) - { - decNode->branchSupport -= count; - decNode = decNode->parent; - } - - while (loc->tipSupport == 0 && loc != root.get()) - { - Node* parent = loc->parent; - if (loc->children.empty()) - { - // this node can be erased - parent->erase(loc); - } - else if (loc->children.size() == 1) - { - // This node can be combined with its child - std::unique_ptr child = - std::move(loc->children.front()); - child->span = merge(loc->span, child->span); - child->parent = parent; - parent->children.emplace_back(std::move(child)); - parent->erase(loc); - } - else - break; - loc = parent; - } - return true; - } + decNode->branchSupport -= count; + decNode = decNode->parent; } - return false; + + while (loc->tipSupport == 0 && loc != root.get()) + { + Node* parent = loc->parent; + if (loc->children.empty()) + { + // this node can be erased + parent->erase(loc); + } + else if (loc->children.size() == 1) + { + // This node can be combined with its child + std::unique_ptr child = + std::move(loc->children.front()); + child->span = merge(loc->span, child->span); + child->parent = parent; + parent->children.emplace_back(std::move(child)); + parent->erase(loc); + } + else + break; + loc = parent; + } + return true; } /** Return count of tip support for the specific ledger. @@ -575,12 +590,7 @@ public: std::uint32_t tipSupport(Ledger const& ledger) const { - Node const* loc; - Seq diffSeq; - std::tie(loc, diffSeq) = find(ledger); - - // Exact match - if (loc && diffSeq == loc->span.end() && diffSeq > ledger.seq()) + if (auto const* loc = findByLedgerID(ledger)) return loc->tipSupport; return 0; } @@ -594,17 +604,16 @@ public: std::uint32_t branchSupport(Ledger const& ledger) const { - Node const* loc; - Seq diffSeq; - std::tie(loc, diffSeq) = find(ledger); - - // Check that ledger is is an exact match or proper - // prefix of loc - if (loc && diffSeq > ledger.seq() && ledger.seq() < loc->span.end()) + Node const* loc = findByLedgerID(ledger); + if (!loc) { - return loc->branchSupport; + Seq diffSeq; + std::tie(loc, diffSeq) = find(ledger); + // Check that ledger is a proper prefix of loc + if (! (diffSeq > ledger.seq() && ledger.seq() < loc->span.end())) + loc = nullptr; } - return 0; + return loc ? loc->branchSupport : 0; } /** Return the preferred ledger ID diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index ee0def94c..83587b813 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -224,12 +224,114 @@ class RCLValidations_test : public beast::unit_test::suite } } + void + testLedgerTrieRCLValidatedLedger() + { + testcase("RCLValidatedLedger LedgerTrie"); + + // This test exposes an issue with the limited 256 + // ancestor hash design of RCLValidatedLedger. + // There is only a single chain of validated ledgers + // but the 256 gap causes a "split" in the LedgerTrie + // due to the lack of ancestry information for a later ledger. + // This exposes a bug in which we are unable to remove + // support for a ledger hash which is already in the trie. + + using Seq = RCLValidatedLedger::Seq; + using ID = RCLValidatedLedger::ID; + + // Max known ancestors for each ledger + Seq const maxAncestors = 256; + std::vector> history; + + // Generate a chain of 256 + 10 ledgers + jtx::Env env(*this); + auto& j = env.journal; + Config config; + auto prev = std::make_shared( + create_genesis, config, std::vector{}, env.app().family()); + history.push_back(prev); + for (auto i = 0; i < (maxAncestors + 10); ++i) + { + auto next = std::make_shared( + *prev, env.app().timeKeeper().closeTime()); + next->updateSkipList(); + history.push_back(next); + prev = next; + } + + LedgerTrie trie; + + // First, create the single branch trie, with ledgers + // separated by exactly 256 ledgers + auto ledg_002 = RCLValidatedLedger{history[1], j}; + auto ledg_258 = RCLValidatedLedger{history[257], j}; + auto ledg_259 = RCLValidatedLedger{history[258], j}; + + trie.insert(ledg_002); + trie.insert(ledg_258, 4); + // trie.dump(std::cout); + // 000000[0,1)(T:0,B:5) + // |-AB868A..36C8[1,3)(T:1,B:5) + // |-AB868A..37C8[3,259)(T:4,B:4) + BEAST_EXPECT(trie.tipSupport(ledg_002) == 1); + BEAST_EXPECT(trie.branchSupport(ledg_002) == 5); + BEAST_EXPECT(trie.tipSupport(ledg_258) == 4); + BEAST_EXPECT(trie.branchSupport(ledg_258) == 4); + + // Move three of the s258 ledgers to s259, which splits the trie + // due to the 256 ancestory limit + BEAST_EXPECT( + trie.remove(ledg_258, 3)); + trie.insert(ledg_259, 3); + trie.getPreferred(1); + // trie.dump(std::cout); + // 000000[0,1)(T:0,B:5) + // |-AB868A..37C9[1,260)(T:3,B:3) + // |-AB868A..36C8[1,3)(T:1,B:2) + // |-AB868A..37C8[3,259)(T:1,B:1) + BEAST_EXPECT(trie.tipSupport(ledg_002) == 1); + BEAST_EXPECT(trie.branchSupport(ledg_002) == 2); + BEAST_EXPECT(trie.tipSupport(ledg_258) == 1); + BEAST_EXPECT(trie.branchSupport(ledg_258) == 1); + BEAST_EXPECT(trie.tipSupport(ledg_259) == 3); + BEAST_EXPECT(trie.branchSupport(ledg_259) == 3); + + // The last call to trie.getPreferred cycled the children of the root + // node to make the new branch the first child (since it has support 3) + // then verify the remove call works + // past bug: remove had assumed the first child of a node in the trie + // which matches is the *only* child in the trie which matches. + // This is **NOT** true with the limited 256 ledger ancestory + // quirk of RCLValidation and prevents deleting the old support + // for ledger 257 + + BEAST_EXPECT( + trie.remove(RCLValidatedLedger{history[257], env.journal}, 1)); + trie.insert(RCLValidatedLedger{history[258], env.journal}, 1); + trie.getPreferred(1); + // trie.dump(std::cout); + // 000000[0,1)(T:0,B:5) + // |-AB868A..37C9[1,260)(T:4,B:4) + // |-AB868A..36C8[1,3)(T:1,B:1) + BEAST_EXPECT(trie.tipSupport(ledg_002) == 1); + BEAST_EXPECT(trie.branchSupport(ledg_002) == 1); + BEAST_EXPECT(trie.tipSupport(ledg_258) == 0); + // 258 no longer lives on a tip in the tree, BUT it is an ancestor + // of 259 which is a tip and therefore gets it's branchSupport value + // implicitly + BEAST_EXPECT(trie.branchSupport(ledg_258) == 4); + BEAST_EXPECT(trie.tipSupport(ledg_259) == 4); + BEAST_EXPECT(trie.branchSupport(ledg_259) == 4); + } + public: void run() override { testChangeTrusted(); testRCLValidatedLedger(); + testLedgerTrieRCLValidatedLedger(); } };