Fix clang-format/tidy

Co-authored-by: Vito Tumas <[email protected]>

src/libxrpl/protocol/MPTIssue.cpp

@@ -91,7 +91,14 @@ mptIssueFromJson(json::Value const& v)
         Throw<json::Error>("mptIssueFromJson MPTID is invalid");
     }
 
-    return MPTIssue{id};
+    MPTIssue const mptIssue{id};
+    auto const& issuer = mptIssue.getIssuer();
+    if (issuer == noAccount() || issuer == xrpAccount())
+    {
+        Throw<json::Error>("mptIssueFromJson issuer must be a valid account");
+    }
+
+    return mptIssue;
 }
 
 std::ostream&

src/libxrpl/protocol/STIssue.cpp

@@ -4,8 +4,10 @@
 #include <xrpl/json/json_value.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Asset.h>
+#include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/MPTIssue.h>
+#include <xrpl/protocol/Rules.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STBase.h>
 #include <xrpl/protocol/Serializer.h>
@@ -31,39 +33,53 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name}
     if (isXRP(Currency::fromRaw(currencyOrAccount)))
     {
         asset_ = xrpIssue();
+        return;
     }
-    // Check if MPT
+
-    else
+    // The next 160-bit field selects the format:
+    //   noAccount  → MPT V1 (pre-fixCleanup3_2_0)
+    //   xrpAccount → MPT V2 (fixCleanup3_2_0)
+    //   else       → regular IOU; the field is the issuer account.
+    //
+    // Both MPT versions carry the 4-byte sequence next, but differ
+    // in byte order:
+    //
+    //   V1 uses add32()/get32(), which swap host↔BE. The source
+    //   bytes (first 4 of MPTID) are already canonical BE per
+    //   makeMptID(), so on LE hosts the swap emits a byte-reversed
+    //   sequence to the wire. Reads invert the same swap, so
+    //   round-trips on a single arch are consistent.
+    //
+    //   V2 uses addRaw()/getRaw(): the canonical BE bytes from
+    //   makeMptID() reach the wire untouched.
+    AccountID const account = AccountID::fromRaw(sit.get160());
+    if (account == noAccount() || account == xrpAccount())
     {
-        // MPT is serialized as:
+        MPTID mptID{};
-        // - 160 bits MPT issuer account
+        auto constexpr kSEQ_SIZE = sizeof(std::uint32_t);
-        // - 160 bits black hole account
+        if (account == noAccount())
-        // - 32 bits sequence
-        AccountID const account = AccountID::fromRaw(sit.get160());
-        // MPT
-        if (noAccount() == account)
         {
-            MPTID mptID;
             std::uint32_t sequence = sit.get32();
-            static_assert(MPTID::size() == sizeof(sequence) + sizeof(currencyOrAccount));
             memcpy(mptID.data(), &sequence, sizeof(sequence));
-            memcpy(
-                mptID.data() + sizeof(sequence),
-                currencyOrAccount.data(),
-                sizeof(currencyOrAccount));
-            MPTIssue const issue{mptID};
-            asset_ = issue;
         }
         else
         {
-            Issue issue;
+            auto const rawBytes = sit.getRaw(kSEQ_SIZE);
-            issue.currency = currencyOrAccount;
+            memcpy(mptID.data(), rawBytes.data(), rawBytes.size());
-            issue.account = account;
-            if (!isConsistent(issue))
-                Throw<std::runtime_error>("invalid issue: currency and account native mismatch");
-            asset_ = issue;
         }
+        static_assert(MPTID::size() == kSEQ_SIZE + sizeof(currencyOrAccount));
+        memcpy(mptID.data() + kSEQ_SIZE, currencyOrAccount.data(), sizeof(currencyOrAccount));
+        MPTIssue const issue{mptID};
+        asset_ = issue;
+        return;
     }
+
+    Issue issue;
+    issue.currency = currencyOrAccount;
+    issue.account = account;
+    if (!isConsistent(issue))
+        Throw<std::runtime_error>("invalid issue: currency and account native mismatch");
+    asset_ = issue;
 }
 
 SerializedTypeID
@@ -96,11 +112,28 @@ STIssue::add(Serializer& s) const
                 s.addBitString(issue.account);
         },
         [&](MPTIssue const& issue) {
+            auto const fixSerializationEnabled = isFeatureEnabled(fixCleanup3_2_0, false);
             s.addBitString(issue.getIssuer());
-            s.addBitString(noAccount());
+            // The sentinel distinguishes V2 (xrpAccount) from V1 (noAccount)
-            std::uint32_t sequence = 0;
+            // during deserialization; see the constructor for the full format
-            memcpy(&sequence, issue.getMptID().data(), sizeof(sequence));
+            // description.
-            s.add32(sequence);
+            s.addBitString(fixSerializationEnabled ? xrpAccount() : noAccount());
+            if (fixSerializationEnabled)
+            {
+                // memcpy preserves the byte pattern exactly, so for V2, addRaw()
+                // emits the same canonical bytes that were in the MPTID.
+                s.addRaw(issue.getMptID().data(), sizeof(std::uint32_t));
+            }
+            else
+            {
+                // Copy the first 4 bytes of the MPTID (the canonical BE sequence)
+                // into a uint32_t so we can pass either to add32() or addRaw().
+                // For V1, add32() applies a native-to-BE swap on top of what is
+                // already a BE-in-memory value, producing LE wire bytes on LE hosts.
+                std::uint32_t sequence = 0;
+                memcpy(&sequence, issue.getMptID().data(), sizeof(sequence));
+                s.add32(sequence);
+            }
         });
 }
 

src/test/protocol/STIssue_test.cpp

@@ -3,13 +3,24 @@
 #include <test/jtx/amount.h>  // IWYU pragma: keep
 
 #include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/hash/uhash.h>
 #include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/json/json_value.h>
 #include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Issue.h>
+#include <xrpl/protocol/MPTIssue.h>
+#include <xrpl/protocol/Rules.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STIssue.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/protocol/UintTypes.h>
+#include <xrpl/protocol/jss.h>
+
+#include <bit>
+#include <cstdint>
+#include <unordered_set>
 
 namespace xrpl::test {
 
@@ -137,12 +148,203 @@ public:
             "000000000000000000000000000000000000000000000002");
     }
 
+    // Hard-coded wire-format fixture.
+    //
+    // Verifies what STIssue::add() actually puts on the wire for the 4-byte
+    // sequence field of an MPT issue.
+    //
+    // Before fix (V1, no amendment): add32() applies a native-to-BE swap on
+    // top of MPTID bytes that are already canonical BE. On LE hosts the two
+    // swaps cancel and the wire bytes end up in LE order — the opposite of
+    // what a conforming client expects.
+    //
+    // After fix (V2, amendment enabled): addRaw() writes the MPTID bytes
+    // verbatim. The wire bytes match the canonical BE encoding from makeMptID().
+    void
+    testMPTWireFormat()
+    {
+        testcase("MPT serialization - serialized sequence bytes are canonical big-endian");
+        using namespace jtx;
+        Account const alice{"alice"};
+        BEAST_EXPECT(std::endian::native == std::endian::little);
+
+        // Sequence 240 = 0x000000F0.
+        // Canonical BE bytes a client would expect: {0x00, 0x00, 0x00, 0xF0}.
+        // Serialized layout: issuer(20) + marker(20) + sequence(4).
+        MPTID const mptID240 = makeMptID(240, alice.id());
+
+        // Before fix: wire sequence bytes are LE-swapped, not canonical.
+        {
+            STIssue const st(sfAsset, Asset{MPTIssue{mptID240}});
+            Serializer s;
+            st.add(s);
+            Slice const sl = s.slice();
+            BEAST_EXPECT(sl.size() == 44);
+            BEAST_EXPECT(sl[40] == 0xF0);  // ← wrong: LSB of 0xF0000000 on LE
+            BEAST_EXPECT(sl[41] == 0x00);
+            BEAST_EXPECT(sl[42] == 0x00);
+            BEAST_EXPECT(sl[43] == 0x00);
+        }
+
+        // After fix: wire sequence bytes are canonical BE.
+        {
+            std::unordered_set<uint256, beast::Uhash<>> const presets{fixCleanup3_2_0};
+            CurrentTransactionRulesGuard const guard(Rules{presets});
+
+            STIssue const st(sfAsset, Asset{MPTIssue{mptID240}});
+            Serializer s;
+            st.add(s);
+            Slice const sl = s.slice();
+            BEAST_EXPECT(sl.size() == 44);
+            BEAST_EXPECT(sl[40] == 0x00);  // ← correct: MSB of 0x000000F0
+            BEAST_EXPECT(sl[41] == 0x00);
+            BEAST_EXPECT(sl[42] == 0x00);
+            BEAST_EXPECT(sl[43] == 0xF0);
+        }
+
+        // 0xDEADBEEF: non-palindromic value makes the byte-order contrast
+        // unambiguous regardless of host endianness.
+        MPTID const mptIDBEEF = makeMptID(0xDEADBEEF, alice.id());
+
+        // Before fix: {0xEF, 0xBE, 0xAD, 0xDE} — LE-swapped.
+        {
+            STIssue const st(sfAsset, Asset{MPTIssue{mptIDBEEF}});
+            Serializer s;
+            st.add(s);
+            Slice const sl = s.slice();
+            BEAST_EXPECT(sl[40] == 0xEF);
+            BEAST_EXPECT(sl[41] == 0xBE);
+            BEAST_EXPECT(sl[42] == 0xAD);
+            BEAST_EXPECT(sl[43] == 0xDE);
+        }
+
+        // After fix: {0xDE, 0xAD, 0xBE, 0xEF} — canonical BE.
+        {
+            std::unordered_set<uint256, beast::Uhash<>> const presets{fixCleanup3_2_0};
+            CurrentTransactionRulesGuard const guard(Rules{presets});
+
+            STIssue const st(sfAsset, Asset{MPTIssue{mptIDBEEF}});
+            Serializer s;
+            st.add(s);
+            Slice const sl = s.slice();
+            BEAST_EXPECT(sl[40] == 0xDE);
+            BEAST_EXPECT(sl[41] == 0xAD);
+            BEAST_EXPECT(sl[42] == 0xBE);
+            BEAST_EXPECT(sl[43] == 0xEF);
+        }
+    }
+
+    // Cross-path test.
+    //
+    // Verifies that the STIssue codec (add/deserialize) agrees with the JSON
+    // path (mptIssueFromJson) on the meaning of a raw MPTID.
+    //
+    // The sentinels (noAccount for V1, xrpAccount for V2) are internal codec
+    // details. Clients only ever hold the raw 24-byte MPTID returned by RPC;
+    // they never construct sentinel bytes themselves.
+    //
+    // Before fix (V1): the deserializer calls get32(), which byte-swaps the
+    // canonical BE sequence bytes on LE hosts. The reconstructed MPTID does
+    // not match the original — codec output diverges from JSON output.
+    //
+    // After fix (V2): the deserializer reads the sequence bytes raw. The
+    // reconstructed MPTID matches exactly — codec and JSON paths agree.
+    void
+    testMPTCrossPath()
+    {
+        testcase(
+            "MPT serialization - decoded MPTID matches canonical value: broken in V1, fixed in V2");
+        using namespace jtx;
+        Account const alice{"alice"};
+
+        // Use a non-palindromic sequence so byte-swapping produces a visibly
+        // different MPTID. seq=1 (0x00000001) would give 0x01000000 when
+        // swapped; 0xDEADBEEF is unambiguous on any host.
+        for (auto const seq : {240u, 0xDEADBEEFu, 1u})
+        {
+            MPTID const canonical = makeMptID(seq, alice.id());
+
+            // The JSON path parses the hex string directly into an MPTID —
+            // always canonical. This is the reference value that the codec
+            // (add/deserialize round-trip) must agree with.
+            json::Value jv;
+            jv[jss::mpt_issuance_id] = to_string(canonical);
+            MPTIssue const fromJson = mptIssueFromJson(jv);
+            BEAST_EXPECT(fromJson.getMptID() == canonical);
+
+            // Before fix: V1 codec writes [issuer][noAccount][add32(seq)].
+            // Simulate the deserialization path with canonical (BE) sequence
+            // bytes and V1 marker: get32() byte-swaps on LE, so the
+            // reconstructed MPTID ≠ canonical and ≠ what mptIssueFromJson
+            // produced.
+            {
+                Serializer s;
+                s.addBitString(alice.id());
+                s.addBitString(noAccount());
+                s.addRaw(canonical.data(), sizeof(std::uint32_t));
+                SerialIter sit(s.slice());
+                STIssue const parsed(sit, sfAsset);
+                BEAST_EXPECT(parsed != Asset{MPTIssue{canonical}});  // ← bug
+                BEAST_EXPECT(parsed != Asset{fromJson});             // ← JSON/binary divergence
+            }
+
+            // After fix: V2 codec writes [issuer][xrpAccount][addRaw(seq)].
+            // Same canonical (BE) sequence bytes, V2 marker: getRaw()
+            // preserves bytes, so the reconstructed MPTID == canonical and
+            // == what mptIssueFromJson produced.
+            {
+                Serializer s;
+                s.addBitString(alice.id());
+                s.addBitString(xrpAccount());
+                s.addRaw(canonical.data(), sizeof(std::uint32_t));
+                SerialIter sit(s.slice());
+                STIssue const parsed(sit, sfAsset);
+                BEAST_EXPECT(parsed == Asset{MPTIssue{canonical}});  // ← fixed
+                BEAST_EXPECT(parsed == Asset{fromJson});             // ← JSON/binary agree
+            }
+        }
+
+        // V1 round-trip (no amendment): xrpld's own add() and the
+        // deserializer are symmetrically wrong, so they cancel and the MPTID
+        // survives intact — the bug is invisible in internal round-trips.
+        {
+            for (auto const seq : {240u, 0xDEADBEEFu, 1u})
+            {
+                MPTID const expected = makeMptID(seq, alice.id());
+                STIssue const original(sfAsset, Asset{MPTIssue{expected}});
+                Serializer s;
+                original.add(s);
+                SerialIter sit(s.slice());
+                BEAST_EXPECT(STIssue(sit, sfAsset) == Asset{MPTIssue{expected}});
+            }
+        }
+
+        // V2 full round-trip (amendment enabled): add() and the deserializer
+        // both use canonical bytes — round-trip is correct and canonical.
+        {
+            std::unordered_set<uint256, beast::Uhash<>> const presets{fixCleanup3_2_0};
+            CurrentTransactionRulesGuard const guard(Rules{presets});
+
+            for (auto const seq : {240u, 0xDEADBEEFu, 1u})
+            {
+                MPTID const expected = makeMptID(seq, alice.id());
+                STIssue const original(sfAsset, Asset{MPTIssue{expected}});
+                Serializer s;
+                original.add(s);
+                SerialIter sit(s.slice());
+                BEAST_EXPECT(STIssue(sit, sfAsset) == Asset{MPTIssue{expected}});
+            }
+        }
+    }
+
     void
     run() override
     {
         // compliments other unit tests to ensure complete coverage
         testConstructor();
         testCompare();
+        testMPTWireFormat();
+        testMPTCrossPath();
     }
 };
 

src/test/rpc/Submit_test.cpp

@@ -2,15 +2,37 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/JTx.h>
 #include <test/jtx/amount.h>
+#include <test/jtx/mpt.h>
 #include <test/jtx/pay.h>
+#include <test/jtx/utility.h>
+#include <test/jtx/vault.h>
 
+#include <xrpl/basics/Blob.h>
+#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/strHex.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/json/json_value.h>
 #include <xrpl/json/to_string.h>
+#include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/HashPrefix.h>
+#include <xrpl/protocol/MPTIssue.h>
+#include <xrpl/protocol/PublicKey.h>
+#include <xrpl/protocol/Rules.h>
+#include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STIssue.h>
+#include <xrpl/protocol/STTx.h>
+#include <xrpl/protocol/SecretKey.h>
 #include <xrpl/protocol/Seed.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/protocol/TxFlags.h>
+#include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/protocol/jss.h>
 
+#include <algorithm>
+#include <cstdint>
+#include <string>
+
 namespace xrpl::test {
 
 class Submit_test : public beast::unit_test::Suite
@@ -86,10 +108,136 @@ public:
         }
     }
 
+    void
+    testSTIssueV1SignedVaultSubmit()
+    {
+        testcase("V1 STIssue signed Vault tx succeeds submit signature verification");
+        using namespace jtx;
+
+        Env env{*this};
+        Account const issuer{"issuer"};
+        Account const owner{"owner"};
+        env.fund(XRP(100'000), issuer, owner);
+        env.close();
+        BEAST_EXPECT(env.current()->rules().enabled(fixCleanup3_2_0));
+
+        MPTTester mpt{env, issuer, kMPT_INIT_NO_FUND};
+        mpt.create({.flags = tfMPTCanTransfer | tfMPTRequireAuth});
+        mpt.authorize({.account = owner});
+        mpt.authorize({.account = issuer, .holder = owner});
+
+        PrettyAsset const asset = mpt.issuanceID();
+        env(pay(issuer, owner, asset(100)));
+        env.close();
+
+        Vault const vault{env};
+        auto [jv, keylet] = vault.create({.owner = owner, .asset = asset});
+        (void)keylet;
+        jv[jss::Fee] = to_string(env.current()->fees().base);
+        jv[jss::Sequence] = env.seq(owner);
+        jv[jss::SigningPubKey] = strHex(owner.pk().slice());
+
+        STTx tx{parse(jv)};
+        tx.sign(owner.pk(), owner.sk());
+        BEAST_EXPECT(tx.checkSign(env.current()->rules()));
+
+        auto const jrr = env.rpc("submit", strHex(tx.getSerializer().slice()))[jss::result];
+        BEAST_EXPECT(jrr[jss::engine_result] == "tesSUCCESS");
+    }
+
+    void
+    testSTIssueV2SignedVaultSubmit()
+    {
+        testcase("V2 STIssue signed Vault tx fails submit signature verification");
+        using namespace jtx;
+
+        Env env{*this};
+        Account const issuer{"issuer"};
+        Account const owner{"owner"};
+        env.fund(XRP(100'000), issuer, owner);
+        env.close();
+        BEAST_EXPECT(env.current()->rules().enabled(fixCleanup3_2_0));
+
+        MPTTester mpt{env, issuer, kMPT_INIT_NO_FUND};
+        mpt.create({.flags = tfMPTCanTransfer | tfMPTRequireAuth});
+        mpt.authorize({.account = owner});
+        mpt.authorize({.account = issuer, .holder = owner});
+
+        PrettyAsset const asset = mpt.issuanceID();
+        env(pay(issuer, owner, asset(100)));
+        env.close();
+
+        Vault const vault{env};
+        auto [jv, keylet] = vault.create({.owner = owner, .asset = asset});
+        (void)keylet;
+        jv[jss::Fee] = to_string(env.current()->fees().base);
+        jv[jss::Sequence] = env.seq(owner);
+        jv[jss::SigningPubKey] = strHex(owner.pk().slice());
+
+        // Model an external client that already writes the V2 STIssue wire
+        // format. The test must not rely on CurrentTransactionRulesGuard to
+        // produce the bytes that are signed.
+        MPTIssue const mptIssue = asset.raw().get<MPTIssue>();
+        auto const serializeV1Asset = [&mptIssue]() {
+            Serializer s;
+            STIssue const st{sfAsset, Asset{mptIssue}};
+            st.addFieldID(s);
+            st.add(s);
+            return s.getData();
+        };
+        auto const serializeV2Asset = [&mptIssue]() {
+            Serializer s;
+            STIssue const st{sfAsset, Asset{mptIssue}};
+            st.addFieldID(s);
+            s.addBitString(mptIssue.getIssuer());
+            s.addBitString(xrpAccount());
+            s.addRaw(mptIssue.getMptID().data(), sizeof(std::uint32_t));
+            return s.getData();
+        };
+        auto const replaceAsset = [this](Blob data, Blob const& from, Blob const& to) {
+            BEAST_EXPECT(from.size() == to.size());
+            auto found = std::ranges::search(data, from);
+            BEAST_EXPECT(!found.empty());
+            if (!found.empty())
+                std::ranges::copy(to, found.begin());
+            return data;
+        };
+
+        Blob const v1Asset = serializeV1Asset();
+        Blob const v2Asset = serializeV2Asset();
+        BEAST_EXPECT(v1Asset != v2Asset);
+
+        STTx tx{parse(jv)};
+        Serializer signingData;
+        signingData.add32(HashPrefix::TxSign);
+        tx.addWithoutSigningFields(signingData);
+        Blob const clientSigningData = replaceAsset(signingData.getData(), v1Asset, v2Asset);
+        auto const sig = sign(owner.pk(), owner.sk(), makeSlice(clientSigningData));
+        Slice const sigSlice{sig.data(), sig.size()};
+        BEAST_EXPECT(verify(owner.pk(), makeSlice(clientSigningData), sigSlice));
+        tx.setFieldVL(sfTxnSignature, sigSlice);
+
+        Serializer txSerializer;
+        tx.add(txSerializer);
+        Blob const clientTx = replaceAsset(txSerializer.getData(), v1Asset, v2Asset);
+
+        SerialIter sit{makeSlice(clientTx)};
+        STTx const submittedTx{sit};
+        BEAST_EXPECT(submittedTx[sfAsset] == asset.raw());
+        BEAST_EXPECT(!submittedTx.checkSign(env.current()->rules()));
+
+        auto const jrr = env.rpc("submit", strHex(clientTx))[jss::result];
+        BEAST_EXPECT(jrr[jss::error] == "invalidTransaction");
+        BEAST_EXPECT(
+            jrr[jss::error_exception].asString().find("Invalid signature") != std::string::npos);
+    }
+
     void
     run() override
     {
         testFailHardValidation();
+        testSTIssueV1SignedVaultSubmit();
+        testSTIssueV2SignedVaultSubmit();
     }
 };