[FOLD] Address levelization issues with Rules

scottschurr · Mar 24, 2022 · edf3aa5 · edf3aa5
1 parent 33c236b
commit edf3aa5
Show file tree

Hide file tree

Showing 16 changed files with 65 additions and 69 deletions.
diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake
@@ -82,6 +82,7 @@ target_sources (xrpl_core PRIVATE
   src/ripple/protocol/impl/PublicKey.cpp
   src/ripple/protocol/impl/Quality.cpp
   src/ripple/protocol/impl/Rate2.cpp
+  src/ripple/protocol/impl/Rules.cpp
   src/ripple/protocol/impl/SField.cpp
   src/ripple/protocol/impl/SOTemplate.cpp
   src/ripple/protocol/impl/STAccount.cpp
@@ -213,6 +214,7 @@ install (
     src/ripple/protocol/PublicKey.h
     src/ripple/protocol/Quality.h
     src/ripple/protocol/Rate.h
+    src/ripple/protocol/Rules.h
     src/ripple/protocol/SField.h
     src/ripple/protocol/SOTemplate.h
     src/ripple/protocol/STAccount.h
@@ -491,7 +493,6 @@ target_sources (rippled PRIVATE
   src/ripple/ledger/impl/RawStateTable.cpp
   src/ripple/ledger/impl/ReadView.cpp
   src/ripple/ledger/impl/View.cpp
-  src/ripple/ledger/impl/Rules.cpp
   #[===============================[
      main sources:
        subdir: net

diff --git a/Builds/levelization/results/loops.txt b/Builds/levelization/results/loops.txt
@@ -40,9 +40,6 @@ Loop: ripple.basics ripple.rpc
 Loop: ripple.core ripple.net
   ripple.net > ripple.core
 
-Loop: ripple.ledger ripple.protocol
-  ripple.ledger > ripple.protocol
-
 Loop: ripple.net ripple.rpc
   ripple.rpc > ripple.net
 

diff --git a/Builds/levelization/results/ordering.txt b/Builds/levelization/results/ordering.txt
@@ -23,6 +23,7 @@ ripple.ledger > ripple.basics
 ripple.ledger > ripple.beast
 ripple.ledger > ripple.core
 ripple.ledger > ripple.json
+ripple.ledger > ripple.protocol
 ripple.net > ripple.basics
 ripple.net > ripple.beast
 ripple.net > ripple.json
@@ -177,7 +178,6 @@ test.protocol > ripple.basics
 test.protocol > ripple.beast
 test.protocol > ripple.crypto
 test.protocol > ripple.json
-test.protocol > ripple.ledger
 test.protocol > ripple.protocol
 test.protocol > test.toplevel
 test.resource > ripple.basics

diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp
@@ -636,7 +636,7 @@ RCLConsensus::Adaptor::doAccept(
         auto const lastVal = ledgerMaster_.getValidatedLedger();
         std::optional<Rules> rules;
         if (lastVal)
-            rules.emplace(*lastVal, app_.config().features);
+            rules = makeRulesGivenLedger(*lastVal, app_.config().features);
         else
             rules.emplace(app_.config().features);
         app_.openLedger().accept(

diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp
@@ -626,7 +626,7 @@ Ledger::setup(Config const& config)
 
     try
     {
-        rules_ = Rules(*this, config.features);
+        rules_ = makeRulesGivenLedger(*this, config.features);
     }
     catch (SHAMapMissingNode const&)
     {

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -1744,7 +1744,7 @@ NetworkOPsImp::switchLastClosedLedger(
         auto const lastVal = app_.getLedgerMaster().getValidatedLedger();
         std::optional<Rules> rules;
         if (lastVal)
-            rules.emplace(*lastVal, app_.config().features);
+            rules = makeRulesGivenLedger(*lastVal, app_.config().features);
         else
             rules.emplace(app_.config().features);
         app_.openLedger().accept(

diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h
@@ -24,8 +24,8 @@
 #include <ripple/app/tx/impl/SignerEntries.h>
 #include <ripple/app/tx/impl/Transactor.h>
 #include <ripple/basics/Log.h>
-#include <ripple/ledger/Rules.h>
 #include <ripple/protocol/Indexes.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/STObject.h>
 #include <ripple/protocol/STTx.h>

diff --git a/src/ripple/app/tx/impl/SignerEntries.cpp b/src/ripple/app/tx/impl/SignerEntries.cpp
@@ -19,7 +19,6 @@
 
 #include <ripple/app/tx/impl/SignerEntries.h>
 #include <ripple/basics/Log.h>
-#include <ripple/ledger/Rules.h>
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/STObject.h>
 #include <cstdint>

diff --git a/src/ripple/app/tx/impl/SignerEntries.h b/src/ripple/app/tx/impl/SignerEntries.h
@@ -23,7 +23,7 @@
 #include <ripple/app/tx/impl/Transactor.h>  // NotTEC
 #include <ripple/basics/Expected.h>         //
 #include <ripple/beast/utility/Journal.h>   // beast::Journal
-#include <ripple/ledger/Rules.h>            // Rules
+#include <ripple/protocol/Rules.h>          // Rules
 #include <ripple/protocol/STTx.h>           // STTx::maxMultiSigners
 #include <ripple/protocol/TER.h>            // temMALFORMED
 #include <ripple/protocol/UintTypes.h>      // AccountID

diff --git a/src/ripple/ledger/ReadView.h b/src/ripple/ledger/ReadView.h
@@ -26,10 +26,10 @@
 #include <ripple/basics/chrono.h>
 #include <ripple/beast/hash/uhash.h>
 #include <ripple/beast/utility/Journal.h>
-#include <ripple/ledger/Rules.h>
 #include <ripple/ledger/detail/ReadViewFwdRange.h>
 #include <ripple/protocol/Indexes.h>
 #include <ripple/protocol/Protocol.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STAmount.h>
 #include <ripple/protocol/STLedgerEntry.h>
 #include <ripple/protocol/STTx.h>
@@ -366,6 +366,11 @@ getCloseAgree(LedgerInfo const& info)
 void
 addRaw(LedgerInfo const&, Serializer&, bool includeHash = false);
 
+Rules
+makeRulesGivenLedger(
+    DigestAwareReadView const& ledger,
+    std::unordered_set<uint256, beast::uhash<>> const& presets);
+
 }  // namespace ripple
 
 #include <ripple/ledger/detail/ReadViewFwdRange.ipp>

diff --git a/src/ripple/ledger/impl/ReadView.cpp b/src/ripple/ledger/impl/ReadView.cpp
@@ -65,4 +65,20 @@ ReadView::txs_type::end() const -> iterator
     return iterator(view_, view_->txsEnd());
 }
 
+Rules
+makeRulesGivenLedger(
+    DigestAwareReadView const& ledger,
+    std::unordered_set<uint256, beast::uhash<>> const& presets)
+{
+    Keylet const k = keylet::amendments();
+    std::optional digest = ledger.digest(k.key);
+    if (digest)
+    {
+        auto const sle = ledger.read(k);
+        if (sle)
+            return Rules(presets, digest, sle->getFieldV256(sfAmendments));
+    }
+    return Rules(presets);
+}
+
 }  // namespace ripple
diff --git a/src/ripple/ledger/Rules.h → src/ripple/protocol/Rules.h b/src/ripple/ledger/Rules.h → src/ripple/protocol/Rules.h
@@ -22,6 +22,7 @@
 
 #include <ripple/basics/base_uint.h>
 #include <ripple/beast/hash/uhash.h>
+#include <ripple/protocol/STVector256.h>
 #include <unordered_set>
 
 namespace ripple {
@@ -34,10 +35,13 @@ class Rules
 private:
     class Impl;
 
+    // Carrying impl by shared_ptr makes Rules comparatively cheap to pass
+    // by value.
     std::shared_ptr<Impl const> impl_;
 
 public:
     Rules(Rules const&) = default;
+
     Rules&
     operator=(Rules const&) = default;
 
@@ -50,27 +54,26 @@ class Rules
     */
     explicit Rules(std::unordered_set<uint256, beast::uhash<>> const& presets);
 
-    /** Construct rules from a ledger.
-
-        The ledger contents are analyzed for rules
-        and amendments and extracted to the object.
-    */
-    explicit Rules(
+private:
+    // Allow a friend function to construct Rules.
+    friend Rules
+    makeRulesGivenLedger(
         DigestAwareReadView const& ledger,
         std::unordered_set<uint256, beast::uhash<>> const& presets);
 
-    /** Returns `true` if a feature is enabled. */
-    bool
-    enabled(uint256 const& id) const;
+    Rules(
+        std::unordered_set<uint256, beast::uhash<>> const& presets,
+        std::optional<uint256> const& digest,
+        STVector256 const& amendments);
 
-    /** Returns `true` if these rules don't match the ledger. */
+public:
+    /** Returns `true` if a feature is enabled. */
     bool
-    changed(DigestAwareReadView const& ledger) const;
+    enabled(uint256 const& feature) const;
 
     /** Returns `true` if two rule sets are identical.
 
-        @note This is for diagnostics. To determine if new
-        rules should be constructed, call changed() first instead.
+        @note This is for diagnostics.
     */
     bool
     operator==(Rules const&) const;

diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h
@@ -21,9 +21,9 @@
 #define RIPPLE_PROTOCOL_STTX_H_INCLUDED
 
 #include <ripple/basics/Expected.h>
-#include <ripple/ledger/Rules.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/PublicKey.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STObject.h>
 #include <ripple/protocol/SecretKey.h>
 #include <ripple/protocol/SeqProxy.h>

diff --git a/src/ripple/ledger/impl/Rules.cpp → src/ripple/protocol/impl/Rules.cpp b/src/ripple/ledger/impl/Rules.cpp → src/ripple/protocol/impl/Rules.cpp
@@ -17,7 +17,9 @@
 */
 //==============================================================================
 
-#include <ripple/ledger/Rules.h>
+#include <ripple/protocol/Rules.h>
+
+#include <ripple/protocol/Indexes.h>
 
 namespace ripple {
 
@@ -34,24 +36,15 @@ class Rules::Impl
     {
     }
 
-    explicit Impl(
-        DigestAwareReadView const& ledger,
-        std::unordered_set<uint256, beast::uhash<>> const& presets)
+    Impl(
+        std::unordered_set<uint256, beast::uhash<>> const& presets,
+        std::optional<uint256> const& digest,
+        STVector256 const& amendments)
         : presets_(presets)
     {
-        auto const k = keylet::amendments();
-        digest_ = ledger.digest(k.key);
-        if (!digest_)
-            return;
-        auto const sle = ledger.read(k);
-        if (!sle)
-        {
-            // LogicError() ?
-            return;
-        }
-
-        for (auto const& item : sle->getFieldV256(sfAmendments))
-            set_.insert(item);
+        digest_ = digest;
+        set_.reserve(amendments.size());
+        set_.insert(amendments.begin(), amendments.end());
     }
 
     bool
@@ -62,17 +55,6 @@ class Rules::Impl
         return set_.count(feature) > 0;
     }
 
-    bool
-    changed(DigestAwareReadView const& ledger) const
-    {
-        auto const digest = ledger.digest(keylet::amendments().key);
-        if (!digest && !digest_)
-            return false;
-        if (!digest || !digest_)
-            return true;
-        return *digest != *digest_;
-    }
-
     bool
     operator==(Impl const& other) const
     {
@@ -84,30 +66,24 @@ class Rules::Impl
     }
 };
 
-Rules::Rules(
-    DigestAwareReadView const& ledger,
-    std::unordered_set<uint256, beast::uhash<>> const& presets)
-    : impl_(std::make_shared<Impl>(ledger, presets))
-{
-}
-
 Rules::Rules(std::unordered_set<uint256, beast::uhash<>> const& presets)
     : impl_(std::make_shared<Impl>(presets))
 {
 }
 
-bool
-Rules::enabled(uint256 const& id) const
+Rules::Rules(
+    std::unordered_set<uint256, beast::uhash<>> const& presets,
+    std::optional<uint256> const& digest,
+    STVector256 const& amendments)
+    : impl_(std::make_shared<Impl>(presets, digest, amendments))
 {
-    assert(impl_);
-    return impl_->enabled(id);
 }
 
 bool
-Rules::changed(DigestAwareReadView const& ledger) const
+Rules::enabled(uint256 const& feature) const
 {
     assert(impl_);
-    return impl_->changed(ledger);
+    return impl_->enabled(feature);
 }
 
 bool

diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp
@@ -22,7 +22,6 @@
 #include <ripple/basics/contract.h>
 #include <ripple/basics/safe_cast.h>
 #include <ripple/json/to_string.h>
-#include <ripple/ledger/ReadView.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/HashPrefix.h>
 #include <ripple/protocol/Protocol.h>

diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp
@@ -20,7 +20,7 @@
 #include <ripple/basics/Slice.h>
 #include <ripple/beast/unit_test.h>
 #include <ripple/json/to_string.h>
-#include <ripple/ledger/Rules.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STAmount.h>
 #include <ripple/protocol/STParsedJSON.h>
 #include <ripple/protocol/STTx.h>
-Original file line number
+Diff line change
@@ Expand Up / @@ -626,7 +626,7 @@ Ledger::setup(Config const& config) @@
         try
         {
-            rules_ = Rules(*this, config.features);
+            rules_ = makeRulesGivenLedger(*this, config.features);
         }
         catch (SHAMapMissingNode const&)
         {
@@ Expand Down @@