Merge remote-tracking branch 'origin' into vvysokikh1/fix-positive-balance-trustline-pay-no-reserve

include/xrpl/basics/TaggedCache.h

@@ -170,6 +170,9 @@ public:
     bool
     retrieve(key_type const& key, T& data);
 
+    mutex_type&
+    peekMutex();
+
     std::vector<key_type>
     getKeys() const;
 

include/xrpl/basics/TaggedCache.ipp

@@ -668,6 +668,29 @@ TaggedCache<
     return true;
 }
 
+template <
+    class Key,
+    class T,
+    bool IsKeyCache,
+    class SharedWeakUnionPointer,
+    class SharedPointerType,
+    class Hash,
+    class KeyEqual,
+    class Mutex>
+inline auto
+TaggedCache<
+    Key,
+    T,
+    IsKeyCache,
+    SharedWeakUnionPointer,
+    SharedPointerType,
+    Hash,
+    KeyEqual,
+    Mutex>::peekMutex() -> mutex_type&
+{
+    return m_mutex;
+}
+
 template <
     class Key,
     class T,

include/xrpl/protocol/detail/features.macro

@@ -32,6 +32,7 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.
 
+XRPL_FIX    (TrustLineOwnerCount,        Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (IncludeKeyletFields,        Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(DynamicMPT,                 Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (TokenEscrowV1,              Supported::yes, VoteBehavior::DefaultNo)

src/libxrpl/ledger/View.cpp

@@ -1790,6 +1790,26 @@ rippleCreditIOU(
             // Receiver reserve is clear.
         }
 
+        // Check if receiver's balance went from zero/negative to positive
+        // In this case, the receiver should now be charged for the trust line
+        // Note: balance is in sender's terms, so receiver going from 0 to
+        // positive means sender going from 0 to negative
+        if (view.rules().enabled(fixTrustLineOwnerCount) &&
+            saBefore >= beast::zero
+            // Sender balance was non-negative (receiver was non-positive).
+            && saBalance < beast::zero
+            // Sender is now negative (receiver is now positive).
+            && !(uFlags & (bSenderHigh ? lsfLowReserve : lsfHighReserve)))
+        // Receiver reserve is not set.
+        {
+            adjustOwnerCount(
+                view, view.peek(keylet::account(uReceiverID)), 1, j);
+
+            sleRippleState->setFieldU32(
+                sfFlags,
+                uFlags | (bSenderHigh ? lsfLowReserve : lsfHighReserve));
+        }
+
         if (bSenderHigh)
             saBalance.negate();
 

src/test/app/Offer_test.cpp

@@ -2409,11 +2409,11 @@ public:
             {"abe",             reserve(env, 0) + 0 * f,    1,   gwPreTrust, 1000,      tecUNFUNDED_OFFER,               f, USD(      0),    0, 0},
             {"bud",             reserve(env, 0) + 1 * f,    1,   gwPreTrust, 1000,      tecUNFUNDED_OFFER,               f, USD(      0),    0, 0},
             {"che",             reserve(env, 0) + 2 * f,    0,   gwPreTrust, 1000, tecINSUF_RESERVE_OFFER,               f, USD(      0),    0, 0},
-            {"dan", drops(10) + reserve(env, 0) + 1 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, drops(10)   + f, USD(0.00001),    0, 0},
-            {"eli", XRP(  20) + reserve(env, 0) + 1 * f, 1000,   gwPreTrust, 1000,             tesSUCCESS, XRP(20) + 1 * f, USD(     20),    0, 0},
+            {"dan", drops(10) + reserve(env, 0) + 1 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, drops(10)   + f, USD(0.00001),    0, features[fixTrustLineOwnerCount] ? 1 : 0},
+            {"eli", XRP(  20) + reserve(env, features[fixTrustLineOwnerCount] ? 1 : 0) + 1 * f, 1000,   gwPreTrust, 1000,             tesSUCCESS, XRP(20)     + f, USD(     20),    0, features[fixTrustLineOwnerCount] ? 1 : 0},
             {"fyn",             reserve(env, 1) + 0 * f,    0,   gwPreTrust, 1000,             tesSUCCESS,               f, USD(      0),    1, 1},
-            {"gar",             reserve(env, 1) + 0 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, XRP( 1) +     f, USD(      1),    1, 1},
-            {"hal",             reserve(env, 1) + 1 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, XRP( 1) +     f, USD(      1),    1, 1},
+            {"gar",             reserve(env, features[fixTrustLineOwnerCount] ? 2 : 1) + 0 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, XRP( 1) +     f, USD(      1),    1, features[fixTrustLineOwnerCount] ? 2 : 1},
+            {"hal",             reserve(env, features[fixTrustLineOwnerCount] ? 2 : 1) + 1 * f,    1,   gwPreTrust, 1000,             tesSUCCESS, XRP( 1) +     f, USD(      1),    1, features[fixTrustLineOwnerCount] ? 2 : 1},
 
             {"ned",             reserve(env, 1) + 0 * f,    1, acctPreTrust, 1000,      tecUNFUNDED_OFFER,           2 * f, USD(      0),    0, 1},
             {"ole",             reserve(env, 1) + 1 * f,    1, acctPreTrust, 1000,      tecUNFUNDED_OFFER,           2 * f, USD(      0),    0, 1},
@@ -2468,6 +2468,7 @@ public:
             BEAST_EXPECT(env.balance(acct, USD.issue()) == t.balanceUsd);
             BEAST_EXPECT(
                 env.balance(acct, xrpIssue()) == t.fundXrp - t.spentXrp);
+
             env.require(offers(acct, t.offers));
             env.require(owners(acct, t.owners));
 
@@ -5463,6 +5464,7 @@ class Offer_manual_test : public OfferBaseUtil_test
         testAll(all);
 
         testAll(all - takerDryOffer - permDEX);
+        testAll(all - fixTrustLineOwnerCount);
     }
 };
 

src/test/app/TrustAndBalance_test.cpp

@@ -458,6 +458,79 @@ class TrustAndBalance_test : public beast::unit_test::suite
         BEAST_EXPECT(wsc->invoke("unsubscribe", jv)[jss::status] == "success");
     }
 
+    void
+    testOwnerCountOnBalanceChange(FeatureBitset features)
+    {
+        testcase("Owner Count on Balance Change");
+        using namespace test::jtx;
+
+        Env env{*this, features};
+        Account gw{"gateway"};
+        Account alice{"alice"};
+        Account market{"market"};
+        bool const aliceHigh = alice.id() > gw.id();
+
+        auto const USD = gw["USD"];
+
+        // Helper lambda to check alice's owner count and reserve flag
+        auto checkAlice = [&](std::uint32_t expectedOwnerCount,
+                              bool expectedReserveSet) {
+            BEAST_EXPECT(
+                env.le(alice)->getFieldU32(sfOwnerCount) == expectedOwnerCount);
+            auto const line =
+                env.le(keylet::line(alice, gw, to_currency("USD")));
+            BEAST_EXPECT(
+                line->isFlag(aliceHigh ? lsfHighReserve : lsfLowReserve) ==
+                expectedReserveSet);
+        };
+
+        env.fund(XRP(10000), gw, alice, market);
+        env.close();
+
+        // create trust lines
+        env(trust(alice, USD(1000)));
+        env(trust(market, USD(1000)));
+        env.close();
+        checkAlice(1, true);
+
+        // gw issues USD to alice and market
+        env(pay(gw, alice, USD(100)));
+        env(pay(gw, market, USD(1000)));
+        env.close();
+        checkAlice(1, true);
+
+        // gw clears asfDefaultRipple
+        env(fclear(gw, asfDefaultRipple));
+        env.close();
+
+        // alice clears trustline limit. This should trigger the check for
+        // default ripple state and charge gw for non-default state
+        env(trust(alice, USD(0)));
+        env.close();
+        checkAlice(1, true);
+
+        // alice clears the balance causing decrement in owners count
+        env(pay(alice, gw, USD(100)));
+        env.close();
+        checkAlice(0, false);
+
+        // market offers USD for XRP
+        env(offer(market, XRP(100), USD(100)));
+        env.close();
+        // Now alice acquires balance again by placing an offer (direct payment
+        // would fail because alice set limit to 0). This offer will cross
+        // market's. Balance goes from 0 to positive - this triggers the new
+        // increment logic
+        env(offer(alice, USD(50), XRP(50)));
+        env.close();
+
+        BEAST_EXPECT(env.balance(alice, USD) == USD(50));
+        if (features[fixTrustLineOwnerCount])
+            checkAlice(1, true);
+        else
+            checkAlice(0, false);
+    }
+
 public:
     void
     run() override
@@ -477,11 +550,13 @@ public:
             testIndirectMultiPath(true, features);
             testIndirectMultiPath(false, features);
             testInvoiceID(features);
+            testOwnerCountOnBalanceChange(features);
         };
 
         using namespace test::jtx;
         auto const sa = testable_amendments();
         testWithFeatures(sa - featurePermissionedDEX);
+        testWithFeatures(sa - fixTrustLineOwnerCount);
         testWithFeatures(sa);
     }
 };

src/xrpld/app/ledger/LedgerHistory.cpp

@@ -27,6 +27,8 @@
 
 namespace ripple {
 
+// FIXME: Need to clean up ledgers by index at some point
+
 LedgerHistory::LedgerHistory(
     beast::insight::Collector::ptr const& collector,
     Application& app)
@@ -45,7 +47,6 @@ LedgerHistory::LedgerHistory(
           std::chrono::minutes{5},
           stopwatch(),
           app_.journal("TaggedCache"))
-    , mLedgersByIndex(256)
     , j_(app.journal("LedgerHistory"))
 {
 }
@@ -62,16 +63,12 @@ LedgerHistory::insert(
         ledger->stateMap().getHash().isNonZero(),
         "ripple::LedgerHistory::insert : nonzero hash");
 
+    std::unique_lock sl(m_ledgers_by_hash.peekMutex());
+
     bool const alreadyHad = m_ledgers_by_hash.canonicalize_replace_cache(
         ledger->info().hash, ledger);
     if (validated)
-    {
         mLedgersByIndex[ledger->info().seq] = ledger->info().hash;
-        JLOG(j_.info()) << "LedgerHistory::insert: mLedgersByIndex size: "
-                        << mLedgersByIndex.size() << " , total size: "
-                        << mLedgersByIndex.size() *
-                (sizeof(LedgerIndex) + sizeof(LedgerHash));
-    }
 
     return alreadyHad;
 }
@@ -79,18 +76,25 @@ LedgerHistory::insert(
 LedgerHash
 LedgerHistory::getLedgerHash(LedgerIndex index)
 {
-    if (auto p = mLedgersByIndex.get(index))
-        return *p;
+    std::unique_lock sl(m_ledgers_by_hash.peekMutex());
+    if (auto it = mLedgersByIndex.find(index); it != mLedgersByIndex.end())
+        return it->second;
     return {};
 }
 
 std::shared_ptr<Ledger const>
 LedgerHistory::getLedgerBySeq(LedgerIndex index)
 {
-    if (auto p = mLedgersByIndex.get(index))
     {
-        uint256 const hash = *p;
-        return getLedgerByHash(hash);
+        std::unique_lock sl(m_ledgers_by_hash.peekMutex());
+        auto it = mLedgersByIndex.find(index);
+
+        if (it != mLedgersByIndex.end())
+        {
+            uint256 hash = it->second;
+            sl.unlock();
+            return getLedgerByHash(hash);
+        }
     }
 
     std::shared_ptr<Ledger const> ret = loadByIndex(index, app_);
@@ -104,16 +108,13 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index)
 
     {
         // Add this ledger to the local tracking by index
+        std::unique_lock sl(m_ledgers_by_hash.peekMutex());
+
         XRPL_ASSERT(
             ret->isImmutable(),
             "ripple::LedgerHistory::getLedgerBySeq : immutable result ledger");
         m_ledgers_by_hash.canonicalize_replace_client(ret->info().hash, ret);
         mLedgersByIndex[ret->info().seq] = ret->info().hash;
-        JLOG(j_.info())
-            << "LedgerHistory::getLedgerBySeq: mLedgersByIndex size: "
-            << mLedgersByIndex.size() << " , total size: "
-            << mLedgersByIndex.size() *
-                (sizeof(LedgerIndex) + sizeof(LedgerHash));
         return (ret->info().seq == index) ? ret : nullptr;
     }
 }
@@ -457,6 +458,8 @@ LedgerHistory::builtLedger(
     XRPL_ASSERT(
         !hash.isZero(), "ripple::LedgerHistory::builtLedger : nonzero hash");
 
+    std::unique_lock sl(m_consensus_validated.peekMutex());
+
     auto entry = std::make_shared<cv_entry>();
     m_consensus_validated.canonicalize_replace_client(index, entry);
 
@@ -497,6 +500,8 @@ LedgerHistory::validatedLedger(
         !hash.isZero(),
         "ripple::LedgerHistory::validatedLedger : nonzero hash");
 
+    std::unique_lock sl(m_consensus_validated.peekMutex());
+
     auto entry = std::make_shared<cv_entry>();
     m_consensus_validated.canonicalize_replace_client(index, entry);
 
@@ -530,13 +535,13 @@ LedgerHistory::validatedLedger(
 bool
 LedgerHistory::fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash)
 {
-    if (auto cur = mLedgersByIndex.get(ledgerIndex))
+    std::unique_lock sl(m_ledgers_by_hash.peekMutex());
+    auto it = mLedgersByIndex.find(ledgerIndex);
+
+    if ((it != mLedgersByIndex.end()) && (it->second != ledgerHash))
     {
-        if (*cur != ledgerHash)
-        {
-            mLedgersByIndex.put(ledgerIndex, ledgerHash);
-            return false;
-        }
+        it->second = ledgerHash;
+        return false;
     }
     return true;
 }

src/xrpld/app/ledger/LedgerHistory.h

@@ -22,7 +22,6 @@
 
 #include <xrpld/app/ledger/Ledger.h>
 #include <xrpld/app/main/Application.h>
-#include <xrpld/core/detail/LRUCache.h>
 
 #include <xrpl/beast/insight/Collector.h>
 #include <xrpl/protocol/RippleLedgerHash.h>
@@ -150,8 +149,7 @@ private:
     ConsensusValidated m_consensus_validated;
 
     // Maps ledger indexes to the corresponding hash.
-    LRUCache<LedgerIndex, LedgerHash, concurrency::ExclusiveMutex>
-        mLedgersByIndex;  // validated ledgers
+    std::map<LedgerIndex, LedgerHash> mLedgersByIndex;  // validated ledgers
 
     beast::Journal j_;
 };

src/xrpld/core/detail/LRUCache.h

@@ -1,250 +0,0 @@
-//------------------------------------------------------------------------------
-/*
-    This file is part of rippled: https://github.com/ripple/rippled
-    Copyright (c) 2012, 2013 Ripple Labs Inc.
-
-    Permission to use, copy, modify, and/or distribute this software for any
-    purpose  with  or without fee is hereby granted, provided that the above
-    copyright notice and this permission notice appear in all copies.
-
-    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
-    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
-    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-*/
-//==============================================================================
-
-#ifndef RIPPLE_APP_LRU_CACHE_H_INCLUDED
-#define RIPPLE_APP_LRU_CACHE_H_INCLUDED
-
-#include <list>
-#include <mutex>
-#include <stdexcept>
-#include <type_traits>
-#include <unordered_map>
-#include <utility>
-
-namespace ripple {
-
-namespace concurrency {
-
-struct SingleThreaded
-{
-    struct mutex_type
-    {
-        void
-        lock() noexcept
-        {
-        }
-        void
-        unlock() noexcept
-        {
-        }
-    };
-    using lock_guard = std::lock_guard<mutex_type>;
-};
-
-struct ExclusiveMutex
-{
-    using mutex_type = std::mutex;
-    using lock_guard = std::lock_guard<mutex_type>;
-};
-
-}  // namespace concurrency
-
-template <
-    class Key,
-    class Value,
-    class Concurrency = concurrency::SingleThreaded>
-class LRUCache
-{
-    using List = std::list<Key>;                     // MRU .. LRU
-    using DataMap = std::unordered_map<Key, Value>;  // Key -> Value
-    using PosMap =
-        std::unordered_map<Key, typename List::iterator>;  // Key -> pos
-                                                           // iterator in the
-                                                           // list
-
-public:
-    explicit LRUCache(std::size_t capacity) : capacity_(capacity)
-    {
-        if (capacity_ == 0)
-            throw std::invalid_argument("LRUCache capacity must be positive.");
-        data_.reserve(capacity_);
-        pos_.reserve(capacity_);
-
-        // TODO:
-        //  static_assert(std::is_default_constructible_v<Value>,
-        //        "LRUCache requires Value to be default-constructible for
-        //        operator[]");
-        //  static_assert(std::is_copy_constructible_v<Key> ||
-        //  std::is_move_constructible_v<Key>,
-        //        "LRUCache requires Key to be copy- or move-constructible");
-    }
-
-    LRUCache(LRUCache const&) = delete;
-
-    LRUCache&
-    operator=(LRUCache const&) = delete;
-
-    LRUCache(LRUCache&&) = delete;
-
-    LRUCache&
-    operator=(LRUCache&&) = delete;
-
-    Value&
-    operator[](Key const& key)
-    {
-        auto g = lock();
-        return insertOrpromote(key);
-    }
-
-    Value&
-    operator[](Key&& key)
-    {
-        auto g = lock();
-        auto it = data_.find(key);
-        if (it != data_.end())
-        {
-            promote(key);
-            return it->second;
-        }
-        evictIfFull();
-        usage_.emplace_front(std::move(key));
-        pos_.emplace(usage_.front(), usage_.begin());
-        auto d = data_.emplace(usage_.front(), Value{});
-        return d.first->second;
-    }
-
-    Value*
-    get(Key const& key)
-    {
-        auto g = lock();
-        auto it = data_.find(key);
-        if (it == data_.end())
-            return nullptr;
-        promote(key);
-        return &it->second;
-    }
-
-    template <class... Args>
-    Value&
-    put(Key const& key, Args&&... args)
-    {
-        auto g = lock();
-        if (auto it = data_.find(key); it != data_.end())
-        {
-            it->second = Value(std::forward<Args>(args)...);
-            promote(key);
-            return it->second;
-        }
-        evictIfFull();
-        usage_.emplace_front(key);
-        pos_.emplace(key, usage_.begin());
-        auto it = data_.emplace(key, Value(std::forward<Args>(args)...));
-        return it.first->second;
-    }
-
-    bool
-    erase(Key const& key)
-    {
-        auto g = lock();
-        auto it = data_.find(key);
-        if (it == data_.end())
-            return false;
-        usage_.erase(pos_.at(key));
-        pos_.erase(key);
-        data_.erase(it);
-        return true;
-    }
-
-    bool
-    contains(Key const& key) const
-    {
-        auto g = lock();
-        return data_.find(key) != data_.end();
-    }
-
-    std::size_t
-    size() const noexcept
-    {
-        auto g = lock();
-        return data_.size();
-    }
-
-    std::size_t
-    capacity() const noexcept
-    {
-        return capacity_;
-    }
-
-    bool
-    empty() const noexcept
-    {
-        auto g = lock();
-        return data_.empty();
-    }
-
-    void
-    clear()
-    {
-        auto g = lock();
-        data_.clear();
-        pos_.clear();
-        usage_.clear();
-    }
-
-private:
-    Value&
-    insertOrpromote(Key const& key)
-    {
-        if (auto it = data_.find(key); it != data_.end())
-        {
-            promote(key);
-            return it->second;
-        }
-        evictIfFull();
-        usage_.emplace_front(key);
-        pos_.emplace(key, usage_.begin());
-        auto it = data_.emplace(key, Value{});
-        return it.first->second;
-    }
-
-    void
-    promote(Key const& key)
-    {
-        auto lit = pos_.at(key);
-        usage_.splice(usage_.begin(), usage_, lit);  // O(1)
-    }
-
-    void
-    evictIfFull()
-    {
-        if (data_.size() < capacity_)
-            return;
-        auto const& k = usage_.back();
-        data_.erase(k);
-        pos_.erase(k);
-        usage_.pop_back();
-    }
-
-    typename Concurrency::lock_guard
-    lock() const
-    {
-        return typename Concurrency::lock_guard{mtx_};
-    }
-
-private:
-    std::size_t const capacity_;
-    DataMap data_;
-    PosMap pos_;
-    List usage_;
-    mutable typename Concurrency::mutex_type mtx_;
-};
-
-}  // namespace ripple
-
-#endif