Make the Relational tests way more comprehensive

- Test combinatorically with a large variety of values.

docker/check-tools.sh

@@ -10,11 +10,13 @@ cmake --version
 conan --version
 g++ --version
 gcc --version
+gcov --version
 gcovr --version
 git --version
 less --version
 make --version
 mold --version
+netstat --version
 ninja --version
 perl --version
 pkg-config --version
@@ -22,3 +24,9 @@ pre-commit --version
 python3 --version
 run-clang-tidy --help
 vim --version
+
+# A simple test to verify that git can clone a repository over HTTPS
+# (i.e. the CA bundle is wired up). Clone to a temp dir and clean up.
+tmp_clone="$(mktemp -d)"
+git clone --depth 1 https://github.com/XRPLF/actions.git "${tmp_clone}/actions"
+rm -rf "${tmp_clone}"

docker/nix.Dockerfile

@@ -47,6 +47,12 @@ COPY --from=builder /tmp/build/result /nix/ci-env
 
 ENV PATH="/nix/ci-env/bin:${PATH}"
 
+# Point HTTPS clients (git, curl, conan, ...) at the CA bundle shipped in the
+# Nix CI environment, so TLS verification works without ca-certificates being
+# installed in the system.
+ENV SSL_CERT_FILE="/nix/ci-env/etc/ssl/certs/ca-bundle.crt"
+ENV GIT_SSL_CAINFO="/nix/ci-env/etc/ssl/certs/ca-bundle.crt"
+
 # Externally-built dynamically-linked ELF binaries hard-code the loader path
 # (e.g. /lib64/ld-linux-x86-64.so.2) in their PT_INTERP header. Install it
 # from the Nix store when the base image doesn't already provide one.
@@ -65,8 +71,8 @@ if [ ! -e "${target}" ]; then
 fi
 EOF
 
-COPY docker/check-tool-versions.sh /tmp/check-tool-versions.sh
-RUN /tmp/check-tool-versions.sh
+COPY docker/check-tools.sh /tmp/check-tools.sh
+RUN /tmp/check-tools.sh
 
 # Sanity-check that the g++/clang++ are able to build binaries, including sanitizer-instrumented ones.
 COPY docker/test_files/cpp_sources/ /tmp/cpp_sources/

include/xrpl/basics/Number.h

@@ -408,33 +408,38 @@ public:
     }
 
     friend constexpr bool
-    operator<(Number const& x, Number const& y) noexcept
+    operator<(Number const& l, Number const& r) noexcept
     {
+        bool const lneg = l.negative_;
+        bool const rneg = r.negative_;
+
         // If the two amounts have different signs (zero is treated as positive)
         // then the comparison is true iff the left is negative.
-        bool const lneg = x.negative_;
-        bool const rneg = y.negative_;
-
         if (lneg != rneg)
             return lneg;
 
-        // Both have same sign and the left is zero: the right must be
-        // greater than 0.
-        if (x.mantissa_ == 0)
-            return y.mantissa_ > 0;
+        // Both have same sign and the left is zero: both must be non-negative.
+        // If the right is greater than 0, then it is larger, so the comparison is true.
+        if (l.mantissa_ == 0)
+            return r.mantissa_ > 0;
 
-        // Both have same sign, the right is zero and the left is non-zero.
-        if (y.mantissa_ == 0)
+        // Both have same sign, the right is zero and the left is non-zero, so the left must be
+        // positive, and thus is larger, so the comparison is false.
+        if (r.mantissa_ == 0)
             return false;
 
         // Both have the same sign, compare by exponents:
-        if (x.exponent_ > y.exponent_)
+        if (l.exponent_ > r.exponent_)
             return lneg;
-        if (x.exponent_ < y.exponent_)
+        if (l.exponent_ < r.exponent_)
             return !lneg;
 
-        // If equal exponents, compare mantissas
-        return x.mantissa_ < y.mantissa_;
+        // If equal signs and exponents, compare mantissas.
+        if (lneg)
+            // If negative, the operator is reversed.
+            return l.mantissa_ > r.mantissa_;
+
+        return l.mantissa_ < r.mantissa_;
     }
 
     /** Return the sign of the amount */

nix/ci-env.nix

@@ -43,6 +43,15 @@ let
     bintools = customBinutils;
   };
 
+  # gcov ships in gcc's `cc` output, but the cc-wrapper doesn't expose it.
+  # Surface the gcov from our rebuilt gcc (linked against the custom glibc, so
+  # it runs under the loader installed in the image) and matching the exact
+  # compiler version, so gcovr can produce coverage reports in the CI env.
+  customGcov = pkgs.runCommand "gcov-custom-for-ci-env" { } ''
+    mkdir -p "$out/bin"
+    ln -s "${customGccCc}/bin/gcov" "$out/bin/gcov"
+  '';
+
   # stdenv built around the rebuilt gcc / custom glibc. Used to rebuild
   # compiler-rt below so its sanitizer runtimes see the custom glibc
   # headers.
@@ -105,11 +114,16 @@ in
     name = "xrpld-ci-env";
     paths = commonPackages ++ [
       customGcc
+      customGcov
       customClangForCiEnv
       customBinutils
+      # CA certificate bundle so HTTPS clients (git, curl, conan) can verify
+      # TLS connections without ca-certificates being installed in the system.
+      pkgs.cacert
     ];
     pathsToLink = [
       "/bin"
+      "/etc/ssl/certs"
       "/lib"
       "/include"
       "/share"

nix/packages.nix

@@ -17,6 +17,7 @@ in
     llvmPackages_22.clang-tools
     less # needed for git diff
     mold
+    nettools # provides netstat, used to debug failures in CI
     ninja
     patchelf
     perl # needed for openssl

src/libxrpl/tx/transactors/nft/NFTokenCancelOffer.cpp

@@ -4,6 +4,7 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
+#include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
@@ -21,8 +22,14 @@ namespace xrpl {
 NotTEC
 NFTokenCancelOffer::preflight(PreflightContext const& ctx)
 {
-    if (auto const& ids = ctx.tx[sfNFTokenOffers];
-        ids.empty() || (ids.size() > kMaxTokenOfferCancelCount))
+    auto const& offerIds = ctx.tx[sfNFTokenOffers];
+
+    if (offerIds.empty() || (offerIds.size() > kMaxTokenOfferCancelCount))
+        return temMALFORMED;
+
+    // Zero offer IDs cannot be passed as ledger entry keys.
+    if (ctx.rules.enabled(fixCleanup3_2_0) &&
+        std::ranges::any_of(offerIds, [](uint256 const& id) { return id.isZero(); }))
         return temMALFORMED;
 
     // In order to prevent unnecessarily overlarge transactions, we

src/test/app/NFToken_test.cpp

@@ -892,6 +892,25 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite
             BEAST_EXPECT(ownerCount(env, buyer) == 1);
         }
 
+        // Only test this with fixCleanup3_2_0 enabled. Without the fix,
+        // an assert-enabled build can crash when Ledger::read() receives
+        // a zero-key offer ID.
+        if (features[fixCleanup3_2_0])
+        {
+            // Zero is not a valid offer ID.
+            env(token::cancelOffer(buyer, {uint256{}}), Ter(temMALFORMED));
+            env.close();
+            BEAST_EXPECT(ownerCount(env, buyer) == 1);
+
+            // List of offer IDs containing zero is invalid.
+            // craftedIndex is not a valid offer index but it is not zero.
+            auto const craftedIndex = keylet::nftoffer(gw, env.seq(gw)).key;
+            env(token::cancelOffer(buyer, {buyerOfferIndex, uint256{}, craftedIndex}),
+                Ter(temMALFORMED));
+            env.close();
+            BEAST_EXPECT(ownerCount(env, buyer) == 1);
+        }
+
         // List of tokens to delete is too long.
         {
             std::vector<uint256> const offers(kMaxTokenOfferCancelCount + 1, buyerOfferIndex);

src/test/basics/Number_test.cpp

@@ -10,6 +10,7 @@
 #include <boost/multiprecision/cpp_dec_float.hpp>
 #include <boost/multiprecision/number.hpp>
 
+#include <algorithm>
 #include <array>
 #include <cctype>
 #include <cstdint>
@@ -20,6 +21,7 @@
 #include <stdexcept>
 #include <string>
 #include <tuple>
+#include <utility>
 
 namespace xrpl {
 
@@ -1386,10 +1388,91 @@ public:
     testRelationals()
     {
         testcase << "test_relationals " << to_string(Number::getMantissaScale());
-        BEAST_EXPECT(!(Number{100} < Number{10}));
-        BEAST_EXPECT(Number{100} > Number{10});
-        BEAST_EXPECT(Number{100} >= Number{10});
-        BEAST_EXPECT(!(Number{100} <= Number{10}));
+
+        {
+            auto const nums = [this]() {
+                // Inequality test cases are built from a list of sorted integers
+                auto const values =
+                    std::to_array<int>({-100, -50, -20, -10, -1, 0, 1, 10, 20, 50, 100});
+                BEAST_EXPECT(std::ranges::is_sorted(values));
+
+                std::vector<Number> result;
+                result.reserve(values.size() + 1);
+                result.emplace_back(-1, 100);
+                for (auto const v : values)
+                {
+                    if (v == 0)
+                        result.emplace_back(-2, -10);
+                    result.emplace_back(v);
+                    if (v == 0)
+                        result.emplace_back(2, -10);
+                }
+                result.emplace_back(1, 100);
+                return result;
+            }();
+
+            BEAST_EXPECT(std::ranges::is_sorted(nums));
+
+            for (auto iter1 = nums.begin(); iter1 != nums.end(); ++iter1)
+            {
+                auto iter2 = iter1;
+                for (++iter2; iter2 != nums.end(); ++iter2)
+                {
+                    Number const& smaller = *iter1;
+                    Number const& larger = *iter2;
+                    std::stringstream ss;
+                    ss << smaller << " < " << larger;
+                    auto const str = ss.str();
+
+                    // The ==/!= operators use a completely different code path than <, etc.
+                    // This helps detect a breakage in one but not the other. It also helps
+                    // verify that the values are being ordered correctly.
+                    BEAST_EXPECTS(smaller != larger, str + " (!=)");
+                    BEAST_EXPECTS(!(smaller == larger), str + " (==)");
+
+                    // true results using operator< and derived operators
+                    BEAST_EXPECTS(smaller < larger, str + " (<)");
+                    BEAST_EXPECTS(larger > smaller, str + " (>)");
+                    BEAST_EXPECTS(larger >= smaller, str + " (>=)");
+                    BEAST_EXPECTS(smaller <= larger, str + " (<=)");
+
+                    // false results using operator< and derived operators
+                    BEAST_EXPECTS(!(larger < smaller), str + " (! <)");
+                    BEAST_EXPECTS(!(smaller > larger), str + " (! >)");
+                    BEAST_EXPECTS(!(smaller >= larger), str + " (! >=)");
+                    BEAST_EXPECTS(!(larger <= smaller), str + " (! <=)");
+                }
+            }
+        }
+
+        {
+            // Equality test cases are <Number, __LINE__>. Number will be compared against itself
+            using Case = std::pair<Number, int>;
+            auto const c = std::to_array<Case>({
+                {700, __LINE__},
+                {50, __LINE__},
+                {1, __LINE__},
+                {0, __LINE__},
+                {-1, __LINE__},
+                {-30, __LINE__},
+                {-600, __LINE__},
+            });
+            for (auto const& [n, line] : c)
+            {
+                auto const str = to_string(n);
+
+                // NOLINTBEGIN(misc-redundant-expression) Explicitly testing operators with
+                // equivalent values
+                expect(n == n, str + " ==", __FILE__, line);
+                expect(!(n != n), str + " !=", __FILE__, line);
+
+                expect(!(n < n), str + " < ", __FILE__, line);
+                expect(!(n > n), str + " >", __FILE__, line);
+                expect(n >= n, str + " >=", __FILE__, line);
+                expect(n <= n, str + " <=", __FILE__, line);
+                // NOLINTEND(misc-redundant-expression)
+            }
+        }
     }
 
     void