Merge remote-tracking branch 'XRPLF/develop' into ximinez/directory

* XRPLF/develop: (21 commits)
  fix: Re-store nodes missing from both backends during online_delete rotation (7763)
  fix: Add amendment sponsor for AccountRootsDeletedClean (7801)
  fix: Update base_uint and test changes released in 3.1.3 (7570)
  fix: Handle rounding just above kMaxRep more accurately (7389)
  fix: Document and assert "after" is never null in invariants (7354)
  ci: Run full matrix only on `Ready to merge` or `Full CI build` labeled PRs (7689)
  fix: Strengthen Clawback invariant checks for MPT balances (7285)
  test: Add unit tests for IP address related functions (7744)
  ci: Add Rust to Nix docker image (7571)
  docs: Add more information about pre-commit hooks and how to set them up (7802)
  test: Add JSON array size tests (7592)
  chore: Enable most readability checks (7772)
  ci: Do not run conflict checker when label is applied (7774)
  chore: Run clang_tidy_check with `pass_filenames: false` from pre-commit (7800)
  feat: Add delegate filter param for account_tx RPC (6126)
  refactor: Move `jss.h` `include` out of `Indexes.h` (7799)
  test: Add tests for check doxygen style (7795)
  style: Add pre-commit hook to check doxygen style (7794)
  style: Unify style for all Doxygen comments (7776)
  fix: Improve Number addition/subtraction rounding (7369)
  ...

.github/workflows/on-pr.yml

@@ -25,11 +25,18 @@ on:
       - unlabeled
 
 concurrency:
-  # A single per-ref group with cancel-in-progress means any newer run (a push
-  # or a label change) supersedes the in-progress one for that ref. Keeping
-  # exactly one authoritative run per ref ensures a fast do-nothing run can never
-  # mask a real build's checks.
-  group: ${{ github.workflow }}-${{ github.ref }}
+  # Use a per-ref group so a newer run (a push, or a change to a label below)
+  # supersedes the in-progress one for that ref. Label events we don't act on get
+  # their own unique group (per run id) instead, keeping them out of the shared
+  # group so real builds keep running. Keep this list in sync with `should-run`.
+  group: >-
+    ${{ github.workflow }}-${{ github.ref }}${{
+      ((github.event.action == 'labeled' || github.event.action == 'unlabeled')
+      && github.event.label.name != 'Ready to merge'
+      && github.event.label.name != 'DraftRunCI'
+      && github.event.label.name != 'Full CI build')
+      && format('-{0}', github.run_id) || ''
+    }}
   cancel-in-progress: true
 
 defaults:
@@ -37,17 +44,21 @@ defaults:
     shell: bash
 
 jobs:
-  # This job determines whether the rest of the workflow should run at all,
-  # based on the current set of labels: it runs when the PR is not a draft
-  # (which should also cover merge-group) or has the 'DraftRunCI' or
-  # 'Full CI build' label. Whether a build then happens, and whether it is the
-  # minimal or full matrix, is decided further below and in the strategy matrix.
+  # This job determines whether the rest of the workflow should run. It runs
+  # when the PR is not a draft (which should also cover merge-group) or has the
+  # 'DraftRunCI' or 'Full CI build' label. For label events it only runs when the
+  # label added or removed is one we act on ('Ready to merge', 'DraftRunCI' or
+  # 'Full CI build'), so unrelated label changes do not trigger a redundant run.
   should-run:
     if: >-
       ${{
-        !github.event.pull_request.draft
-        || contains(github.event.pull_request.labels.*.name, 'DraftRunCI')
-        || contains(github.event.pull_request.labels.*.name, 'Full CI build')
+        ((github.event.action != 'labeled' && github.event.action != 'unlabeled')
+          || github.event.label.name == 'Ready to merge'
+          || github.event.label.name == 'DraftRunCI'
+          || github.event.label.name == 'Full CI build')
+        && (!github.event.pull_request.draft
+          || contains(github.event.pull_request.labels.*.name, 'DraftRunCI')
+          || contains(github.event.pull_request.labels.*.name, 'Full CI build'))
       }}
     runs-on: ubuntu-latest
     steps:

conan.lock

@@ -10,22 +10,22 @@
         "rocksdb/10.5.1#4a197eca381a3e5ae8adf8cffa5aacd0%1782392413.075713",
         "re2/20251105#8579cfd0bda4daf0683f9e3898f964b4%1782392402.431897",
         "protobuf/6.33.5#ff253ead763bd8d9904a52979cd21e81%1782392410.233933",
-        "openssl/3.6.3#f806de8933e3bf6f01016c6a888cee2e%1783945160.863288",
+        "openssl/3.6.3#1163d4ddc603907084d08a6a0c6e580f%1782307150.583886",
         "nudb/2.0.9#11149c73f8f2baff9a0198fe25971fc7%1782392402.297166",
-        "mpt-crypto/0.4.0-rc4#ffdba12f2332357f0d8b0ae944cfff52%1784138702.932355",
+        "mpt-crypto/0.4.0-rc2#a580f2f9ad0e795de696aa62d54fb9af%1782425834.488828",
         "lz4/1.10.0#982d9b673900f665a1da109e09c17cab%1782392402.164188",
         "libiconv/1.17#9923bc6dc6f106646d6967e0039a5ada%1782392792.775744",
         "libbacktrace/cci.20210118#a7691bfccd8caaf66309df196790a5a1%1782392402.420732",
         "libarchive/3.8.7#c446109bd1f1d8ba7936c94189bc50e6%1782392403.066892",
         "jemalloc/5.3.1#1fc58d55316041f10fbc1e8a2eae632a%1776700028.228",
         "gtest/1.17.0#5224b3b3ff3b4ce1133cbdd27d53ee7d%1782392402.791979",
-        "grpc/1.81.1#f729f6d75992d20f9c72828e9142d62f%1783945160.094135",
+        "grpc/1.81.1#5217e6ef0544c42b46f4af35d5e7f649%1782307148.845616",
         "ed25519/2015.03#ae761bdc52730a843f0809bdf6c1b1f6%1782307148.15562",
         "date/3.0.4#862e11e80030356b53c2c38599ceb32b%1782392402.538492",
         "c-ares/1.34.6#545240bb1c40e2cacd4362d6b8967650%1782392402.681654",
         "bzip2/1.0.8#c470882369c2d95c5c77e970c0c7e321%1782392402.296732",
         "boost/1.91.0#ea540ca2133d831b560036aa24dece3c%1782392419.475605",
-        "abseil/20250127.0#9ef01c1451a8340f9022e46238c0fbb6%1783945159.651047"
+        "abseil/20250127.0#bb0baf1f362bc4a725a24eddd419b8f7%1782307147.395833"
     ],
     "build_requires": [
         "zlib/1.3.2#1cb806da49011867778ffb6ac7190fcb%1782392402.122708",
@@ -38,7 +38,7 @@
         "b2/5.4.2#ffd6084a119587e70f11cd45d1a386e2%1782392402.624226",
         "automake/1.16.5#b91b7c384c3deaa9d535be02da14d04f%1755524470.56",
         "autoconf/2.71#51077f068e61700d65bb05541ea1e4b0%1731054366.86",
-        "abseil/20250127.0#9ef01c1451a8340f9022e46238c0fbb6%1783945159.651047"
+        "abseil/20250127.0#bb0baf1f362bc4a725a24eddd419b8f7%1782307147.395833"
     ],
     "python_requires": [],
     "overrides": {

conanfile.py

@@ -134,7 +134,7 @@ class Xrpl(ConanFile):
         if self.options.jemalloc:
             self.requires("jemalloc/5.3.1")
         self.requires("lz4/1.10.0", force=True)
-        self.requires("mpt-crypto/0.4.0-rc4", transitive_headers=True)
+        self.requires("mpt-crypto/0.4.0-rc2", transitive_headers=True)
         self.requires("protobuf/6.33.5", force=True)
         if self.options.rocksdb:
             self.requires("rocksdb/10.5.1")

include/xrpl/basics/TaggedCache.ipp

@@ -3,9 +3,6 @@
 #include <xrpl/basics/IntrusivePointer.ipp>
 #include <xrpl/basics/Log.h>  // IWYU pragma: keep
 #include <xrpl/basics/TaggedCache.h>
-#include <xrpl/basics/scope.h>
-
-#include <algorithm>
 
 namespace xrpl {
 
@@ -604,42 +601,8 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
     std::vector<key_type> v;
 
     {
-        // Keep track of how many iterations are needed. Exit the loop if the number of retries gets
-        // absurd. (Note that if this somehow ever happens, one more allocation will be done under
-        // lock, which is undesirable, but really should be almost impossible.)
-        std::size_t allocationIterations = 0;
-        std::unique_lock lock(mutex_);
-        for (auto size = cache_.size(); v.capacity() < size && allocationIterations < 20;
-             size = cache_.size())
-        {
-            ScopeUnlock const unlock(lock);
-            if (allocationIterations > 0)
-            {
-                JLOG(journal_.info())
-                    << "getKeys(): Cache grew beyond allocated capacity after "
-                    << allocationIterations << " prior attempt(s). Have " << v.capacity()
-                    << ", need " << size << ". Retrying allocation";
-            }
-            // Allocate the current size plus a little extra, in case the cache grows while
-            // allocating. Each time another allocation is needed, the extra also gets bigger until
-            // it ultimately doubles the size + 1.
-            constexpr std::size_t baseShift = 5;
-            auto const bufferOffset = std::min(allocationIterations, std::size_t{baseShift});
-            auto const bufferShift = baseShift - bufferOffset;
-            size += (size >> bufferShift) + 1;
-            v.reserve(size);
-            ++allocationIterations;
-        }
-        if (v.capacity() < cache_.size())
-        {
-            // LCOV_EXCL_START
-            UNREACHABLE("xrpl::TaggedCache::getKeys(): failed to allocate sufficient capacity");
-            v.reserve(cache_.size());
-            // LCOV_EXCL_STOP
-        }
-        XRPL_ASSERT(lock.owns_lock(), "xrpl::TaggedCache::getKeys(): owns lock");
-        XRPL_ASSERT(
-            v.capacity() >= cache_.size(), "xrpl::TaggedCache::getKeys(): sufficient capacity");
+        std::scoped_lock const lock(mutex_);
+        v.reserve(cache_.size());
         for (auto const& _ : cache_)
             v.push_back(_.first);
     }

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FEATURE(DefragDirectories,           Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(Sponsor,                     Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(BatchV1_1,                   Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(LendingProtocolV1_1,         Supported::No,  VoteBehavior::DefaultNo)

src/libxrpl/ledger/ApplyView.cpp

@@ -26,6 +26,14 @@ namespace xrpl {
 
 namespace directory {
 
+struct Gap
+{
+    uint64_t const page;
+    SLE::pointer node;
+    uint64_t const nextPage;
+    SLE::pointer next;
+};
+
 std::uint64_t
 createRoot(
     ApplyView& view,
@@ -126,7 +134,9 @@ insertPage(
     if (page == 0)
         return std::nullopt;
     if (!view.rules().enabled(fixDirectoryLimit) && page >= kDirNodeMaxPages)  // Old pages limit
+    {
         return std::nullopt;
+    }
 
     // We are about to create a new node; we'll link it to
     // the chain first:
@@ -147,12 +157,8 @@ insertPage(
     // Save some space by not specifying the value 0 since it's the default.
     if (page != 1)
         node->setFieldU64(sfIndexPrevious, page - 1);
-    XRPL_ASSERT_PARTS(!nextPage, "xrpl::directory::insertPage", "nextPage has default value");
-    /* Reserved for future use when directory pages may be inserted in
-     * between two other pages instead of only at the end of the chain.
     if (nextPage)
         node->setFieldU64(sfIndexNext, nextPage);
-    */
     describe(node);
     view.insert(node);
 
@@ -168,7 +174,7 @@ ApplyView::dirAdd(
     uint256 const& key,
     std::function<void(SLE::ref)> const& describe)
 {
-    auto root = peek(directory);
+    auto const root = peek(directory);
 
     if (!root)
     {
@@ -178,6 +184,43 @@ ApplyView::dirAdd(
 
     auto [page, node, indexes] = directory::findPreviousPage(*this, directory, root);
 
+    if (rules().enabled(featureDefragDirectories))
+    {
+        // If there are more nodes than just the root, and there's no space in
+        // the last one, walk backwards to find one with space, or to find one
+        // missing.
+        std::optional<directory::Gap> gapPages;
+        while (page && indexes.size() >= kDIR_NODE_MAX_PAGES)
+        {
+            // Find a page with space, or a gap in pages.
+            auto [prevPage, prevNode, prevIndexes] =
+                directory::findPreviousPage(*this, directory, node);
+            if (!gapPages && prevPage != page - 1)
+                gapPages.emplace(prevPage, prevNode, page, node);
+            page = prevPage;
+            node = prevNode;
+            indexes = prevIndexes;
+        }
+        // We looped through all the pages back to the root.
+        if (!page)
+        {
+            // If we found a gap, use it.
+            if (gapPages)
+            {
+                return directory::insertPage(
+                    *this,
+                    gapPages->page,
+                    gapPages->node,
+                    gapPages->nextPage,
+                    gapPages->next,
+                    key,
+                    directory,
+                    describe);
+            }
+            std::tie(page, node, indexes) = directory::findPreviousPage(*this, directory, root);
+        }
+    }
+
     // If there's space, we use it:
     if (indexes.size() < kDirNodeMaxEntries)
     {

src/libxrpl/tx/PreflightHelpers.h

@@ -1,66 +0,0 @@
-#pragma once
-
-#include <xrpl/beast/utility/Zero.h>
-#include <xrpl/protocol/STAmount.h>
-#include <xrpl/protocol/UintTypes.h>
-
-#include <cstddef>
-
-template <class T>
-inline bool
-checkBounds(T const& value, T const& min, T const& max)
-{
-    return value >= min && value <= max;
-}
-
-template <class T>
-inline bool
-checkMax(T const& value, T const& max)
-{
-    return value <= max;
-}
-
-template <class T>
-inline bool
-checkSize(T const& value, std::size_t const max)
-{
-    return value.size() <= max;
-}
-
-template <class T>
-inline bool
-checkSizeNonEmpty(T const& value, std::size_t const max)
-{
-    return value.size() <= max && !value.empty();
-}
-
-// Checks whether a hash-like identifier field (e.g. a uint256 object ID) is
-// unset/zero.
-template <class T>
-inline bool
-isZeroId(T const& id)
-{
-    return id == beast::kZero;
-}
-
-// Checks whether an amount is a strictly positive XRP amount.
-inline bool
-isPositiveXRPAmount(xrpl::STAmount const& amount)
-{
-    return xrpl::isXRP(amount) && amount > beast::kZero;
-}
-
-// Checks whether an amount (of any asset type) is strictly positive.
-inline bool
-isPositiveAmount(xrpl::STAmount const& amount)
-{
-    return amount > beast::kZero;
-}
-
-// Checks whether a currency code is the reserved "bad"/XRP currency code,
-// i.e. not a valid IOU currency.
-inline bool
-isBadCurrency(xrpl::Currency const& currency)
-{
-    return xrpl::badCurrency() == currency;
-}

src/libxrpl/tx/transactors/check/CheckCancel.cpp

@@ -1,12 +1,10 @@
 #include <xrpl/tx/transactors/check/CheckCancel.h>
 
 #include <xrpl/basics/Log.h>
-#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/protocol/AccountID.h>
-#include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -21,9 +19,6 @@ namespace xrpl {
 NotTEC
 CheckCancel::preflight(PreflightContext const& ctx)
 {
-    if (ctx.rules.enabled(fixCleanup3_3_0) && ctx.tx[sfCheckID] == beast::kZero)
-        return temMALFORMED;
-
     return tesSUCCESS;
 }
 

src/libxrpl/tx/transactors/check/CheckCash.cpp

@@ -2,7 +2,6 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/scope.h>
-#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/PaymentSandbox.h>
@@ -52,9 +51,6 @@ CheckCash::checkExtraFeatures(xrpl::PreflightContext const& ctx)
 NotTEC
 CheckCash::preflight(PreflightContext const& ctx)
 {
-    if (ctx.rules.enabled(fixCleanup3_3_0) && ctx.tx[sfCheckID] == beast::kZero)
-        return temMALFORMED;
-
     // Exactly one of Amount or DeliverMin must be present.
     auto const optAmount = ctx.tx[~sfAmount];
     auto const optDeliverMin = ctx.tx[~sfDeliverMin];

src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp

@@ -28,12 +28,11 @@
 #include <xrpl/protocol/STLedgerEntry.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/TER.h>
+#include <xrpl/protocol/UintTypes.h>
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/applySteps.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <memory>
 #include <system_error>
 #include <variant>
@@ -102,10 +101,10 @@ NotTEC
 escrowCreatePreflightHelper<Issue>(PreflightContext const& ctx)
 {
     STAmount const amount = ctx.tx[sfAmount];
-    if (amount.native() || !isPositiveAmount(amount))
+    if (amount.native() || amount <= beast::kZero)
         return temBAD_AMOUNT;
 
-    if (isBadCurrency(amount.get<Issue>().currency))
+    if (badCurrency() == amount.get<Issue>().currency)
         return temBAD_CURRENCY;
 
     return tesSUCCESS;
@@ -119,7 +118,7 @@ escrowCreatePreflightHelper<MPTIssue>(PreflightContext const& ctx)
         return temDISABLED;
 
     auto const amount = ctx.tx[sfAmount];
-    if (amount.native() || amount.mpt() > MPTAmount{kMaxMpTokenAmount} || !isPositiveAmount(amount))
+    if (amount.native() || amount.mpt() > MPTAmount{kMaxMpTokenAmount} || amount <= beast::kZero)
         return temBAD_AMOUNT;
 
     return tesSUCCESS;
@@ -142,7 +141,7 @@ EscrowCreate::preflight(PreflightContext const& ctx)
     }
     else
     {
-        if (!isPositiveXRPAmount(amount))
+        if (amount <= beast::kZero)
             return temBAD_AMOUNT;
     }
 

src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp

@@ -27,8 +27,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <expected>
 #include <optional>
 #include <variant>
@@ -50,7 +48,7 @@ LoanBrokerCoverClawback::preflight(PreflightContext const& ctx)
     if (!brokerID && !amount)
         return temINVALID;
 
-    if (brokerID && isZeroId(*brokerID))
+    if (brokerID && *brokerID == beast::kZero)
         return temINVALID;
 
     if (amount)

src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp

@@ -16,8 +16,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 bool
@@ -29,11 +27,11 @@ LoanBrokerCoverDeposit::checkExtraFeatures(PreflightContext const& ctx)
 NotTEC
 LoanBrokerCoverDeposit::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanBrokerID]))
+    if (ctx.tx[sfLoanBrokerID] == beast::kZero)
         return temINVALID;
 
     auto const dstAmount = ctx.tx[sfAmount];
-    if (!isPositiveAmount(dstAmount))
+    if (dstAmount <= beast::kZero)
         return temBAD_AMOUNT;
 
     if (!isLegalNet(dstAmount))

src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Number.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/LendingHelpers.h>
@@ -19,8 +20,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 bool
@@ -32,11 +31,11 @@ LoanBrokerCoverWithdraw::checkExtraFeatures(PreflightContext const& ctx)
 NotTEC
 LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanBrokerID]))
+    if (ctx.tx[sfLoanBrokerID] == beast::kZero)
         return temINVALID;
 
     auto const dstAmount = ctx.tx[sfAmount];
-    if (!isPositiveAmount(dstAmount))
+    if (dstAmount <= beast::kZero)
         return temBAD_AMOUNT;
 
     if (!isLegalNet(dstAmount))
@@ -44,7 +43,7 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx)
 
     if (auto const destination = ctx.tx[~sfDestination])
     {
-        if (isZeroId(*destination))
+        if (*destination == beast::kZero)
         {
             return temMALFORMED;
         }

src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp

@@ -18,8 +18,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 bool
@@ -31,7 +29,7 @@ LoanBrokerDelete::checkExtraFeatures(PreflightContext const& ctx)
 NotTEC
 LoanBrokerDelete::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanBrokerID]))
+    if (ctx.tx[sfLoanBrokerID] == beast::kZero)
         return temINVALID;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Number.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
@@ -19,8 +20,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <memory>
 #include <vector>
 
@@ -58,13 +57,13 @@ LoanBrokerSet::preflight(PreflightContext const& ctx)
             tx.isFieldPresent(sfCoverRateLiquidation))
             return temINVALID;
 
-        if (isZeroId(tx[sfLoanBrokerID]))
+        if (tx[sfLoanBrokerID] == beast::kZero)
             return temINVALID;
     }
 
     if (auto const vaultID = tx.at(~sfVaultID))
     {
-        if (isZeroId(*vaultID))
+        if (*vaultID == beast::kZero)
             return temINVALID;
     }
 

src/libxrpl/tx/transactors/lending/LoanDelete.cpp

@@ -16,8 +16,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 bool
@@ -29,7 +27,7 @@ LoanDelete::checkExtraFeatures(PreflightContext const& ctx)
 NotTEC
 LoanDelete::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanID]))
+    if (ctx.tx[sfLoanID] == beast::kZero)
         return temINVALID;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/lending/LoanManage.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Number.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/View.h>
@@ -23,8 +24,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <algorithm>
 #include <cstdint>
 namespace xrpl {
@@ -44,7 +43,7 @@ LoanManage::getFlagsMask(PreflightContext const& ctx)
 NotTEC
 LoanManage::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanID]))
+    if (ctx.tx[sfLoanID] == beast::kZero)
         return temINVALID;
 
     // Flags are mutually exclusive

src/libxrpl/tx/transactors/lending/LoanPay.cpp

@@ -25,8 +25,6 @@
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/transactors/lending/LoanManage.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <algorithm>
 #include <bit>
 #include <cstdint>
@@ -50,10 +48,10 @@ LoanPay::getFlagsMask(PreflightContext const& ctx)
 NotTEC
 LoanPay::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfLoanID]))
+    if (ctx.tx[sfLoanID] == beast::kZero)
         return temINVALID;
 
-    if (!isPositiveAmount(ctx.tx[sfAmount]))
+    if (ctx.tx[sfAmount] <= beast::kZero)
         return temBAD_AMOUNT;
 
     // The loan payment flags are all mutually exclusive. If more than one is

src/libxrpl/tx/transactors/lending/LoanSet.cpp

@@ -28,8 +28,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <cstddef>
 #include <cstdint>
 #include <limits>
@@ -140,7 +138,7 @@ LoanSet::preflight(PreflightContext const& ctx)
             return *ret;
     }
 
-    if (auto const brokerID = ctx.tx[~sfLoanBrokerID]; brokerID && isZeroId(*brokerID))
+    if (auto const brokerID = ctx.tx[~sfLoanBrokerID]; brokerID && *brokerID == beast::kZero)
         return temINVALID;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/payment/Payment.cpp

@@ -36,8 +36,6 @@
 #include <xrpl/tx/applySteps.h>
 #include <xrpl/tx/paths/RippleCalc.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <algorithm>
 #include <cstdint>
 #include <limits>
@@ -145,7 +143,7 @@ Payment::preflight(PreflightContext const& ctx)
     // A zero DomainID is invalid for a PermissionedDomain ledger entry because
     // keylet::permissionedDomain(uint256) uses the DomainID as the ledger key.
     if (auto const domainID = tx[~sfDomainID];
-        ctx.rules.enabled(fixCleanup3_2_0) && domainID && isZeroId(*domainID))
+        ctx.rules.enabled(fixCleanup3_2_0) && domainID && *domainID == beast::kZero)
         return temMALFORMED;
 
     bool const partialPaymentAllowed = tx.isFlag(tfPartialPayment);
@@ -185,13 +183,13 @@ Payment::preflight(PreflightContext const& ctx)
                         << "Payment destination account not specified.";
         return temDST_NEEDED;
     }
-    if (hasMax && !isPositiveAmount(maxSourceAmount))
+    if (hasMax && maxSourceAmount <= beast::kZero)
     {
         JLOG(j.trace()) << "Malformed transaction: bad max amount: "
                         << maxSourceAmount.getFullText();
         return temBAD_AMOUNT;
     }
-    if (!isPositiveAmount(dstAmount))
+    if (dstAmount <= beast::kZero)
     {
         JLOG(j.trace()) << "Malformed transaction: bad dst amount: " << dstAmount.getFullText();
         return temBAD_AMOUNT;

src/libxrpl/tx/transactors/payment_channel/PaymentChannelClaim.cpp

@@ -22,8 +22,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <cstdint>
 #include <optional>
 
@@ -44,15 +42,15 @@ PaymentChannelClaim::getFlagsMask(PreflightContext const&)
 NotTEC
 PaymentChannelClaim::preflight(PreflightContext const& ctx)
 {
-    if (ctx.rules.enabled(fixCleanup3_2_0) && isZeroId(ctx.tx[sfChannel]))
+    if (ctx.rules.enabled(fixCleanup3_2_0) && ctx.tx[sfChannel] == beast::kZero)
         return temMALFORMED;
 
     auto const bal = ctx.tx[~sfBalance];
-    if (bal && !isPositiveXRPAmount(*bal))
+    if (bal && (!isXRP(*bal) || *bal <= beast::kZero))
         return temBAD_AMOUNT;
 
     auto const amt = ctx.tx[~sfAmount];
-    if (amt && !isPositiveXRPAmount(*amt))
+    if (amt && (!isXRP(*amt) || *amt <= beast::kZero))
         return temBAD_AMOUNT;
 
     if (bal && amt && *bal > *amt)

src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/tx/transactors/payment_channel/PaymentChannelCreate.h>
 
 #include <xrpl/basics/chrono.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/View.h>
@@ -21,8 +22,6 @@
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/applySteps.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <memory>
 
 namespace xrpl {
@@ -58,7 +57,7 @@ PaymentChannelCreate::makeTxConsequences(PreflightContext const& ctx)
 NotTEC
 PaymentChannelCreate::preflight(PreflightContext const& ctx)
 {
-    if (!isPositiveXRPAmount(ctx.tx[sfAmount]))
+    if (!isXRP(ctx.tx[sfAmount]) || (ctx.tx[sfAmount] <= beast::kZero))
         return temBAD_AMOUNT;
 
     if (ctx.tx[sfAccount] == ctx.tx[sfDestination])

src/libxrpl/tx/transactors/payment_channel/PaymentChannelFund.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/tx/transactors/payment_channel/PaymentChannelFund.h>
 
 #include <xrpl/beast/utility/Journal.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
@@ -19,8 +20,6 @@
 #include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/applySteps.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 TxConsequences
@@ -32,10 +31,10 @@ PaymentChannelFund::makeTxConsequences(PreflightContext const& ctx)
 NotTEC
 PaymentChannelFund::preflight(PreflightContext const& ctx)
 {
-    if (ctx.rules.enabled(fixCleanup3_2_0) && isZeroId(ctx.tx[sfChannel]))
+    if (ctx.rules.enabled(fixCleanup3_2_0) && ctx.tx[sfChannel] == beast::kZero)
         return temMALFORMED;
 
-    if (!isPositiveXRPAmount(ctx.tx[sfAmount]))
+    if (!isXRP(ctx.tx[sfAmount]) || (ctx.tx[sfAmount] <= beast::kZero))
         return temBAD_AMOUNT;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainDelete.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/tx/transactors/permissioned_domain/PermissionedDomainDelete.h>
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/protocol/Indexes.h>
@@ -11,15 +12,13 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 NotTEC
 PermissionedDomainDelete::preflight(PreflightContext const& ctx)
 {
     auto const domain = ctx.tx.getFieldH256(sfDomainID);
-    if (isZeroId(domain))
+    if (domain == beast::kZero)
         return temMALFORMED;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp

@@ -1,5 +1,6 @@
 #include <xrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.h>
 
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
@@ -16,8 +17,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <memory>
 #include <utility>
 
@@ -40,7 +39,7 @@ PermissionedDomainSet::preflight(PreflightContext const& ctx)
         return err;
 
     auto const domain = ctx.tx.at(~sfDomainID);
-    if (domain && isZeroId(*domain))
+    if (domain && *domain == beast::kZero)
         return temMALFORMED;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/token/Clawback.cpp

@@ -22,8 +22,6 @@
 #include <xrpl/tx/ApplyContext.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <algorithm>
 #include <variant>
 
@@ -46,7 +44,7 @@ preflightHelper<Issue>(PreflightContext const& ctx)
     // The issuer field is used for the token holder instead
     AccountID const& holder = clawAmount.getIssuer();
 
-    if (issuer == holder || isXRP(clawAmount) || !isPositiveAmount(clawAmount))
+    if (issuer == holder || isXRP(clawAmount) || clawAmount <= beast::kZero)
         return temBAD_AMOUNT;
 
     return tesSUCCESS;
@@ -69,7 +67,7 @@ preflightHelper<MPTIssue>(PreflightContext const& ctx)
     if (ctx.tx[sfAccount] == *mptHolder)
         return temMALFORMED;
 
-    if (clawAmount.mpt() > MPTAmount{kMaxMpTokenAmount} || !isPositiveAmount(clawAmount))
+    if (clawAmount.mpt() > MPTAmount{kMaxMpTokenAmount} || clawAmount <= beast::kZero)
         return temBAD_AMOUNT;
 
     return tesSUCCESS;

src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/tx/transactors/token/MPTokenIssuanceCreate.h>
 
 #include <xrpl/beast/utility/Journal.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
@@ -20,8 +21,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <cstdint>
 #include <expected>
 #include <memory>
@@ -88,7 +87,7 @@ MPTokenIssuanceCreate::preflight(PreflightContext const& ctx)
 
     if (auto const domain = ctx.tx[~sfDomainID])
     {
-        if (isZeroId(*domain))
+        if (*domain == beast::kZero)
             return temMALFORMED;
 
         // Domain present implies that MPTokenIssuance is not public

src/libxrpl/tx/transactors/token/TrustSet.cpp

@@ -25,8 +25,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <cstdint>
 #include <unordered_set>
 
@@ -100,7 +98,7 @@ TrustSet::preflight(PreflightContext const& ctx)
         return temBAD_LIMIT;
     }
 
-    if (isBadCurrency(saLimitAmount.get<Issue>().currency))
+    if (badCurrency() == saLimitAmount.get<Issue>().currency)
     {
         JLOG(j.trace()) << "Malformed transaction: specifies XRP as IOU";
         return temBAD_CURRENCY;

src/libxrpl/tx/transactors/vault/VaultDelete.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/MPTokenHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
@@ -18,14 +19,12 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 namespace xrpl {
 
 NotTEC
 VaultDelete::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfVaultID]))
+    if (ctx.tx[sfVaultID] == beast::kZero)
     {
         JLOG(ctx.j.debug()) << "VaultDelete: zero/empty vault ID.";
         return temMALFORMED;

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

@@ -23,8 +23,6 @@
 #include <xrpl/protocol/XRPAmount.h>
 #include <xrpl/tx/Transactor.h>
 
-#include <libxrpl/tx/PreflightHelpers.h>
-
 #include <stdexcept>
 
 namespace xrpl {
@@ -44,18 +42,18 @@ shouldWaiveWithdrawal(ReadView const& view, AccountID const& account, SLE::const
 NotTEC
 VaultWithdraw::preflight(PreflightContext const& ctx)
 {
-    if (isZeroId(ctx.tx[sfVaultID]))
+    if (ctx.tx[sfVaultID] == beast::kZero)
     {
         JLOG(ctx.j.debug()) << "VaultWithdraw: zero/empty vault ID.";
         return temMALFORMED;
     }
 
-    if (!isPositiveAmount(ctx.tx[sfAmount]))
+    if (ctx.tx[sfAmount] <= beast::kZero)
         return temBAD_AMOUNT;
 
     if (auto const destination = ctx.tx[~sfDestination])
     {
-        if (isZeroId(*destination))
+        if (*destination == beast::kZero)
         {
             return temMALFORMED;
         }

src/test/app/Check_test.cpp

@@ -1257,13 +1257,6 @@ class Check_test : public beast::unit_test::Suite
             env.close();
         }
 
-        // Can't run pre-amendment behavior due to assertion failure.
-        if (features[fixCleanup3_3_0])
-        {
-            env(check::cash(bob, uint256{}, usd(20)), Ter(temMALFORMED));
-            env.close();
-        }
-
         // alice creates her checks ahead of time.
         uint256 const chkIdU{getCheckIndex(alice, env.seq(alice))};
         env(check::create(alice, bob, usd(20)));
@@ -1711,13 +1704,6 @@ class Check_test : public beast::unit_test::Suite
         // Non-existent check.
         env(check::cancel(bob, getCheckIndex(alice, env.seq(alice))), Ter(tecNO_ENTRY));
         env.close();
-
-        // Can't run pre-amendment behavior due to assertion failure.
-        if (features[fixCleanup3_3_0])
-        {
-            env(check::cancel(bob, uint256{}), Ter(temMALFORMED));
-            env.close();
-        }
     }
 
     void