From bb491431bdc3021c78cd5aa88b07339ea4ec8fa6 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 3 Sep 2025 20:51:40 -0400 Subject: [PATCH] Review feedback from @dangell7 and @mvadari - Rewrite isTesSuccess to use TERSubset::operator bool - Add Transactor::preflightSigValidated for expensive operations that should be done after signature validation. These things would have been done after preflight2 before this refactor. - Split Batch and EscrowFinish preflight to use preflightSigValidated, too. --- include/xrpl/protocol/TER.h | 3 ++- src/xrpld/app/tx/detail/Batch.cpp | 18 +++++++++++++++++- src/xrpld/app/tx/detail/Batch.h | 3 +++ src/xrpld/app/tx/detail/Escrow.cpp | 9 +++++++++ src/xrpld/app/tx/detail/Escrow.h | 3 +++ src/xrpld/app/tx/detail/Transactor.cpp | 6 ++++++ src/xrpld/app/tx/detail/Transactor.h | 13 ++++++++++++- 7 files changed, 52 insertions(+), 3 deletions(-) diff --git a/include/xrpl/protocol/TER.h b/include/xrpl/protocol/TER.h index 9ace6b80f8..0a3a3b999e 100644 --- a/include/xrpl/protocol/TER.h +++ b/include/xrpl/protocol/TER.h @@ -673,7 +673,8 @@ isTerRetry(TER x) noexcept inline bool isTesSuccess(TER x) noexcept { - return (x == tesSUCCESS); + // Makes use of TERSubset::operator bool() + return !(x); } inline bool diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 5f05d9919e..5b0f5b4d8f 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -241,7 +241,6 @@ Batch::preflight(PreflightContext const& ctx) } // Validation Inner Batch Txns - std::unordered_set requiredSigners; std::unordered_set uniqueHashes; std::unordered_map> accountSeqTicket; @@ -371,6 +370,23 @@ Batch::preflight(PreflightContext const& ctx) } } } + } + + return tesSUCCESS; +} + +NotTEC +Batch::preflightSigValidated(PreflightContext const& ctx) +{ + auto const parentBatchId = ctx.tx.getTransactionID(); + auto const outerAccount = ctx.tx.getAccountID(sfAccount); + auto const& rawTxns = ctx.tx.getFieldArray(sfRawTransactions); + + // Build the signers list + std::unordered_set requiredSigners; + for (STObject const& rb : rawTxns) + { + auto const innerAccount = rb.getAccountID(sfAccount); // If the inner account is the same as the outer account, do not add the // inner account to the required signers set. diff --git a/src/xrpld/app/tx/detail/Batch.h b/src/xrpld/app/tx/detail/Batch.h index c8a264f328..427cc6bf34 100644 --- a/src/xrpld/app/tx/detail/Batch.h +++ b/src/xrpld/app/tx/detail/Batch.h @@ -49,6 +49,9 @@ public: static NotTEC preflight(PreflightContext const& ctx); + static NotTEC + preflightSigValidated(PreflightContext const& ctx); + static NotTEC checkSign(PreclaimContext const& ctx); diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index fa906eb22a..0cf07c8c07 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -650,6 +650,15 @@ EscrowFinish::preflight(PreflightContext const& ctx) if (static_cast(cb) != static_cast(fb)) return temMALFORMED; + return tesSUCCESS; +} + +NotTEC +EscrowFinish::preflightSigValidated(PreflightContext const& ctx) +{ + auto const cb = ctx.tx[~sfCondition]; + auto const fb = ctx.tx[~sfFulfillment]; + if (cb && fb) { auto& router = ctx.app.getHashRouter(); diff --git a/src/xrpld/app/tx/detail/Escrow.h b/src/xrpld/app/tx/detail/Escrow.h index 9696515963..fd08b4764a 100644 --- a/src/xrpld/app/tx/detail/Escrow.h +++ b/src/xrpld/app/tx/detail/Escrow.h @@ -69,6 +69,9 @@ public: static NotTEC preflight(PreflightContext const& ctx); + static NotTEC + preflightSigValidated(PreflightContext const& ctx); + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 5988199393..3d2bb8197e 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -267,6 +267,12 @@ Transactor::getFlagsMask(PreflightContext const& ctx) return tfUniversalMask; } +NotTEC +Transactor::preflightSigValidated(PreflightContext const& ctx) +{ + return tesSUCCESS; +} + TER Transactor::checkPermission(ReadView const& view, STTx const& tx) { diff --git a/src/xrpld/app/tx/detail/Transactor.h b/src/xrpld/app/tx/detail/Transactor.h index cb58b5eca2..29a8df7574 100644 --- a/src/xrpld/app/tx/detail/Transactor.h +++ b/src/xrpld/app/tx/detail/Transactor.h @@ -215,6 +215,10 @@ public: static NotTEC preflight(PreflightContext const& ctx); + // Optional, rarely needed, if the transaction does any expensive + // checks after the signature is verified. + static NotTEC preflightSigValidated(PreflightContext const& ctx); + * Do not try to call preflight1 or preflight2 directly. * Do not check whether relevant amendments are enabled in preflight. Instead, define isEnabled. @@ -284,6 +288,10 @@ protected: static std::uint32_t getFlagsMask(PreflightContext const& ctx); + // Base class always returns tesSUCCESS + static NotTEC + preflightSigValidated(PreflightContext const& ctx); + static bool validDataLength(std::optional const& slice, std::size_t maxLength); @@ -393,7 +401,10 @@ Transactor::invokePreflight(PreflightContext const& ctx) if (auto const ret = T::preflight(ctx)) return ret; - return preflight2(ctx); + if (auto const ret = preflight2(ctx)) + return ret; + + return T::preflightSigValidated(ctx); } template