Merge branch 'develop' into Bronek/fortify_pseudo_account

include/xrpl/protocol/AMMCore.h

@@ -48,14 +48,6 @@ class STObject;
 class STAmount;
 class Rules;
 
-/** Calculate AMM account ID.
- */
-AccountID
-ammAccountID(
-    std::uint16_t prefix,
-    uint256 const& parentHash,
-    uint256 const& ammID);
-
 /** Calculate Liquidity Provider Token (LPT) Currency.
  */
 Currency

include/xrpl/protocol/TER.h

@@ -225,6 +225,8 @@ enum TERcodes : TERUnderlyingType {
     terQUEUED,       // Transaction is being held in TxQ until fee drops
     terPRE_TICKET,   // Ticket is not yet in ledger but might be on its way
     terNO_AMM,       // AMM doesn't exist for the asset pair
+    terADDRESS_COLLISION,  // Failed to allocate AccountID when trying to
+                           // create a pseudo-account
 };
 
 //------------------------------------------------------------------------------

include/xrpl/protocol/detail/features.macro

@@ -33,6 +33,7 @@
 // in include/xrpl/protocol/Feature.h.
 
 // Check flags in Credential transactions
+XRPL_FIX    (PseudoAccount,              Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (InvalidTxFlags,             Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (FrozenLPTokenTransfer,      Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(DeepFreeze,                 Supported::yes, VoteBehavior::DefaultNo)

src/libxrpl/protocol/AMMCore.cpp

@@ -39,18 +39,6 @@
 
 namespace ripple {
 
-AccountID
-ammAccountID(
-    std::uint16_t prefix,
-    uint256 const& parentHash,
-    uint256 const& ammID)
-{
-    ripesha_hasher rsh;
-    auto const hash = sha512Half(prefix, parentHash, ammID);
-    rsh(hash.data(), hash.size());
-    return AccountID{static_cast<ripesha_hasher::result_type>(rsh)};
-}
-
 Currency
 ammLPTCurrency(Currency const& cur1, Currency const& cur2)
 {

src/libxrpl/protocol/TER.cpp

@@ -228,6 +228,7 @@ transResults()
         MAKE_ERROR(terQUEUED,                 "Held until escalated fee drops."),
         MAKE_ERROR(terPRE_TICKET,             "Ticket is not yet in ledger."),
         MAKE_ERROR(terNO_AMM,                 "AMM doesn't exist for the asset pair."),
+        MAKE_ERROR(terADDRESS_COLLISION,      "Failed to allocate an unique account address"),
 
         MAKE_ERROR(tesSUCCESS,                "The transaction was applied. Only final in a validated ledger."),
     };

src/test/app/AMM_test.cpp

@@ -59,6 +59,17 @@ private:
                 XRP(10'000), USD(10'000), IOUAmount{10'000'000, 0}));
         });
 
+        // XRP to IOU, without fixPseudoAccount
+        testAMM(
+            [&](AMM& ammAlice, Env&) {
+                BEAST_EXPECT(ammAlice.expectBalances(
+                    XRP(10'000), USD(10'000), IOUAmount{10'000'000, 0}));
+            },
+            {},
+            0,
+            {},
+            {supported_amendments() - fixPseudoAccount});
+
         // IOU to IOU
         testAMM(
             [&](AMM& ammAlice, Env&) {

src/test/ledger/Invariants_test.cpp

@@ -827,6 +827,74 @@ class Invariants_test : public beast::unit_test::suite
             },
             XRPAmount{},
             STTx{ttPAYMENT, [](STObject& tx) {}});
+
+        doInvariantCheck(
+            {{"pseudo-account created by a wrong transaction type"}},
+            [](Account const&, Account const&, ApplyContext& ac) {
+                Account const A3{"A3"};
+                Keylet const acctKeylet = keylet::account(A3);
+                auto const sleNew = std::make_shared<SLE>(acctKeylet);
+                sleNew->setFieldU32(sfSequence, 0);
+                sleNew->setFieldH256(sfAMMID, uint256(1));
+                sleNew->setFieldU32(
+                    sfFlags,
+                    lsfDisableMaster | lsfDefaultRipple | lsfDefaultRipple);
+                ac.view().insert(sleNew);
+                return true;
+            },
+            XRPAmount{},
+            STTx{ttPAYMENT, [](STObject& tx) {}});
+
+        doInvariantCheck(
+            {{"account created with wrong starting sequence number"}},
+            [](Account const&, Account const&, ApplyContext& ac) {
+                Account const A3{"A3"};
+                Keylet const acctKeylet = keylet::account(A3);
+                auto const sleNew = std::make_shared<SLE>(acctKeylet);
+                sleNew->setFieldU32(sfSequence, ac.view().seq());
+                sleNew->setFieldH256(sfAMMID, uint256(1));
+                sleNew->setFieldU32(
+                    sfFlags,
+                    lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
+                ac.view().insert(sleNew);
+                return true;
+            },
+            XRPAmount{},
+            STTx{ttAMM_CREATE, [](STObject& tx) {}});
+
+        doInvariantCheck(
+            {{"pseudo-account created with wrong flags"}},
+            [](Account const&, Account const&, ApplyContext& ac) {
+                Account const A3{"A3"};
+                Keylet const acctKeylet = keylet::account(A3);
+                auto const sleNew = std::make_shared<SLE>(acctKeylet);
+                sleNew->setFieldU32(sfSequence, 0);
+                sleNew->setFieldH256(sfAMMID, uint256(1));
+                sleNew->setFieldU32(
+                    sfFlags, lsfDisableMaster | lsfDefaultRipple);
+                ac.view().insert(sleNew);
+                return true;
+            },
+            XRPAmount{},
+            STTx{ttAMM_CREATE, [](STObject& tx) {}});
+
+        doInvariantCheck(
+            {{"pseudo-account created with wrong flags"}},
+            [](Account const&, Account const&, ApplyContext& ac) {
+                Account const A3{"A3"};
+                Keylet const acctKeylet = keylet::account(A3);
+                auto const sleNew = std::make_shared<SLE>(acctKeylet);
+                sleNew->setFieldU32(sfSequence, 0);
+                sleNew->setFieldH256(sfAMMID, uint256(1));
+                sleNew->setFieldU32(
+                    sfFlags,
+                    lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth |
+                        lsfRequireDestTag);
+                ac.view().insert(sleNew);
+                return true;
+            },
+            XRPAmount{},
+            STTx{ttAMM_CREATE, [](STObject& tx) {}});
     }
 
     void

src/xrpld/app/tx/detail/AMMCreate.cpp

@@ -219,64 +219,36 @@ applyCreate(
     auto const ammKeylet = keylet::amm(amount.issue(), amount2.issue());
 
     // Mitigate same account exists possibility
-    auto const ammAccount = [&]() -> Expected<AccountID, TER> {
-        std::uint16_t constexpr maxAccountAttempts = 256;
-        for (auto p = 0; p < maxAccountAttempts; ++p)
-        {
-            auto const ammAccount =
-                ammAccountID(p, sb.info().parentHash, ammKeylet.key);
-            if (!sb.read(keylet::account(ammAccount)))
-                return ammAccount;
-        }
-        return Unexpected(tecDUPLICATE);
-    }();
-
+    auto const maybeAccount = createPseudoAccount(sb, ammKeylet.key);
     // AMM account already exists (should not happen)
-    if (!ammAccount)
+    if (!maybeAccount)
     {
-        JLOG(j_.error()) << "AMM Instance: AMM already exists.";
-        return {ammAccount.error(), false};
+        JLOG(j_.error()) << "AMM Instance: failed to create pseudo account.";
+        return {maybeAccount.error(), false};
     }
+    auto const accountId = (**maybeAccount)[sfAccount];
 
     // LP Token already exists. (should not happen)
     auto const lptIss = ammLPTIssue(
-        amount.issue().currency, amount2.issue().currency, *ammAccount);
-    if (sb.read(keylet::line(*ammAccount, lptIss)))
+        amount.issue().currency, amount2.issue().currency, accountId);
+    if (sb.read(keylet::line(accountId, lptIss)))
     {
         JLOG(j_.error()) << "AMM Instance: LP Token already exists.";
         return {tecDUPLICATE, false};
     }
 
-    // Create AMM Root Account.
-    auto sleAMMRoot = std::make_shared<SLE>(keylet::account(*ammAccount));
-    sleAMMRoot->setAccountID(sfAccount, *ammAccount);
-    sleAMMRoot->setFieldAmount(sfBalance, STAmount{});
-    std::uint32_t const seqno{
-        ctx_.view().rules().enabled(featureDeletableAccounts)
-            ? ctx_.view().seq()
-            : 1};
-    sleAMMRoot->setFieldU32(sfSequence, seqno);
-    // Ignore reserves requirement, disable the master key, allow default
-    // rippling (AMM LPToken can be used in payments and offer crossing but
-    // not as a token in another AMM), and enable deposit authorization to
-    // prevent payments into AMM.
     // Note, that the trustlines created by AMM have 0 credit limit.
     // This prevents shifting the balance between accounts via AMM,
     // or sending unsolicited LPTokens. This is a desired behavior.
     // A user can only receive LPTokens through affirmative action -
     // either an AMMDeposit, TrustSet, crossing an offer, etc.
-    sleAMMRoot->setFieldU32(
-        sfFlags, lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
-    // Link the root account and AMM object
-    sleAMMRoot->setFieldH256(sfAMMID, ammKeylet.key);
-    sb.insert(sleAMMRoot);
 
     // Calculate initial LPT balance.
     auto const lpTokens = ammLPTokens(amount, amount2, lptIss);
 
     // Create ltAMM
     auto ammSle = std::make_shared<SLE>(ammKeylet);
-    ammSle->setAccountID(sfAccount, *ammAccount);
+    ammSle->setAccountID(sfAccount, accountId);
     ammSle->setFieldAmount(sfLPTokenBalance, lpTokens);
     auto const& [issue1, issue2] = std::minmax(amount.issue(), amount2.issue());
     ammSle->setFieldIssue(sfAsset, STIssue{sfAsset, issue1});
@@ -287,9 +259,9 @@ applyCreate(
 
     // Add owner directory to link the root account and AMM object.
     if (auto const page = sb.dirInsert(
-            keylet::ownerDir(*ammAccount),
+            keylet::ownerDir(accountId),
             ammSle->key(),
-            describeOwnerDir(*ammAccount)))
+            describeOwnerDir(accountId)))
     {
         ammSle->setFieldU64(sfOwnerNode, *page);
     }
@@ -301,7 +273,7 @@ applyCreate(
     sb.insert(ammSle);
 
     // Send LPT to LP.
-    auto res = accountSend(sb, *ammAccount, account_, lpTokens, ctx_.journal);
+    auto res = accountSend(sb, accountId, account_, lpTokens, ctx_.journal);
     if (res != tesSUCCESS)
     {
         JLOG(j_.debug()) << "AMM Instance: failed to send LPT " << lpTokens;
@@ -312,7 +284,7 @@ applyCreate(
         if (auto const res = accountSend(
                 sb,
                 account_,
-                *ammAccount,
+                accountId,
                 amount,
                 ctx_.journal,
                 WaiveTransferFee::Yes))
@@ -321,7 +293,7 @@ applyCreate(
         if (!isXRP(amount))
         {
             if (SLE::pointer sleRippleState =
-                    sb.peek(keylet::line(*ammAccount, amount.issue()));
+                    sb.peek(keylet::line(accountId, amount.issue()));
                 !sleRippleState)
                 return tecINTERNAL;
             else
@@ -350,7 +322,7 @@ applyCreate(
         return {res, false};
     }
 
-    JLOG(j_.debug()) << "AMM Instance: success " << *ammAccount << " "
+    JLOG(j_.debug()) << "AMM Instance: success " << accountId << " "
                      << ammKeylet.key << " " << lpTokens << " " << amount << " "
                      << amount2;
     auto addOrderBook =

src/xrpld/app/tx/detail/InvariantCheck.cpp

@@ -882,6 +882,8 @@ ValidNewAccountRoot::visitEntry(
     {
         accountsCreated_++;
         accountSeq_ = (*after)[sfSequence];
+        pseudoAccount_ = after->isFieldPresent(sfAMMID);
+        flags_ = after->getFlags();
     }
 }
 
@@ -909,8 +911,22 @@ ValidNewAccountRoot::finalize(
          tx.getTxnType() == ttXCHAIN_ADD_ACCOUNT_CREATE_ATTESTATION) &&
         result == tesSUCCESS)
     {
-        std::uint32_t const startingSeq{
-            view.rules().enabled(featureDeletableAccounts) ? view.seq() : 1};
+        bool const pseudoAccount =
+            (pseudoAccount_ && view.rules().enabled(fixPseudoAccount));
+
+        if (pseudoAccount && tx.getTxnType() != ttAMM_CREATE)
+        {
+            JLOG(j.fatal()) << "Invariant failed: pseudo-account created by a "
+                               "wrong transaction type";
+            return false;
+        }
+
+        std::uint32_t const startingSeq =                     //
+            pseudoAccount                                     //
+            ? 0                                               //
+            : view.rules().enabled(featureDeletableAccounts)  //
+                ? view.seq()                                  //
+                : 1;
 
         if (accountSeq_ != startingSeq)
         {
@@ -918,6 +934,20 @@ ValidNewAccountRoot::finalize(
                                "wrong starting sequence number";
             return false;
         }
+
+        if (pseudoAccount)
+        {
+            std::uint32_t const expected =
+                (lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
+            if (flags_ != expected)
+            {
+                JLOG(j.fatal())
+                    << "Invariant failed: pseudo-account created with "
+                       "wrong flags";
+                return false;
+            }
+        }
+
         return true;
     }
 

src/xrpld/app/tx/detail/InvariantCheck.h

@@ -438,6 +438,8 @@ class ValidNewAccountRoot
 {
     std::uint32_t accountsCreated_ = 0;
     std::uint32_t accountSeq_ = 0;
+    bool pseudoAccount_ = false;
+    std::uint32_t flags_ = 0;
 
 public:
     void

src/xrpld/ledger/View.h

@@ -430,6 +430,11 @@ dirNext(
 [[nodiscard]] std::function<void(SLE::ref)>
 describeOwnerDir(AccountID const& account);
 
+/** Create a pseudo-account
+ */
+[[nodiscard]] Expected<std::shared_ptr<SLE>, TER>
+createPseudoAccount(ApplyView& view, uint256 const& pseudoOwnerKey);
+
 // VFALCO NOTE Both STAmount parameters should just
 //             be "Amount", a unit-less number.
 //

src/xrpld/ledger/detail/View.cpp

@@ -26,6 +26,7 @@
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/Quality.h>
+#include <xrpl/protocol/digest.h>
 #include <xrpl/protocol/st.h>
 
 #include <optional>
@@ -929,6 +930,62 @@ describeOwnerDir(AccountID const& account)
     };
 }
 
+Expected<std::shared_ptr<SLE>, TER>
+createPseudoAccount(ApplyView& view, uint256 const& pseudoOwnerKey)
+{
+    auto const accountId = [&]() -> AccountID {
+        AccountID ret = beast::zero;
+        // This number must not be changed without an amendment
+        constexpr int maxAccountAttempts = 256;
+        for (auto i = 0; i < maxAccountAttempts; ++i)
+        {
+            ripesha_hasher rsh;
+            auto const hash =
+                sha512Half(i, view.info().parentHash, pseudoOwnerKey);
+            rsh(hash.data(), hash.size());
+            ret = static_cast<ripesha_hasher::result_type>(rsh);
+            if (!view.read(keylet::account(ret)))
+                return ret;
+        }
+        return ret;
+    }();
+
+    if (accountId == beast::zero)
+    {
+        TER const code = view.rules().enabled(fixPseudoAccount) && view.open()
+            ? TER(terADDRESS_COLLISION)
+            : TER(tecDUPLICATE);
+        return Unexpected(code);
+    }
+
+    // Create pseudo-account.
+    auto account = std::make_shared<SLE>(keylet::account(accountId));
+    account->setAccountID(sfAccount, accountId);
+    account->setFieldAmount(sfBalance, STAmount{});
+
+    // Pseudo-accounts can't submit transactions, so set the sequence number
+    // to 0 to make them easier to spot and verify, and add an extra level
+    // of protection.
+    std::uint32_t const seqno =                           //
+        view.rules().enabled(fixPseudoAccount)            //
+        ? 0                                               //
+        : view.rules().enabled(featureDeletableAccounts)  //
+            ? view.seq()                                  //
+            : 1;
+    account->setFieldU32(sfSequence, seqno);
+    // Ignore reserves requirement, disable the master key, allow default
+    // rippling, and enable deposit authorization to prevent payments into
+    // pseudo-account.
+    account->setFieldU32(
+        sfFlags, lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
+    // Link the root account and AMM object
+    account->setFieldH256(sfAMMID, pseudoOwnerKey);
+
+    view.insert(account);
+
+    return account;
+}
+
 TER
 trustCreate(
     ApplyView& view,