make SequenceNumber a signed integer (BumpSeq proposal 08-Feb-2018)

src/invariant/LedgerEntryIsValid.cpp

@@ -106,6 +106,10 @@ LedgerEntryIsValid::checkIsValid(AccountEntry const& ae) const
     {
         return fmt::format("Account balance ({}) is negative", ae.balance);
     }
+    if (ae.seqNum < 0)
+    {
+        return fmt::format("Account seqNum ({}) is negative", ae.seqNum);
+    }
     if (ae.numSubEntries > INT32_MAX)
     {
         return fmt::format("Account numSubEntries ({}) exceeds limit ({})",

src/ledger/AccountFrame.cpp

@@ -233,10 +233,9 @@ AccountFrame::loadAccount(AccountID const& accountID, Database& db)
                                 "inflationdest, homedomain, thresholds, "
                                 "flags, lastmodified "
                                 "FROM accounts WHERE accountid=:v1");
-    int64 seqNumS;
     auto& st = prep.statement();
     st.exchange(into(account.balance));
-    st.exchange(into(seqNumS));
+    st.exchange(into(account.seqNum));
     st.exchange(into(account.numSubEntries));
     st.exchange(into(inflationDest, inflationDestInd));
     st.exchange(into(homeDomain));
@@ -255,7 +254,6 @@ AccountFrame::loadAccount(AccountID const& accountID, Database& db)
         return nullptr;
     }
 
-    account.seqNum = static_cast<uint64>(seqNumS);
     account.homeDomain = homeDomain;
 
     bn::decode_b64(thresholds.begin(), thresholds.end(),
@@ -459,12 +457,10 @@ AccountFrame::storeUpdate(LedgerDelta& delta, Database& db, bool insert)
     string thresholds(bn::encode_b64(mAccountEntry.thresholds));
 
     {
-        int64 seqNumS = safeConvertToSigned<int64>(mAccountEntry.seqNum);
-
         soci::statement& st = prep.statement();
         st.exchange(use(actIDStrKey, "id"));
         st.exchange(use(mAccountEntry.balance, "v1"));
-        st.exchange(use(seqNumS, "v2"));
+        st.exchange(use(mAccountEntry.seqNum, "v2"));
         st.exchange(use(mAccountEntry.numSubEntries, "v3"));
         st.exchange(use(inflationDestStrKey, inflation_ind, "v4"));
         string homeDomain(mAccountEntry.homeDomain);

src/ledger/LedgerHeaderFrame.cpp

@@ -61,10 +61,16 @@ LedgerHeaderFrame::getHash() const
     return mHash;
 }
 
+SequenceNumber
+LedgerHeaderFrame::getStartingSequenceNumber(uint32 ledgerSeq)
+{
+    return static_cast<SequenceNumber>(ledgerSeq) << 32;
+}
+
 SequenceNumber
 LedgerHeaderFrame::getStartingSequenceNumber() const
 {
-    return static_cast<uint64_t>(mHeader.ledgerSeq) << 32;
+    return getStartingSequenceNumber(mHeader.ledgerSeq);
 }
 
 uint64_t

src/ledger/LedgerHeaderFrame.h

@@ -35,6 +35,7 @@ class LedgerHeaderFrame
     Hash const& getHash() const;
 
     // returns the first sequence number to use for new accounts
+    static SequenceNumber getStartingSequenceNumber(uint32 ledgerSeq);
     SequenceNumber getStartingSequenceNumber() const;
 
     // methods to generate IDs

src/ledger/LedgerTestUtils.cpp

@@ -91,6 +91,10 @@ makeValid(AccountEntry& a)
         }
     }
     a.numSubEntries = (uint32)a.signers.size();
+    if (a.seqNum < 0)
+    {
+        a.seqNum = -a.seqNum;
+    }
     a.flags = a.flags & MASK_ACCOUNT_FLAGS;
 }
 

src/simulation/LoadGenerator.cpp

@@ -487,7 +487,8 @@ LoadGenerator::createAccount(size_t i, uint32_t ledgerNum)
     auto accountName = "Account-" + to_string(i);
     return make_shared<AccountInfo>(
         i, txtest::getAccount(accountName.c_str()), 0,
-        (static_cast<SequenceNumber>(ledgerNum) << 32), ledgerNum, *this);
+        LedgerHeaderFrame::getStartingSequenceNumber(ledgerNum), ledgerNum,
+        *this);
 }
 
 vector<LoadGenerator::AccountInfoPtr>
@@ -755,7 +756,7 @@ LoadGenerator::AccountInfo::createDirectly(Application& app)
     AccountEntry& account = a.getAccount();
     auto ledger = app.getLedgerManager().getLedgerNum();
     account.balance = LOADGEN_ACCOUNT_BALANCE;
-    account.seqNum = ((SequenceNumber)ledger) << 32;
+    account.seqNum = LedgerHeaderFrame::getStartingSequenceNumber(ledger);
     a.touch(ledger);
     LedgerDelta delta(app.getLedgerManager().getCurrentLedgerHeader(),
                       app.getDatabase());

src/test/TestExceptions.cpp

@@ -269,6 +269,8 @@ throwIf(BumpSequenceResult const& result)
     {
     case BUMP_SEQUENCE_SUCCESS:
         break;
+    case BUMP_SEQUENCE_BAD_SEQ:
+        throw ex_BUMP_SEQUENCE_BAD_SEQ{};
     default:
         throw ex_UNKNOWN{};
     }

src/test/TestExceptions.h

@@ -47,6 +47,8 @@ TEST_EXCEPTION(ex_ALLOW_TRUST_TRUST_NOT_REQUIRED)
 TEST_EXCEPTION(ex_ALLOW_TRUST_CANT_REVOKE)
 TEST_EXCEPTION(ex_ALLOW_TRUST_SELF_NOT_ALLOWED)
 
+TEST_EXCEPTION(ex_BUMP_SEQUENCE_BAD_SEQ)
+
 TEST_EXCEPTION(ex_CREATE_ACCOUNT_MALFORMED)
 TEST_EXCEPTION(ex_CREATE_ACCOUNT_UNDERFUNDED)
 TEST_EXCEPTION(ex_CREATE_ACCOUNT_LOW_RESERVE)

src/transactions/BumpSequenceOpFrame.cpp

@@ -59,6 +59,14 @@ BumpSequenceOpFrame::doApply(Application& app, LedgerDelta& delta,
 bool
 BumpSequenceOpFrame::doCheckValid(Application& app)
 {
+    if (mBumpSequenceOp.bumpTo < 0)
+    {
+        app.getMetrics()
+            .NewMeter({"op-bump-sequence", "invalid", "bad-seq"}, "operation")
+            .Mark();
+        innerResult().code(BUMP_SEQUENCE_BAD_SEQ);
+        return false;
+    }
     return true;
 }
 }

src/transactions/BumpSequenceTests.cpp

@@ -48,10 +48,10 @@ TEST_CASE("bump sequence", "[tx][bumpsequence]")
             }
             SECTION("large bump")
             {
-                auto newSeq = UINT64_MAX;
+                auto newSeq = INT64_MAX;
                 a.bumpSequence(newSeq);
                 REQUIRE(a.loadSequenceNumber() == newSeq);
-                SECTION("no more tx when UINT64_MAX is reached")
+                SECTION("no more tx when INT64_MAX is reached")
                 {
                     REQUIRE_THROWS_AS(a.pay(root, 1), ex_txBAD_SEQ);
                 }
@@ -63,6 +63,12 @@ TEST_CASE("bump sequence", "[tx][bumpsequence]")
                 // tx consumes sequence, bumpSequence doesn't do anything
                 REQUIRE(a.loadSequenceNumber() == oldSeq + 1);
             }
+            SECTION("bad seq")
+            {
+                REQUIRE_THROWS_AS(a.bumpSequence(-1), ex_BUMP_SEQUENCE_BAD_SEQ);
+                REQUIRE_THROWS_AS(a.bumpSequence(INT64_MIN),
+                                  ex_BUMP_SEQUENCE_BAD_SEQ);
+            }
         });
     }
     SECTION("not supported")

src/transactions/MergeOpFrame.cpp

@@ -4,6 +4,7 @@
 
 #include "transactions/MergeOpFrame.h"
 #include "database/Database.h"
+#include "ledger/LedgerHeaderFrame.h"
 #include "ledger/TrustFrame.h"
 #include "main/Application.h"
 #include "medida/meter.h"
@@ -94,11 +95,12 @@ MergeOpFrame::doApply(Application& app, LedgerDelta& delta,
 
     if (ledgerManager.getCurrentLedgerVersion() >= 10)
     {
-        SequenceNumber maxSeqNum =
-            ledgerManager.getCurrentLedgerHeader().ledgerSeq;
-        maxSeqNum = (maxSeqNum << 32) - 1;
+        SequenceNumber maxSeq = LedgerHeaderFrame::getStartingSequenceNumber(
+            ledgerManager.getCurrentLedgerHeader().ledgerSeq);
 
-        if (mSourceAccount->getSeqNum() > maxSeqNum)
+        // don't allow the account to be merged if recreating it would cause it
+        // to jump backwards
+        if (mSourceAccount->getSeqNum() >= maxSeq)
         {
             app.getMetrics()
                 .NewMeter({"op-merge", "failure", "too-far"}, "operation")

src/transactions/MergeTests.cpp

@@ -623,22 +623,32 @@ TEST_CASE("merge", "[tx][merge]")
     SECTION("merge too far")
     {
         for_versions_from(10, *app, [&]() {
-            SequenceNumber maxSeqNum = app->getLedgerManager().getLedgerNum();
-            maxSeqNum = (maxSeqNum << 32) - 1;
-            maxSeqNum--; // a1.merge will increment at the transaction level
+            SequenceNumber curStartSeqNum =
+                LedgerHeaderFrame::getStartingSequenceNumber(
+                    app->getLedgerManager().getLedgerNum());
+            auto maxSeqNum = curStartSeqNum - 1;
+
+            auto txFrame = root.tx({a1.op(accountMerge(b1))});
+            txFrame->addSignature(a1.getSecretKey());
+
             SECTION("at max = success")
             {
                 a1.bumpSequence(maxSeqNum);
                 REQUIRE(a1.loadSequenceNumber() == maxSeqNum);
-                a1.merge(b1);
+                REQUIRE(applyCheck(txFrame, *app));
             }
             SECTION("passed max = failure")
             {
                 maxSeqNum++;
                 a1.bumpSequence(maxSeqNum);
                 REQUIRE(a1.loadSequenceNumber() == maxSeqNum);
-                REQUIRE_THROWS_AS(a1.merge(b1),
-                                  ex_ACCOUNT_MERGE_SEQNUM_TOO_FAR);
+
+                REQUIRE(!applyCheck(txFrame, *app));
+                REQUIRE(txFrame->getResult()
+                            .result.results()[0]
+                            .tr()
+                            .accountMergeResult()
+                            .code() == ACCOUNT_MERGE_SEQNUM_TOO_FAR);
             }
         });
     }

src/transactions/TransactionFrame.cpp

@@ -310,7 +310,7 @@ TransactionFrame::commonValid(SignatureChecker& signatureChecker,
         {
             current = mSigningAccount->getSeqNum();
         }
-        if (current == UINT64_MAX || current + 1 != mEnvelope.tx.seqNum)
+        if (current == INT64_MAX || current + 1 != mEnvelope.tx.seqNum)
         {
             app.getMetrics()
                 .NewMeter({"transaction", "invalid", "bad-seq"}, "transaction")

src/util/types.h

@@ -80,24 +80,4 @@ bool iequals(std::string const& a, std::string const& b);
 bool operator>=(Price const& a, Price const& b);
 bool operator>(Price const& a, Price const& b);
 bool operator==(Price const& a, Price const& b);
-
-template <typename T, typename UT = std::make_unsigned<T>::type,
-          class = typename std::enable_if<std::is_signed<T>::value>::type>
-T
-safeConvertToSigned(UT x)
-{
-    T res;
-    UT m = static_cast<UT>(std::numeric_limits<T>::max());
-    if (x <= m)
-    {
-        res = x;
-    }
-    else
-    {
-        UT d = (x - m - 1);
-        assert(d <= static_cast<UT>(std::numeric_limits<T>::max()));
-        res = std::numeric_limits<T>::min() + static_cast<T>(d);
-    }
-    return res;
-}
 }

src/xdr/Stellar-ledger-entries.x

@@ -11,7 +11,7 @@ typedef PublicKey AccountID;
 typedef opaque Thresholds[4];
 typedef string string32<32>;
 typedef string string64<64>;
-typedef uint64 SequenceNumber;
+typedef int64 SequenceNumber;
 typedef opaque DataValue<64>; 
 
 enum AssetType

src/xdr/Stellar-transaction.x

@@ -671,9 +671,9 @@ default:
 enum BumpSequenceResultCode
 {
     // codes considered as "success" for the operation
-    BUMP_SEQUENCE_SUCCESS = 0
+    BUMP_SEQUENCE_SUCCESS = 0,
     // codes considered as "failure" for the operation
-    // (this operation never fails)
+    BUMP_SEQUENCE_BAD_SEQ = -1 // `bumpTo` is not within bounds
 };
 
 union BumpSequenceResult switch (BumpSequenceResultCode code)