Improve pseudo-transaction handling (RIPD-1454, RIPD-1455):

Adds additional checks to prevent relaying and retrying pseudo-transactions.

Builds/VisualStudio2015/RippleD.vcxproj

@@ -4247,6 +4247,10 @@
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
     </ClCompile>
+    <ClCompile Include="..\..\src\test\app\PseudoTx_test.cpp">
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
+    </ClCompile>
     <ClCompile Include="..\..\src\test\app\Regression_test.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>

Builds/VisualStudio2015/RippleD.vcxproj.filters

@@ -5037,6 +5037,9 @@
     <ClCompile Include="..\..\src\test\app\PayStrand_test.cpp">
       <Filter>test\app</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\src\test\app\PseudoTx_test.cpp">
+      <Filter>test\app</Filter>
+    </ClCompile>
     <ClCompile Include="..\..\src\test\app\Regression_test.cpp">
       <Filter>test\app</Filter>
     </ClCompile>

src/ripple/app/consensus/RCLConsensus.cpp

@@ -155,7 +155,6 @@ RCLConsensus::relay(RCLCxTx const& tx)
     if (app_.getHashRouter().shouldRelay(tx.id()))
     {
         auto const slice = tx.tx_.slice();
-
         protocol::TMTransaction msg;
         msg.set_rawtransaction(slice.data(), slice.size());
         msg.set_status(protocol::tsNEW);
@@ -479,6 +478,12 @@ RCLConsensus::doAccept(
 
                     SerialIter sit(it.second.tx().tx_.slice());
                     auto txn = std::make_shared<STTx const>(sit);
+
+                    // Disputed pseudo-transactions that were not accepted
+                    // can't be succesfully applied in the next ledger
+                    if (isPseudoTx(*txn))
+                        continue;
+
                     retriableTxs.insert(txn);
 
                     anyDisputes = true;

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

@@ -77,10 +77,10 @@ invoke_preclaim(PreclaimContext const& ctx)
     // list one, preflight will have already a flagged a failure.
     auto const id = ctx.tx.getAccountID(sfAccount);
     auto const baseFee = T::calculateBaseFee(ctx);
-    TER result;
+
     if (id != zero)
     {
-        result = T::checkSeq(ctx);
+        TER result = T::checkSeq(ctx);
 
         if (result != tesSUCCESS)
             return { result, baseFee };
@@ -95,17 +95,9 @@ invoke_preclaim(PreclaimContext const& ctx)
         if (result != tesSUCCESS)
             return { result, baseFee };
 
-        result = T::preclaim(ctx);
-
-        if (result != tesSUCCESS)
-            return{ result, baseFee };
-    }
-    else
-    {
-        result = tesSUCCESS;
     }
 
-    return { tesSUCCESS, baseFee };
+    return{ T::preclaim(ctx), baseFee };
 }
 
 static

src/ripple/protocol/STTx.h

@@ -167,6 +167,9 @@ bool passesLocalChecks (STObject const& st, std::string&);
 std::shared_ptr<STTx const>
 sterilize (STTx const& stx);
 
+/** Check whether a transaction is a pseudo-transaction */
+bool isPseudoTx(STObject const& tx);
+
 } // ripple
 
 #endif

src/ripple/protocol/impl/STTx.cpp

@@ -504,6 +504,11 @@ bool passesLocalChecks (STObject const& st, std::string& reason)
         return false;
     }
 
+    if (isPseudoTx(st))
+    {
+        reason = "Cannot submit pseudo transactions.";
+        return false;
+    }
     return true;
 }
 
@@ -516,4 +521,14 @@ sterilize (STTx const& stx)
     return std::make_shared<STTx const>(std::ref(sit));
 }
 
+bool
+isPseudoTx(STObject const& tx)
+{
+    auto t = tx[~sfTransactionType];
+    if (!t)
+        return false;
+    auto tt = static_cast<TxType>(*t);
+    return tt == ttAMENDMENT || tt == ttFEE;
+}
+
 } // ripple

src/test/app/PseudoTx_test.cpp

@@ -0,0 +1,114 @@
+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2017 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/app/tx/apply.h>
+#include <ripple/protocol/STAccount.h>
+#include <string>
+#include <test/jtx.h>
+#include <vector>
+
+namespace ripple {
+namespace test {
+
+struct PseudoTx_test : public beast::unit_test::suite
+{
+    std::vector<STTx>
+    getPseudoTxs(std::uint32_t seq)
+    {
+        std::vector<STTx> res;
+
+        res.emplace_back(STTx(ttFEE, [&](auto& obj) {
+            obj[sfAccount] = AccountID();
+            obj[sfLedgerSequence] = seq;
+            obj[sfBaseFee] = 0;
+            obj[sfReserveBase] = 0;
+            obj[sfReserveIncrement] = 0;
+            obj[sfReferenceFeeUnits] = 0;
+        }));
+
+        res.emplace_back(STTx(ttAMENDMENT, [&](auto& obj) {
+            obj.setAccountID(sfAccount, AccountID());
+            obj.setFieldH256(sfAmendment, uint256(2));
+            obj.setFieldU32(sfLedgerSequence, seq);
+        }));
+
+        return res;
+    }
+
+    std::vector<STTx>
+    getRealTxs()
+    {
+        std::vector<STTx> res;
+
+        res.emplace_back(STTx(ttACCOUNT_SET, [&](auto& obj) {
+            obj[sfAccount] = AccountID(1);
+        }));
+
+        res.emplace_back(STTx(ttPAYMENT, [&](auto& obj) {
+            obj.setAccountID(sfAccount, AccountID(2));
+            obj.setAccountID(sfDestination, AccountID(3));
+        }));
+
+        return res;
+    }
+
+    void
+    testPrevented()
+    {
+        using namespace jtx;
+        Env env(*this);
+
+        for (auto const& stx : getPseudoTxs(env.closed()->seq() + 1))
+        {
+            std::string reason;
+            BEAST_EXPECT(isPseudoTx(stx));
+            BEAST_EXPECT(!passesLocalChecks(stx, reason));
+            BEAST_EXPECT(reason == "Cannot submit pseudo transactions.");
+            env.app().openLedger().modify(
+                [&](OpenView& view, beast::Journal j) {
+                    auto const result =
+                        ripple::apply(env.app(), view, stx, tapNONE, j);
+                    BEAST_EXPECT(!result.second && result.first == temINVALID);
+                    return result.second;
+                });
+        }
+    }
+
+    void
+    testAllowed()
+    {
+        for (auto const& stx : getRealTxs())
+        {
+            std::string reason;
+            BEAST_EXPECT(!isPseudoTx(stx));
+            BEAST_EXPECT(passesLocalChecks(stx, reason));
+        }
+    }
+
+    void
+    run() override
+    {
+        testPrevented();
+        testAllowed();
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(PseudoTx, app, ripple);
+
+}  // test
+}  // ripple

src/test/app/TxQ_test.cpp

@@ -35,6 +35,13 @@
 #include <test/jtx/WSClient.h>
 
 namespace ripple {
+
+namespace detail {
+extern
+std::vector<std::string>
+supportedAmendments ();
+}
+
 namespace test {
 
 class TxQ_test : public beast::unit_test::suite
@@ -96,7 +103,8 @@ class TxQ_test : public beast::unit_test::suite
 
     static
     std::unique_ptr<Config>
-    makeConfig(std::map<std::string, std::string> extra = {})
+    makeConfig(std::map<std::string, std::string> extraTxQ = {},
+        std::map<std::string, std::string> extraVoting = {})
     {
         auto p = test::jtx::envconfig();
         auto& section = p->section("transaction_queue");
@@ -105,8 +113,24 @@ class TxQ_test : public beast::unit_test::suite
         section.set("max_ledger_counts_to_store", "100");
         section.set("retry_sequence_percent", "25");
         section.set("zero_basefee_transaction_feelevel", "100000000000");
-        for (auto const& value : extra)
+
+        for (auto const& value : extraTxQ)
             section.set(value.first, value.second);
+
+        // Some tests specify different fee settings that are enabled by
+        // a FeeVote
+        if (!extraVoting.empty())
+        {
+
+            auto& votingSection = p->section("voting");
+            for (auto const & value : extraVoting)
+            {
+                votingSection.set(value.first, value.second);
+            }
+
+            // In order for the vote to occur, we must run as a validator
+            p->section("validation_seed").legacy("shUwVw52ofnCUX5m7kPTKzJdr4HEH");
+        }
         return p;
     }
 
@@ -114,37 +138,22 @@ class TxQ_test : public beast::unit_test::suite
     initFee(jtx::Env& env, std::size_t expectedInLedger, std::uint32_t base,
         std::uint32_t units, std::uint32_t reserve, std::uint32_t increment)
     {
-        // Change the reserve fee so we can create an account
+        // Run past the flag ledger so that a Fee change vote occurs and
-        // with a lower balance.
+        // lowers the reserve fee.  This will allow creating accounts with lower
-        STTx feeTx(ttFEE,
+        // balances.
-            [&](auto& obj)
+        for(auto i = env.current()->seq(); i <= 257; ++i)
-        {
+            env.close();
-            obj[sfAccount] = AccountID();
-            obj[sfLedgerSequence] = env.current()->info().seq;
-            obj[sfBaseFee] = base;
-            obj[sfReferenceFeeUnits] = units;
-            obj[sfReserveBase] = reserve;
-            obj[sfReserveIncrement] = increment;
-        });
 
-        auto const changed = env.app().openLedger().modify(
-            [&](OpenView& view, beast::Journal j)
-        {
-            auto const result = ripple::apply(
-                env.app(), view, feeTx,
-                tapNONE, j);
-            BEAST_EXPECT(result.second && result.first ==
-                tesSUCCESS);
-            return result.second;
-        });
-        BEAST_EXPECT(changed);
         // Pad a couple of txs to keep the median at the default
         env(noop(env.master));
         env(noop(env.master));
+
         // Close the ledger with a delay to force the TxQ stats
         // to stay at the default.
         env.close(env.now() + 5s, 10000ms);
-        checkMetrics(env, 0, boost::none, 0, expectedInLedger, 256);
+        checkMetrics(env, 0,
+            2 * (ripple::detail::supportedAmendments().size() + 1),
+                0, expectedInLedger, 256);
         auto const fees = env.current()->fees();
         BEAST_EXPECT(fees.base == base);
         BEAST_EXPECT(fees.units == units);
@@ -694,7 +703,11 @@ public:
     {
         using namespace jtx;
 
-        Env env(*this, makeConfig({ { "minimum_txn_in_ledger_standalone", "3" } }),
+        Env env(
+            *this,
+            makeConfig(
+                {{"minimum_txn_in_ledger_standalone", "3"}},
+                {{"account_reserve", "200"}, {"owner_reserve", "50"}}),
             features(featureFeeEscalation));
 
         auto alice = Account("alice");
@@ -709,15 +722,17 @@ public:
         checkMetrics(env, 0, boost::none, 0, 3, 256);
 
         initFee(env, 3, 10, 10, 200, 50);
+        auto const initQueueMax =
+            2 * (ripple::detail::supportedAmendments().size() + 1);
 
         // Create several accounts while the fee is cheap so they all apply.
         env.fund(drops(2000), noripple(alice));
         env.fund(XRP(500000), noripple(bob, charlie, daria));
-        checkMetrics(env, 0, boost::none, 4, 3, 256);
+        checkMetrics(env, 0, initQueueMax, 4, 3, 256);
 
         // Alice - price starts exploding: held
         env(noop(alice), queued);
-        checkMetrics(env, 1, boost::none, 4, 3, 256);
+        checkMetrics(env, 1, initQueueMax, 4, 3, 256);
 
         auto aliceSeq = env.seq(alice);
         auto bobSeq = env.seq(bob);
@@ -726,31 +741,31 @@ public:
         // Alice - try to queue a second transaction, but leave a gap
         env(noop(alice), seq(aliceSeq + 2), fee(100),
             ter(terPRE_SEQ));
-        checkMetrics(env, 1, boost::none, 4, 3, 256);
+        checkMetrics(env, 1, initQueueMax, 4, 3, 256);
 
         // Alice - queue a second transaction. Yay.
         env(noop(alice), seq(aliceSeq + 1), fee(13),
             queued);
-        checkMetrics(env, 2, boost::none, 4, 3, 256);
+        checkMetrics(env, 2, initQueueMax, 4, 3, 256);
 
         // Alice - queue a third transaction. Yay.
         env(noop(alice), seq(aliceSeq + 2), fee(17),
             queued);
-        checkMetrics(env, 3, boost::none, 4, 3, 256);
+        checkMetrics(env, 3, initQueueMax, 4, 3, 256);
 
         // Bob - queue a transaction
         env(noop(bob), queued);
-        checkMetrics(env, 4, boost::none, 4, 3, 256);
+        checkMetrics(env, 4, initQueueMax, 4, 3, 256);
 
         // Bob - queue a second transaction
         env(noop(bob), seq(bobSeq + 1), fee(50),
             queued);
-        checkMetrics(env, 5, boost::none, 4, 3, 256);
+        checkMetrics(env, 5, initQueueMax, 4, 3, 256);
 
         // Charlie - queue a transaction, with a higher fee
         // than default
         env(noop(charlie), fee(15), queued);
-        checkMetrics(env, 6, boost::none, 4, 3, 256);
+        checkMetrics(env, 6, initQueueMax, 4, 3, 256);
 
         BEAST_EXPECT(env.seq(alice) == aliceSeq);
         BEAST_EXPECT(env.seq(bob) == bobSeq);
@@ -1148,8 +1163,11 @@ public:
     {
         using namespace jtx;
 
-        Env env(*this,
+        Env env(
-            makeConfig({ { "minimum_txn_in_ledger_standalone", "3" } }),
+            *this,
+            makeConfig(
+                {{"minimum_txn_in_ledger_standalone", "3"}},
+                {{"account_reserve", "200"}, {"owner_reserve", "50"}}),
             features(featureFeeEscalation));
 
         auto alice = Account("alice");
@@ -1158,18 +1176,20 @@ public:
         auto queued = ter(terQUEUED);
 
         initFee(env, 3, 10, 10, 200, 50);
+        auto const initQueueMax =
+            2 * (ripple::detail::supportedAmendments().size() + 1);
 
         BEAST_EXPECT(env.current()->fees().base == 10);
 
-        checkMetrics(env, 0, boost::none, 0, 3, 256);
+        checkMetrics(env, 0, initQueueMax, 0, 3, 256);
 
         env.fund(drops(5000), noripple(alice));
         env.fund(XRP(50000), noripple(bob));
-        checkMetrics(env, 0, boost::none, 2, 3, 256);
+        checkMetrics(env, 0, initQueueMax, 2, 3, 256);
         auto USD = bob["USD"];
 
         env(offer(alice, USD(5000), drops(5000)), require(owners(alice, 1)));
-        checkMetrics(env, 0, boost::none, 3, 3, 256);
+        checkMetrics(env, 0, initQueueMax, 3, 3, 256);
 
         env.close();
         checkMetrics(env, 0, 6, 0, 3, 256);

src/test/unity/app_test_unity.cpp

@@ -49,3 +49,4 @@
 #include <test/app/ValidatorSite_test.cpp>
 #include <test/app/SetTrust_test.cpp>
 #include <test/app/Ticket_test.cpp>
+#include <test/app/PseudoTx_test.cpp>