From d82461ea70cae48197079fb3bcc8c6210573786b Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 11 Jul 2025 18:42:39 -0400 Subject: [PATCH] Make preflight1 and preflight2 private static Transactor functions - They should never be called by derived classes. --- src/xrpld/app/tx/detail/Batch.cpp | 5 - src/xrpld/app/tx/detail/Escrow.cpp | 8 -- src/xrpld/app/tx/detail/Transactor.cpp | 135 +++++++++++++------------ src/xrpld/app/tx/detail/Transactor.h | 27 ++--- 4 files changed, 82 insertions(+), 93 deletions(-) diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 601f755ff8..00595f55a1 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -378,11 +378,6 @@ Batch::doPreflight(PreflightContext const& ctx) requiredSigners.insert(innerAccount); } - // LCOV_EXCL_START - if (auto const ret = detail::preflight2(ctx); !isTesSuccess(ret)) - return ret; - // LCOV_EXCL_STOP - // Validation Batch Signers std::unordered_set batchSigners; if (ctx.tx.isFieldPresent(sfBatchSigners)) diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index b8532a6efe..e2601dbb76 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -650,14 +650,6 @@ EscrowFinish::doPreflight(PreflightContext const& ctx) if (static_cast(cb) != static_cast(fb)) return temMALFORMED; - // Verify the transaction signature. If it doesn't work - // then don't do any more work. - { - auto const ret = detail::preflight2(ctx); - if (!isTesSuccess(ret)) - return ret; - } - if (cb && fb) { auto& router = ctx.app.getHashRouter(); diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index b27400767b..8f0ed78b88 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -112,69 +112,6 @@ preflightCheckSigningKey(STObject const& sigObject, beast::Journal j) return tesSUCCESS; } -/** Performs early sanity checks on the account and fee fields */ -NotTEC -preflight1(PreflightContext const& ctx, std::uint32_t flagMask) -{ - // This is inappropriate in preflight0, because only Change transactions - // skip this function, and those do not allow an sfTicketSequence field. - if (ctx.tx.isFieldPresent(sfTicketSequence) && - !ctx.rules.enabled(featureTicketBatch)) - { - return temMALFORMED; - } - - if (ctx.tx.isFieldPresent(sfDelegate)) - { - if (!ctx.rules.enabled(featurePermissionDelegation)) - return temDISABLED; - - if (ctx.tx[sfDelegate] == ctx.tx[sfAccount]) - return temBAD_SIGNER; - } - - if (auto const ret = preflight0(ctx, flagMask)) - return ret; - - auto const id = ctx.tx.getAccountID(sfAccount); - if (id == beast::zero) - { - JLOG(ctx.j.warn()) << "preflight1: bad account id"; - return temBAD_SRC_ACCOUNT; - } - - // No point in going any further if the transaction fee is malformed. - auto const fee = ctx.tx.getFieldAmount(sfFee); - if (!fee.native() || fee.negative() || !isLegalAmount(fee.xrp())) - { - JLOG(ctx.j.debug()) << "preflight1: invalid fee"; - return temBAD_FEE; - } - - if (auto const ret = preflightCheckSigningKey(ctx.tx, ctx.j)) - return ret; - - // An AccountTxnID field constrains transaction ordering more than the - // Sequence field. Tickets, on the other hand, reduce ordering - // constraints. Because Tickets and AccountTxnID work against one - // another the combination is unsupported and treated as malformed. - // - // We return temINVALID for such transactions. - if (ctx.tx.getSeqProxy().isTicket() && - ctx.tx.isFieldPresent(sfAccountTxnID)) - return temINVALID; - - if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) - return temINVALID_FLAG; - - XRPL_ASSERT( - ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || - !ctx.rules.enabled(featureBatch), - "Inner batch transaction must have a parent batch ID."); - - return tesSUCCESS; -} - std::optional preflightCheckSimulateKeys( ApplyFlags flags, @@ -217,11 +154,77 @@ preflightCheckSimulateKeys( return {}; } +} // namespace detail + +/** Performs early sanity checks on the account and fee fields */ +NotTEC +Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) +{ + // This is inappropriate in preflight0, because only Change transactions + // skip this function, and those do not allow an sfTicketSequence field. + if (ctx.tx.isFieldPresent(sfTicketSequence) && + !ctx.rules.enabled(featureTicketBatch)) + { + return temMALFORMED; + } + + if (ctx.tx.isFieldPresent(sfDelegate)) + { + if (!ctx.rules.enabled(featurePermissionDelegation)) + return temDISABLED; + + if (ctx.tx[sfDelegate] == ctx.tx[sfAccount]) + return temBAD_SIGNER; + } + + if (auto const ret = preflight0(ctx, flagMask)) + return ret; + + auto const id = ctx.tx.getAccountID(sfAccount); + if (id == beast::zero) + { + JLOG(ctx.j.warn()) << "preflight1: bad account id"; + return temBAD_SRC_ACCOUNT; + } + + // No point in going any further if the transaction fee is malformed. + auto const fee = ctx.tx.getFieldAmount(sfFee); + if (!fee.native() || fee.negative() || !isLegalAmount(fee.xrp())) + { + JLOG(ctx.j.debug()) << "preflight1: invalid fee"; + return temBAD_FEE; + } + + if (auto const ret = detail::preflightCheckSigningKey(ctx.tx, ctx.j)) + return ret; + + // An AccountTxnID field constrains transaction ordering more than the + // Sequence field. Tickets, on the other hand, reduce ordering + // constraints. Because Tickets and AccountTxnID work against one + // another the combination is unsupported and treated as malformed. + // + // We return temINVALID for such transactions. + if (ctx.tx.getSeqProxy().isTicket() && + ctx.tx.isFieldPresent(sfAccountTxnID)) + return temINVALID; + + if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) + return temINVALID_FLAG; + + XRPL_ASSERT( + ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || + !ctx.rules.enabled(featureBatch), + "Inner batch transaction must have a parent batch ID."); + + return tesSUCCESS; +} + /** Checks whether the signature appears valid */ NotTEC -preflight2(PreflightContext const& ctx) +Transactor::preflight2(PreflightContext const& ctx) { - if (auto const ret = preflightCheckSimulateKeys(ctx.flags, ctx.tx, ctx.j)) + if (auto const ret = + detail::preflightCheckSimulateKeys(ctx.flags, ctx.tx, ctx.j)) // Skips following checks if the transaction is being simulated, // regardless of success or failure return *ret; @@ -236,8 +239,6 @@ preflight2(PreflightContext const& ctx) return tesSUCCESS; } -} // namespace detail - //------------------------------------------------------------------------------ Transactor::Transactor(ApplyContext& ctx) diff --git a/src/xrpld/app/tx/detail/Transactor.h b/src/xrpld/app/tx/detail/Transactor.h index e5e932f566..b2ed320c1b 100644 --- a/src/xrpld/app/tx/detail/Transactor.h +++ b/src/xrpld/app/tx/detail/Transactor.h @@ -319,6 +319,18 @@ private: STObject const& sigObject); void trapTransaction(uint256) const; + + // Helper functions for preflight checks. Do not use directly. + /** Performs early sanity checks on the account and fee fields. + + (And passes flagMask to preflight0) + */ + static NotTEC + preflight1(PreflightContext const& ctx, std::uint32_t flagMask); + + /** Checks whether the signature appears valid */ + static NotTEC + preflight2(PreflightContext const& ctx); }; inline bool @@ -340,13 +352,6 @@ namespace detail { NotTEC preflightCheckSigningKey(STObject const& sigObject, beast::Journal j); -/** Performs early sanity checks on the account and fee fields. - - (And passes flagMask to preflight0) -*/ -NotTEC -preflight1(PreflightContext const& ctx, std::uint32_t flagMask); - /** Checks the special signing key state needed for simulation * * Normally called from preflight2 with ctx.tx. @@ -356,10 +361,6 @@ preflightCheckSimulateKeys( ApplyFlags flags, STObject const& sigObject, beast::Journal j); - -/** Checks whether the signature appears valid */ -NotTEC -preflight2(PreflightContext const& ctx); } // namespace detail // Defined in Change.cpp @@ -374,13 +375,13 @@ Transactor::preflight(PreflightContext const& ctx) if (!T::isEnabled(ctx)) return temDISABLED; - if (auto const ret = ripple::detail::preflight1(ctx, T::getFlagsMask(ctx))) + if (auto const ret = preflight1(ctx, T::getFlagsMask(ctx))) return ret; if (auto const ret = T::doPreflight(ctx)) return ret; - return ripple::detail::preflight2(ctx); + return preflight2(ctx); } template