From 5b981dc7fb8d1a409a52e57b755f696ec2637e54 Mon Sep 17 00:00:00 2001 From: Kassaking7 <96991820+Kassaking7@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:49:21 -0400 Subject: [PATCH] TransactionProposalCreate edge cases and tests (#7958) --- .../proposal/TransactionProposalCreate.cpp | 80 ++++---- .../app/TransactionProposalCreate_test.cpp | 183 +++++++++++++----- 2 files changed, 178 insertions(+), 85 deletions(-) diff --git a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp index 1b6c5b3986..3a397cac00 100644 --- a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp +++ b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp @@ -41,10 +41,39 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx) STObject const proposedTx = ctx.tx.getFieldObject(sfProposedTransaction); - if (!proposedTx.isFieldPresent(sfTransactionType) || !proposedTx.isFieldPresent(sfAccount)) + // The proposed transaction must pass its own static checks under the + // current rules, so no statically-dead proposal can be stored. This also + // guarantees every field common to all transactions (TransactionType, + // Account, Fee, Sequence, ...) is present, since applyTemplate throws + // otherwise; the checks below can therefore read those fields directly + // without re-checking presence. TapDryRun accepts the unsigned canonical + // form without a signature check; TapProposal additionally skips + // signature-presence checks (e.g. Batch signer matching), which are + // deferred to submission time (On-Chain Cosigner spec §5.3.1.2). A + // proposedTx that fails here is rejected with its own type's preflight + // code (or temMALFORMED if it isn't even a valid instance of that type), + // ahead of the Cosigner-specific structural checks below. + try { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " - "lacks TransactionType or Account."; + STTx const stx{STObject{proposedTx}}; + auto const inner = + xrpl::preflight(ctx.registry, ctx.rules, stx, TapDryRun | TapProposal, ctx.j); + if (!isTesSuccess(inner.ter)) + { + JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " + "failed preflight: " + << transHuman(inner.ter); + // Surface the proposed transaction type's own preflight code + // rather than collapsing it to a generic error (On-Chain Cosigner + // spec §5.3.1). + return inner.ter; + } + } + catch (std::exception const& e) + { + JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn is " + "malformed: " + << e.what(); return temMALFORMED; } @@ -76,15 +105,6 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx) return temBAD_SIGNER; } - // The proposed transaction's fee is charged to the target account when - // the completed transaction is submitted, so it must be fixed now. - if (!proposedTx.isFieldPresent(sfFee) || !proposedTx.isFieldPresent(sfSequence)) - { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " - "lacks Fee or Sequence."; - return temMALFORMED; - } - // The proposed transaction must be ticket-based: it must carry a // TicketSequence and must not use a live Sequence. Sequence is a required // common field, so "no Sequence" is expressed as a Sequence of 0 rather @@ -95,33 +115,17 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx) if (!proposedTx.isFieldPresent(sfTicketSequence) || proposedTx.getFieldU32(sfSequence) != 0) return temSEQ_AND_TICKET; - // The proposed transaction must pass its own static checks under the - // current rules, so no statically-dead proposal can be stored. TapDryRun - // accepts the unsigned canonical form without a signature check; - // TapProposal additionally skips signature-presence checks (e.g. Batch - // signer matching), which are deferred to submission time (On-Chain - // Cosigner spec §5.3.1.2). - try + // If this transaction itself is paying with a Ticket, and the proposed + // transaction is targeting that same account and Ticket, then applying + // this transaction consumes the very Ticket the proposal depends on + // before the proposal is even stored: the proposal would be dead on + // arrival, and its only recourse would be TransactionProposalCancel. + if (ctx.tx.getSeqProxy().isTicket() && + proposedTx.getAccountID(sfAccount) == ctx.tx.getAccountID(sfAccount) && + proposedTx.getFieldU32(sfTicketSequence) == ctx.tx.getSeqProxy().value()) { - STTx const stx{STObject{proposedTx}}; - auto const inner = - xrpl::preflight(ctx.registry, ctx.rules, stx, TapDryRun | TapProposal, ctx.j); - if (!isTesSuccess(inner.ter)) - { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " - "failed preflight: " - << transHuman(inner.ter); - // Surface the proposed transaction type's own preflight code - // rather than collapsing it to a generic error (On-Chain Cosigner - // spec §5.3.1). - return inner.ter; - } - } - catch (std::exception const& e) - { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn is " - "malformed: " - << e.what(); + JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " + "reuses the Ticket this transaction itself consumes."; return temMALFORMED; } diff --git a/src/test/app/TransactionProposalCreate_test.cpp b/src/test/app/TransactionProposalCreate_test.cpp index 0dc2d04ec8..0f8fa60fd6 100644 --- a/src/test/app/TransactionProposalCreate_test.cpp +++ b/src/test/app/TransactionProposalCreate_test.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -122,29 +123,49 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, target) == 1); }; - // A pseudo-transaction is never submittable by an account. + // An unrecognized TransactionType cannot even be constructed as an + // STTx (there is no format to validate it against), so it is + // rejected the same as any other malformed payload. + { + json::Value tx = payload(); + tx[jss::TransactionType] = 65535; + reject(tx, temMALFORMED); + } + + // A pseudo-transaction is never submittable by an account. This + // payload also carries Payment-shaped fields (Amount, Destination) + // that aren't part of EnableAmendment's own template, so it fails + // STTx construction before ever reaching our own isPseudoTx check. { json::Value tx = payload(); tx[jss::TransactionType] = jss::EnableAmendment; - reject(tx, temINVALID); + reject(tx, temMALFORMED); } // An inner batch transaction bypasses the ordinary signature checks. + // The proposed transaction's own preflight rejects a standalone + // tfInnerBatchTxn (no enclosing Batch, no parentBatchId) with its own + // more specific code before reaching our own tfInnerBatchTxn check. { json::Value tx = payload(); tx[jss::Flags] = tfInnerBatchTxn; - reject(tx, temINVALID); + reject(tx, temINVALID_INNER_BATCH); } - // Proposals do not nest. + // Proposals do not nest. This payload also isn't a valid instance of + // TransactionProposalCreate's own template (it lacks Expiration and + // ProposedTransaction), so it fails STTx construction before ever + // reaching our own isProposalTx check. { json::Value tx = payload(); tx[jss::TransactionType] = "TransactionProposalCreate"; - reject(tx, temINVALID); + reject(tx, temMALFORMED); } // Nor may a proposed Batch smuggle a nested proposal in as one of its - // own inner transactions. + // own inner transactions. A second, ordinary inner transaction rides + // along only to satisfy Batch's own minimum of two inner + // transactions; it isn't itself the point of this case. { json::Value const nestedProposal = proposal::create(target, payload(), proposal::expiration(env, 100s)); @@ -153,7 +174,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite target, targetTicketSeq, tfAllOrNothing, - {proposal::innerTx(nestedProposal, env.seq(target))}); + {proposal::innerTx(nestedProposal, env.seq(target)), + proposal::innerTx(pay(target, bob, XRP(1)), env.seq(target) + 1)}); reject(tx, temINVALID); } @@ -170,6 +192,32 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite reject(tx, temSEQ_AND_TICKET); } + // If this TransactionProposalCreate itself pays with a Ticket, and the + // proposed transaction targets that same account and Ticket, applying + // this transaction consumes the Ticket the proposal depends on before + // the proposal is even stored: it would be dead on arrival. target is + // proposing for itself here, so this is the Ticket it is about to pay + // with and the Ticket its own proposed payload names. + { + std::uint32_t const selfTicketSeq = proposal::createTicket(env, target); + + json::Value const tx = + proposal::unsignedPayload(env, pay(target, bob, XRP(1)), selfTicketSeq); + env(proposal::create(target, tx, expiration), + ticket::Use(selfTicketSeq), + Ter(temMALFORMED)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, selfTicketSeq)); + BEAST_EXPECT(env.le(keylet::ticket(target.id(), selfTicketSeq))); + BEAST_EXPECT(ownerCount(env, target) == 2); + + // Consume the leftover Ticket so target's OwnerCount is back to + // just its original Ticket for the remaining cases below. + env(noop(target), ticket::Use(selfTicketSeq)); + env.close(); + BEAST_EXPECT(ownerCount(env, target) == 1); + } + // A payload that fails its own transaction type's preflight surfaces // that type's own code, not a generic error (On-Chain Cosigner spec §5.3.1.2). { @@ -195,6 +243,17 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // including combinations that could never verify: an empty container, a // key with no signature, a signature with no key, and a signature next to // the empty SigningPubKey the canonical form requires. + // + // The rejection code is not uniform, though. A fill that leaves actual + // signature bytes behind (a non-empty TxnSignature, or one inside a + // Signers entry) is, for some containers, intercepted before ever + // reaching our own hasSignatureField check: the payload's own top-level + // fields are checked by Transactor::preflight2's dry-run simulate-key + // logic, and LoanSet forwards its CounterpartySignature through that same + // logic, both yielding temINVALID instead. SponsorSignature and + // BatchSigners are not inspected that way — only their presence is + // checked elsewhere — so they always reach our own check regardless of + // what they hold. void testRejectedSignatureFields(FeatureBitset features) { @@ -250,8 +309,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite proposal::innerTx(pay(target, bob, drops(++paid)), env.seq(target) + 1)}); }; - auto reject = [&](json::Value const& proposedTx) { - env(proposal::create(target, proposedTx, expiration), Ter(temBAD_SIGNER)); + auto reject = [&](json::Value const& proposedTx, TER expected) { + env(proposal::create(target, proposedTx, expiration), Ter(expected)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); BEAST_EXPECT(ownerCount(env, target) == 1); @@ -259,38 +318,55 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // Every way of filling in a signature. The payload's own signature // fields and a co-signature object hold the same three members, so the - // same fills apply to both. - std::vector> const fills{ - [&](json::Value& o) { o[jss::SigningPubKey] = key; }, - [&](json::Value& o) { o[sfTxnSignature.getJsonName()] = sig; }, - [&](json::Value& o) { - o[jss::SigningPubKey] = ""; - o[sfTxnSignature.getJsonName()] = sig; - }, + // same fills apply to both. `signs` marks a fill that leaves actual + // signature bytes behind, which some containers' own dry-run + // simulate-key check reacts to (see the class comment above). + struct Fill + { + std::function apply; + bool signs; + }; + + std::vector const fills{ + {.apply = [&](json::Value& o) { o[jss::SigningPubKey] = key; }, .signs = false}, + {.apply = [&](json::Value& o) { o[sfTxnSignature.getJsonName()] = sig; }, + .signs = true}, + {.apply = + [&](json::Value& o) { + o[jss::SigningPubKey] = ""; + o[sfTxnSignature.getJsonName()] = sig; + }, + .signs = true}, // Signed the ordinary way, which is the likeliest way one of these // arrives here. - [&](json::Value& o) { - o[jss::SigningPubKey] = key; - o[sfTxnSignature.getJsonName()] = sig; - }, + {.apply = + [&](json::Value& o) { + o[jss::SigningPubKey] = key; + o[sfTxnSignature.getJsonName()] = sig; + }, + .signs = true}, // Multi-signed: the signer's own key is empty and the signatures // sit in a nested Signers array. Each entry needs all three of // Account, SigningPubKey and TxnSignature to parse at all, so only // their values can vary. - [&](json::Value& o) { - o[jss::SigningPubKey] = ""; - auto& signer = o[sfSigners.getJsonName()][0u][sfSigner.getJsonName()]; - signer[jss::Account] = bob.human(); - signer[jss::SigningPubKey] = key; - signer[sfTxnSignature.getJsonName()] = sig; - }, - [&](json::Value& o) { - o[jss::SigningPubKey] = ""; - auto& signer = o[sfSigners.getJsonName()][0u][sfSigner.getJsonName()]; - signer[jss::Account] = bob.human(); - signer[jss::SigningPubKey] = ""; - signer[sfTxnSignature.getJsonName()] = sig; - }, + {.apply = + [&](json::Value& o) { + o[jss::SigningPubKey] = ""; + auto& signer = o[sfSigners.getJsonName()][0u][sfSigner.getJsonName()]; + signer[jss::Account] = bob.human(); + signer[jss::SigningPubKey] = key; + signer[sfTxnSignature.getJsonName()] = sig; + }, + .signs = true}, + {.apply = + [&](json::Value& o) { + o[jss::SigningPubKey] = ""; + auto& signer = o[sfSigners.getJsonName()][0u][sfSigner.getJsonName()]; + signer[jss::Account] = bob.human(); + signer[jss::SigningPubKey] = ""; + signer[sfTxnSignature.getJsonName()] = sig; + }, + .signs = true}, }; // Every place a signature could sit, on a payload of a type that @@ -298,27 +374,35 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // BatchSigners to a Batch, while a Sponsor's signature and the // payload's own signature fields sit on any transaction. A signature // is no more storable for being a field its transaction type expects - // (On-Chain Cosigner spec §6.1, §6.6.3). + // (On-Chain Cosigner spec §6.1, §6.6.3). `checksSignatureContent` + // marks a place whose own preflight forwards the container through a + // dry-run simulate-key check, the same as the payload's own top-level + // fields. struct Place { std::function payload; std::function at; + bool checksSignatureContent; }; std::vector const places{ - {.payload = payment, .at = [](json::Value& tx) -> json::Value& { return tx; }}, + {.payload = payment, + .at = [](json::Value& tx) -> json::Value& { return tx; }, + .checksSignatureContent = true}, {.payload = loanSet, .at = [](json::Value& tx) -> json::Value& { auto& o = tx[sfCounterpartySignature.getJsonName()]; o = json::Value{json::ValueType::Object}; return o; - }}, + }, + .checksSignatureContent = true}, {.payload = sponsoredPayment, .at = [](json::Value& tx) -> json::Value& { auto& o = tx[sfSponsorSignature.getJsonName()]; o = json::Value{json::ValueType::Object}; return o; - }}, + }, + .checksSignatureContent = false}, // A BatchSigners entry names the account it speaks for; the other // two co-signatures are fixed by the transaction they belong to and // do not. @@ -327,7 +411,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite auto& o = tx[sfBatchSigners.getJsonName()][0u][sfBatchSigner.getJsonName()]; o[jss::Account] = bob.human(); return o; - }}, + }, + .checksSignatureContent = false}, }; for (auto const& place : places) @@ -335,36 +420,40 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite for (auto const& fill : fills) { json::Value tx = place.payload(); - fill(place.at(tx)); - reject(tx); + fill.apply(place.at(tx)); + reject(tx, place.checksSignatureContent && fill.signs ? temINVALID : temBAD_SIGNER); } } // Every place but the payload itself: a co-signature object is // disqualifying by its presence alone, so each is rejected left empty // too. The payload's own fields have no such case — left alone they are - // the canonical form. + // the canonical form. An empty container never trips a simulate-key + // check, so this is temBAD_SIGNER regardless of checksSignatureContent. for (std::size_t i = 1; i < places.size(); ++i) { json::Value tx = places[i].payload(); places[i].at(tx); - reject(tx); + reject(tx, temBAD_SIGNER); } // Nor does the payload have a counterpart for an absent SigningPubKey: // in a co-signature object an absent member is just an unfilled one, // but at the top level it is not the same as an empty one, with or - // without a signature beside it. + // without a signature beside it. SigningPubKey is a required common + // field, so its absence means proposedTx isn't a valid instance of its + // own type; that's caught while constructing it as an STTx, before + // reaching our own hasEmptySigningPubKey check. { json::Value tx = payment(); tx.removeMember(jss::SigningPubKey); - reject(tx); + reject(tx, temMALFORMED); } { json::Value tx = payment(); tx.removeMember(jss::SigningPubKey); tx[sfTxnSignature.getJsonName()] = sig; - reject(tx); + reject(tx, temMALFORMED); } }