Improve directory insertion & deletion (RIPD-1353, RIPD-1488):

This commit introduces the "SortedDirectories" amendment, which
addresses two distinct issues:

First, it corrects a technical flaw that could, in some edge cases,
prevent an empty intermediate page from being deleted.

Second, it sorts directory entries within a page (other than order
book page entries, which remain strictly FIFO). This makes insert
operations deterministic, instead of pseudo-random and reliant on
temporal ordering.

Lastly, it removes the ability to perform a "soft delete" where
the page number of the item to delete need not be known if the
item is in the first 20 pages, and enforces a maximum limit to
the number of pages that a directory can span.

Builds/VisualStudio2015/RippleD.vcxproj

@@ -2117,6 +2117,10 @@
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
     </ClCompile>
+    <ClCompile Include="..\..\src\ripple\ledger\impl\ApplyView.cpp">
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
+    </ClCompile>
     <ClCompile Include="..\..\src\ripple\ledger\impl\ApplyViewBase.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>

Builds/VisualStudio2015/RippleD.vcxproj.filters

@@ -2760,6 +2760,9 @@
     <ClCompile Include="..\..\src\ripple\ledger\impl\ApplyStateTable.cpp">
       <Filter>ripple\ledger\impl</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\src\ripple\ledger\impl\ApplyView.cpp">
+      <Filter>ripple\ledger\impl</Filter>
+    </ClCompile>
     <ClCompile Include="..\..\src\ripple\ledger\impl\ApplyViewBase.cpp">
       <Filter>ripple\ledger\impl</Filter>
     </ClCompile>

src/ripple/app/main/Amendments.cpp

@@ -55,7 +55,8 @@ supportedAmendments ()
         { "86E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B72EAACED318F74886AE90 CryptoConditionsSuite" },
         { "42EEA5E28A97824821D4EF97081FE36A54E9593C6E4F20CBAE098C69D2E072DC fix1373" },
         { "DC9CA96AEA1DCF83E527D1AFC916EFAF5D27388ECA4060A88817C1238CAEE0BF EnforceInvariants" },
-        { "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" }
+        { "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" },
+        { "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" }
     };
 }
 

src/ripple/app/tx/impl/CancelTicket.cpp

@@ -79,7 +79,7 @@ CancelTicket::doApply ()
 
     auto viewJ = ctx_.app.journal ("View");
     TER const result = dirDelete (ctx_.view (), false, hint,
-        getOwnerDirIndex (ticket_owner), ticketId, false, (hint == 0), viewJ);
+        keylet::ownerDir (ticket_owner), ticketId, false, (hint == 0), viewJ);
 
     adjustOwnerCount(view(), view().peek(
         keylet::account(ticket_owner)), -1, viewJ);

src/ripple/app/tx/impl/CreateOffer.cpp

@@ -1291,74 +1291,74 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel)
     // We need to place the remainder of the offer into its order book.
     auto const offer_index = getOfferIndex (account_, uSequence);
 
-    std::uint64_t uOwnerNode;
-
     // Add offer to owner's directory.
-    std::tie(result, std::ignore) = dirAdd(sb, uOwnerNode,
+    auto const ownerNode = dirAdd(sb, keylet::ownerDir (account_),
-        keylet::ownerDir (account_), offer_index,
+        offer_index, false, describeOwnerDir (account_), viewJ);
-        describeOwnerDir (account_), viewJ);
 
-    if (result == tesSUCCESS)
+    if (!ownerNode)
-    {
-        // Update owner count.
-        adjustOwnerCount(sb, sleCreator, 1, viewJ);
-
-        JLOG (j_.trace()) <<
-            "adding to book: " << to_string (saTakerPays.issue ()) <<
-            " : " << to_string (saTakerGets.issue ());
-
-        Book const book { saTakerPays.issue(), saTakerGets.issue() };
-        std::uint64_t uBookNode;
-        bool isNewBook;
-
-        // Add offer to order book, using the original rate
-        // before any crossing occured.
-        auto dir = keylet::quality (keylet::book (book), uRate);
-
-        std::tie(result, isNewBook) = dirAdd (sb, uBookNode,
-            dir, offer_index, [&](SLE::ref sle)
-            {
-                sle->setFieldH160 (sfTakerPaysCurrency,
-                    saTakerPays.issue().currency);
-                sle->setFieldH160 (sfTakerPaysIssuer,
-                    saTakerPays.issue().account);
-                sle->setFieldH160 (sfTakerGetsCurrency,
-                    saTakerGets.issue().currency);
-                sle->setFieldH160 (sfTakerGetsIssuer,
-                    saTakerGets.issue().account);
-                sle->setFieldU64 (sfExchangeRate, uRate);
-            }, viewJ);
-
-        if (result == tesSUCCESS)
-        {
-            auto sleOffer = std::make_shared<SLE>(ltOFFER, offer_index);
-            sleOffer->setAccountID (sfAccount, account_);
-            sleOffer->setFieldU32 (sfSequence, uSequence);
-            sleOffer->setFieldH256 (sfBookDirectory, dir.key);
-            sleOffer->setFieldAmount (sfTakerPays, saTakerPays);
-            sleOffer->setFieldAmount (sfTakerGets, saTakerGets);
-            sleOffer->setFieldU64 (sfOwnerNode, uOwnerNode);
-            sleOffer->setFieldU64 (sfBookNode, uBookNode);
-            if (expiration)
-                sleOffer->setFieldU32 (sfExpiration, *expiration);
-            if (bPassive)
-                sleOffer->setFlag (lsfPassive);
-            if (bSell)
-                sleOffer->setFlag (lsfSell);
-            sb.insert(sleOffer);
-
-            if (isNewBook)
-                ctx_.app.getOrderBookDB().addOrderBook(book);
-        }
-    }
-
-    if (result != tesSUCCESS)
     {
         JLOG (j_.debug()) <<
-            "final result: " << transToken (result);
+            "final result: failed to add offer to owner's directory";
+        return { tecDIR_FULL, true };
     }
 
-    return { result, true };
+    // Update owner count.
+    adjustOwnerCount(sb, sleCreator, 1, viewJ);
+
+    JLOG (j_.trace()) <<
+        "adding to book: " << to_string (saTakerPays.issue ()) <<
+        " : " << to_string (saTakerGets.issue ());
+
+    Book const book { saTakerPays.issue(), saTakerGets.issue() };
+
+    // Add offer to order book, using the original rate
+    // before any crossing occured.
+    auto dir = keylet::quality (keylet::book (book), uRate);
+    bool const bookExisted = static_cast<bool>(sb.peek (dir));
+
+    auto const bookNode = dirAdd (sb, dir, offer_index, true,
+        [&](SLE::ref sle)
+        {
+            sle->setFieldH160 (sfTakerPaysCurrency,
+                saTakerPays.issue().currency);
+            sle->setFieldH160 (sfTakerPaysIssuer,
+                saTakerPays.issue().account);
+            sle->setFieldH160 (sfTakerGetsCurrency,
+                saTakerGets.issue().currency);
+            sle->setFieldH160 (sfTakerGetsIssuer,
+                saTakerGets.issue().account);
+            sle->setFieldU64 (sfExchangeRate, uRate);
+        }, viewJ);
+
+    if (!bookNode)
+    {
+        JLOG (j_.debug()) <<
+            "final result: failed to add offer to book";
+        return { tecDIR_FULL, true };
+    }
+
+    auto sleOffer = std::make_shared<SLE>(ltOFFER, offer_index);
+    sleOffer->setAccountID (sfAccount, account_);
+    sleOffer->setFieldU32 (sfSequence, uSequence);
+    sleOffer->setFieldH256 (sfBookDirectory, dir.key);
+    sleOffer->setFieldAmount (sfTakerPays, saTakerPays);
+    sleOffer->setFieldAmount (sfTakerGets, saTakerGets);
+    sleOffer->setFieldU64 (sfOwnerNode, *ownerNode);
+    sleOffer->setFieldU64 (sfBookNode, *bookNode);
+    if (expiration)
+        sleOffer->setFieldU32 (sfExpiration, *expiration);
+    if (bPassive)
+        sleOffer->setFlag (lsfPassive);
+    if (bSell)
+        sleOffer->setFlag (lsfSell);
+    sb.insert(sleOffer);
+
+    if (!bookExisted)
+        ctx_.app.getOrderBookDB().addOrderBook(book);
+
+    JLOG (j_.debug()) << "final result: success";
+
+    return { tesSUCCESS, true };
 }
 
 TER

src/ripple/app/tx/impl/CreateTicket.cpp

@@ -99,27 +99,24 @@ CreateTicket::doApply ()
             sleTicket->setAccountID (sfTarget, target_account);
     }
 
-    std::uint64_t hint;
-
     auto viewJ = ctx_.app.journal ("View");
 
-    auto result = dirAdd(view(), hint, keylet::ownerDir (account_),
+    auto const page = dirAdd(view(), keylet::ownerDir (account_),
-        sleTicket->key(), describeOwnerDir (account_), viewJ);
+        sleTicket->key(), false, describeOwnerDir (account_), viewJ);
 
     JLOG(j_.trace()) <<
         "Creating ticket " << to_string (sleTicket->key()) <<
-        ": " << transHuman (result.first);
+        ": " << (page ? "success" : "failure");
 
-    if (result.first == tesSUCCESS)
+    if (!page)
-    {
+        return tecDIR_FULL;
-        sleTicket->setFieldU64(sfOwnerNode, hint);
 
-        // If we succeeded, the new entry counts agains the
+    sleTicket->setFieldU64(sfOwnerNode, *page);
-        // creator's reserve.
-        adjustOwnerCount(view(), sle, 1, viewJ);
-    }
 
-    return result.first;
+    // If we succeeded, the new entry counts agains the
+    // creator's reserve.
+    adjustOwnerCount(view(), sle, 1, viewJ);
+    return tesSUCCESS;
 }
 
 }

src/ripple/app/tx/impl/Escrow.cpp

@@ -255,13 +255,11 @@ EscrowCreate::doApply()
 
     // Add escrow to owner directory
     {
-        uint64_t page;
+        auto page = dirAdd(ctx_.view(), keylet::ownerDir(account), slep->key(),
-        auto result = dirAdd(ctx_.view(), page,
+            false, describeOwnerDir(account), ctx_.app.journal ("View"));
-            keylet::ownerDir(account), slep->key(),
+        if (!page)
-            describeOwnerDir(account), ctx_.app.journal ("View"));
+            return tecDIR_FULL;
-        if (! isTesSuccess(result.first))
+        (*slep)[sfOwnerNode] = *page;
-            return result.first;
-        (*slep)[sfOwnerNode] = page;
     }
 
     // Deduct owner's balance, increment owner count
@@ -431,7 +429,7 @@ EscrowFinish::doApply()
     {
         auto const page = (*slep)[sfOwnerNode];
         TER const ter = dirDelete(ctx_.view(), true,
-            page, keylet::ownerDir(account).key,
+            page, keylet::ownerDir(account),
                 k.key, false, page == 0, ctx_.app.journal ("View"));
         if (! isTesSuccess(ter))
             return ter;
@@ -497,7 +495,7 @@ EscrowCancel::doApply()
     {
         auto const page = (*slep)[sfOwnerNode];
         TER const ter = dirDelete(ctx_.view(), true,
-            page, keylet::ownerDir(account).key,
+            page, keylet::ownerDir(account),
                 k.key, false, page == 0, ctx_.app.journal ("View"));
         if (! isTesSuccess(ter))
             return ter;

src/ripple/app/tx/impl/PayChan.cpp

@@ -134,7 +134,7 @@ closeChannel (
     // Remove PayChan from owner directory
     {
         auto const page = (*slep)[sfOwnerNode];
-        TER const ter = dirDelete (view, true, page, keylet::ownerDir (src).key,
+        TER const ter = dirDelete (view, true, page, keylet::ownerDir (src),
             key, false, page == 0, j);
         if (!isTesSuccess (ter))
             return ter;
@@ -239,13 +239,11 @@ PayChanCreate::doApply()
 
     // Add PayChan to owner directory
     {
-        uint64_t page;
+        auto page = dirAdd (ctx_.view(), keylet::ownerDir(account), slep->key(),
-        auto result = dirAdd (ctx_.view (), page, keylet::ownerDir (account),
+            false, describeOwnerDir (account), ctx_.app.journal ("View"));
-            slep->key (), describeOwnerDir (account),
+        if (!page)
-            ctx_.app.journal ("View"));
+            return tecDIR_FULL;
-        if (!isTesSuccess (result.first))
+        (*slep)[sfOwnerNode] = *page;
-            return result.first;
-        (*slep)[sfOwnerNode] = page;
     }
 
     // Deduct owner's balance, increment owner count

src/ripple/app/tx/impl/SetSignerList.cpp

@@ -236,24 +236,21 @@ SetSignerList::replaceSignerList ()
 
     auto viewJ = ctx_.app.journal ("View");
     // Add the signer list to the account's directory.
-    std::uint64_t hint;
+    auto page = dirAdd(ctx_.view (), ownerDirKeylet,
-
+        signerListKeylet.key, false, describeOwnerDir (account_), viewJ);
-    auto result = dirAdd(ctx_.view (), hint, ownerDirKeylet,
-        signerListKeylet.key, describeOwnerDir (account_), viewJ);
 
     JLOG(j_.trace()) << "Create signer list for account " <<
-        toBase58(account_) << ": " << transHuman (result.first);
+        toBase58(account_) << ": " << (page ? "success" : "failure");
 
-    if (result.first == tesSUCCESS)
+    if (!page)
-    {
+        return tecDIR_FULL;
-        signerList->setFieldU64 (sfOwnerNode, hint);
 
-        // If we succeeded, the new entry counts against the
+    signerList->setFieldU64 (sfOwnerNode, *page);
-        // creator's reserve.
-        adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
-    }
 
-    return result.first;
+    // If we succeeded, the new entry counts against the
+    // creator's reserve.
+    adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
+    return tesSUCCESS;
 }
 
 TER
@@ -293,7 +290,7 @@ SetSignerList::removeSignersFromLedger (Keylet const& accountKeylet,
 
     auto viewJ = ctx_.app.journal ("View");
     TER const result  = dirDelete(ctx_.view(), false, hint,
-        ownerDirKeylet.key, signerListKeylet.key, false, (hint == 0), viewJ);
+        ownerDirKeylet, signerListKeylet.key, false, (hint == 0), viewJ);
 
     if (result == tesSUCCESS)
         adjustOwnerCount(view(),

src/ripple/ledger/ApplyView.h

@@ -22,6 +22,7 @@
 
 #include <ripple/ledger/RawView.h>
 #include <ripple/ledger/ReadView.h>
+#include <boost/optional.hpp>
 
 namespace ripple {
 
@@ -102,8 +103,16 @@ operator&(ApplyFlags const& lhs,
 class ApplyView
     : public ReadView
 {
-public:
+private:
+    /** Add an entry to a directory using the specified insert strategy */
+    boost::optional<std::uint64_t>
+    dirAdd (
+        bool preserveOrder,
+        Keylet const& directory,
+        uint256 const& key,
+        std::function<void(std::shared_ptr<SLE> const&)> const& describe);
 
+public:
     ApplyView () = default;
 
     /** Returns the tx apply flags.
@@ -212,6 +221,113 @@ public:
         std::uint32_t cur, std::uint32_t next)
     {};
 
+    /** Append an entry to a directory
+
+        Entries in the directory will be stored in order of insertion, i.e. new
+        entries will always be added at the tail end of the last page.
+
+        @param directory the base of the directory
+        @param key the entry to insert
+        @param describe callback to add required entries to a new page
+
+        @return a \c boost::optional which, if insertion was successful,
+                will contain the page number in which the item was stored.
+
+        @note this function may create a page (including a root page), if no
+              page with space is available. This function will only fail if the
+              page counter exceeds the protocol-defined maximum number of
+              allowable pages.
+    */
+    /** @{ */
+    boost::optional<std::uint64_t>
+    dirAppend (
+        Keylet const& directory,
+        uint256 const& key,
+        std::function<void(std::shared_ptr<SLE> const&)> const& describe)
+    {
+        return dirAdd (true, directory, key, describe);
+    }
+
+    boost::optional<std::uint64_t>
+    dirAppend (
+        Keylet const& directory,
+        Keylet const& key,
+        std::function<void(std::shared_ptr<SLE> const&)> const& describe)
+    {
+        return dirAppend (directory, key.key, describe);
+    }
+    /** @} */
+
+    /** Insert an entry to a directory
+
+        Entries in the directory will be stored in a semi-random order, but
+        each page will be maintained in sorted order.
+
+        @param directory the base of the directory
+        @param key the entry to insert
+        @param describe callback to add required entries to a new page
+
+        @return a \c boost::optional which, if insertion was successful,
+                will contain the page number in which the item was stored.
+
+        @note this function may create a page (including a root page), if no
+              page with space is available.this function will only fail if the
+              page counter exceeds the protocol-defined maximum number of
+              allowable pages.
+    */
+    /** @{ */
+    boost::optional<std::uint64_t>
+    dirInsert (
+        Keylet const& directory,
+        uint256 const& key,
+        std::function<void(std::shared_ptr<SLE> const&)> const& describe)
+    {
+        return dirAdd (false, directory, key, describe);
+    }
+
+    boost::optional<std::uint64_t>
+    dirInsert (
+        Keylet const& directory,
+        Keylet const& key,
+        std::function<void(std::shared_ptr<SLE> const&)> const& describe)
+    {
+        return dirInsert (directory, key.key, describe);
+    }
+    /** @} */
+
+    /** Remove an entry from a directory
+
+        @param directory the base of the directory
+        @param page the page number for this page
+        @param key the entry to remove
+        @param keepRoot if deleting the last entry, don't
+                        delete the root page (i.e. the directory itself).
+
+        @return \c true if the entry was found and deleted and
+                \c false otherwise.
+
+        @note This function will remove zero or more pages from the directory;
+              the root page will not be deleted even if it is empty, unless
+              \p keepRoot is not set and the directory is empty.
+    */
+    /** @{ */
+    bool
+    dirRemove (
+        Keylet const& directory,
+        std::uint64_t page,
+        uint256 const& key,
+        bool keepRoot);
+
+    bool
+    dirRemove (
+        Keylet const& directory,
+        std::uint64_t page,
+        Keylet const& key,
+        bool keepRoot)
+    {
+        return dirRemove (directory, page, key.key, keepRoot);
+    }
+    /** @} */
 };
 
 } // ripple

src/ripple/ledger/View.h

@@ -223,36 +223,21 @@ dirNext (ApplyView& view,
 std::function<void (SLE::ref)>
 describeOwnerDir(AccountID const& account);
 
-// <--     uNodeDir: For deletion, present to make dirDelete efficient.
+// deprecated
-// -->   uRootIndex: The index of the base of the directory.  Nodes are based off of this.
+boost::optional<std::uint64_t>
-// --> uLedgerIndex: Value to add to directory.
-// Only append. This allow for things that watch append only structure to just monitor from the last node on ward.
-// Within a node with no deletions order of elements is sequential.  Otherwise, order of elements is random.
-
-/** Add an entry to directory, creating the directory if necessary
-
-    @param uNodeDir node of entry - makes deletion efficient
-    @param uRootIndex The index of the base of the directory.
-                      Nodes are based off of this.
-    @param uLedgerIndex Value to add to directory.
-
-    @return a pair containing a code indicating success or
-            failure, and if successful, a boolean indicating
-            whether the directory was just created.
-*/
-std::pair<TER, bool>
 dirAdd (ApplyView& view,
-    std::uint64_t&                      uNodeDir,      // Node of entry.
     Keylet const&                       uRootIndex,
     uint256 const&                      uLedgerIndex,
+    bool                                strictOrder,
     std::function<void (SLE::ref)>      fDescriber,
     beast::Journal j);
 
+// deprecated
 TER
 dirDelete (ApplyView& view,
     const bool           bKeepRoot,
-    const std::uint64_t& uNodeDir,      // Node item is mentioned in.
+    std::uint64_t        uNodeDir,      // Node item is mentioned in.
-    uint256 const&       uRootIndex,
+    Keylet const&        uRootIndex,
     uint256 const&       uLedgerIndex,  // Item being deleted
     const bool           bStable,
     const bool           bSoft,

src/ripple/ledger/impl/ApplyView.cpp

@@ -0,0 +1,275 @@
+//------------------------------------------------------------------------------
+/*
+    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.
+*/
+//==============================================================================
+
+#include <BeastConfig.h>
+#include <ripple/ledger/ApplyView.h>
+#include <ripple/basics/contract.h>
+#include <ripple/protocol/Protocol.h>
+#include <cassert>
+
+namespace ripple {
+
+boost::optional<std::uint64_t>
+ApplyView::dirAdd (
+    bool preserveOrder,
+    Keylet const& directory,
+    uint256 const& key,
+    std::function<void(std::shared_ptr<SLE> const&)> const& describe)
+{
+    auto root = peek(directory);
+
+    if (! root)
+    {
+        // No root, make it.
+        root = std::make_shared<SLE>(directory);
+        root->setFieldH256 (sfRootIndex, directory.key);
+        describe (root);
+
+        STVector256 v;
+        v.push_back (key);
+        root->setFieldV256 (sfIndexes, v);
+
+        insert (root);
+        return std::uint64_t{0};
+    }
+
+    std::uint64_t page = root->getFieldU64(sfIndexPrevious);
+
+    auto node = root;
+
+    if (page)
+    {
+        node = peek (keylet::page(directory, page));
+        if (!node)
+            LogicError ("Directory chain: root back-pointer broken.");
+    }
+
+    auto indexes = node->getFieldV256(sfIndexes);
+
+    // If there's space, we use it:
+    if (indexes.size () < dirNodeMaxEntries)
+    {
+        if (preserveOrder)
+        {
+            if (std::find(indexes.begin(), indexes.end(), key) != indexes.end())
+                LogicError ("dirInsert: double insertion");
+
+            indexes.push_back(key);
+        }
+        else
+        {
+            // We can't be sure if this page is already sorted because
+            // it may be a legacy page we haven't yet touched. Take
+            // the time to sort it.
+            std::sort (indexes.begin(), indexes.end());
+
+            auto pos = std::lower_bound(indexes.begin(), indexes.end(), key);
+
+            if (pos != indexes.end() && key == *pos)
+                LogicError ("dirInsert: double insertion");
+
+            indexes.insert (pos, key);
+        }
+
+        node->setFieldV256 (sfIndexes, indexes);
+        update(node);
+        return page;
+    }
+
+    // Check whether we're out of pages.
+    if (++page >= dirNodeMaxPages)
+        return boost::none;
+
+    // We are about to create a new node; we'll link it to
+    // the chain first:
+    node->setFieldU64 (sfIndexNext, page);
+    update(node);
+
+    root->setFieldU64 (sfIndexPrevious, page);
+    update(root);
+
+    // Insert the new key:
+    indexes.clear();
+    indexes.push_back (key);
+
+    node = std::make_shared<SLE>(keylet::page(directory, page));
+    node->setFieldH256 (sfRootIndex, directory.key);
+    node->setFieldV256 (sfIndexes, indexes);
+
+    // Save some space by not specifying the value 0 since
+    // it's the default.
+    if (page != 1)
+        node->setFieldU64 (sfIndexPrevious, page - 1);
+    describe (node);
+    insert (node);
+
+    return page;
+}
+
+bool
+ApplyView::dirRemove (
+    Keylet const& directory,
+    std::uint64_t page,
+    uint256 const& key,
+    bool keepRoot)
+{
+    auto node = peek(keylet::page(directory, page));
+
+    if (!node)
+        return false;
+
+    std::uint64_t constexpr rootPage = 0;
+
+    {
+        auto entries = node->getFieldV256(sfIndexes);
+
+        auto it = std::find(entries.begin(), entries.end(), key);
+
+        if (entries.end () == it)
+            return false;
+
+        // We always preserve the relative order when we remove.
+        entries.erase(it);
+
+        node->setFieldV256(sfIndexes, entries);
+        update(node);
+
+        if (!entries.empty())
+            return true;
+    }
+
+    // The current page is now empty; check if it can be
+    // deleted, and, if so, whether the entire directory
+    // can now be removed.
+    auto prevPage = node->getFieldU64(sfIndexPrevious);
+    auto nextPage = node->getFieldU64(sfIndexNext);
+
+    // The first page is the directory's root node and is
+    // treated specially: it can never be deleted even if
+    // it is empty, unless we plan on removing the entire
+    // directory.
+    if (page == rootPage)
+    {
+        if (nextPage == page && prevPage != page)
+            LogicError ("Directory chain: fwd link broken");
+
+        if (prevPage == page && nextPage != page)
+            LogicError ("Directory chain: rev link broken");
+
+        // Older versions of the code would, in some cases,
+        // allow the last page to be empty. Remove such
+        // pages if we stumble on them:
+        if (nextPage == prevPage && nextPage != page)
+        {
+            auto last = peek(keylet::page(directory, nextPage));
+            if (!last)
+                LogicError ("Directory chain: fwd link broken.");
+
+            if (last->getFieldV256 (sfIndexes).empty())
+            {
+                // Update the first page's linked list and
+                // mark it as updated.
+                node->setFieldU64 (sfIndexNext, page);
+                node->setFieldU64 (sfIndexPrevious, page);
+                update(node);
+
+                // And erase the empty last page:
+                erase(last);
+
+                // Make sure our local values reflect the
+                // updated information:
+                nextPage = page;
+                prevPage = page;
+            }
+        }
+
+        if (keepRoot)
+            return true;
+
+        // If there's no other pages, erase the root:
+        if (nextPage == page && prevPage == page)
+            erase(node);
+
+        return true;
+    }
+
+    // This can never happen for nodes other than the root:
+    if (nextPage == page)
+        LogicError ("Directory chain: fwd link broken");
+
+    if (prevPage == page)
+        LogicError ("Directory chain: rev link broken");
+
+    // This node isn't the root, so it can either be in the
+    // middle of the list, or at the end. Unlink it first
+    // and then check if that leaves the list with only a
+    // root:
+    auto prev = peek(keylet::page(directory, prevPage));
+    if (!prev)
+        LogicError ("Directory chain: fwd link broken.");
+    // Fix previous to point to its new next.
+    prev->setFieldU64(sfIndexNext, nextPage);
+    update (prev);
+
+    auto next = peek(keylet::page(directory, nextPage));
+    if (!next)
+        LogicError ("Directory chain: rev link broken.");
+    // Fix next to point to its new previous.
+    next->setFieldU64(sfIndexPrevious, prevPage);
+    update(next);
+
+    // The page is no longer linked. Delete it.
+    erase(node);
+
+    // Check whether the next page is the last page and, if
+    // so, whether it's empty. If it is, delete it.
+    if (nextPage != rootPage &&
+        next->getFieldU64 (sfIndexNext) == rootPage &&
+        next->getFieldV256 (sfIndexes).empty())
+    {
+        // Since next doesn't point to the root, it
+        // can't be pointing to prev.
+        erase(next);
+
+        // The previous page is now the last page:
+        prev->setFieldU64(sfIndexNext, rootPage);
+        update (prev);
+
+        // And the root points to the the last page:
+        auto root = peek(keylet::page(directory, rootPage));
+        if (!root)
+            LogicError ("Directory chain: root link broken.");
+        root->setFieldU64(sfIndexPrevious, prevPage);
+        update (root);
+
+        nextPage = rootPage;
+    }
+
+    // If we're not keeping the root, then check to see if
+    // it's left empty. If so, delete it as well.
+    if (!keepRoot && nextPage == rootPage && prevPage == rootPage)
+    {
+        if (prev->getFieldV256 (sfIndexes).empty())
+            erase(prev);
+    }
+
+    return true;
+}
+
+} // ripple

src/ripple/ledger/impl/View.cpp

@@ -19,11 +19,13 @@
 
 #include <BeastConfig.h>
 #include <ripple/basics/chrono.h>
+#include <ripple/ledger/BookDirs.h>
 #include <ripple/ledger/ReadView.h>
 #include <ripple/ledger/View.h>
 #include <ripple/basics/contract.h>
 #include <ripple/basics/Log.h>
 #include <ripple/basics/StringUtilities.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/st.h>
 #include <ripple/protocol/Protocol.h>
 #include <ripple/protocol/Quality.h>
@@ -803,19 +805,28 @@ describeOwnerDir(AccountID const& account)
     };
 }
 
-std::pair<TER, bool>
+boost::optional<std::uint64_t>
 dirAdd (ApplyView& view,
-    std::uint64_t&                          uNodeDir,
     Keylet const&                           dir,
     uint256 const&                          uLedgerIndex,
+    bool                                    strictOrder,
     std::function<void (SLE::ref)>          fDescriber,
     beast::Journal j)
 {
+    if (view.rules().enabled(featureSortedDirectories))
+    {
+        if (strictOrder)
+            return view.dirAppend(dir, uLedgerIndex, fDescriber);
+        else
+            return view.dirInsert(dir, uLedgerIndex, fDescriber);
+    }
+
     JLOG (j.trace()) << "dirAdd:" <<
         " dir=" << to_string (dir.key) <<
         " uLedgerIndex=" << to_string (uLedgerIndex);
 
     auto sleRoot = view.peek(dir);
+    std::uint64_t uNodeDir = 0;
 
     if (! sleRoot)
     {
@@ -833,9 +844,7 @@ dirAdd (ApplyView& view,
             "dirAdd: created root " << to_string (dir.key) <<
             " for entry " << to_string (uLedgerIndex);
 
-        uNodeDir = 0;
+        return uNodeDir;
-
-        return { tesSUCCESS, true };
     }
 
     SLE::pointer sleNode;
@@ -865,7 +874,7 @@ dirAdd (ApplyView& view,
     // Add to new node.
     else if (!++uNodeDir)
     {
-        return { tecDIR_FULL, false };
+        return boost::none;
     }
     else
     {
@@ -901,28 +910,36 @@ dirAdd (ApplyView& view,
     JLOG (j.trace()) <<
         "dirAdd:  appending: Node: " << strHex (uNodeDir);
 
-    return { tesSUCCESS, false };
+    return uNodeDir;
 }
 
 // Ledger must be in a state for this to work.
 TER
 dirDelete (ApplyView& view,
     const bool                      bKeepRoot,      // --> True, if we never completely clean up, after we overflow the root node.
-    const std::uint64_t&            uNodeDir,       // --> Node containing entry.
+    std::uint64_t                   uNodeDir,       // --> Node containing entry.
-    uint256 const&                  uRootIndex,     // --> The index of the base of the directory.  Nodes are based off of this.
+    Keylet const&                   root,           // --> The index of the base of the directory.  Nodes are based off of this.
     uint256 const&                  uLedgerIndex,   // --> Value to remove from directory.
     const bool                      bStable,        // --> True, not to change relative order of entries.
     const bool                      bSoft,          // --> True, uNodeDir is not hard and fast (pass uNodeDir=0).
     beast::Journal j)
 {
+    if (view.rules().enabled(featureSortedDirectories))
+    {
+        if (view.dirRemove(root, uNodeDir, uLedgerIndex, bKeepRoot))
+            return tesSUCCESS;
+
+        return tefBAD_LEDGER;
+    }
+
     std::uint64_t uNodeCur = uNodeDir;
     SLE::pointer sleNode =
-        view.peek(keylet::page(uRootIndex, uNodeCur));
+        view.peek(keylet::page(root, uNodeCur));
 
     if (!sleNode)
     {
         JLOG (j.warn()) << "dirDelete: no such node:" <<
-            " uRootIndex=" << to_string (uRootIndex) <<
+            " root=" << to_string (root.key) <<
             " uNodeDir=" << strHex (uNodeDir) <<
             " uLedgerIndex=" << to_string (uLedgerIndex);
 
@@ -936,7 +953,7 @@ dirDelete (ApplyView& view,
             // Go the extra mile. Even if node doesn't exist, try the next node.
 
             return dirDelete (view, bKeepRoot,
-                uNodeDir + 1, uRootIndex, uLedgerIndex, bStable, true, j);
+                uNodeDir + 1, root, uLedgerIndex, bStable, true, j);
         }
         else
         {
@@ -961,7 +978,7 @@ dirDelete (ApplyView& view,
         {
             // Go the extra mile. Even if entry not in node, try the next node.
             return dirDelete (view, bKeepRoot, uNodeDir + 1,
-                uRootIndex, uLedgerIndex, bStable, true, j);
+                root, uLedgerIndex, bStable, true, j);
         }
 
         return tefBAD_LEDGER;
@@ -1015,7 +1032,7 @@ dirDelete (ApplyView& view,
             else
             {
                 // Have only a root node and a last node.
-                auto sleLast = view.peek(keylet::page(uRootIndex, uNodeNext));
+                auto sleLast = view.peek(keylet::page(root, uNodeNext));
 
                 assert (sleLast);
 
@@ -1037,9 +1054,8 @@ dirDelete (ApplyView& view,
         {
             // Not root and not last node. Can delete node.
 
-            auto slePrevious =
+            auto slePrevious = view.peek(keylet::page(root, uNodePrevious));
-                view.peek(keylet::page(uRootIndex, uNodePrevious));
+            auto sleNext = view.peek(keylet::page(root, uNodeNext));
-            auto sleNext = view.peek(keylet::page(uRootIndex, uNodeNext));
             assert (slePrevious);
             if (!slePrevious)
             {
@@ -1072,8 +1088,7 @@ dirDelete (ApplyView& view,
         else
         {
             // Last and only node besides the root.
-            auto sleRoot = view.peek (keylet::page(uRootIndex));
+            auto sleRoot = view.peek(root);
-
             assert (sleRoot);
 
             if (sleRoot->getFieldV256 (sfIndexes).empty ())
@@ -1122,86 +1137,77 @@ trustCreate (ApplyView& view,
         ltRIPPLE_STATE, uIndex);
     view.insert (sleRippleState);
 
-    std::uint64_t   uLowNode;
+    auto lowNode = dirAdd (view, keylet::ownerDir (uLowAccountID),
-    std::uint64_t   uHighNode;
+        sleRippleState->key(), false, describeOwnerDir (uLowAccountID), j);
 
-    TER terResult;
+    if (!lowNode)
+        return tecDIR_FULL;
 
-    std::tie (terResult, std::ignore) = dirAdd (view,
+    auto highNode = dirAdd (view, keylet::ownerDir (uHighAccountID),
-        uLowNode, keylet::ownerDir (uLowAccountID),
+        sleRippleState->key(), false, describeOwnerDir (uHighAccountID), j);
-        sleRippleState->key(),
-        describeOwnerDir (uLowAccountID), j);
 
-    if (tesSUCCESS == terResult)
+    if (!highNode)
+        return tecDIR_FULL;
+
+    const bool bSetDst = saLimit.getIssuer () == uDstAccountID;
+    const bool bSetHigh = bSrcHigh ^ bSetDst;
+
+    assert (sleAccount->getAccountID (sfAccount) ==
+        (bSetHigh ? uHighAccountID : uLowAccountID));
+    auto slePeer = view.peek (keylet::account(
+        bSetHigh ? uLowAccountID : uHighAccountID));
+    assert (slePeer);
+
+    // Remember deletion hints.
+    sleRippleState->setFieldU64 (sfLowNode, *lowNode);
+    sleRippleState->setFieldU64 (sfHighNode, *highNode);
+
+    sleRippleState->setFieldAmount (
+        bSetHigh ? sfHighLimit : sfLowLimit, saLimit);
+    sleRippleState->setFieldAmount (
+        bSetHigh ? sfLowLimit : sfHighLimit,
+        STAmount ({saBalance.getCurrency (),
+                   bSetDst ? uSrcAccountID : uDstAccountID}));
+
+    if (uQualityIn)
+        sleRippleState->setFieldU32 (
+            bSetHigh ? sfHighQualityIn : sfLowQualityIn, uQualityIn);
+
+    if (uQualityOut)
+        sleRippleState->setFieldU32 (
+            bSetHigh ? sfHighQualityOut : sfLowQualityOut, uQualityOut);
+
+    std::uint32_t uFlags = bSetHigh ? lsfHighReserve : lsfLowReserve;
+
+    if (bAuth)
     {
-        std::tie (terResult, std::ignore) = dirAdd (view,
+        uFlags |= (bSetHigh ? lsfHighAuth : lsfLowAuth);
-            uHighNode, keylet::ownerDir (uHighAccountID),
+    }
-            sleRippleState->key(),
+    if (bNoRipple)
-            describeOwnerDir (uHighAccountID), j);
+    {
+        uFlags |= (bSetHigh ? lsfHighNoRipple : lsfLowNoRipple);
+    }
+    if (bFreeze)
+    {
+        uFlags |= (!bSetHigh ? lsfLowFreeze : lsfHighFreeze);
     }
 
-    if (tesSUCCESS == terResult)
+    if ((slePeer->getFlags() & lsfDefaultRipple) == 0)
     {
-        const bool bSetDst = saLimit.getIssuer () == uDstAccountID;
+        // The other side's default is no rippling
-        const bool bSetHigh = bSrcHigh ^ bSetDst;
+        uFlags |= (bSetHigh ? lsfLowNoRipple : lsfHighNoRipple);
-
-        assert (sleAccount->getAccountID (sfAccount) ==
-            (bSetHigh ? uHighAccountID : uLowAccountID));
-        auto slePeer = view.peek (keylet::account(
-            bSetHigh ? uLowAccountID : uHighAccountID));
-        assert (slePeer);
-
-        // Remember deletion hints.
-        sleRippleState->setFieldU64 (sfLowNode, uLowNode);
-        sleRippleState->setFieldU64 (sfHighNode, uHighNode);
-
-        sleRippleState->setFieldAmount (
-            bSetHigh ? sfHighLimit : sfLowLimit, saLimit);
-        sleRippleState->setFieldAmount (
-            bSetHigh ? sfLowLimit : sfHighLimit,
-            STAmount ({saBalance.getCurrency (),
-                       bSetDst ? uSrcAccountID : uDstAccountID}));
-
-        if (uQualityIn)
-            sleRippleState->setFieldU32 (
-                bSetHigh ? sfHighQualityIn : sfLowQualityIn, uQualityIn);
-
-        if (uQualityOut)
-            sleRippleState->setFieldU32 (
-                bSetHigh ? sfHighQualityOut : sfLowQualityOut, uQualityOut);
-
-        std::uint32_t uFlags = bSetHigh ? lsfHighReserve : lsfLowReserve;
-
-        if (bAuth)
-        {
-            uFlags |= (bSetHigh ? lsfHighAuth : lsfLowAuth);
-        }
-        if (bNoRipple)
-        {
-            uFlags |= (bSetHigh ? lsfHighNoRipple : lsfLowNoRipple);
-        }
-        if (bFreeze)
-        {
-            uFlags |= (!bSetHigh ? lsfLowFreeze : lsfHighFreeze);
-        }
-
-        if ((slePeer->getFlags() & lsfDefaultRipple) == 0)
-        {
-            // The other side's default is no rippling
-            uFlags |= (bSetHigh ? lsfLowNoRipple : lsfHighNoRipple);
-        }
-
-        sleRippleState->setFieldU32 (sfFlags, uFlags);
-        adjustOwnerCount(view, sleAccount, 1, j);
-
-        // ONLY: Create ripple balance.
-        sleRippleState->setFieldAmount (sfBalance, bSetHigh ? -saBalance : saBalance);
-
-        view.creditHook (uSrcAccountID,
-            uDstAccountID, saBalance, saBalance.zeroed());
     }
 
-    return terResult;
+    sleRippleState->setFieldU32 (sfFlags, uFlags);
+    adjustOwnerCount(view, sleAccount, 1, j);
+
+    // ONLY: Create ripple balance.
+    sleRippleState->setFieldAmount (sfBalance, bSetHigh ? -saBalance : saBalance);
+
+    view.creditHook (uSrcAccountID,
+        uDstAccountID, saBalance, saBalance.zeroed());
+
+    return tesSUCCESS;
 }
 
 TER
@@ -1223,7 +1229,7 @@ trustDelete (ApplyView& view,
     terResult   = dirDelete(view,
         false,
         uLowNode,
-        getOwnerDirIndex (uLowAccountID),
+        keylet::ownerDir (uLowAccountID),
         sleRippleState->key(),
         false,
         !bLowNode,
@@ -1236,7 +1242,7 @@ trustDelete (ApplyView& view,
         terResult   = dirDelete (view,
             false,
             uHighNode,
-            getOwnerDirIndex (uHighAccountID),
+            keylet::ownerDir (uHighAccountID),
             sleRippleState->key(),
             false,
             !bHighNode,
@@ -1261,14 +1267,12 @@ offerDelete (ApplyView& view,
 
     // Detect legacy directories.
     bool bOwnerNode = sle->isFieldPresent (sfOwnerNode);
-    std::uint64_t uOwnerNode = sle->getFieldU64 (sfOwnerNode);
     uint256 uDirectory = sle->getFieldH256 (sfBookDirectory);
-    std::uint64_t uBookNode  = sle->getFieldU64 (sfBookNode);
 
-    TER terResult  = dirDelete (view, false, uOwnerNode,
+    TER terResult  = dirDelete (view, false, sle->getFieldU64 (sfOwnerNode),
-        getOwnerDirIndex (owner), offerIndex, false, !bOwnerNode, j);
+        keylet::ownerDir (owner), offerIndex, false, !bOwnerNode, j);
-    TER terResult2 = dirDelete (view, false, uBookNode,
+    TER terResult2 = dirDelete (view, false, sle->getFieldU64 (sfBookNode),
-        uDirectory, offerIndex, true, false, j);
+        keylet::page (uDirectory), offerIndex, true, false, j);
 
     if (tesSUCCESS == terResult)
         adjustOwnerCount(view, view.peek(

src/ripple/protocol/Feature.h

@@ -66,7 +66,8 @@ class FeatureCollections
          "Escrow",
          "CryptoConditionsSuite",
          "fix1373",
-         "EnforceInvariants"};
+         "EnforceInvariants",
+         "SortedDirectories"};
 
     std::vector<uint256> features;
     boost::container::flat_map<uint256, std::size_t> featureToIndex;
@@ -158,6 +159,7 @@ extern uint256 const featureEscrow;
 extern uint256 const featureCryptoConditionsSuite;
 extern uint256 const fix1373;
 extern uint256 const featureEnforceInvariants;
+extern uint256 const featureSortedDirectories;
 
 } // ripple
 

src/ripple/protocol/Protocol.h

@@ -49,6 +49,9 @@ std::size_t constexpr oversizeMetaDataCap = 5200;
 /** The maximum number of entries per directory page */
 std::size_t constexpr dirNodeMaxEntries = 32;
 
+/** The maximum number of pages allowed in a directory */
+std::uint64_t constexpr dirNodeMaxPages = 262144;
+
 /** A ledger index. */
 using LedgerIndex = std::uint32_t;
 

src/ripple/protocol/STVector256.h

@@ -144,6 +144,18 @@ public:
         return mValue;
     }
 
+    std::vector<uint256>::iterator
+    insert(std::vector<uint256>::const_iterator pos, uint256 const& value)
+    {
+        return mValue.insert(pos, value);
+    }
+
+    std::vector<uint256>::iterator
+    insert(std::vector<uint256>::const_iterator pos, uint256&& value)
+    {
+        return mValue.insert(pos, std::move(value));
+    }
+
     void
     push_back (uint256 const& v)
     {

src/ripple/protocol/impl/Feature.cpp

@@ -113,5 +113,6 @@ uint256 const featureEscrow = *getRegisteredFeature("Escrow");
 uint256 const featureCryptoConditionsSuite = *getRegisteredFeature("CryptoConditionsSuite");
 uint256 const fix1373 = *getRegisteredFeature("fix1373");
 uint256 const featureEnforceInvariants = *getRegisteredFeature("EnforceInvariants");
+uint256 const featureSortedDirectories = *getRegisteredFeature("SortedDirectories");
 
 } // ripple

src/ripple/unity/ledger.cpp

@@ -20,6 +20,7 @@
 #include <BeastConfig.h>
 
 #include <ripple/ledger/impl/ApplyStateTable.cpp>
+#include <ripple/ledger/impl/ApplyView.cpp>
 #include <ripple/ledger/impl/ApplyViewBase.cpp>
 #include <ripple/ledger/impl/ApplyViewImpl.cpp>
 #include <ripple/ledger/impl/BookDirs.cpp>

src/test/ledger/Directory_test.cpp

@@ -15,69 +15,431 @@
 */
 //==============================================================================
 
-#include <BeastConfig.h>
+#include <ripple/beast/xor_shift_engine.h>
-#include <test/jtx.h>
+#include <ripple/ledger/BookDirs.h>
 #include <ripple/ledger/Directory.h>
+#include <ripple/ledger/Sandbox.h>
+#include <ripple/protocol/Feature.h>
+#include <ripple/protocol/JsonFields.h>
+#include <ripple/protocol/Protocol.h>
+#include <test/jtx.h>
+#include <algorithm>
 
 namespace ripple {
 namespace test {
 
 struct Directory_test : public beast::unit_test::suite
 {
-    void testDirectory()
+    // Map [0-15576] into a a unique 3 letter currency code
+    std::string
+    currcode (std::size_t i)
+    {
+        // There are only 17576 possible combinations
+        BEAST_EXPECT (i < 17577);
+
+        std::string code;
+
+        for (int j = 0; j != 3; ++j)
+        {
+            code.push_back ('A' + (i % 26));
+            i /= 26;
+        }
+
+        return code;
+    }
+
+    // Insert n empty pages, numbered [0, ... n - 1], in the
+    // specified directory:
+    void
+    makePages(
+        Sandbox& sb,
+        uint256 const& base,
+        std::uint64_t n)
+    {
+        for (std::uint64_t i = 0; i < n; ++i)
+        {
+            auto p = std::make_shared<SLE>(keylet::page(base, i));
+
+            p->setFieldV256 (sfIndexes, STVector256{});
+
+            if (i + 1 == n)
+                p->setFieldU64 (sfIndexNext, 0);
+            else
+                p->setFieldU64 (sfIndexNext, i + 1);
+
+            if (i == 0)
+                p->setFieldU64 (sfIndexPrevious, n - 1);
+            else
+                p->setFieldU64 (sfIndexPrevious, i - 1);
+
+            sb.insert (p);
+        }
+    }
+
+    void testDirectoryOrdering()
     {
         using namespace jtx;
-        Env env(*this);
+
         auto gw = Account("gw");
         auto USD = gw["USD"];
+        auto alice = Account("alice");
+        auto bob = Account("bob");
 
         {
-            auto dir = Dir(*env.current(),
+            testcase ("Directory Ordering (without 'SortedDirectories' amendment");
-                keylet::ownerDir(Account("alice")));
+
-            BEAST_EXPECT(std::begin(dir) == std::end(dir));
+            Env env(*this, all_features_except(featureSortedDirectories));
-            BEAST_EXPECT(std::end(dir) == dir.find(uint256(), uint256()));
+            env.fund(XRP(10000000), alice, bob, gw);
+
+            // Insert 400 offers from Alice, then one from Bob:
+            for (std::size_t i = 1; i <= 400; ++i)
+                env(offer(alice, USD(10), XRP(10)));
+
+            // Check Alice's directory: it should contain one
+            // entry for each offer she added. Within each
+            // page, the entries should be in sorted order.
+            {
+                auto dir = Dir(*env.current(),
+                    keylet::ownerDir(alice));
+
+                std::uint32_t lastSeq = 1;
+
+                // Check that the orders are sequential by checking
+                // that their sequence numbers are:
+                for (auto iter = dir.begin(); iter != std::end(dir); ++iter) {
+                    BEAST_EXPECT(++lastSeq == (*iter)->getFieldU32(sfSequence));
+                }
+                BEAST_EXPECT(lastSeq != 1);
+            }
         }
 
-        env.fund(XRP(10000), "alice", "bob", gw);
-
-        auto i = 10;
-        for (; i <= 400; i += 10)
-            env(offer("alice", USD(i), XRP(10)));
-        env(offer("bob", USD(500), XRP(10)));
-
         {
-            auto dir = Dir(*env.current(),
+            testcase ("Directory Ordering (with 'SortedDirectories' amendment)");
-                keylet::ownerDir(Account("bob")));
+
-            BEAST_EXPECT(std::begin(dir)->get()->
+            Env env(*this, with_features(featureSortedDirectories));
-                getFieldAmount(sfTakerPays) == USD(500));
+            env.fund(XRP(10000000), alice, gw);
+
+            for (std::size_t i = 1; i <= 400; ++i)
+                env(offer(alice, USD(i), XRP(i)));
+            env.close();
+
+            // Check Alice's directory: it should contain one
+            // entry for each offer she added, and, within each
+            // page the entries should be in sorted order.
+            {
+                auto const view = env.closed();
+
+                std::uint64_t page = 0;
+
+                do
+                {
+                    auto p = view->read(keylet::page(keylet::ownerDir(alice), page));
+
+                    // Ensure that the entries in the page are sorted
+                    auto const& v = p->getFieldV256(sfIndexes);
+                    BEAST_EXPECT (std::is_sorted(v.begin(), v.end()));
+
+                    // Ensure that the page contains the correct orders by
+                    // calculating which sequence numbers belong here.
+                    std::uint32_t minSeq = 2 + (page * dirNodeMaxEntries);
+                    std::uint32_t maxSeq = minSeq + dirNodeMaxEntries;
+
+                    for (auto const& e : v)
+                    {
+                        auto c = view->read(keylet::child(e));
+                        BEAST_EXPECT(c);
+                        BEAST_EXPECT(c->getFieldU32(sfSequence) >= minSeq);
+                        BEAST_EXPECT(c->getFieldU32(sfSequence) < maxSeq);
+                    }
+
+                    page = p->getFieldU64(sfIndexNext);
+                } while (page != 0);
+            }
+
+            // Now check the orderbook: it should be in the order we placed
+            // the offers.
+            auto book = BookDirs(*env.current(),
+                Book({xrpIssue(), USD.issue()}));
+            int count = 1;
+
+            for (auto const& offer : book)
+            {
+                count++;
+                BEAST_EXPECT(offer->getFieldAmount(sfTakerPays) == USD(count));
+                BEAST_EXPECT(offer->getFieldAmount(sfTakerGets) == XRP(count));
+            }
+        }
+    }
+
+    void
+    testDirIsEmpty()
+    {
+        testcase ("dirIsEmpty");
+
+        using namespace jtx;
+        auto const alice = Account("alice");
+        auto const bob = Account("bob");
+        auto const charlie = Account ("charlie");
+        auto const gw = Account ("gw");
+
+        beast::xor_shift_engine eng;
+
+        Env env(*this, with_features(featureSortedDirectories, featureMultiSign));
+
+        env.fund(XRP(1000000), alice, charlie, gw);
+        env.close();
+
+        // alice should have an empty directory.
+        BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+
+        // Give alice a signer list, then there will be stuff in the directory.
+        env(signers(alice, 1, { { bob, 1} }));
+        env.close();
+        BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+
+        env(signers(alice, jtx::none));
+        env.close();
+        BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+
+        std::vector<IOU> const currencies = [this,&eng,&gw]()
+        {
+            std::vector<IOU> c;
+
+            c.reserve((2 * dirNodeMaxEntries) + 3);
+
+            while (c.size() != c.capacity())
+                c.push_back(gw[currcode(c.size())]);
+
+            return c;
+        }();
+
+        // First, Alices creates a lot of trustlines, and then
+        // deletes them in a different order:
+        {
+            auto cl = currencies;
+
+            for (auto const& c : cl)
+            {
+                env(trust(alice, c(50)));
+                env.close();
+            }
+
+            BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+
+            std::shuffle (cl.begin(), cl.end(), eng);
+
+            for (auto const& c : cl)
+            {
+                env(trust(alice, c(0)));
+                env.close();
+            }
+
+            BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
         }
 
-        auto dir = Dir(*env.current(),
+        // Now, Alice creates offers to buy currency, creating
-            keylet::ownerDir(Account("alice")));
+        // implicit trust lines.
-        i = 0;
+        {
-        for (auto const& e : dir)
+            auto cl = currencies;
-            BEAST_EXPECT(e->getFieldAmount(sfTakerPays) == USD(i += 10));
 
-        BEAST_EXPECT(std::begin(dir) != std::end(dir));
+            BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-        BEAST_EXPECT(std::end(dir) ==
+
-            dir.find(std::begin(dir).page().key,
+            for (auto c : currencies)
-                uint256()));
+            {
-        BEAST_EXPECT(std::begin(dir) ==
+                env(trust(charlie, c(50)));
-            dir.find(std::begin(dir).page().key,
+                env.close();
-                std::begin(dir).index()));
+                env(pay(gw, charlie, c(50)));
-        auto entry = std::next(std::begin(dir), 32);
+                env.close();
-        auto it = dir.find(entry.page().key, entry.index());
+                env(offer(alice, c(50), XRP(50)));
-        BEAST_EXPECT(it != std::end(dir));
+                env.close();
-        BEAST_EXPECT((*it)->getFieldAmount(sfTakerPays) == USD(330));
+            }
+
+            BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+
+            // Now fill the offers in a random order. Offer
+            // entries will drop, and be replaced by trust
+            // lines that are implicitly created.
+            std::shuffle (cl.begin(), cl.end(), eng);
+
+            for (auto const& c : cl)
+            {
+                env(offer(charlie, XRP(50), c(50)));
+                env.close();
+            }
+            BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+            // Finally, Alice now sends the funds back to
+            // Charlie. The implicitly created trust lines
+            // should drop away:
+            std::shuffle (cl.begin(), cl.end(), eng);
+
+            for (auto const& c : cl)
+            {
+                env(pay(alice, charlie, c(50)));
+                env.close();
+            }
+
+            BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+        }
+    }
+
+    void
+    testRipd1353()
+    {
+        testcase("RIPD-1353 Empty Offer Directories");
+
+        using namespace jtx;
+        Env env(*this, with_features(featureSortedDirectories));
+
+        auto const gw = Account{"gateway"};
+        auto const alice = Account{"alice"};
+        auto const USD = gw["USD"];
+
+        env.fund(XRP(10000), alice, gw);
+        env.trust(USD(1000), alice);
+        env(pay(gw, alice, USD(1000)));
+
+        auto const firstOfferSeq = env.seq(alice);
+
+        // Fill up three pages of offers
+        for (int i = 0; i < 3; ++i)
+            for (int j = 0; j < dirNodeMaxEntries; ++j)
+                env(offer(alice, XRP(1), USD(1)));
+        env.close();
+
+        // remove all the offers. Remove the middle page last
+        for (auto page : {0, 2, 1})
+        {
+            for (int i = 0; i < dirNodeMaxEntries; ++i)
+            {
+                Json::Value cancelOffer;
+                cancelOffer[jss::Account] = alice.human();
+                cancelOffer[jss::OfferSequence] =
+                    Json::UInt(firstOfferSeq + page * dirNodeMaxEntries + i);
+                cancelOffer[jss::TransactionType] = "OfferCancel";
+                env(cancelOffer);
+                env.close();
+            }
+        }
+
+        // All the offers have been cancelled, so the book
+        // should have no entries and be empty:
+        {
+            Sandbox sb(env.closed().get(), tapNONE);
+            uint256 const bookBase = getBookBase({xrpIssue(), USD.issue()});
+
+            BEAST_EXPECT(dirIsEmpty (sb, keylet::page(bookBase)));
+            BEAST_EXPECT (!sb.succ(bookBase, getQualityNext(bookBase)));
+        }
+
+        // Alice returns the USD she has to the gateway
+        // and removes her trust line. Her owner directory
+        // should now be empty:
+        {
+            env.trust(USD(0), alice);
+            env(pay(alice, gw, alice["USD"](1000)));
+            env.close();
+            BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
+        }
+    }
+
+    void
+    testEmptyChain()
+    {
+        testcase("Empty Chain on Delete");
+
+        using namespace jtx;
+        Env env(*this, with_features(featureSortedDirectories));
+
+        auto const gw = Account{"gateway"};
+        auto const alice = Account{"alice"};
+        auto const USD = gw["USD"];
+
+        env.fund(XRP(10000), alice);
+        env.close();
+
+        uint256 base;
+        base.SetHex("fb71c9aa3310141da4b01d6c744a98286af2d72ab5448d5adc0910ca0c910880");
+
+        uint256 item;
+        item.SetHex("bad0f021aa3b2f6754a8fe82a5779730aa0bbbab82f17201ef24900efc2c7312");
+
+        {
+            // Create a chain of three pages:
+            Sandbox sb(env.closed().get(), tapNONE);
+            makePages (sb, base, 3);
+
+            // Insert an item in the middle page:
+            {
+                auto p = sb.peek (keylet::page(base, 1));
+                BEAST_EXPECT(p);
+
+                STVector256 v;
+                v.push_back (item);
+                p->setFieldV256 (sfIndexes, v);
+                sb.update(p);
+            }
+
+            // Now, try to delete the item from the middle
+            // page. This should cause all pages to be deleted:
+            BEAST_EXPECT (sb.dirRemove (keylet::page(base, 0), 1, keylet::unchecked(item), false));
+            BEAST_EXPECT (!sb.peek(keylet::page(base, 2)));
+            BEAST_EXPECT (!sb.peek(keylet::page(base, 1)));
+            BEAST_EXPECT (!sb.peek(keylet::page(base, 0)));
+        }
+
+        {
+            // Create a chain of four pages:
+            Sandbox sb(env.closed().get(), tapNONE);
+            makePages (sb, base, 4);
+
+            // Now add items on pages 1 and 2:
+            {
+                auto p1 = sb.peek (keylet::page(base, 1));
+                BEAST_EXPECT(p1);
+
+                STVector256 v1;
+                v1.push_back (~item);
+                p1->setFieldV256 (sfIndexes, v1);
+                sb.update(p1);
+
+                auto p2 = sb.peek (keylet::page(base, 2));
+                BEAST_EXPECT(p2);
+
+                STVector256 v2;
+                v2.push_back (item);
+                p2->setFieldV256 (sfIndexes, v2);
+                sb.update(p2);
+            }
+
+            // Now, try to delete the item from page 2.
+            // This should cause pages 2 and 3 to be
+            // deleted:
+            BEAST_EXPECT (sb.dirRemove (keylet::page(base, 0), 2, keylet::unchecked(item), false));
+            BEAST_EXPECT (!sb.peek(keylet::page(base, 3)));
+            BEAST_EXPECT (!sb.peek(keylet::page(base, 2)));
+
+            auto p1 = sb.peek(keylet::page(base, 1));
+            BEAST_EXPECT (p1);
+            BEAST_EXPECT (p1->getFieldU64 (sfIndexNext) == 0);
+            BEAST_EXPECT (p1->getFieldU64 (sfIndexPrevious) == 0);
+
+            auto p0 = sb.peek(keylet::page(base, 0));
+            BEAST_EXPECT (p0);
+            BEAST_EXPECT (p0->getFieldU64 (sfIndexNext) == 1);
+            BEAST_EXPECT (p0->getFieldU64 (sfIndexPrevious) == 1);
+        }
     }
 
     void run() override
     {
-        testDirectory();
+        testDirectoryOrdering();
+        testDirIsEmpty();
+        testRipd1353();
+        testEmptyChain();
     }
 };
 
 BEAST_DEFINE_TESTSUITE(Directory,ledger,ripple);
 
-} // test
+}
-} // ripple
+}

src/test/ledger/View_test.cpp

@@ -835,107 +835,8 @@ class GetAmendments_test
     }
 };
 
-class DirIsEmpty_test
-    : public beast::unit_test::suite
-{
-    void
-    testDirIsEmpty()
-    {
-        using namespace jtx;
-        auto const alice = Account("alice");
-        auto const bogie = Account("bogie");
-
-        Env env(*this, with_features(featureMultiSign));
-
-        env.fund(XRP(10000), alice);
-        env.close();
-
-        // alice should have an empty directory.
-        BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-
-        // Give alice a signer list, then there will be stuff in the directory.
-        env(signers(alice, 1, { { bogie, 1} }));
-        env.close();
-        BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-
-        env(signers(alice, jtx::none));
-        env.close();
-        BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-
-        // The next test is a bit awkward.  It tests the case where alice
-        // uses 3 directory pages and then deletes all entries from the
-        // first 2 pages.  dirIsEmpty() should still return false in this
-        // circumstance.
-        //
-        // Fill alice's directory with implicit trust lines (produced by
-        // taking offers) and then remove all but the last one.
-        auto const becky = Account ("becky");
-        auto const gw = Account ("gw");
-        env.fund(XRP(10000), becky, gw);
-        env.close();
-
-        static_assert (64 >= (2 * dirNodeMaxEntries), "");
-
-        // Generate 64 currencies named AAA -> AAP and ADA -> ADP.
-        std::vector<IOU> currencies;
-        currencies.reserve(64);
-        for (char b = 'A'; b <= 'D'; ++b)
-        {
-            for (char c = 'A'; c <= 'P'; ++c)
-            {
-                currencies.push_back(gw[std::string("A") + b + c]);
-                IOU const& currency = currencies.back();
-
-                // Establish trust lines.
-                env(trust(becky, currency(50)));
-                env.close();
-                env(pay(gw, becky, currency(50)));
-                env.close();
-                env(offer(alice, currency(50), XRP(10)));
-                env(offer(becky, XRP(10), currency(50)));
-                env.close();
-            }
-        }
-
-        // Set up one more currency that alice will hold onto.  We expect
-        // this one to go in the third directory page.
-        IOU const lastCurrency = gw["ZZZ"];
-        env(trust(becky, lastCurrency(50)));
-        env.close();
-        env(pay(gw, becky, lastCurrency(50)));
-        env.close();
-        env(offer(alice, lastCurrency(50), XRP(10)));
-        env(offer(becky, XRP(10), lastCurrency(50)));
-        env.close();
-
-        BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-
-        // Now alice gives all the currencies except the last one back to becky.
-        for (auto currency : currencies)
-        {
-            env(pay(alice, becky, currency(50)));
-            env.close();
-        }
-
-        // This is the crux of the test.
-        BEAST_EXPECT(! dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-
-        // Give the last currency to becky.  Now alice's directory is empty.
-        env(pay(alice, becky, lastCurrency(50)));
-        env.close();
-
-        BEAST_EXPECT(dirIsEmpty (*env.closed(), keylet::ownerDir(alice)));
-    }
-
-    void run() override
-    {
-        testDirIsEmpty();
-    }
-};
-
 BEAST_DEFINE_TESTSUITE(View,ledger,ripple);
 BEAST_DEFINE_TESTSUITE(GetAmendments,ledger,ripple);
-BEAST_DEFINE_TESTSUITE(DirIsEmpty, ledger,ripple);
 
 }  // test
 }  // ripple

src/test/rpc/AccountLinesRPC_test.cpp

@@ -33,6 +33,8 @@ class AccountLinesRPC_test : public beast::unit_test::suite
 public:
     void testAccountLines()
     {
+        testcase ("acccount_lines");
+
         using namespace test::jtx;
         Env env(*this);
         {
@@ -284,6 +286,7 @@ public:
 
     void testAccountLineDelete()
     {
+        testcase ("Entry pointed to by marker is removed");
         using namespace test::jtx;
         Env env(*this);
 
@@ -308,35 +311,35 @@ public:
 
         auto const USD = gw1["USD"];
         auto const EUR = gw2["EUR"];
-        env(trust(alice, EUR(200)));
+        env(trust(alice, USD(200)));
-        env(trust(becky, USD(200)));
+        env(trust(becky, EUR(200)));
-        env(trust(cheri, USD(200)));
+        env(trust(cheri, EUR(200)));
         env.close();
 
         // becky gets 100 USD from gw1.
-        env(pay(gw1, becky, USD(100)));
+        env(pay(gw2, becky, EUR(100)));
         env.close();
 
-        // alice offers to buy 100 USD for 100 XRP.
+        // alice offers to buy 100 EUR for 100 XRP.
-        env(offer(alice, USD(100), XRP(100)));
+        env(offer(alice, EUR(100), XRP(100)));
         env.close();
 
-        // becky offers to buy 100 XRP for 100 USD.
+        // becky offers to buy 100 XRP for 100 EUR.
-        env(offer(becky, XRP(100), USD(100)));
+        env(offer(becky, XRP(100), EUR(100)));
         env.close();
 
         // Get account_lines for alice.  Limit at 1, so we get a marker.
         auto const linesBeg = env.rpc ("json", "account_lines",
             R"({"account": ")" + alice.human() + R"(", )"
             R"("limit": 1})");
-        BEAST_EXPECT(linesBeg[jss::result][jss::lines][0u][jss::currency] == "EUR");
+        BEAST_EXPECT(linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD");
         BEAST_EXPECT(linesBeg[jss::result].isMember(jss::marker));
 
-        // alice pays 100 USD to cheri.
+        // alice pays 100 EUR to cheri.
-        env(pay(alice, cheri, USD(100)));
+        env(pay(alice, cheri, EUR(100)));
         env.close();
 
-        // Since alice paid all her USD to cheri, alice should no longer
+        // Since alice paid all her EUR to cheri, alice should no longer
         // have a trust line to gw1.  So the old marker should now be invalid.
         auto const linesEnd = env.rpc ("json", "account_lines",
             R"({"account": ")" + alice.human() + R"(", )"
@@ -349,6 +352,8 @@ public:
     // test API V2
     void testAccountLines2()
     {
+        testcase ("V2: acccount_lines");
+
         using namespace test::jtx;
         Env env(*this);
         {
@@ -779,6 +784,8 @@ public:
     // test API V2
     void testAccountLineDelete2()
     {
+        testcase ("V2: account_lines with removed marker");
+
         using namespace test::jtx;
         Env env(*this);
 
@@ -787,12 +794,12 @@ public:
         //
         // It isn't easy to explicitly delete a trust line, so we do so in a
         // round-about fashion.  It takes 4 actors:
-        //   o Gateway gw1 issues USD
+        //   o Gateway gw1 issues EUR
-        //   o alice offers to buy 100 USD for 100 XRP.
+        //   o alice offers to buy 100 EUR for 100 XRP.
-        //   o becky offers to sell 100 USD for 100 XRP.
+        //   o becky offers to sell 100 EUR for 100 XRP.
-        // There will now be an inferred trustline between alice and gw1.
+        // There will now be an inferred trustline between alice and gw2.
-        //   o alice pays her 100 USD to cheri.
+        //   o alice pays her 100 EUR to cheri.
-        // alice should now have no USD and no trustline to gw1.
+        // alice should now have no EUR and no trustline to gw2.
         Account const alice {"alice"};
         Account const becky {"becky"};
         Account const cheri {"cheri"};
@@ -803,21 +810,21 @@ public:
 
         auto const USD = gw1["USD"];
         auto const EUR = gw2["EUR"];
-        env(trust(alice, EUR(200)));
+        env(trust(alice, USD(200)));
-        env(trust(becky, USD(200)));
+        env(trust(becky, EUR(200)));
-        env(trust(cheri, USD(200)));
+        env(trust(cheri, EUR(200)));
         env.close();
 
-        // becky gets 100 USD from gw1.
+        // becky gets 100 EUR from gw1.
-        env(pay(gw1, becky, USD(100)));
+        env(pay(gw2, becky, EUR(100)));
         env.close();
 
-        // alice offers to buy 100 USD for 100 XRP.
+        // alice offers to buy 100 EUR for 100 XRP.
-        env(offer(alice, USD(100), XRP(100)));
+        env(offer(alice, EUR(100), XRP(100)));
         env.close();
 
-        // becky offers to buy 100 XRP for 100 USD.
+        // becky offers to buy 100 XRP for 100 EUR.
-        env(offer(becky, XRP(100), USD(100)));
+        env(offer(becky, XRP(100), EUR(100)));
         env.close();
 
         // Get account_lines for alice.  Limit at 1, so we get a marker.
@@ -829,17 +836,17 @@ public:
             R"("params": [ )"
             R"({"account": ")" + alice.human() + R"(", )"
             R"("limit": 1}]})");
-        BEAST_EXPECT(linesBeg[jss::result][jss::lines][0u][jss::currency] == "EUR");
+        BEAST_EXPECT(linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD");
         BEAST_EXPECT(linesBeg[jss::result].isMember(jss::marker));
         BEAST_EXPECT(linesBeg.isMember(jss::jsonrpc) && linesBeg[jss::jsonrpc] == "2.0");
         BEAST_EXPECT(linesBeg.isMember(jss::ripplerpc) && linesBeg[jss::ripplerpc] == "2.0");
         BEAST_EXPECT(linesBeg.isMember(jss::id) && linesBeg[jss::id] == 5);
 
         // alice pays 100 USD to cheri.
-        env(pay(alice, cheri, USD(100)));
+        env(pay(alice, cheri, EUR(100)));
         env.close();
 
-        // Since alice paid all her USD to cheri, alice should no longer
+        // Since alice paid all her EUR to cheri, alice should no longer
         // have a trust line to gw1.  So the old marker should now be invalid.
         auto const linesEnd = env.rpc ("json2", "{ "
             R"("method" : "account_lines",)"

src/test/rpc/AccountObjects_test.cpp

@@ -29,96 +29,85 @@ namespace ripple {
 namespace test {
 
 static char const* bobs_account_objects[] = {
-R"json(
+R"json({
-{
+  "Account" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-  "Balance": {
+  "BookDirectory" : "50AD0A9E54D2B381288D535EB724E4275FFBF41580D28A925D038D7EA4C68000",
-    "currency": "USD",
+  "BookNode" : "0000000000000000",
-    "issuer": "rrrrrrrrrrrrrrrrrrrrBZbvji",
+  "Flags" : 65536,
-    "value": "-1000"
+  "LedgerEntryType" : "Offer",
-  },
+  "OwnerNode" : "0000000000000000",
-  "Flags": 131072,
+  "Sequence" : 4,
-  "HighLimit": {
+  "TakerGets" : {
-    "currency": "USD",
+    "currency" : "USD",
-    "issuer": "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
+    "issuer" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-    "value": "1000"
-  },
-  "HighNode": "0000000000000000",
-  "LedgerEntryType": "RippleState",
-  "LowLimit": {
-    "currency": "USD",
-    "issuer": "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW",
-    "value": "0"
-  },
-  "LowNode": "0000000000000000",
-  "index":
-    "D89BC239086183EB9458C396E643795C1134963E6550E682A190A5F021766D43"
-})json"
-,
-R"json(
-{
-  "Balance": {
-    "currency": "USD",
-    "issuer": "rrrrrrrrrrrrrrrrrrrrBZbvji",
-    "value": "-1000"
-  },
-  "Flags": 131072,
-  "HighLimit": {
-    "currency": "USD",
-    "issuer": "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-    "value": "1000"
-  },
-  "HighNode": "0000000000000000",
-  "LedgerEntryType": "RippleState",
-  "LowLimit": {
-    "currency": "USD",
-    "issuer": "r9cZvwKU3zzuZK9JFovGg1JC5n7QiqNL8L",
-    "value": "0"
-  },
-  "LowNode": "0000000000000000",
-  "index":
-    "D13183BCFFC9AAC9F96AEBB5F66E4A652AD1F5D10273AEB615478302BEBFD4A4"
-})json"
-,
-R"json(
-{
-  "Account": "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-  "BookDirectory":
-  "50AD0A9E54D2B381288D535EB724E4275FFBF41580D28A925D038D7EA4C68000",
-  "BookNode": "0000000000000000",
-  "Flags": 65536,
-  "LedgerEntryType": "Offer",
-  "OwnerNode": "0000000000000000",
-  "Sequence": 4,
-  "TakerGets": {
-    "currency": "USD",
-    "issuer": "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-    "value": "1"
-  },
-  "TakerPays": "100000000",
-  "index":
-    "A984D036A0E562433A8377CA57D1A1E056E58C0D04818F8DFD3A1AA3F217DD82"
-})json"
-,
-R"json(
-{
-  "Account": "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
-  "BookDirectory":
-  "B025997A323F5C3E03DDF1334471F5984ABDE31C59D463525D038D7EA4C68000",
-  "BookNode": "0000000000000000",
-  "Flags": 65536,
-  "LedgerEntryType": "Offer",
-  "OwnerNode": "0000000000000000",
-  "Sequence": 5,
-  "TakerGets": {
-    "currency": "USD",
-    "issuer" : "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW",
     "value" : "1"
   },
   "TakerPays" : "100000000",
-  "index" :
+  "index" : "A984D036A0E562433A8377CA57D1A1E056E58C0D04818F8DFD3A1AA3F217DD82"
-    "CAFE32332D752387B01083B60CC63069BA4A969C9730836929F841450F6A718E"
+})json"
-}
+,
-)json"
+R"json({
+    "Account" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
+    "BookDirectory" : "B025997A323F5C3E03DDF1334471F5984ABDE31C59D463525D038D7EA4C68000",
+    "BookNode" : "0000000000000000",
+    "Flags" : 65536,
+    "LedgerEntryType" : "Offer",
+    "OwnerNode" : "0000000000000000",
+    "Sequence" : 5,
+    "TakerGets" : {
+        "currency" : "USD",
+        "issuer" : "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW",
+        "value" : "1"
+    },
+    "TakerPays" : "100000000",
+    "index" : "CAFE32332D752387B01083B60CC63069BA4A969C9730836929F841450F6A718E"
+})json"
+,
+R"json({
+    "Balance" : {
+        "currency" : "USD",
+        "issuer" : "rrrrrrrrrrrrrrrrrrrrBZbvji",
+        "value" : "-1000"
+    },
+    "Flags" : 131072,
+    "HighLimit" : {
+        "currency" : "USD",
+        "issuer" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
+        "value" : "1000"
+    },
+    "HighNode" : "0000000000000000",
+    "LedgerEntryType" : "RippleState",
+    "LowLimit" : {
+        "currency" : "USD",
+        "issuer" : "r9cZvwKU3zzuZK9JFovGg1JC5n7QiqNL8L",
+        "value" : "0"
+    },
+    "LowNode" : "0000000000000000",
+    "index" : "D13183BCFFC9AAC9F96AEBB5F66E4A652AD1F5D10273AEB615478302BEBFD4A4"
+})json"
+,
+R"json({
+    "Balance" : {
+        "currency" : "USD",
+        "issuer" : "rrrrrrrrrrrrrrrrrrrrBZbvji",
+        "value" : "-1000"
+    },
+    "Flags" : 131072,
+    "HighLimit" : {
+        "currency" : "USD",
+        "issuer" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK",
+        "value" : "1000"
+    },
+    "HighNode" : "0000000000000000",
+    "LedgerEntryType" : "RippleState",
+    "LowLimit" : {
+        "currency" : "USD",
+        "issuer" : "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW",
+        "value" : "0"
+    },
+    "LowNode" : "0000000000000000",
+    "index" : "D89BC239086183EB9458C396E643795C1134963E6550E682A190A5F021766D43"
+})json"
 };
 
 class AccountObjects_test : public beast::unit_test::suite
@@ -280,7 +269,9 @@ public:
                 aobj.removeMember("PreviousTxnID");
                 aobj.removeMember("PreviousTxnLgrSeq");
 
-                BEAST_EXPECT( aobj == bobj[i]);
+                if (aobj != bobj[i])
+                   std::cout << "Fail at " << i << ": " << aobj << std::endl;
+                BEAST_EXPECT(aobj == bobj[i]);
             }
         }
         // test request with type parameter as filter, unstepped
@@ -298,7 +289,7 @@ public:
                 aobj.removeMember("PreviousTxnID");
                 aobj.removeMember("PreviousTxnLgrSeq");
 
-                BEAST_EXPECT( aobj == bobj[i]);
+                BEAST_EXPECT( aobj == bobj[i+2]);
             }
         }
         // test stepped one-at-a-time with limit=1, resume from prev marker
@@ -316,7 +307,8 @@ public:
 
                 aobj.removeMember("PreviousTxnID");
                 aobj.removeMember("PreviousTxnLgrSeq");
-                BEAST_EXPECT( aobj == bobj[i]);
+
+                BEAST_EXPECT(aobj == bobj[i]);
 
                 auto resume_marker = resp[jss::result][jss::marker];
                 params[jss::marker] = resume_marker;

src/test/rpc/AccountOffers_test.cpp

@@ -108,21 +108,21 @@ public:
         {
             BEAST_EXPECT(jro[0u][jss::quality]                   == "100000000");
             BEAST_EXPECT(jro[0u][jss::taker_gets][jss::currency] == "USD");
-            BEAST_EXPECT(jro[0u][jss::taker_gets][jss::issuer]   == bob.human());
+            BEAST_EXPECT(jro[0u][jss::taker_gets][jss::issuer]   == gw.human());
             BEAST_EXPECT(jro[0u][jss::taker_gets][jss::value]    == "1");
             BEAST_EXPECT(jro[0u][jss::taker_pays]                == "100000000");
 
-            BEAST_EXPECT(jro[1u][jss::quality]                   == "100000000");
+            BEAST_EXPECT(jro[1u][jss::quality]                   == "5000000");
             BEAST_EXPECT(jro[1u][jss::taker_gets][jss::currency] == "USD");
             BEAST_EXPECT(jro[1u][jss::taker_gets][jss::issuer]   == gw.human());
-            BEAST_EXPECT(jro[1u][jss::taker_gets][jss::value]    == "1");
+            BEAST_EXPECT(jro[1u][jss::taker_gets][jss::value]    == "2");
-            BEAST_EXPECT(jro[1u][jss::taker_pays]                == "100000000");
+            BEAST_EXPECT(jro[1u][jss::taker_pays]                == "10000000");
 
-            BEAST_EXPECT(jro[2u][jss::quality]                   == "5000000");
+            BEAST_EXPECT(jro[2u][jss::quality]                   == "100000000");
             BEAST_EXPECT(jro[2u][jss::taker_gets][jss::currency] == "USD");
-            BEAST_EXPECT(jro[2u][jss::taker_gets][jss::issuer]   == gw.human());
+            BEAST_EXPECT(jro[2u][jss::taker_gets][jss::issuer]   == bob.human());
-            BEAST_EXPECT(jro[2u][jss::taker_gets][jss::value]    == "2");
+            BEAST_EXPECT(jro[2u][jss::taker_gets][jss::value]    == "1");
-            BEAST_EXPECT(jro[2u][jss::taker_pays]                == "10000000");
+            BEAST_EXPECT(jro[2u][jss::taker_pays]                == "100000000");
         }
 
         {

src/test/rpc/OwnerInfo_test.cpp

@@ -120,26 +120,26 @@ class OwnerInfo_test : public beast::unit_test::suite
 
         BEAST_EXPECT (
             lines[0u][sfBalance.fieldName] ==
-            (STAmount{Issue{to_currency("USD"), noAccount()}, 0}
-                 .value().getJson(0)));
-        BEAST_EXPECT (
-            lines[0u][sfHighLimit.fieldName] ==
-            alice["USD"](1000).value().getJson(0));
-        BEAST_EXPECT (
-            lines[0u][sfLowLimit.fieldName] ==
-            USD(0).value().getJson(0));
-
-        BEAST_EXPECT (
-            lines[1u][sfBalance.fieldName] ==
             (STAmount{Issue{to_currency("CNY"), noAccount()}, 0}
                  .value().getJson(0)));
         BEAST_EXPECT (
-            lines[1u][sfHighLimit.fieldName] ==
+            lines[0u][sfHighLimit.fieldName] ==
             alice["CNY"](1000).value().getJson(0));
         BEAST_EXPECT (
-            lines[1u][sfLowLimit.fieldName] ==
+            lines[0u][sfLowLimit.fieldName] ==
             gw["CNY"](0).value().getJson(0));
 
+        BEAST_EXPECT (
+            lines[1u][sfBalance.fieldName] ==
+            (STAmount{Issue{to_currency("USD"), noAccount()}, 0}
+                .value().getJson(0)));
+        BEAST_EXPECT (
+            lines[1u][sfHighLimit.fieldName] ==
+            alice["USD"](1000).value().getJson(0));
+        BEAST_EXPECT (
+            lines[1u][sfLowLimit.fieldName] ==
+            USD(0).value().getJson(0));
+
         if (! BEAST_EXPECT (result[jss::accepted].isMember(jss::offers)))
             return;
         auto offers = result[jss::accepted][jss::offers];
@@ -163,26 +163,26 @@ class OwnerInfo_test : public beast::unit_test::suite
 
         BEAST_EXPECT (
             lines[0u][sfBalance.fieldName] ==
-            (STAmount{Issue{to_currency("USD"), noAccount()}, -50}
-                 .value().getJson(0)));
-        BEAST_EXPECT (
-            lines[0u][sfHighLimit.fieldName] ==
-            alice["USD"](1000).value().getJson(0));
-        BEAST_EXPECT (
-            lines[0u][sfLowLimit.fieldName] ==
-            gw["USD"](0).value().getJson(0));
-
-        BEAST_EXPECT (
-            lines[1u][sfBalance.fieldName] ==
             (STAmount{Issue{to_currency("CNY"), noAccount()}, -50}
                  .value().getJson(0)));
         BEAST_EXPECT (
-            lines[1u][sfHighLimit.fieldName] ==
+            lines[0u][sfHighLimit.fieldName] ==
             alice["CNY"](1000).value().getJson(0));
         BEAST_EXPECT (
-            lines[1u][sfLowLimit.fieldName] ==
+            lines[0u][sfLowLimit.fieldName] ==
             gw["CNY"](0).value().getJson(0));
 
+        BEAST_EXPECT (
+            lines[1u][sfBalance.fieldName] ==
+            (STAmount{Issue{to_currency("USD"), noAccount()}, -50}
+                .value().getJson(0)));
+        BEAST_EXPECT (
+            lines[1u][sfHighLimit.fieldName] ==
+            alice["USD"](1000).value().getJson(0));
+        BEAST_EXPECT (
+            lines[1u][sfLowLimit.fieldName] ==
+            gw["USD"](0).value().getJson(0));
+
         if (! BEAST_EXPECT (result[jss::current].isMember(jss::offers)))
             return;
         offers = result[jss::current][jss::offers];
@@ -190,16 +190,14 @@ class OwnerInfo_test : public beast::unit_test::suite
         if (! BEAST_EXPECT (offers.isArray() && offers.size() == 2))
             return;
 
-        // first offer is same as in accepted.
         BEAST_EXPECT (
-            offers[0u] == result[jss::accepted][jss::offers][0u]);
+            offers[1u] == result[jss::accepted][jss::offers][0u]);
-        // second offer, for CNY
         BEAST_EXPECT (
-            offers[1u][jss::Account] == alice.human());
+            offers[0u][jss::Account] == alice.human());
         BEAST_EXPECT (
-            offers[1u][sfTakerGets.fieldName] == XRP(1000).value().getJson(0));
+            offers[0u][sfTakerGets.fieldName] == XRP(1000).value().getJson(0));
         BEAST_EXPECT (
-            offers[1u][sfTakerPays.fieldName] == CNY(2).value().getJson(0));
+            offers[0u][sfTakerPays.fieldName] == CNY(2).value().getJson(0));
     }
 
 public: