fix signerlist import crashbug

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

@@ -33,6 +33,7 @@
 #include <ripple/protocol/STTx.h>
 #include <ripple/basics/base64.h>
 #include <ripple/app/misc/Manifest.h>
+#include <ripple/app/tx/impl/SetSignerList.h>
 
 namespace ripple {
 
@@ -191,6 +192,13 @@ Import::preflight(PreflightContext const& ctx)
     if (!stpTrans)
         return temMALFORMED;
 
+    if (stpTrans->isFieldPresent(sfTicketSequence))
+    {
+        JLOG(ctx.j.warn())
+            << "Import: cannot use TicketSequence XPOP.";
+        return temMALFORMED;
+    }
+
     // check if txn is emitted or a psuedo
     if (isPseudoTx(*stpTrans) || stpTrans->isFieldPresent(sfEmitDetails))
     {
@@ -952,16 +960,24 @@ Import::doApply()
 
     // why aren't transactors stateful, why does this need to be recomputed each time...
     STAmount burn = stpTrans->getFieldAmount(sfFee);
+
+    if (!isXRP(burn) || burn < beast::zero)
+    {
+        JLOG(ctx_.journal.warn())
+            << "Import: inner fee was not XRP value.";
+        return tefINTERNAL;
+    }
+
     uint32_t importSequence = stpTrans->getFieldU32(sfSequence);
 
     auto const id = ctx_.tx[sfAccount];
 
     auto const k = keylet::account(id);
-    auto const ksl = keylet::signers(id);
-
     auto sle = view().peek(k);
 
-    std::optional<STArray> setSignerEntries;
+    std::optional<
+    std::vector<ripple::SignerEntries::SignerEntry>> setSignerEntries;
+    uint32_t setSignerQuorum { 0 };
     std::optional<AccountID> setRegularKey;
     auto const signingKey = ctx_.tx.getSigningPubKey();
     bool const signedWithMaster = !signingKey.empty() && calcAccountID(PublicKey(makeSlice(signingKey))) == id;
@@ -973,9 +989,50 @@ Import::doApply()
     {
         if (tt == ttSIGNER_LIST_SET)
         {
-            JLOG(ctx_.journal.warn()) << "SingerListSet";
+            if (stpTrans->isFieldPresent(sfSignerQuorum) &&
-            // key import: signer list
+                stpTrans->isFieldPresent(sfSignerEntries))
-            setSignerEntries = stpTrans->getFieldArray(sfSignerEntries);
+            {
+                auto const entries = stpTrans->getFieldArray(sfSignerEntries);
+
+                if (entries.empty())
+                {
+                    JLOG(ctx_.journal.warn()) << "Import: SignerListSet entires empty, skipping.";
+                }
+                else
+                {
+                    JLOG(ctx_.journal.trace()) << "Import: SingerListSet";
+                    // key import: signer list
+                    setSignerEntries.emplace();
+                    setSignerEntries->reserve(entries.size());
+                    for (auto const& e: entries)
+                    {
+                        if (!e.isFieldPresent(sfAccount) || !e.isFieldPresent(sfSignerWeight))
+                        {
+                            JLOG(ctx_.journal.warn())
+                                << "Import: SignerListSet entry lacked a required field (Account/SignerWeight). "
+                                << "Skipping SignerListSet.";
+                            setSignerEntries = std::nullopt;
+                            break;
+                        }
+
+                        std::optional<uint256> tag;
+                        if (e.isFieldPresent(sfWalletLocator))
+                            tag = e.getFieldH256(sfWalletLocator);
+
+                        setSignerEntries->emplace_back(
+                            e.getAccountID(sfAccount),
+                            e.getFieldU16(sfSignerWeight),
+                            tag);
+                    }
+                    setSignerQuorum = stpTrans->getFieldU32(sfSignerQuorum);
+                }
+            }
+            else
+            {
+                JLOG(ctx_.journal.warn())
+                    << "Import: SingerListSet lacked either populated SignerEntries or SignerQuorum, ignoring.";
+            }
+
         }
         else if (tt == ttREGULAR_KEY_SET)
         {
@@ -987,6 +1044,18 @@ Import::doApply()
 
     bool const create = !sle;
 
+    // compute the amount they receive first because the amount is maybe needed for computing setsignerlist later
+    STAmount startBal = create ? STAmount(INITIAL_IMPORT_XRP) : sle->getFieldAmount(sfBalance);
+    STAmount finalBal = startBal + burn;
+
+    // this should never happen
+    if (finalBal < startBal)
+    {
+        JLOG(ctx_.journal.warn())
+            << "Import: logic error finalBal < startBal.";
+        return tefINTERNAL;
+    }
+
     if (create)
     {
         // Create the account.
@@ -999,20 +1068,9 @@ Import::doApply()
 
         sle->setFieldU32(sfImportSequence, importSequence);
         sle->setFieldU32(sfSequence, seqno);
+        sle->setFieldU32(sfOwnerCount, 0);
 
-        STAmount initBal = STAmount(INITIAL_IMPORT_XRP) + burn;
+        sle->setFieldAmount(sfBalance, finalBal);
-
-        if (initBal <= beast::zero)
-        {
-            JLOG(ctx_.journal.warn())
-                << "Import: inital balance <= 0";
-
-            return tefINTERNAL;
-        }
-
-        JLOG(ctx_.journal.warn()) << "Import: inital balance" << initBal;
-
-        sle->setFieldAmount(sfBalance, initBal);
 
     }
     else if (sle->getFieldU32(sfImportSequence) >= importSequence)
@@ -1023,25 +1081,19 @@ Import::doApply()
         return tefINTERNAL;
     }
 
-    if (setSignerEntries) {
+    if (setRegularKey)
-        JLOG(ctx_.journal.warn()) << "signer list set";
-        auto signerList = std::make_shared<SLE>(ksl);
-        signerList->setFieldArray(sfSignerEntries, *setSignerEntries);
-        view().insert(signerList);
-    } 
-    else if (setRegularKey)
     {
-        JLOG(ctx_.journal.warn()) << "set regular key";
+        JLOG(ctx_.journal.warn()) << "Import: actioning SetRegularKey " << *setRegularKey << " acc: " << id;
         sle->setAccountID(sfRegularKey, *setRegularKey);
     }
 
     if (create)
     {
-        JLOG(ctx_.journal.warn()) << "create";
+        JLOG(ctx_.journal.trace()) << "Import: creating account " << id;
         if (!signedWithMaster)
         {
             // disable master if the account is created using non-master key
-            JLOG(ctx_.journal.warn()) << "create - disable master";
+            JLOG(ctx_.journal.warn()) << "Import: keying of " << id << " is unclear - disable master";
             sle->setAccountID(sfRegularKey, noAccount());
             sle->setFieldU32(sfFlags, lsfDisableMaster);
         }
@@ -1050,23 +1102,13 @@ Import::doApply()
     else
     {
         // account already exists
-        JLOG(ctx_.journal.warn()) << "update - import sequence";
+        JLOG(ctx_.journal.trace()) << "Import: updating existing account " << id;
         sle->setFieldU32(sfImportSequence, importSequence);
-
+        sle->setFieldAmount(sfBalance, finalBal);
-        // credit the PoB
-        if (burn > beast::zero)
-        {
-            JLOG(ctx_.journal.warn()) << "update - credit burn";
-            STAmount startBal = sle->getFieldAmount(sfBalance);
-            STAmount finalBal = startBal + burn;
-            if (finalBal > startBal)
-                sle->setFieldAmount(sfBalance, finalBal);
-        }
 
         view().update(sle);
     }
 
-
     // todo: manifest sequence needs to be recorded to prevent certain types of replay attack
     auto const infoVL = getVLInfo(*xpop, ctx_.journal);
 
@@ -1079,7 +1121,7 @@ Import::doApply()
     if (!sleVL)
     {
         // create VL import seq counter
-        JLOG(ctx_.journal.warn()) << "create vl seq - insert import sequence + public key";
+        JLOG(ctx_.journal.trace()) << "Import: create vl seq - insert import sequence + public key";
         sleVL = std::make_shared<SLE>(keyletVL);
         sleVL->setFieldU32(sfImportSequence, infoVL->first);
         sleVL->setFieldVL(sfPublicKey, infoVL->second.slice());
@@ -1087,7 +1129,7 @@ Import::doApply()
     }
     else
     {
-        JLOG(ctx_.journal.warn()) << "update vl - insert import sequence";
+        JLOG(ctx_.journal.trace()) << "Import: update vl";
         uint32_t current = sleVL->getFieldU32(sfImportSequence);
 
         if (current > infoVL->first)
@@ -1107,6 +1149,40 @@ Import::doApply()
         }
     }
 
+    /// this logic is executed last after the account might have already been created    
+    if (setSignerEntries)
+    {
+        JLOG(ctx_.journal.trace())
+            << "Import: actioning SignerListSet "
+            << "quorum: " << setSignerQuorum << " "
+            << "size: " << setSignerEntries->size();
+    
+        Sandbox sb(&view());
+        TER result = 
+        SetSignerList::replaceSignersFromLedger(
+            ctx_.app,
+            sb,
+            ctx_.journal,
+            id,
+            setSignerQuorum,
+            *setSignerEntries,
+            finalBal.xrp());
+
+        if (result == tesSUCCESS)
+        {
+            JLOG(ctx_.journal.trace())
+                << "Import: successful SignerListSet";
+            sb.apply(ctx_.rawView());
+        }
+        else
+        {
+            JLOG(ctx_.journal.warn())
+                << "Import: SetSignerList failed with code "
+                << result << " acc: " << id;
+        }
+    } 
+
+
     return tesSUCCESS;
 }
 

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

@@ -312,70 +312,14 @@ SetSignerList::validateQuorumAndSignerEntries(
 TER
 SetSignerList::replaceSignerList()
 {
-    auto const accountKeylet = keylet::account(account_);
+    return replaceSignersFromLedger(
-    auto const ownerDirKeylet = keylet::ownerDir(account_);
-    auto const signerListKeylet = keylet::signers(account_);
-
-    // This may be either a create or a replace.  Preemptively remove any
-    // old signer list.  May reduce the reserve, so this is done before
-    // checking the reserve.
-    if (TER const ter = removeSignersFromLedger(
             ctx_.app,
-            view(),
+            ctx_.view(),
-            accountKeylet,
+            j_,
-            ownerDirKeylet,
+            account_,
-            signerListKeylet,
+            quorum_,
-            j_))
+            signers_,
-        return ter;
+            mPriorBalance);
-
-    auto const sle = view().peek(accountKeylet);
-    if (!sle)
-        return tefINTERNAL;
-
-    // Compute new reserve.  Verify the account has funds to meet the reserve.
-    std::uint32_t const oldOwnerCount{(*sle)[sfOwnerCount]};
-
-    // The required reserve changes based on featureMultiSignReserve...
-    int addedOwnerCount{1};
-    std::uint32_t flags{lsfOneOwnerCount};
-    if (!ctx_.view().rules().enabled(featureMultiSignReserve))
-    {
-        addedOwnerCount = signerCountBasedOwnerCountDelta(
-            signers_.size(), ctx_.view().rules());
-        flags = 0;
-    }
-
-    XRPAmount const newReserve{
-        view().fees().accountReserve(oldOwnerCount + addedOwnerCount)};
-
-    // We check the reserve against the starting balance because we want to
-    // allow dipping into the reserve to pay fees.  This behavior is consistent
-    // with CreateTicket.
-    if (mPriorBalance < newReserve)
-        return tecINSUFFICIENT_RESERVE;
-
-    // Everything's ducky.  Add the ltSIGNER_LIST to the ledger.
-    auto signerList = std::make_shared<SLE>(signerListKeylet);
-    view().insert(signerList);
-    writeSignersToSLE(signerList, flags);
-
-    auto viewJ = ctx_.app.journal("View");
-    // Add the signer list to the account's directory.
-    auto const page = ctx_.view().dirInsert(
-        ownerDirKeylet, signerListKeylet, describeOwnerDir(account_));
-
-    JLOG(j_.trace()) << "Create signer list for account " << toBase58(account_)
-                     << ": " << (page ? "success" : "failure");
-
-    if (!page)
-        return tecDIR_FULL;
-
-    signerList->setFieldU64(sfOwnerNode, *page);
-
-    // If we succeeded, the new entry counts against the
-    // creator's reserve.
-    adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
-    return tesSUCCESS;
 }
 
 TER
@@ -400,21 +344,24 @@ SetSignerList::destroySignerList()
 
 void
 SetSignerList::writeSignersToSLE(
+    ApplyView& view,
     SLE::pointer const& ledgerEntry,
-    std::uint32_t flags) const
+    std::uint32_t flags,
+    std::uint32_t quorum,
+    std::vector<SignerEntries::SignerEntry> const& signers)
 {
     // Assign the quorum, default SignerListID, and flags.
-    ledgerEntry->setFieldU32(sfSignerQuorum, quorum_);
+    ledgerEntry->setFieldU32(sfSignerQuorum, quorum);
     ledgerEntry->setFieldU32(sfSignerListID, defaultSignerListID_);
     if (flags)  // Only set flags if they are non-default (default is zero).
         ledgerEntry->setFieldU32(sfFlags, flags);
 
     bool const expandedSignerList =
-        ctx_.view().rules().enabled(featureExpandedSignerList);
+        view.rules().enabled(featureExpandedSignerList);
 
     // Create the SignerListArray one SignerEntry at a time.
-    STArray toLedger(signers_.size());
+    STArray toLedger(signers.size());
-    for (auto const& entry : signers_)
+    for (auto const& entry : signers)
     {
         toLedger.emplace_back(sfSignerEntry);
         STObject& obj = toLedger.back();

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

@@ -73,6 +73,15 @@ public:
         beast::Journal j);
 
 private:
+    static
+    void
+    writeSignersToSLE(
+        ApplyView& view,
+        SLE::pointer const& ledgerEntry,
+        std::uint32_t flags,
+        std::uint32_t quorum,
+        std::vector<SignerEntries::SignerEntry> const& signers);
+
     static std::tuple<
         NotTEC,
         std::uint32_t,
@@ -96,6 +105,84 @@ private:
     void
     writeSignersToSLE(SLE::pointer const& ledgerEntry, std::uint32_t flags)
         const;
+
+public:
+    template <typename V>
+    static TER
+    replaceSignersFromLedger(
+        Application& app,
+        V& view,
+        beast::Journal j,
+        AccountID const& acc,
+        std::uint32_t quorum,
+        std::vector<SignerEntries::SignerEntry> const& signers,
+        XRPAmount const mPriorBalance)
+    {
+        auto const accountKeylet = keylet::account(acc);
+        auto const ownerDirKeylet = keylet::ownerDir(acc);
+        auto const signerListKeylet = keylet::signers(acc);
+
+        // This may be either a create or a replace.  Preemptively remove any
+        // old signer list.  May reduce the reserve, so this is done before
+        // checking the reserve.
+        if (TER const ter = removeSignersFromLedger(
+                app,
+                view,
+                accountKeylet,
+                ownerDirKeylet,
+                signerListKeylet,
+                j))
+            return ter;
+
+        auto const sle = view.peek(accountKeylet);
+        if (!sle)
+            return tefINTERNAL;
+
+        // Compute new reserve.  Verify the account has funds to meet the reserve.
+        std::uint32_t const oldOwnerCount{(*sle)[sfOwnerCount]};
+
+        // The required reserve changes based on featureMultiSignReserve...
+        int addedOwnerCount{1};
+        std::uint32_t flags{lsfOneOwnerCount};
+        if (!view.rules().enabled(featureMultiSignReserve))
+        {
+            addedOwnerCount = signerCountBasedOwnerCountDelta(
+                signers.size(), view.rules());
+            flags = 0;
+        }
+
+        XRPAmount const newReserve{
+            view.fees().accountReserve(oldOwnerCount + addedOwnerCount)};
+
+        // We check the reserve against the starting balance because we want to
+        // allow dipping into the reserve to pay fees.  This behavior is consistent
+        // with CreateTicket.
+        if (mPriorBalance < newReserve)
+            return tecINSUFFICIENT_RESERVE;
+
+        // Everything's ducky.  Add the ltSIGNER_LIST to the ledger.
+        auto signerList = std::make_shared<SLE>(signerListKeylet);
+        view.insert(signerList);
+        writeSignersToSLE(view, signerList, flags, quorum, signers);
+
+        // Add the signer list to the account's directory.
+        auto const page = view.dirInsert(
+            ownerDirKeylet, signerListKeylet, describeOwnerDir(acc));
+
+        JLOG(j.trace()) << "Create signer list for account " << toBase58(acc)
+                         << ": " << (page ? "success" : "failure");
+
+        if (!page)
+            return tecDIR_FULL;
+
+        signerList->setFieldU64(sfOwnerNode, *page);
+
+        // If we succeeded, the new entry counts against the
+        // creator's reserve.
+        adjustOwnerCount(view, sle, addedOwnerCount, j);
+        return tesSUCCESS;
+    }
+
 };
 
 }  // namespace ripple

src/test/app/Import_test.cpp

@@ -33,7 +33,12 @@ getRaw) outer txid:
 #include <ripple/protocol/Import.h>
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
-
+#define BEAST_REQUIRE(x)     \
+    {                        \
+        BEAST_EXPECT(!!(x)); \
+        if (!(x))            \
+            return;          \
+    }
 namespace ripple {
 namespace test {
 
@@ -2541,6 +2546,7 @@ class Import_test : public beast::unit_test::suite
                 postAlice == preAlice + feeDrops + XRP(2) + XRP(0.000002));
             auto const [signers, signersSle] =
                 signersKeyAndSle(*env.current(), alice);
+            BEAST_REQUIRE(!!signersSle);
             auto const signerEntries =
                 signersSle->getFieldArray(sfSignerEntries);
             BEAST_EXPECT(signerEntries.size() == 1);