Merge branch 'develop' into ximinez/after-is-never-null

- Expand the description in InvariantChecker_PROTOTYPE::visitEntry.
- Add an explicit assertion in "XRPLNotCreated::visitEntry".

docker/check-tools.sh

@@ -9,6 +9,8 @@ clang-format --version
 cmake --version
 conan --version
 curl --version
+doxygen --version
+dpkg-buildpackage --version
 g++ --version
 gcc --version
 gcov --version
@@ -24,6 +26,7 @@ perl --version
 pkg-config --version
 pre-commit --version
 python3 --version
+rpmbuild --version
 run-clang-tidy --help
 vim --version
 

docker/install-sanitizer-libs.sh

@@ -27,63 +27,87 @@ fi
 echo "Detected OS: ${ID} ${VERSION_ID:-}"
 
 case "${ID}" in
-    debian)
-        apt-get update -y
-        apt-get install -y --no-install-recommends \
-            libasan8 \
-            libtsan2 \
-            libubsan1
-
-        apt-get clean
-        rm -rf /var/lib/apt/lists/*
+    ubuntu | debian | rhel | centos | rocky | almalinux)
+        echo "Supported OS detected: ${ID}"
         ;;
-
-    ubuntu)
-        apt-get update -y
-        apt-get install -y --no-install-recommends \
-            gnupg \
-            software-properties-common
-        add-apt-repository -y ppa:ubuntu-toolchain-r/test
-        apt-get update -y
-        apt-get install -y --no-install-recommends \
-            libasan8 \
-            libtsan2 \
-            libubsan1
-
-        apt-get clean
-        rm -rf /var/lib/apt/lists/*
-        ;;
-
-    rhel | centos | rocky | almalinux)
-        dnf install -y \
-            libasan8 \
-            libtsan2 \
-            libubsan
-
-        dnf clean -y all
-        rm -rf /var/cache/dnf/*
-        ;;
-
     *)
         echo "ERROR: unsupported OS '${ID}'. Supported: debian, ubuntu, rhel-family" >&2
         exit 1
         ;;
 esac
 
-# Verify that every expected library is now resolvable by the dynamic linker.
-missing=0
-for lib in libasan.so.8 libtsan.so.2 libubsan.so.1; do
-    if ldconfig -p | grep -q "${lib}"; then
-        echo "OK: ${lib} found"
-    else
-        echo "ERROR: ${lib} not found after installation" >&2
-        missing=$((missing + 1))
-    fi
-done
+function preinstall() {
+    case "${ID}" in
+        ubuntu)
+            apt-get update -y
+            apt-get install -y --no-install-recommends \
+                gnupg \
+                software-properties-common
+            add-apt-repository -y ppa:ubuntu-toolchain-r/test
+            ;;
+    esac
+}
 
-if [ "${missing}" -ne 0 ]; then
-    echo "ERROR: ${missing} library/libraries missing" >&2
-    exit 1
-fi
+function install() {
+    case "${ID}" in
+        debian | ubuntu)
+            apt-get update -y
+            apt-get install -y --no-install-recommends \
+                libasan8 \
+                libtsan2 \
+                libubsan1
+            ;;
+
+        rhel | centos | rocky | almalinux)
+            dnf install -y \
+                libasan8 \
+                libtsan2 \
+                libubsan
+            ;;
+    esac
+}
+
+function postinstall() {
+    # Don't clear cache in non-CI environments
+    if [ -z "${CI:-}" ]; then
+        echo "Not running in CI environment; skipping cache cleanup"
+        return
+    fi
+
+    case "${ID}" in
+        debian | ubuntu)
+            apt-get clean
+            rm -rf /var/lib/apt/lists/*
+            ;;
+
+        rhel | centos | rocky | almalinux)
+            dnf clean -y all
+            rm -rf /var/cache/dnf/*
+            ;;
+    esac
+}
+
+function verify() {
+    # Verify that every expected library is now resolvable by the dynamic linker.
+    missing=0
+    for lib in libasan.so.8 libtsan.so.2 libubsan.so.1; do
+        if ldconfig -p | grep -q "${lib}"; then
+            echo "OK: ${lib} found"
+        else
+            echo "ERROR: ${lib} not found after installation" >&2
+            missing=$((missing + 1))
+        fi
+    done
+
+    if [ "${missing}" -ne 0 ]; then
+        echo "ERROR: ${missing} library/libraries missing" >&2
+        exit 1
+    fi
+}
+
+preinstall
+install
+postinstall
+verify
 
 echo "All sanitizer runtime libraries installed successfully."

include/xrpl/tx/invariants/InvariantCheck.h

@@ -65,9 +65,13 @@ public:
     /**
      * @brief called for each ledger entry in the current transaction.
      *
-     * @param isDelete true if the SLE is being deleted
-     * @param before ledger entry before modification by the transaction
-     * @param after ledger entry after modification by the transaction
+     * @param isDelete true if the SLE is being deleted.
+     * @param before ledger entry before modification by the
+     *  transaction.
+     * @param after ledger entry after modification by the transaction.
+     *  `after` IS NEVER NULL. `isDelete` is the only correct way to check for deletions.
+     *  Check for null defensively, but do not make any logic decisions based on whether `after` is
+     *  set, because it will always be set.
      */
     void
     visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after);

nix/packages.nix

@@ -12,6 +12,8 @@ in
     cmake
     conan
     curlMinimal # needed for codecov/codecov-action
+    doxygen
+    dpkg # needed for dpkg-buildpackage
     gcovr
     git
     gnumake
@@ -26,6 +28,7 @@ in
     pkg-config
     pre-commit
     python3
+    rpm # needed for rpmbuild
     runClangTidy
     vim
   ];

src/libxrpl/protocol/STAmount.cpp

@@ -1250,30 +1250,16 @@ hasInvalidAmount(STBase const& field, int depth, beast::Journal j)
         return true;
     }
 
-    // Dispatch on the serialized type tag rather than RTTI: this is on the invariant-checking path
-    // and a dynamic_cast chain over every field of every modified entry is measurably expensive.
-    // The object-like tags below all denote STObject subclasses (STLedgerEntry, STTx), so the
-    // downcast is sound; nested fields are only ever plain STI_OBJECT / STI_ARRAY containers.
-    // safeDowncast keeps a dynamic_cast validity assert in debug builds while compiling to
-    // static_cast in release.
-    switch (field.getSType())
-    {
-        case STI_AMOUNT: {
-            auto const& amount = safeDowncast<STAmount const&>(field);
-            return !isLegalMPT(amount) || !isLegalNet(amount);
-        }
+    if (auto const amount = dynamic_cast<STAmount const*>(&field))
+        return !isLegalMPT(*amount) || !isLegalNet(*amount);
 
-        case STI_OBJECT:
-        case STI_LEDGERENTRY:
-        case STI_TRANSACTION:
-            return hasInvalidAmount(safeDowncast<STObject const&>(field), depth + 1, j);
+    if (auto const object = dynamic_cast<STObject const*>(&field))
+        return hasInvalidAmount(*object, depth + 1, j);
 
-        case STI_ARRAY:
-            return hasInvalidAmount(safeDowncast<STArray const&>(field), depth + 1, j);
+    if (auto const array = dynamic_cast<STArray const*>(&field))
+        return hasInvalidAmount(*array, depth + 1, j);
 
-        default:
-            return false;
-    }
+    return false;
 }
 
 bool

src/libxrpl/tx/invariants/InvariantCheck.cpp

@@ -135,24 +135,28 @@ XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a
         }
     }
 
-    if (after)
+    if (!after)
     {
-        switch (after->getType())
-        {
-            case ltACCOUNT_ROOT:
-                drops_ += (*after)[sfBalance].xrp().drops();
-                break;
-            case ltPAYCHAN:
-                if (!isDelete)
-                    drops_ += ((*after)[sfAmount] - (*after)[sfBalance]).xrp().drops();
-                break;
-            case ltESCROW:
-                if (!isDelete && isXRP((*after)[sfAmount]))
-                    drops_ += (*after)[sfAmount].xrp().drops();
-                break;
-            default:
-                break;
-        }
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::XRPNotCreated::visitEntry : after can't be null");
+        return;
+        // LCOV_EXCL_STOP
+    }
+    switch (after->getType())
+    {
+        case ltACCOUNT_ROOT:
+            drops_ += (*after)[sfBalance].xrp().drops();
+            break;
+        case ltPAYCHAN:
+            if (!isDelete)
+                drops_ += ((*after)[sfAmount] - (*after)[sfBalance]).xrp().drops();
+            break;
+        case ltESCROW:
+            if (!isDelete && isXRP((*after)[sfAmount]))
+                drops_ += (*after)[sfAmount].xrp().drops();
+            break;
+        default:
+            break;
     }
 }