From eae0a354153f63c7e3c5a188e8c0d157b49c97a1 Mon Sep 17 00:00:00 2001 From: Timothy Banks Date: Thu, 3 Sep 2026 17:06:27 -0400 Subject: [PATCH 01/16] fix: Use a hardened hash on the STPathElement --- .cspell.config.yaml | 1 + include/xrpl/protocol/PathAsset.h | 29 +++- include/xrpl/protocol/STPathSet.h | 161 +++++++++++++++++--- include/xrpl/protocol/detail/STVar.h | 3 +- src/libxrpl/protocol/STPathSet.cpp | 67 ++++++-- src/test/app/Path_test.cpp | 219 ++++++++++++++++++++++++++- src/xrpld/rpc/detail/Pathfinder.cpp | 31 ++-- src/xrpld/rpc/detail/Pathfinder.h | 2 +- 8 files changed, 452 insertions(+), 61 deletions(-) diff --git a/.cspell.config.yaml b/.cspell.config.yaml index c1af739255..a706c344de 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -141,6 +141,7 @@ words: - hwrap - ifndef - inequation + - Injectivity - insuf - insuff - invasively diff --git a/include/xrpl/protocol/PathAsset.h b/include/xrpl/protocol/PathAsset.h index ebf6fb68a4..02de9aa7df 100644 --- a/include/xrpl/protocol/PathAsset.h +++ b/include/xrpl/protocol/PathAsset.h @@ -5,9 +5,11 @@ #include #include +#include #include #include #include +#include #include namespace xrpl { @@ -121,9 +123,32 @@ operator==(PathAsset const& lhs, PathAsset const& rhs) template void -hash_append(Hasher& h, PathAsset const& pathAsset) +hash_append(Hasher& h, PathAsset const& pathAsset) noexcept { - std::visit([&](T const& e) { hash_append(h, e); }, pathAsset.value()); + using beast::hash_append; + using Variant = std::remove_cvref_t; + + static_assert( + std::variant_size_v < 0xFFu, + "PathAsset's discriminant must fit in a byte, leaving 0xFF reserved."); + + // std::visit is not noexcept: it throws bad_variant_access when the variant + // is valueless_by_exception. + if (pathAsset.value().valueless_by_exception()) [[unlikely]] + { + hash_append(h, static_cast(0xFFu)); + return; + } + + hash_append(h, static_cast(pathAsset.value().index())); + std::visit( + [&](T const& e) noexcept { + static_assert( + noexcept(hash_append(h, e)), + "Every PathAsset alternative must be nothrow-hashable."); + hash_append(h, e); + }, + pathAsset.value()); } inline bool diff --git a/include/xrpl/protocol/STPathSet.h b/include/xrpl/protocol/STPathSet.h index 5768721111..b91d899071 100644 --- a/include/xrpl/protocol/STPathSet.h +++ b/include/xrpl/protocol/STPathSet.h @@ -12,6 +12,8 @@ #include #include +#include +#include #include #include #include @@ -65,7 +67,7 @@ public: PathAsset const& asset, AccountID const& issuer); - [[nodiscard]] auto + [[nodiscard]] std::uint32_t getNodeType() const; [[nodiscard]] bool @@ -109,9 +111,6 @@ public: [[nodiscard]] bool isType(Type const& pe) const; - [[nodiscard]] size_t - getHash() const; - bool operator==(STPathElement const& t) const; @@ -120,6 +119,17 @@ private: getHash(STPathElement const& element); }; +template +void +hash_append(Hasher& h, STPathElement const& e) noexcept +{ + using beast::hash_append; + hash_append(h, (e.getNodeType() & STPathElement::TypeAccount) != 0u); + hash_append(h, e.getAccountID()); + hash_append(h, e.getPathAsset()); + hash_append(h, e.getIssuerID()); +} + class STPath final : public CountedObject { std::vector path_; @@ -176,9 +186,10 @@ template void hash_append(Hasher& h, STPath const& p) noexcept { + using beast::hash_append; for (auto const& e : p) { - beast::hash_append(h, e.getHash()); + hash_append(h, e); } } @@ -188,13 +199,39 @@ hash_append(Hasher& h, STPath const& p) noexcept class STPathSet final : public STBase, public CountedObject { std::vector value_; - xrpl::hardened_hash_set seenHashes_; + + /** + * Deduplication index over `value_`, for pathfinding. + * The use of a std::unique_ptr is intentional as it + * only requires 8 additional bytes of storage for the pointer + * as opposed to 64 bytes with an optional. This keeps the size + * of the STPathSet to within the `STVar::kMaxSize` limit of 72 bytes. + */ + std::unique_ptr> seen_; public: + struct DeduplicationTag + { + }; + STPathSet() = default; + /** + * Deduplication tagged constructor. + * Use when you want to ensure that the STPathSet does not contain duplicate paths. + */ + explicit STPathSet(DeduplicationTag); STPathSet(SField const& n); STPathSet(SerialIter& sit, SField const& name); + STPathSet(STPathSet const& other); + STPathSet(STPathSet&&) = default; + + STPathSet& + operator=(STPathSet const& other); + STPathSet& + operator=(STPathSet&&) = default; + + ~STPathSet() override = default; void add(Serializer& s) const override; @@ -204,6 +241,16 @@ public: [[nodiscard]] SerializedTypeID getSType() const override; + /** + * @brief assembleAdd adds a path to the set by combining a base path and a tail element. + * + * @param base The base path. + * @param tail The tail element. + * @return true if the path was added, false if it was a duplicate and not added. + * @remarks Requires the STPathSet to be constructed with the DeduplicationTag. The return value + * indicates whether the combined path was inserted (true) or rejected as a duplicate (false). + * It is fine for callers to ignore the return value. + */ bool assembleAdd(STPath const& base, STPathElement const& tail); @@ -229,22 +276,61 @@ public: [[nodiscard]] bool empty() const; - void + /** + * @brief pushBack adds a path to the set. + * + * @param e The path to add. + * @return true if the path was added, false if it was a duplicate and not added. + * @remarks If the STPathSet was constructed with the DeduplicationTag, then this method will + * check for duplicates and only add the path if it is not already present in the + * set. If the STPathSet was constructed without the DeduplicationTag, + * then this method will always add the path to the set, regardless of duplicates. + * It is fine for callers to ignore the return value. + */ + bool pushBack(STPath const& e); + /** + * @brief emplaceBack adds a path to the set. + * + * @param args The arguments to construct the path with. + * @return true if the path was added, false if it was a duplicate and not added. + * @remarks If the STPathSet was constructed with the DeduplicationTag, then this method will + * check for duplicates and only add the path if it is not already present in the + * set. If the STPathSet was constructed without the DeduplicationTag, + * then this method will always add the path to the set, regardless of duplicates. + * It is fine for callers to ignore the return value. + * @note The path is constructed before the duplicate check, so on a false + * return the constructed path is discarded and any argument + * forwarded as an rvalue is left in a moved-from state. Use + * pushBack when the caller needs to keep its path on rejection. + */ template - void + bool emplaceBack(Args&&... args); - [[nodiscard]] bool - contains(STPath const& path) const; - private: STBase* copy(std::size_t n, void* buf) const override; STBase* move(std::size_t n, void* buf) override; + /** + * @brief Append a path via `append`, then register it in the deduplication index. + * + * @param append Invoked with `value_`; must append exactly one path to it. + * @return true if the path was kept, false if it was a duplicate and was rolled back. + * @remarks Appends to the vector before touching the index, so that a failed allocation + * there leaves both containers untouched rather than leaving the index holding + * a path the vector does not. If the index insert reports a duplicate, or + * throws, the append is rolled back so the two containers stay consistent; in + * the throwing case the exception propagates. With no index (constructed + * without the DeduplicationTag) the append is unconditional. + */ + template + bool + appendUnique(Append&& append); + friend class detail::STVar; }; @@ -336,7 +422,7 @@ inline STPathElement::STPathElement( hashValue_ = getHash(*this); } -inline auto +inline std::uint32_t STPathElement::getNodeType() const { return type_; @@ -545,25 +631,50 @@ STPathSet::empty() const return value_.empty(); } -inline void -STPathSet::pushBack(STPath const& e) +template +inline bool +STPathSet::appendUnique(Append&& append) { - value_.push_back(e); - seenHashes_.emplace(value_.back()); -} + // Append to the vector first, so that a failed allocation there leaves both + // containers untouched rather than leaving the index holding a path the + // vector does not. + append(value_); -template -inline void -STPathSet::emplaceBack(Args&&... args) -{ - value_.emplace_back(std::forward(args)...); - seenHashes_.emplace(value_.back()); + if (seen_ == nullptr) + { + return true; + } + + try + { + if (!seen_->insert(value_.back()).second) + { + // Already present: roll back the append. + value_.pop_back(); + return false; + } + } + catch (...) + { + // The index insert failed, so roll back the append to keep the vector + // and the index consistent. + value_.pop_back(); + throw; + } + return true; } inline bool -STPathSet::contains(STPath const& path) const +STPathSet::pushBack(STPath const& e) { - return seenHashes_.contains(path); + return appendUnique([&](auto& value) { value.push_back(e); }); +} + +template +inline bool +STPathSet::emplaceBack(Args&&... args) +{ + return appendUnique([&](auto& value) { value.emplace_back(std::forward(args)...); }); } } // namespace xrpl diff --git a/include/xrpl/protocol/detail/STVar.h b/include/xrpl/protocol/detail/STVar.h index 56f868b665..72a310546e 100644 --- a/include/xrpl/protocol/detail/STVar.h +++ b/include/xrpl/protocol/detail/STVar.h @@ -34,10 +34,11 @@ concept ValidConstructSTArgs = // and includes a small-object allocation optimization. class STVar { -private: +public: // The largest "small object" we can accommodate static constexpr std::size_t kMaxSize = 72; +private: alignas(std::max_align_t) std::byte d_[kMaxSize] = {}; STBase* p_ = nullptr; diff --git a/src/libxrpl/protocol/STPathSet.cpp b/src/libxrpl/protocol/STPathSet.cpp index 658aaa65dd..2c074c3f2f 100644 --- a/src/libxrpl/protocol/STPathSet.cpp +++ b/src/libxrpl/protocol/STPathSet.cpp @@ -1,6 +1,8 @@ #include +#include #include +#include #include #include #include @@ -11,10 +13,12 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -31,6 +35,11 @@ STPathElement::getHash(STPathElement const& element) // NIKB NOTE: This doesn't have to be a secure hash as speed is more // important. We don't even really need to fully hash the whole // base_uint here, as a few bytes would do for our use. + // + // The note above is only true because the result of this function reaches + // nothing but STPathElement::operator==, where it is a fast-reject + // prefilter ahead of the field comparisons that decide the answer. Do not + // use it to key a container. for (auto const x : element.getAccountID()) hashAccount += (hashAccount * 257) ^ x; @@ -51,10 +60,49 @@ STPathElement::getHash(STPathElement const& element) return (hashAccount ^ hashCurrency ^ hashIssuer); } -[[nodiscard]] size_t -STPathElement::getHash() const +// For guidance on deciding which option to pursue: +// 1. Try to decrease the size of the STPathSet first. For instance, if a std::optional was +// injected into the type, could you get the same functionality using a std::unique_ptr instead? +// 2. If the size of the STPathSet is already as small as it can be, then consider what the cost +// of increasing STVar::kMaxSize would be on all the other STVar types. Each of those types +// will carry the additional cost of accommodating the larger STPathSet in their SBO. +// 3. If the cost of increasing STVar::kMaxSize is too high, then heap allocate the STPathSet and +// remove this static_assert. +static_assert( + sizeof(STPathSet) <= detail::STVar::kMaxSize, + "STPathSet is too large to fit in STVar's small object optimization. Please verify if it " + "should, if the kMaxSize should be increased, or if STPathSet should be stored on the heap " + "instead of in STVar."); + +STPathSet::STPathSet(DeduplicationTag) : seen_{std::make_unique>()} { - return STPathElement::getHash(*this); +} + +STPathSet::STPathSet(STPathSet const& other) + : STBase{other} + , CountedObject{other} + , value_{other.value_} + , seen_{ + other.seen_ != nullptr ? std::make_unique>(*other.seen_) + : nullptr} +{ +} + +STPathSet& +STPathSet::operator=(STPathSet const& other) +{ + if (this == &other) + { + return *this; + } + auto newSeen = other.seen_ != nullptr + ? std::make_unique>(*other.seen_) + : nullptr; + STBase::operator=(other); + CountedObject::operator=(other); + value_ = other.value_; + seen_ = std::move(newSeen); + return *this; } STPathSet::STPathSet(SerialIter& sit, SField const& name) : STBase(name) @@ -72,7 +120,8 @@ STPathSet::STPathSet(SerialIter& sit, SField const& name) : STBase(name) Throw("empty path"); } - pushBack(path); + // Move rather than converting the vector to an STPath by copy. + value_.emplace_back(std::move(path)); path.clear(); if (iType == STPathElement::TypeNone) @@ -132,16 +181,10 @@ STPathSet::move(std::size_t n, void* buf) bool STPathSet::assembleAdd(STPath const& base, STPathElement const& tail) { // assemble base+tail and add it to the set if it's not a duplicate + XRPL_ASSERT(seen_ != nullptr, "xrpl::STPathSet::assembleAdd : DeduplicationTag"); STPath combined = base; combined.pushBack(tail); - - if (!seenHashes_.insert(combined).second) - { - return false; - } - - value_.push_back(std::move(combined)); - return true; + return appendUnique([&](auto& value) { value.push_back(std::move(combined)); }); } bool diff --git a/src/test/app/Path_test.cpp b/src/test/app/Path_test.cpp index cd61668b03..5ecad1a420 100644 --- a/src/test/app/Path_test.cpp +++ b/src/test/app/Path_test.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -46,16 +47,20 @@ #include #include +#include #include #include +#include #include #include #include #include +#include #include #include #include #include +#include namespace xrpl::test { @@ -1943,7 +1948,7 @@ public: static constexpr AccountID kAccountID7{kAccount7}; static constexpr AccountID kAccountID8{kAccount8}; - auto ps = STPathSet{}; + auto ps = STPathSet{STPathSet::DeduplicationTag{}}; auto createPathElements = [](auto const& account1, auto const& account2) { auto base = STPath{}; @@ -2017,6 +2022,215 @@ public: BEAST_EXPECT(ps.size() == 6); } + void + testPushBackDeduplication() + { + testcase("STPathSet::pushBack/emplaceBack deduplication"); + + // pushBack and emplaceBack reject duplicates on a set built with the + // DeduplicationTag, and append unconditionally without it. Both + // report which happened. The unconditional case is the one the wire + // and JSON paths rely on: collapsing duplicates there would change the + // signed content of a transaction. + + static constexpr AccountID kAccountID1{"A3F19C7B2E5D08146FB93A7C0E2D5184BC6F3A09"}; + static constexpr AccountID kAccountID2{"1D7E4B90C2A6F3851E0B9D47A2C5F8136E0A4B7D"}; + static constexpr AccountID kAccountID3{"F08C36A1D95E27B40CA1F63E8D204B7950E1C3A6"}; + + auto makePath = [](AccountID const& account) { + auto p = STPath{}; + p.pushBack(STPathElement{STPathElement::TypeAccount, account, xrpCurrency(), account}); + return p; + }; + + auto const first = makePath(kAccountID1); + auto const second = makePath(kAccountID2); + auto const third = makePath(kAccountID3); + + // Deduplicating set: the second insert of a path is rejected, and the + // rejection is reported rather than silently swallowed. + { + auto ps = STPathSet{STPathSet::DeduplicationTag{}}; + + BEAST_EXPECT(ps.pushBack(first)); + BEAST_EXPECT(ps.size() == 1); + + BEAST_EXPECT(!ps.pushBack(first)); + BEAST_EXPECT(ps.size() == 1); + + // emplaceBack sees paths registered by pushBack... + BEAST_EXPECT(!ps.emplaceBack(first)); + BEAST_EXPECT(ps.size() == 1); + + BEAST_EXPECT(ps.emplaceBack(second)); + BEAST_EXPECT(ps.size() == 2); + + // ...and pushBack sees paths registered by emplaceBack. + BEAST_EXPECT(!ps.pushBack(second)); + BEAST_EXPECT(ps.size() == 2); + + // emplaceBack's forwarding form registers the same way. + BEAST_EXPECT(ps.emplaceBack(std::vector{third.front()})); + BEAST_EXPECT(ps.size() == 3); + BEAST_EXPECT(!ps.pushBack(third)); + BEAST_EXPECT(ps.size() == 3); + + // A rejected duplicate must not disturb what is already stored. + BEAST_EXPECT(ps[0] == first); + BEAST_EXPECT(ps[1] == second); + BEAST_EXPECT(ps[2] == third); + } + + // Without the tag there is no index, so duplicates are appended and + // both methods report success every time. + { + auto plain = STPathSet{}; + BEAST_EXPECT(plain.pushBack(first)); + BEAST_EXPECT(plain.pushBack(first)); + BEAST_EXPECT(plain.emplaceBack(first)); + BEAST_EXPECT(plain.size() == 3); + + auto named = STPathSet{sfPaths}; + BEAST_EXPECT(named.pushBack(first)); + BEAST_EXPECT(named.pushBack(first)); + BEAST_EXPECT(named.size() == 2); + } + } + + void + testPathHashInjectivity() + { + testcase("STPathElement hash injectivity"); + + auto const zeroCurrency = + STPathElement{AccountID{}, PathAsset{Currency{}}, AccountID{}, true}; + auto const zeroMPT = STPathElement{AccountID{}, PathAsset{MPTID{}}, AccountID{}, true}; + + BEAST_EXPECT(!(zeroCurrency == zeroMPT)); + + auto path = [](std::vector const& elements) { + auto p = STPath{}; + for (auto const& element : elements) + p.pushBack(element); + return p; + }; + + auto const currencyFirst = path({zeroCurrency, zeroMPT}); + auto const mptFirst = path({zeroMPT, zeroCurrency}); + + BEAST_EXPECT(!(currencyFirst == mptFirst)); + + auto const hasher = HardenedHash<>{}; + BEAST_EXPECT(hasher(currencyFirst) != hasher(mptFirst)); + + auto mask = std::vector{0, 0, 1, 1}; + auto hashes = std::set{}; + auto orderings = 0uz; + do + { + auto elements = std::vector{}; + for (auto const isMPT : mask) + { + elements.push_back(isMPT != 0 ? zeroMPT : zeroCurrency); + } + hashes.insert(hasher(path(elements))); + ++orderings; + } while (std::ranges::next_permutation(mask).found); + + BEAST_EXPECT(orderings == 6); + BEAST_EXPECT(hashes.size() == orderings); + + auto seen = hardened_hash_set{}; + for (auto const& p : {currencyFirst, mptFirst}) + { + seen.emplace(p); + } + BEAST_EXPECT(seen.size() == 2); + + // The other half of the invariant: equal elements must hash equally. + // STPathElement::operator== masks type_ down to the TypeAccount bit, so + // elements whose remaining type bits differ still compare equal -- + // hashing the full type_ would give them distinct hashes and silently + // defeat deduplication. + static constexpr AccountID kAccount{"A3F19C7B2E5D08146FB93A7C0E2D5184BC6F3A09"}; + static constexpr AccountID kIssuer{"1D7E4B90C2A6F3851E0B9D47A2C5F8136E0A4B7D"}; + + auto const equivalent = std::vector>{ + // forceAsset toggles TypeCurrency on an XRP asset. + {STPathElement{kAccount, PathAsset{xrpCurrency()}, kIssuer, true}, + STPathElement{kAccount, PathAsset{xrpCurrency()}, kIssuer, false}}, + // An explicit type mask vs. one derived from the populated fields. + {STPathElement{STPathElement::TypeAccount, kAccount, xrpCurrency(), kIssuer}, + STPathElement{kAccount, PathAsset{xrpCurrency()}, kIssuer, false}}, + }; + + for (auto const& [lhs, rhs] : equivalent) + { + BEAST_EXPECT(lhs.getNodeType() != rhs.getNodeType()); + BEAST_EXPECT(lhs == rhs); + + auto const lhsPath = path({lhs}); + auto const rhsPath = path({rhs}); + BEAST_EXPECT(hasher(lhsPath) == hasher(rhsPath)); + + auto equal = hardened_hash_set{}; + equal.emplace(lhsPath); + equal.emplace(rhsPath); + BEAST_EXPECT(equal.size() == 1); + } + } + + void + testDeserializationPreservesDuplicates() + { + testcase("STPathSet deserialization preserves duplicate paths"); + + // The `Paths` field of a signed transaction must round-trip byte for + // byte. The deduplication index exists solely for pathfinding, so the + // deserializing constructor must never engage it: collapsing duplicates + // on parse would silently change the signed content of a transaction. + + static constexpr AccountID kAccountID1{"A3F19C7B2E5D08146FB93A7C0E2D5184BC6F3A09"}; + static constexpr AccountID kAccountID2{"1D7E4B90C2A6F3851E0B9D47A2C5F8136E0A4B7D"}; + + auto const element = + STPathElement{kAccountID1, PathAsset{xrpCurrency()}, kAccountID2, true}; + + auto path = STPath{}; + path.pushBack(element); + + static constexpr auto kDuplicates = 64uz; + + auto original = STPathSet{sfPaths}; + for (auto i = 0uz; i < kDuplicates; ++i) + { + original.pushBack(path); + } + + // No index was requested, so nothing is deduplicated on the way in. + BEAST_EXPECT(original.size() == kDuplicates); + + auto s = Serializer{}; + original.add(s); + + auto sit = SerialIter{s.slice()}; + auto const parsed = STPathSet{sit, sfPaths}; + + // The duplicates survive the round trip... + BEAST_EXPECT(parsed.size() == kDuplicates); + BEAST_EXPECT(parsed.isEquivalent(original)); + + // ...and re-serializing reproduces the original bytes exactly. + auto serialized = Serializer{}; + parsed.add(serialized); + BEAST_EXPECT(serialized.getData() == s.getData()); + + // A parsed set holds no index, so appending to it stays append-only. + auto appended = parsed; + appended.pushBack(path); + BEAST_EXPECT(appended.size() == kDuplicates + 1); + } + void run() override { @@ -2031,6 +2245,9 @@ public: issuesPathNegativeRippleClientIssue23Larger(); qualityPathsQualitySetAndTest(); testAssembleAddDeduplication(); + testPushBackDeduplication(); + testPathHashInjectivity(); + testDeserializationPreservesDuplicates(); trustAutoClearTrustNormalClear(); trustAutoClearTrustAutoClear(); norippleCombinations(); diff --git a/src/xrpld/rpc/detail/Pathfinder.cpp b/src/xrpld/rpc/detail/Pathfinder.cpp index 1f530a1165..c3e74fa4eb 100644 --- a/src/xrpld/rpc/detail/Pathfinder.cpp +++ b/src/xrpld/rpc/detail/Pathfinder.cpp @@ -862,10 +862,11 @@ Pathfinder::addPathsForType( return it->second; // Otherwise, if the type has no nodes, return the empty path. - if (pathType.empty()) - return paths_[pathType]; - if (continueCallback && !continueCallback()) - return paths_[{}]; + if (pathType.empty() || (continueCallback && !continueCallback())) + { + static auto const kEmptyPath = PathType{}; + return paths_.try_emplace(kEmptyPath, STPathSet::DeduplicationTag{}).first->second; + } // Otherwise, get the paths for the parent PathType by calling // addPathsForType recursively. @@ -873,7 +874,7 @@ Pathfinder::addPathsForType( parentPathType.pop_back(); STPathSet const& parentPaths = addPathsForType(parentPathType, continueCallback); - STPathSet& pathsOut = paths_[pathType]; + STPathSet& pathsOut = paths_.try_emplace(pathType, STPathSet::DeduplicationTag{}).first->second; JLOG(j_.debug()) << "getPaths< adding onto '" << pathTypeToString(parentPathType) << "' to get '" << pathTypeToString(pathType) << "'"; @@ -959,15 +960,6 @@ Pathfinder::isNoRippleOut(STPath const& currentPath) return endElement.hasCurrency() && isNoRipple(fromAccount, toAccount, endElement.getCurrency()); } -void -addUniquePath(STPathSet& pathSet, STPath const& path) -{ - if (!pathSet.contains(path)) - { - pathSet.pushBack(path); - } -} - void Pathfinder::addLink( STPath const& currentPath, // The path to build from @@ -999,7 +991,7 @@ Pathfinder::addLink( { // non-default path to XRP destination JLOG(j_.trace()) << "complete path found ax: " << currentPath.getJson(JsonOptions::Values::None); - addUniquePath(completePaths_, currentPath); + completePaths_.pushBack(currentPath); } } else @@ -1107,7 +1099,7 @@ Pathfinder::addLink( JLOG(j_.trace()) << "complete path found ae: " << currentPath.getJson(JsonOptions::Values::None); - addUniquePath(completePaths_, currentPath); + completePaths_.pushBack(currentPath); } } else if (!bDestOnly) @@ -1237,11 +1229,12 @@ Pathfinder::addLink( // complete JLOG(j_.trace()) << "complete path found bx: " << currentPath.getJson(JsonOptions::Values::None); - addUniquePath(completePaths_, newPath); + completePaths_.pushBack(newPath); } else { - incompletePaths.pushBack(newPath); + [[maybe_unused]] auto result = incompletePaths.pushBack(newPath); + XRPL_ASSERT(result, "xrpl::Pathfinder::addLink : unique path"); } } else if (!currentPath.hasSeen( @@ -1283,7 +1276,7 @@ Pathfinder::addLink( // complete JLOG(j_.trace()) << "complete path found ba: " << currentPath.getJson(JsonOptions::Values::None); - addUniquePath(completePaths_, newPath); + completePaths_.pushBack(newPath); } else { diff --git a/src/xrpld/rpc/detail/Pathfinder.h b/src/xrpld/rpc/detail/Pathfinder.h index aeacd218d2..0b4da6abde 100644 --- a/src/xrpld/rpc/detail/Pathfinder.h +++ b/src/xrpld/rpc/detail/Pathfinder.h @@ -207,7 +207,7 @@ private: std::shared_ptr rLCache_; STPathElement source_; - STPathSet completePaths_; + STPathSet completePaths_{STPathSet::DeduplicationTag{}}; std::vector pathRanks_; std::map paths_; From ea6226b8b9fa2f60e608b6314312d0fb894642dc Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 4 Sep 2026 06:17:43 -0400 Subject: [PATCH 02/16] fix: Prevent `simulate` from updating the orderbook db --- include/xrpl/tx/ApplyContext.h | 9 +++++++++ src/libxrpl/tx/ApplyContext.cpp | 9 +++++++++ src/libxrpl/tx/transactors/dex/AMMCreate.cpp | 4 +--- src/libxrpl/tx/transactors/dex/OfferCreate.cpp | 5 ++--- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/include/xrpl/tx/ApplyContext.h b/include/xrpl/tx/ApplyContext.h index e827e69f01..7be7f34b0b 100644 --- a/include/xrpl/tx/ApplyContext.h +++ b/include/xrpl/tx/ApplyContext.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -129,6 +130,14 @@ public: view_->rawDestroyXRP(fee); } + /** + * Registers a newly-created order book directory with the shared, + * process-wide OrderBookDB, unless this transaction is being applied + * under TapDryRun. + */ + void + addOrderBook(Book const& book); + ApplyViewContext getApplyViewContext() { diff --git a/src/libxrpl/tx/ApplyContext.cpp b/src/libxrpl/tx/ApplyContext.cpp index 50f46fceef..96dcd5f587 100644 --- a/src/libxrpl/tx/ApplyContext.cpp +++ b/src/libxrpl/tx/ApplyContext.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include #include #include @@ -54,6 +56,13 @@ ApplyContext::apply(TER ter) return view_->apply(base_, tx, ter, parentBatchId_, (flags_ & TapDryRun) != 0u, journal); } +void +ApplyContext::addOrderBook(Book const& book) +{ + if ((flags_ & TapDryRun) == TapNone) + registry.get().getOrderBookDB().addOrderBook(book); +} + std::size_t ApplyContext::size() { diff --git a/src/libxrpl/tx/transactors/dex/AMMCreate.cpp b/src/libxrpl/tx/transactors/dex/AMMCreate.cpp index 7c7d35497a..2bc9aa5ba1 100644 --- a/src/libxrpl/tx/transactors/dex/AMMCreate.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMCreate.cpp @@ -2,8 +2,6 @@ #include #include -#include -#include #include #include #include @@ -397,7 +395,7 @@ applyCreate(ApplyContext& ctx, Sandbox& sb, AccountID const& account, beast::Jou Book const book{assetIn, assetOut, std::nullopt}; auto const dir = keylet::quality(keylet::book(book), uRate); if (auto const bookExisted = static_cast(sb.read(dir)); !bookExisted) - ctx.registry.get().getOrderBookDB().addOrderBook(book); + ctx.addOrderBook(book); }; addOrderBook(amount.asset(), amount2.asset(), getRate(amount2, amount)); addOrderBook(amount2.asset(), amount.asset(), getRate(amount, amount2)); diff --git a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp index 57ba6eff0d..6c3c04f1a0 100644 --- a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp +++ b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -634,7 +633,7 @@ OfferCreate::applyHybrid( bookArr.pushBack(std::move(bookInfo)); if (!bookExists) - ctx_.registry.get().getOrderBookDB().addOrderBook(book); + ctx_.addOrderBook(book); sleOffer->setFieldArray(sfAdditionalBooks, bookArr); return tesSUCCESS; @@ -1014,7 +1013,7 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel) sb.insert(sleOffer); if (!bookExisted) - ctx_.registry.get().getOrderBookDB().addOrderBook(book); + ctx_.addOrderBook(book); JLOG(j_.debug()) << "final result: success"; From 0db7b766e698c027abee2918d0ec23411af72e0c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 4 Sep 2026 06:32:08 -0400 Subject: [PATCH 03/16] fix: Trim unknown fields when parsing incoming peer protobuf messages --- src/xrpld/overlay/detail/ProtocolMessage.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/xrpld/overlay/detail/ProtocolMessage.h b/src/xrpld/overlay/detail/ProtocolMessage.h index 88f50e1e2e..9c9b3e0138 100644 --- a/src/xrpld/overlay/detail/ProtocolMessage.h +++ b/src/xrpld/overlay/detail/ProtocolMessage.h @@ -278,6 +278,8 @@ parseMessageContent(MessageHeader const& header, Buffers const& buffers) return {}; } + m->DiscardUnknownFields(); + return m; } From 6099940c2cbb3b07958832c0be0f7c7c782ae671 Mon Sep 17 00:00:00 2001 From: Timothy Banks Date: Fri, 4 Sep 2026 10:32:36 -0400 Subject: [PATCH 04/16] fix: Unbounded Database Seek via TMGetLedger --- .../xrpl/nodestore/detail/DatabaseNodeImp.h | 2 +- src/test/overlay/PeerTest.cpp | 133 +++++++++++++++++ src/test/overlay/PeerTest.h | 94 ++++++++++++ src/test/overlay/TMGetLedger_test.cpp | 135 ++++++++++++++++++ src/test/overlay/TMGetObjectByHash_test.cpp | 116 +-------------- src/xrpld/overlay/detail/PeerImp.cpp | 17 ++- src/xrpld/overlay/detail/PeerImp.h | 9 +- 7 files changed, 381 insertions(+), 125 deletions(-) create mode 100644 src/test/overlay/PeerTest.cpp create mode 100644 src/test/overlay/PeerTest.h create mode 100644 src/test/overlay/TMGetLedger_test.cpp diff --git a/include/xrpl/nodestore/detail/DatabaseNodeImp.h b/include/xrpl/nodestore/detail/DatabaseNodeImp.h index 33a2e27939..9ba81a7323 100644 --- a/include/xrpl/nodestore/detail/DatabaseNodeImp.h +++ b/include/xrpl/nodestore/detail/DatabaseNodeImp.h @@ -92,7 +92,7 @@ public: void importDatabase(Database& source) override { - importInternal(*backend_.get(), source); + importInternal(*backend_, source); } void diff --git a/src/test/overlay/PeerTest.cpp b/src/test/overlay/PeerTest.cpp new file mode 100644 index 0000000000..6ca6db384b --- /dev/null +++ b/src/test/overlay/PeerTest.cpp @@ -0,0 +1,133 @@ +#include + +#include + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +namespace xrpl::test { + +PeerTest::PeerTest( + Application& app, + std::shared_ptr const& slot, + http_request_type&& request, + PublicKey const& publicKey, + ProtocolVersion protocol, + resource::Consumer consumer, + std::unique_ptr&& streamPtr, + OverlayImpl& overlay) + : PeerImp{ + app, + id++, + slot, + std::move(request), + publicKey, + protocol, + consumer, + std::move(streamPtr), + overlay} +{ +} + +void +PeerTest::run() +{ +} + +void +PeerTest::send(std::shared_ptr const& message) +{ + lastSentMessage_ = message; +} + +std::shared_ptr +PeerTest::getLastSentMessage() const +{ + return lastSentMessage_; +} + +void +PeerTest::runProcessGetObjectByHash(std::shared_ptr const& message) +{ + PeerImp::processGetObjectByHash(message); +} + +void +PeerTest::runProcessLedgerRequest( + std::shared_ptr const& message, + std::vector nodeIDs) +{ + PeerImp::processLedgerRequest(message, std::move(nodeIDs)); +} + +resource::Charge +PeerTest::getCurrentFeeCharge() const +{ + return PeerImp::currentFeeCharge(); +} + +void +PeerTest::resetId() +{ + id = 0; +} + +std::shared_ptr +makePeerTest(jtx::Env& env, PeerTest::SharedContext const& context, ProtocolVersion protocolVersion) +{ + using SocketType = boost::asio::ip::tcp::socket; + + auto& overlay = dynamic_cast(env.app().getOverlay()); + boost::beast::http::request request; + auto streamPtr = + std::make_unique(SocketType(env.app().getIOContext()), *context); + + beast::ip::Endpoint const local(boost::asio::ip::make_address("172.1.1.1"), 51235); + beast::ip::Endpoint const remote(boost::asio::ip::make_address("172.1.1.2"), 51235); + + PublicKey const key{std::get<0>(randomKeyPair(KeyType::Ed25519))}; + auto consumer = overlay.resourceManager().newInboundEndpoint(remote); + auto [slot, _] = overlay.peerFinder().newInboundSlot(local, remote); + + auto peer = std::make_shared( + env.app(), + slot, + std::move(request), + key, + protocolVersion, + consumer, + std::move(streamPtr), + overlay); + + overlay.addActive(peer); + return peer; +} + +} // namespace xrpl::test diff --git a/src/test/overlay/PeerTest.h b/src/test/overlay/PeerTest.h new file mode 100644 index 0000000000..a081399bac --- /dev/null +++ b/src/test/overlay/PeerTest.h @@ -0,0 +1,94 @@ +#pragma once + +#include + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include + +namespace xrpl::test { + +/** + * Test peer that captures sent messages for verification. + */ +class PeerTest : public PeerImp +{ +private: + inline static Peer::id_t id{}; + std::shared_ptr lastSentMessage_; + +public: + using MiddleType = boost::beast::tcp_stream; + using SharedContext = std::shared_ptr; + using StreamType = boost::beast::ssl_stream; + + PeerTest( + Application& app, + std::shared_ptr const& slot, + http_request_type&& request, + PublicKey const& publicKey, + ProtocolVersion protocol, + resource::Consumer consumer, + std::unique_ptr&& streamPtr, + OverlayImpl& overlay); + + ~PeerTest() override = default; + + void + run() override; + + void + send(std::shared_ptr const& m) override; + + std::shared_ptr + getLastSentMessage() const; + + // Synchronous test access to the JobQueue-dispatched processor. + // The production path runs this on JtLedgerReq; tests need a + // synchronous entry point to inspect the reply via send(). + // PeerImp::processGetObjectByHash is `protected` so the derived + // test subclass can call it directly. + void + runProcessGetObjectByHash(std::shared_ptr const& m); + + void + runProcessLedgerRequest( + std::shared_ptr const& m, + std::vector nodeIDs); + + resource::Charge + getCurrentFeeCharge() const; + + static void + resetId(); +}; + +std::shared_ptr +makePeerTest( + jtx::Env& env, + PeerTest::SharedContext const& context, + ProtocolVersion protocolVersion); + +} // namespace xrpl::test diff --git a/src/test/overlay/TMGetLedger_test.cpp b/src/test/overlay/TMGetLedger_test.cpp new file mode 100644 index 0000000000..2f072e0efe --- /dev/null +++ b/src/test/overlay/TMGetLedger_test.cpp @@ -0,0 +1,135 @@ +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +namespace xrpl::test { + +using namespace jtx; + +class TMGetLedger_test : public beast::unit_test::Suite +{ + PeerTest::SharedContext context_{makeSslContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + // Build a well-formed TMGetLedger node request carrying `numNodeIds` node + // IDs. + static std::shared_ptr + createRequest(std::size_t const numNodeIds) + { + auto request = std::make_shared(); + request->set_itype(protocol::liTX_NODE); + + // A uint256-sized ledger hash so the request passes the earlier + // structural validation and reaches the node-ID checks. + uint256 const ledgerHash{1}; + request->set_ledgerhash(ledgerHash.data(), ledgerHash.size()); + + // Valid, deserializable SHAMap node IDs (the root node ID, repeated). + // The count is what the hard bound cares about. + auto const rootNodeId = SHAMapNodeID{}.getRawString(); + for (std::size_t i = 0; i < numNodeIds; ++i) + { + request->add_nodeids(rootNodeId); + } + + return request; + } + + void + testNodeIdHardLimit(std::size_t const numNodeIds, bool const expectRejected) + { + testcase("Node ID Hard Limit"); + + Env env{*this}; + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + peer->onMessage(createRequest(numNodeIds)); + + // An over-limit request is charged kFeeInvalidData; an in-limit request should not be. + // The JobQueue handler may run concurrently and update the fee for in-limit requests. + BEAST_EXPECT( + expectRejected ? (peer->getCurrentFeeCharge() == resource::kFeeInvalidData) + : !(peer->getCurrentFeeCharge() == resource::kFeeInvalidData)); + } + + void + testProcessLedgerRequestReplyCapped(std::size_t const numNodeIds) + { + testcase("Process Ledger Request Reply Capped"); + + Env env{*this}; + env.close(); + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + + // Request the account-state root node of the closed ledger, asking + // for far more node IDs than the hard bound allows. + auto request = createRequest(numNodeIds); + request->clear_ledgerhash(); + request->set_itype(protocol::liAS_NODE); + request->set_ltype(protocol::ltCLOSED); + + peer->runProcessLedgerRequest(request, std::vector(numNodeIds)); + + auto sentMessage = peer->getLastSentMessage(); + BEAST_EXPECT(sentMessage != nullptr); + if (!sentMessage) + { + return; + } + + auto const& buffer = sentMessage->getBuffer(compression::Compressed::Off); + BEAST_EXPECT(buffer.size() > 6); + + // Skip the message header (6 bytes: 4 for size, 2 for type). + protocol::TMLedgerData reply; + BEAST_EXPECT(reply.ParseFromArray(buffer.data() + 6, buffer.size() - 6) == true); + + BEAST_EXPECT(reply.type() == protocol::liAS_NODE); + BEAST_EXPECT(reply.nodes_size() > 0); + BEAST_EXPECT(reply.nodes_size() <= static_cast(tuning::kHardMaxReplyNodes)); + } + + void + run() override + { + auto const limit = static_cast(tuning::kHardMaxReplyNodes); + testNodeIdHardLimit(limit + 1, true); + testNodeIdHardLimit(limit, false); + testNodeIdHardLimit(limit - 1, false); + testProcessLedgerRequestReplyCapped(limit + 1); + testProcessLedgerRequestReplyCapped(limit); + testProcessLedgerRequestReplyCapped(limit - 1); + } +}; + +BEAST_DEFINE_TESTSUITE(TMGetLedger, overlay, xrpl); + +} // namespace xrpl::test diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp index 6c9105164a..7c614d447c 100644 --- a/src/test/overlay/TMGetObjectByHash_test.cpp +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -1,9 +1,8 @@ #include +#include #include #include -#include -#include #include #include #include @@ -12,16 +11,9 @@ #include #include #include -#include #include #include -#include -#include -#include -#include #include -#include -#include #include #include @@ -49,111 +41,9 @@ using namespace jtx; */ class TMGetObjectByHash_test : public beast::unit_test::Suite { - using middle_type = boost::beast::tcp_stream; - using stream_type = boost::beast::ssl_stream; - using socket_type = boost::asio::ip::tcp::socket; - using shared_context = std::shared_ptr; - /** - * Test peer that captures sent messages for verification. - */ - class PeerTest : public PeerImp - { - public: - PeerTest( - Application& app, - std::shared_ptr const& slot, - http_request_type&& request, - PublicKey const& publicKey, - ProtocolVersion protocol, - resource::Consumer consumer, - std::unique_ptr&& streamPtr, - OverlayImpl& overlay) - : PeerImp( - app, - id++, - slot, - std::move(request), - publicKey, - protocol, - consumer, - std::move(streamPtr), - overlay) - { - } - - ~PeerTest() override = default; - - void - run() override - { - } - - void - send(std::shared_ptr const& m) override - { - lastSentMessage_ = m; - } - - std::shared_ptr - getLastSentMessage() const - { - return lastSentMessage_; - } - - // Synchronous test access to the JobQueue-dispatched processor. - // The production path runs this on JtLedgerReq; tests need a - // synchronous entry point to inspect the reply via send(). - // PeerImp::processGetObjectByHash is `protected` so the derived - // test subclass can call it directly. - void - runProcessGetObjectByHash(std::shared_ptr const& m) - { - processGetObjectByHash(m); - } - - static void - resetId() - { - id = 0; - } - - private: - inline static Peer::id_t id = 0; - std::shared_ptr lastSentMessage_; - }; - - shared_context context_{makeSslContext("")}; + PeerTest::SharedContext context_{makeSslContext("")}; ProtocolVersion protocolVersion_{1, 7}; - std::shared_ptr - createPeer(jtx::Env& env) - { - auto& overlay = dynamic_cast(env.app().getOverlay()); - boost::beast::http::request request; - auto streamPtr = - std::make_unique(socket_type(env.app().getIOContext()), *context_); - - beast::ip::Endpoint const local(boost::asio::ip::make_address("172.1.1.1"), 51235); - beast::ip::Endpoint const remote(boost::asio::ip::make_address("172.1.1.2"), 51235); - - PublicKey const key(std::get<0>(randomKeyPair(KeyType::Ed25519))); - auto consumer = overlay.resourceManager().newInboundEndpoint(remote); - auto [slot, _] = overlay.peerFinder().newInboundSlot(local, remote); - - auto peer = std::make_shared( - env.app(), - slot, - std::move(request), - key, - protocolVersion_, - consumer, - std::move(streamPtr), - overlay); - - overlay.addActive(peer); - return peer; - } - static std::shared_ptr createRequest(size_t const numObjects, Env& env) { @@ -203,7 +93,7 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite Env env(*this); PeerTest::resetId(); - auto peer = createPeer(env); + auto peer = makePeerTest(env, context_, protocolVersion_); auto request = createRequest(numObjects, env); peer->runProcessGetObjectByHash(request); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index c56ea5797f..eddeb01897 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1487,10 +1487,21 @@ PeerImp::onMessage(std::shared_ptr const& m) // Verify ledger node counts. Full parsing of the node IDs is deferred to the job, so the I/O // thread is not burdened with SHAMapNodeID deserialization for every TMGetLedger message. - if (itype != protocol::liBASE && m->nodeids_size() <= 0) + if (itype != protocol::liBASE) { - badData("Invalid ledger node IDs"); - return; + if (m->nodeids_size() <= 0) + { + badData("Invalid ledger node IDs"); + return; + } + + if (m->nodeids_size() > tuning::kHardMaxReplyNodes) + { + badData( + "Requested number of ledger node IDs must be less than or equal to " + + std::to_string(tuning::kHardMaxReplyNodes)); + return; + } } // Verify query type diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index 0f229bf9d8..e87c12279e 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -696,19 +696,12 @@ private: std::shared_ptr getTxSet(std::shared_ptr const& m) const; +protected: void processLedgerRequest( std::shared_ptr const& m, std::vector nodeIDs); -protected: - // Kept `protected` so test subclasses (see - // TMGetObjectByHash_test) can drive the - // synchronous processor and the differential-pricing helper without - // routing through the JobQueue or going through `friend` plumbing. - // Production callers reach these members only via - // `onMessage(TMGetObjectByHash)` → JobQueue → `processGetObjectByHash`. - /** * Process a generic-query TMGetObjectByHash message. * From b190f2b14ff00c22e0e231462156c7c90ee7539e Mon Sep 17 00:00:00 2001 From: Timothy Banks Date: Tue, 8 Sep 2026 10:38:16 -0400 Subject: [PATCH 05/16] test: Add ProtocolMessage harness for testing TMPing --- src/test/overlay/ProtocolMessage_test.cpp | 296 ++++++++++++++++++++++ 1 file changed, 296 insertions(+) create mode 100644 src/test/overlay/ProtocolMessage_test.cpp diff --git a/src/test/overlay/ProtocolMessage_test.cpp b/src/test/overlay/ProtocolMessage_test.cpp new file mode 100644 index 0000000000..08e039f606 --- /dev/null +++ b/src/test/overlay/ProtocolMessage_test.cpp @@ -0,0 +1,296 @@ +#include +#include +#include + +#include + +#include +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace xrpl::test { + +class ProtocolMessage_test : public beast::unit_test::Suite +{ + struct TestHandler + { + bool compression = false; + int beginCount = 0; + int messageCount = 0; + int endCount = 0; + int unknownCount = 0; + std::uint16_t lastType = 0; + + [[nodiscard]] bool + compressionEnabled() const + { + return compression; + } + + void + onMessageUnknown(std::uint16_t type) + { + ++unknownCount; + lastType = type; + } + + void + onMessageBegin( + std::uint16_t type, + std::shared_ptr<::google::protobuf::Message> const&, + std::size_t, + std::size_t, + bool) + { + ++beginCount; + lastType = type; + } + + template + void + onMessage(std::shared_ptr const&) + { + ++messageCount; + } + + void + onMessageEnd(std::uint16_t, std::shared_ptr<::google::protobuf::Message> const&) + { + ++endCount; + } + + [[nodiscard]] static std::size_t + maxManifestsMessageSize() + { + return std::numeric_limits::max(); + } + }; + + // Wire bytes: `type` (2 bytes) + unknown field tag (2 bytes) + length varint (2 bytes, as these + // tests all use unknownFieldSize >= 128). + static constexpr std::size_t kPingProtoOverheadWithUnknownLen = 6; + static constexpr std::size_t kMinimumPingSizeWithEmptyUnknownField = + compression::kHeaderBytes + kPingProtoOverheadWithUnknownLen; + static constexpr std::size_t kMinimumPingSizeCompressedWithEmptyUnknownField = + compression::kHeaderBytesCompressed + kPingProtoOverheadWithUnknownLen; + + static std::vector + makePingBuffer(std::size_t unknownFieldSize, bool compressed = false) + { + auto ping = protocol::TMPing{}; + ping.set_type(protocol::TMPing::ptPING); + if (unknownFieldSize > 0) + { + ping.mutable_unknown_fields()->AddLengthDelimited( + 42, std::string(unknownFieldSize, 'A')); + } + + if (!compressed) + { + auto m = Message{ping, protocol::mtPING}; + return m.getBuffer(compression::Compressed::Off); + } + + // Message::compress() refuses to compress pings (mtPING is not in its + // allow-list), so getBuffer(Compressed::On) would just return the + // uncompressed bytes. Roll it by hand here to get a compressed + // ping message on the wire. + auto payload = std::string{}; + ping.SerializeToString(&payload); + + auto deflated = std::vector{}; + auto const deflatedSize = compression::compress( + payload.data(), + payload.size(), + [&](std::size_t sz) { + deflated.resize(sz); + return deflated.data(); + }, + compression::Algorithm::LZ4); + deflated.resize(deflatedSize); + + auto const type = static_cast(protocol::mtPING); + auto buffer = std::vector{}; + auto pack = [&buffer](std::uint32_t value) { + buffer.push_back(static_cast((value >> 24) & 0x0F)); + buffer.push_back(static_cast((value >> 16) & 0xFF)); + buffer.push_back(static_cast((value >> 8) & 0xFF)); + buffer.push_back(static_cast(value & 0xFF)); + }; + + pack(static_cast(deflated.size())); // compressed payload size + buffer.push_back(static_cast((type >> 8) & 0xFF)); + buffer.push_back(static_cast(type & 0xFF)); + pack(static_cast(payload.size())); // uncompressed size + buffer[0] |= static_cast(compression::Algorithm::LZ4); + + buffer.insert(buffer.end(), deflated.begin(), deflated.end()); + return buffer; + } + + static std::optional + declaredPingSize(std::vector const& buffer) + { + auto ec = boost::system::error_code{}; + auto const seq = std::array{boost::asio::buffer(buffer)}; + if (auto const header = xrpl::detail::parseMessageHeader(ec, seq, buffer.size())) + { + return header->uncompressedSize + header->headerSize; + } + return std::nullopt; + } + + static std::pair + invoke(std::vector const& buffer, TestHandler& handler) + { + auto const seq = std::array{boost::asio::buffer(buffer)}; + auto hint = 0uz; + return invokeProtocolMessage(seq, handler, hint); + } + + void + testOversizedPingRejected() + { + testcase("oversized ping rejected before dispatch"); + + auto runLocalTest = [&](std::size_t size, bool compressed = false) { + auto const buffer = makePingBuffer(size, compressed); + auto const declared = declaredPingSize(buffer); + if (BEAST_EXPECT(declared.has_value())) + BEAST_EXPECT(*declared > kMaximumPingMessageSize); + BEAST_EXPECT(buffer.size() < kMaximumMessageSize); + + auto handler = TestHandler{}; + handler.compression = compressed; + auto const [bytes, ec] = invoke(buffer, handler); + + BEAST_EXPECT(ec == make_error_code(boost::system::errc::message_size)); + BEAST_EXPECT(bytes == 0); + BEAST_EXPECT(handler.beginCount == 0); + BEAST_EXPECT(handler.messageCount == 0); + BEAST_EXPECT(handler.endCount == 0); + }; + // Just over the cap, and comfortably over it. + runLocalTest(kMaximumPingMessageSize + 1 - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest((2 * kMaximumPingMessageSize) - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest( + kMaximumPingMessageSize + 1 - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + runLocalTest( + (2 * kMaximumPingMessageSize) - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + } + + void + testOversizedPingRejectedFromHeaderAlone() + { + testcase("oversized ping rejected from header alone"); + + auto runLocalTest = [&](std::size_t size, bool compressed = false) { + auto const full = makePingBuffer(size, compressed); + auto const headerSize = + compressed ? compression::kHeaderBytesCompressed : compression::kHeaderBytes; + + // Only the header has arrived; the declared payload is still in flight. + auto const headerOnly = + std::vector{full.begin(), full.begin() + headerSize}; + BEAST_EXPECT(headerOnly.size() < full.size()); + + auto handler = TestHandler{}; + handler.compression = compressed; + auto const [bytes, ec] = invoke(headerOnly, handler); + + BEAST_EXPECT(ec == make_error_code(boost::system::errc::message_size)); + BEAST_EXPECT(bytes == 0); + BEAST_EXPECT(handler.beginCount == 0); + BEAST_EXPECT(handler.messageCount == 0); + BEAST_EXPECT(handler.endCount == 0); + }; + runLocalTest(kMaximumPingMessageSize + 1 - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest((2 * kMaximumPingMessageSize) - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest( + kMaximumPingMessageSize + 1 - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + runLocalTest( + (2 * kMaximumPingMessageSize) - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + } + + void + testNormalPingDispatched() + { + testcase("normal ping dispatched"); + + auto runLocalTest = [&](std::size_t size, bool compressed = false) { + auto const buffer = makePingBuffer(size, compressed); + auto const declared = declaredPingSize(buffer); + if (BEAST_EXPECT(declared.has_value())) + BEAST_EXPECT(*declared <= kMaximumPingMessageSize); + + auto handler = TestHandler{}; + handler.compression = compressed; + auto const [bytes, ec] = invoke(buffer, handler); + + BEAST_EXPECT(!ec); + BEAST_EXPECT(bytes == buffer.size()); + BEAST_EXPECT(handler.beginCount == 1); + BEAST_EXPECT(handler.messageCount == 1); + BEAST_EXPECT(handler.endCount == 1); + }; + runLocalTest(0); + runLocalTest(0, true); + } + + void + testPingWithSmallUnknownFieldDispatched() + { + testcase("ping with small unknown field still dispatched"); + + auto runLocalTest = [&](std::size_t size, bool compressed = false) { + auto const buffer = makePingBuffer(size, compressed); + auto const declared = declaredPingSize(buffer); + if (BEAST_EXPECT(declared.has_value())) + BEAST_EXPECT(*declared <= kMaximumPingMessageSize); + + auto handler = TestHandler{}; + handler.compression = compressed; + auto const [bytes, ec] = invoke(buffer, handler); + + BEAST_EXPECT(!ec); + BEAST_EXPECT(bytes == buffer.size()); + BEAST_EXPECT(handler.beginCount == 1); + BEAST_EXPECT(handler.messageCount == 1); + BEAST_EXPECT(handler.endCount == 1); + }; + // Well under the cap, one byte under it, and exactly at it. + runLocalTest((kMaximumPingMessageSize / 2) - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest(kMaximumPingMessageSize - 1 - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest(kMaximumPingMessageSize - kMinimumPingSizeWithEmptyUnknownField); + runLocalTest( + (kMaximumPingMessageSize / 2) - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + runLocalTest( + kMaximumPingMessageSize - 1 - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + runLocalTest( + kMaximumPingMessageSize - kMinimumPingSizeCompressedWithEmptyUnknownField, true); + } + + void + run() override + { + testOversizedPingRejected(); + testOversizedPingRejectedFromHeaderAlone(); + testNormalPingDispatched(); + testPingWithSmallUnknownFieldDispatched(); + } +}; + +BEAST_DEFINE_TESTSUITE(ProtocolMessage, overlay, xrpl); + +} // namespace xrpl::test From 796f2f8f1eedb88ce7e3f4b198fff5540a4f9f1f Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:10:48 +0200 Subject: [PATCH 06/16] fix: Relax Loan Invariants to allow zero-principal LoanPay transaction --- include/xrpl/tx/invariants/LoanInvariant.h | 8 +- src/libxrpl/tx/invariants/LoanInvariant.cpp | 27 +++++- .../app/invariants/InvariantsVault_test.cpp | 86 ++++++++++++++++++- src/test/app/lending/LoanRounding_test.cpp | 41 +++++---- 4 files changed, 131 insertions(+), 31 deletions(-) diff --git a/include/xrpl/tx/invariants/LoanInvariant.h b/include/xrpl/tx/invariants/LoanInvariant.h index 34ce1a4dc2..8cbdefc911 100644 --- a/include/xrpl/tx/invariants/LoanInvariant.h +++ b/include/xrpl/tx/invariants/LoanInvariant.h @@ -38,9 +38,11 @@ namespace xrpl { * f. A Loan must reference a live `ltLOAN_BROKER`, and that broker must * reference a live `ltVAULT`. * g. Post-conditions for the Loan paid down by a successful `ttLOAN_PAY`: - * `PaymentRemaining > 0` after: `PrincipalOutstanding` and - * `PaymentRemaining` strictly decrease; `NextPaymentDueDate` - * advances by N * `PaymentInterval`, N > 0. + * `PaymentRemaining > 0` after: neither `PrincipalOutstanding` nor + * `TotalValueOutstanding` increases, and at least one of them + * strictly decreases; + * `PaymentRemaining` strictly decreases; + * `NextPaymentDueDate` advances by N * `PaymentInterval`, N > 0. * `PaymentRemaining == 0` after: pinned by checks 1 and 5b. * */ diff --git a/src/libxrpl/tx/invariants/LoanInvariant.cpp b/src/libxrpl/tx/invariants/LoanInvariant.cpp index b34d7088be..f87a7620af 100644 --- a/src/libxrpl/tx/invariants/LoanInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanInvariant.cpp @@ -231,17 +231,38 @@ ValidLoan::finalize( // must show that payment in its balance and schedule. A payment that clears // the loan outright instead drives PaymentRemaining to zero, which the // fully-paid-off and zero due-date checks above pin. + // + // PrincipalOutstanding may stay put on a non-final pay: at integer + // scale, fixCleanup3_2_0 rounds principal up so a fractional + // amortization step does not reduce it. Interest (TVO) still falls. + // Neither balance may grow: a payment never adds to what is owed, + // since late-payment penalties are charged in the same transaction + // rather than tracked in TotalValueOutstanding. if (isTesSuccess(result) && txType == ttLOAN_PAY) { if (before && after->at(sfPaymentRemaining) != 0) { - if (!(after->at(sfPrincipalOutstanding) < before->at(sfPrincipalOutstanding))) + if (after->at(sfPrincipalOutstanding) > before->at(sfPrincipalOutstanding)) { - JLOG(j.fatal()) << "Invariant failed: loan pay must strictly decrease " + JLOG(j.fatal()) << "Invariant failed: loan pay must not increase " "PrincipalOutstanding on a non-full-repayment"; return false; } - if (!(after->at(sfPaymentRemaining) < before->at(sfPaymentRemaining))) + if (after->at(sfTotalValueOutstanding) > before->at(sfTotalValueOutstanding)) + { + JLOG(j.fatal()) << "Invariant failed: loan pay must not increase " + "TotalValueOutstanding on a non-full-repayment"; + return false; + } + if (after->at(sfPrincipalOutstanding) == before->at(sfPrincipalOutstanding) && + after->at(sfTotalValueOutstanding) == before->at(sfTotalValueOutstanding)) + { + JLOG(j.fatal()) << "Invariant failed: loan pay must decrease " + "PrincipalOutstanding or TotalValueOutstanding " + "on a non-full-repayment"; + return false; + } + if (after->at(sfPaymentRemaining) >= before->at(sfPaymentRemaining)) { JLOG(j.fatal()) << "Invariant failed: loan pay must decrease " "PaymentRemaining on a non-full-repayment"; diff --git a/src/test/app/invariants/InvariantsVault_test.cpp b/src/test/app/invariants/InvariantsVault_test.cpp index dcf783a1b5..e264b91cb1 100644 --- a/src/test/app/invariants/InvariantsVault_test.cpp +++ b/src/test/app/invariants/InvariantsVault_test.cpp @@ -1224,33 +1224,51 @@ class InvariantsVault_test : public InvariantsBase // ttLOAN_PAY success post-conditions. A loan left with payments still // remaining after a successful payment must show that payment in its - // balance and schedule: PrincipalOutstanding and PaymentRemaining both - // strictly decrease, and NextPaymentDueDate advances by a positive - // multiple of PaymentInterval. Each case seeds the same loan, then applies + // balance and schedule: neither PrincipalOutstanding nor + // TotalValueOutstanding may increase, at least one of them must + // strictly decrease, PaymentRemaining must strictly decrease, and + // NextPaymentDueDate must advance by a positive multiple of + // PaymentInterval. Each failing case seeds the same loan, then applies // an after-image that breaks exactly one of those conditions. { struct Case { Number principal; + Number totalValue; std::uint32_t remaining; std::uint32_t dueDate; std::string expected; }; auto const cases = std::to_array({ {.principal = Number(100), + .totalValue = Number(150), .remaining = 1, .dueDate = 110, - .expected = "loan pay must strictly decrease PrincipalOutstanding"}, + .expected = "loan pay must decrease PrincipalOutstanding or " + "TotalValueOutstanding"}, + {.principal = Number(110), + .totalValue = Number(150), + .remaining = 1, + .dueDate = 110, + .expected = "loan pay must not increase PrincipalOutstanding"}, {.principal = Number(50), + .totalValue = Number(160), + .remaining = 1, + .dueDate = 110, + .expected = "loan pay must not increase TotalValueOutstanding"}, + {.principal = Number(50), + .totalValue = Number(150), .remaining = 2, .dueDate = 110, .expected = "loan pay must decrease PaymentRemaining"}, {.principal = Number(50), + .totalValue = Number(150), .remaining = 1, .dueDate = 100, .expected = "loan pay must advance NextPaymentDueDate"}, // Advanced, but not by a whole number of payment intervals. {.principal = Number(50), + .totalValue = Number(150), .remaining = 1, .dueDate = 105, .expected = "loan pay must advance NextPaymentDueDate"}, @@ -1291,6 +1309,7 @@ class InvariantsVault_test : public InvariantsBase if (!BEAST_EXPECT(sleLoan)) continue; sleLoan->at(sfPrincipalOutstanding) = c.principal; + sleLoan->at(sfTotalValueOutstanding) = c.totalValue; sleLoan->setFieldU32(sfPaymentRemaining, c.remaining); sleLoan->setFieldU32(sfNextPaymentDueDate, c.dueDate); ac.view().update(sleLoan); @@ -1303,6 +1322,65 @@ class InvariantsVault_test : public InvariantsBase BEAST_EXPECT(result == tecINVARIANT_FAILED); BEAST_EXPECT(sink.messages().str().contains(c.expected)); } + + // Principal may stick while TotalValueOutstanding falls. This + // after-image is only a Loan mutation, so other (vault) invariants + // still fail under Full scope; ValidLoan itself must not. + { + Env env{*this, all_}; + Account const a1{"A1"}; + Account const a2{"A2"}; + env.fund(XRP(1000), a1, a2); + auto const keys = createClosedXrpBroker(a1, env); + if (!keys) + { + fail(); + } + else + { + auto const& brokerKeylet = keys->second; + OpenView ov{*env.current()}; + auto const loanKeylet = + keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(1)); + { + auto sleLoan = makeLoanSle(brokerKeylet.key, 1, a2.id()); + sleLoan->at(sfPrincipalOutstanding) = Number(100); + sleLoan->at(sfTotalValueOutstanding) = Number(150); + sleLoan->at(sfPaymentInterval) = 10u; + sleLoan->setFieldU32(sfPaymentRemaining, 2); + sleLoan->setFieldU32(sfNextPaymentDueDate, 100); + ov.rawInsert(sleLoan); + } + + STTx const tx{ + ttLOAN_PAY, [](STObject& t) { t.setFieldAmount(sfAmount, XRPAmount(50)); }}; + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + ApplyContext ac{ + env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, TapNone, jlog}; + CurrentTransactionRulesGuard const rulesGuard(ov.rules()); + + auto sleLoan = ac.view().peek(loanKeylet); + if (BEAST_EXPECT(sleLoan)) + { + sleLoan->at(sfPrincipalOutstanding) = Number(100); + sleLoan->at(sfTotalValueOutstanding) = Number(140); + sleLoan->setFieldU32(sfPaymentRemaining, 1); + sleLoan->setFieldU32(sfNextPaymentDueDate, 110); + ac.view().update(sleLoan); + + auto transactor = makeTransactor(ac); + if (BEAST_EXPECT(transactor)) + { + std::ignore = transactor->checkInvariants( + tesSUCCESS, XRPAmount{}, Transactor::InvariantScope::Full); + auto const logs = sink.messages().str(); + BEAST_EXPECT(!logs.contains("Invariant failed: Loan")); + BEAST_EXPECT(!logs.contains("loan pay")); + } + } + } + } } // ttLOAN_MANAGE (default): the write-off is rounded downward at the diff --git a/src/test/app/lending/LoanRounding_test.cpp b/src/test/app/lending/LoanRounding_test.cpp index ded1c816a2..4a2063ed77 100644 --- a/src/test/app/lending/LoanRounding_test.cpp +++ b/src/test/app/lending/LoanRounding_test.cpp @@ -415,6 +415,9 @@ private: // The test pays one period at a time across three LoanPay // transactions and verifies the loan completes (paymentRemaining=0) // with totals matching the loan's economics (1 principal + 2 interest). + // Also run under featureLendingProtocolV1_1: ValidLoan must allow the + // two sticking pays (TVO falls, PO does not) and the final clear + // (PaymentRemaining 0, NextPaymentDueDate 0). void testIntegerScalePrincipalSticks(FeatureBitset features) { @@ -446,28 +449,19 @@ private: env(pay(issuer, borrower, asset(10'000))); env.close(); - Vault const vault{env}; - auto [vaultTx, vaultKeylet] = vault.create({.owner = lender, .asset = asset}); - env(vaultTx); - env.close(); + // createVaultAndBroker promotes the vault to ClosedEnded under + // featureLendingProtocolV1_1 (LoanBrokerSet rejects open-ended). + BrokerParameters const params{ + .vaultDeposit = Number{5'000}, + .debtMax = Number{100}, + .coverRateMin = TenthBips32{0}, + .coverDeposit = 0, + .managementFeeRate = TenthBips16{0}, + .coverRateLiquidation = TenthBips32{0}}; + BrokerInfo const broker = createVaultAndBroker(env, asset, lender, params); - env(vault.deposit({.depositor = lender, .id = vaultKeylet.key, .amount = asset(5'000)})); - env.close(); - - auto const brokerKeylet = - keylet::loanBroker(lender.id(), SeqProxy::rawSequence(env.seq(lender))); - env(loan_broker::set(lender, vaultKeylet.key), - loan_broker::kDebtMaximum(Number{100}), - Fee(env.current()->fees().base * 2)); - env.close(); - - auto const brokerStateBefore = env.le(brokerKeylet); - if (!BEAST_EXPECT(brokerStateBefore)) - return; - auto const loanSequence = brokerStateBefore->at(sfLoanSequence); - auto const loanKeylet = keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(loanSequence)); - - env(loan::set(borrower, brokerKeylet.key, Number{1}), + auto const loanKeylet = nextLoanKeylet(env, broker); + env(loan::set(borrower, broker.brokerID, Number{1}), Sig(sfCounterpartySignature, lender), loan::kInterestRate(TenthBips32{50'000}), loan::kPaymentTotal(3), @@ -499,6 +493,8 @@ private: BEAST_EXPECT(sle->at(sfPrincipalOutstanding) == expectedPO[i]); BEAST_EXPECT(sle->at(sfTotalValueOutstanding) == expectedTVO[i]); BEAST_EXPECT(sle->at(sfPaymentRemaining) == expectedRemaining[i]); + if (expectedRemaining[i] == 0) + BEAST_EXPECT(sle->at(~sfNextPaymentDueDate).value_or(0) == 0); } // Borrower paid 3 total regardless of fee split (1 principal + 2 @@ -1226,6 +1222,9 @@ private: testBugVaultWithdrawDustVsAssetsTotal(all_ - fixCleanup3_4_0); testBugVaultWithdrawDustVsAssetsTotal(all_); testBugInterestDueDeltaCrash(); + // all_ excludes V1.1; amendmentCombinations never pairs it with the + // sticking schedule. Run that combination explicitly. + testIntegerScalePrincipalSticks(all_ | featureLendingProtocolV1_1); } // Tests run under each entry in amendmentCombinations(). From 9aebb5ebea07e324d05c4d504b6b313c6fca316b Mon Sep 17 00:00:00 2001 From: Timothy Banks Date: Tue, 8 Sep 2026 14:44:43 -0400 Subject: [PATCH 07/16] fix: Cap TMTransactions list size and charge fee for undeserializable transactions --- src/test/overlay/PeerTest.cpp | 33 ++++++++ src/test/overlay/PeerTest.h | 16 +++- src/test/overlay/TMGetLedger_test.cpp | 34 ++++---- src/test/overlay/TMGetObjectByHash_test.cpp | 25 +++--- src/test/overlay/TMTransaction_test.cpp | 60 ++++++++++++++ src/test/overlay/TMTransactions_test.cpp | 88 +++++++++++++++++++++ src/xrpld/overlay/detail/PeerImp.cpp | 11 +++ 7 files changed, 235 insertions(+), 32 deletions(-) create mode 100644 src/test/overlay/TMTransaction_test.cpp create mode 100644 src/test/overlay/TMTransactions_test.cpp diff --git a/src/test/overlay/PeerTest.cpp b/src/test/overlay/PeerTest.cpp index 6ca6db384b..341febb25b 100644 --- a/src/test/overlay/PeerTest.cpp +++ b/src/test/overlay/PeerTest.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -99,6 +100,38 @@ PeerTest::resetId() id = 0; } +bool +PeerTest::compressionEnabled() const +{ + if (compressionEnabled_.has_value()) + { + return *compressionEnabled_; + } + return PeerImp::compressionEnabled(); +} + +void +PeerTest::compressionEnabled(std::optional enabled) +{ + compressionEnabled_ = enabled; +} + +bool +PeerTest::txReduceRelayEnabled() const +{ + if (reduceRelayEnabled_.has_value()) + { + return *reduceRelayEnabled_; + } + return PeerImp::txReduceRelayEnabled(); +} + +void +PeerTest::txReduceRelayEnabled(std::optional enabled) +{ + reduceRelayEnabled_ = enabled; +} + std::shared_ptr makePeerTest(jtx::Env& env, PeerTest::SharedContext const& context, ProtocolVersion protocolVersion) { diff --git a/src/test/overlay/PeerTest.h b/src/test/overlay/PeerTest.h index a081399bac..f7b2815da3 100644 --- a/src/test/overlay/PeerTest.h +++ b/src/test/overlay/PeerTest.h @@ -26,6 +26,7 @@ #include #include +#include #include namespace xrpl::test { @@ -35,9 +36,10 @@ namespace xrpl::test { */ class PeerTest : public PeerImp { -private: inline static Peer::id_t id{}; std::shared_ptr lastSentMessage_; + std::optional compressionEnabled_; + std::optional reduceRelayEnabled_; public: using MiddleType = boost::beast::tcp_stream; @@ -83,6 +85,18 @@ public: static void resetId(); + + bool + compressionEnabled() const override; + + void + compressionEnabled(std::optional enabled); + + bool + txReduceRelayEnabled() const override; + + void + txReduceRelayEnabled(std::optional enabled); }; std::shared_ptr diff --git a/src/test/overlay/TMGetLedger_test.cpp b/src/test/overlay/TMGetLedger_test.cpp index 2f072e0efe..9088e6fa65 100644 --- a/src/test/overlay/TMGetLedger_test.cpp +++ b/src/test/overlay/TMGetLedger_test.cpp @@ -44,13 +44,11 @@ class TMGetLedger_test : public beast::unit_test::Suite auto request = std::make_shared(); request->set_itype(protocol::liTX_NODE); - // A uint256-sized ledger hash so the request passes the earlier - // structural validation and reaches the node-ID checks. + // A uint256-sized ledger hash, as a well-formed request carries. uint256 const ledgerHash{1}; request->set_ledgerhash(ledgerHash.data(), ledgerHash.size()); - // Valid, deserializable SHAMap node IDs (the root node ID, repeated). - // The count is what the hard bound cares about. + // Valid, deserializable SHAMap node IDs. auto const rootNodeId = SHAMapNodeID{}.getRawString(); for (std::size_t i = 0; i < numNodeIds; ++i) { @@ -61,9 +59,9 @@ class TMGetLedger_test : public beast::unit_test::Suite } void - testNodeIdHardLimit(std::size_t const numNodeIds, bool const expectRejected) + testNodeIdCountAccepted(std::size_t const numNodeIds, bool const expectRejected) { - testcase("Node ID Hard Limit"); + testcase("Node ID Count Accepted"); Env env{*this}; PeerTest::resetId(); @@ -71,17 +69,18 @@ class TMGetLedger_test : public beast::unit_test::Suite auto peer = makePeerTest(env, context_, protocolVersion_); peer->onMessage(createRequest(numNodeIds)); - // An over-limit request is charged kFeeInvalidData; an in-limit request should not be. - // The JobQueue handler may run concurrently and update the fee for in-limit requests. + // A request outside the accepted node-ID count is charged kFeeInvalidData; one inside + // it is not. The JobQueue handler may run concurrently and update the fee in the + // accepted case. BEAST_EXPECT( expectRejected ? (peer->getCurrentFeeCharge() == resource::kFeeInvalidData) : !(peer->getCurrentFeeCharge() == resource::kFeeInvalidData)); } void - testProcessLedgerRequestReplyCapped(std::size_t const numNodeIds) + testProcessLedgerRequestNodeCount(std::size_t const numNodeIds) { - testcase("Process Ledger Request Reply Capped"); + testcase("Process Ledger Request Node Count"); Env env{*this}; env.close(); @@ -89,8 +88,7 @@ class TMGetLedger_test : public beast::unit_test::Suite auto peer = makePeerTest(env, context_, protocolVersion_); - // Request the account-state root node of the closed ledger, asking - // for far more node IDs than the hard bound allows. + // Ask for the account-state root node of the closed ledger. auto request = createRequest(numNodeIds); request->clear_ledgerhash(); request->set_itype(protocol::liAS_NODE); @@ -121,12 +119,12 @@ class TMGetLedger_test : public beast::unit_test::Suite run() override { auto const limit = static_cast(tuning::kHardMaxReplyNodes); - testNodeIdHardLimit(limit + 1, true); - testNodeIdHardLimit(limit, false); - testNodeIdHardLimit(limit - 1, false); - testProcessLedgerRequestReplyCapped(limit + 1); - testProcessLedgerRequestReplyCapped(limit); - testProcessLedgerRequestReplyCapped(limit - 1); + testNodeIdCountAccepted(limit + 1, true); + testNodeIdCountAccepted(limit, false); + testNodeIdCountAccepted(limit - 1, false); + testProcessLedgerRequestNodeCount(limit + 1); + testProcessLedgerRequestNodeCount(limit); + testProcessLedgerRequestNodeCount(limit - 1); } }; diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp index 7c614d447c..84495fec37 100644 --- a/src/test/overlay/TMGetObjectByHash_test.cpp +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -33,11 +33,11 @@ namespace xrpl::test { using namespace jtx; /** - * Test for TMGetObjectByHash reply size limiting. + * Coverage for the TMGetObjectByHash object-count bound. * - * This verifies the fix that limits TMGetObjectByHash replies to - * tuning::hardMaxReplyNodes to prevent excessive memory usage and - * potential DoS attacks from peers requesting large numbers of objects. + * A generic query names some number of objects; the number of entries the + * reply carries is bounded by tuning::kHardMaxReplyNodes. These cases pin that + * bound at and either side of its boundary. */ class TMGetObjectByHash_test : public beast::unit_test::Suite { @@ -63,7 +63,7 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite NodeObjectType::Ledger, std::move(data), hash, nodeStore.earliestLedgerSeq()); } - // Create a request with more objects than hardMaxReplyNodes + // Name every stored object in a single generic query. auto request = std::make_shared(); request->set_type(protocol::TMGetObjectByHash_ObjectType_otLEDGER); request->set_query(true); @@ -78,17 +78,16 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite } /** - * Test that reply is limited to hardMaxReplyNodes when more objects - * are requested than the limit allows. + * Check the object count a generic-query reply carries. * * `onMessage(TMGetObjectByHash)` dispatches the generic-query path * to the JobQueue, so tests invoke the synchronous processor * directly via `runProcessGetObjectByHash`. */ void - testReplyLimit(size_t const numObjects, int const expectedReplySize) + testReplyObjectCount(size_t const numObjects, int const expectedReplySize) { - testcase("Reply Limit"); + testcase("Reply Object Count"); Env env(*this); PeerTest::resetId(); @@ -110,7 +109,7 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite protocol::TMGetObjectByHash reply; BEAST_EXPECT(reply.ParseFromArray(buffer.data() + 6, buffer.size() - 6) == true); - // Verify the reply is limited to expectedReplySize + // The reply carries the expected number of objects. BEAST_EXPECT(reply.objects_size() == expectedReplySize); } @@ -118,9 +117,9 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite run() override { int const limit = static_cast(tuning::kHardMaxReplyNodes); - testReplyLimit(limit + 1, limit); - testReplyLimit(limit, limit); - testReplyLimit(limit - 1, limit - 1); + testReplyObjectCount(limit + 1, limit); + testReplyObjectCount(limit, limit); + testReplyObjectCount(limit - 1, limit - 1); } }; diff --git a/src/test/overlay/TMTransaction_test.cpp b/src/test/overlay/TMTransaction_test.cpp new file mode 100644 index 0000000000..b208b3d81b --- /dev/null +++ b/src/test/overlay/TMTransaction_test.cpp @@ -0,0 +1,60 @@ +#include +#include +#include + +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include + +namespace xrpl::test { + +using namespace jtx; + +class TMTransaction_test : public beast::unit_test::Suite +{ + PeerTest::SharedContext context_{makeSslContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + void + testFailureDeserializingTransactionIsCharged() + { + testcase("Undeserializable Transaction Is Charged"); + + Env env{*this, envconfig()}; + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + auto tx = std::make_shared(); + tx->set_status(protocol::tsNEW); + + // Bytes that are not a serialized transaction, so deserialization fails. + tx->set_rawtransaction("\x01\x02\x03", 3); + + peer->onMessage(tx); + BEAST_EXPECT(peer->getCurrentFeeCharge() == resource::kFeeInvalidData); + } + + void + run() override + { + testFailureDeserializingTransactionIsCharged(); + } +}; + +BEAST_DEFINE_TESTSUITE(TMTransaction, overlay, xrpl); + +} // namespace xrpl::test diff --git a/src/test/overlay/TMTransactions_test.cpp b/src/test/overlay/TMTransactions_test.cpp new file mode 100644 index 0000000000..67d36cc04b --- /dev/null +++ b/src/test/overlay/TMTransactions_test.cpp @@ -0,0 +1,88 @@ +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include + +namespace xrpl::test { + +using namespace jtx; + +class TMTransactions_test : public beast::unit_test::Suite +{ + PeerTest::SharedContext context_{makeSslContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + static std::shared_ptr + createRequest(std::size_t const numTransactions) + { + auto request = std::make_shared(); + for (std::size_t i = 0; i < numTransactions; ++i) + { + request->mutable_transactions()->Add(protocol::TMTransaction{}); + } + return request; + } + + void + testTransactionCountAccepted(std::size_t const numTransactions, bool const expectRejected) + { + testcase("Transaction Count Accepted"); + + static constexpr auto kLimitExceededMessage = "TMTransactions: transaction list too large"; + auto foundExpectedLog = false; + Env env{ + *this, + envconfig(), + std::make_unique(kLimitExceededMessage, &foundExpectedLog)}; + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + peer->txReduceRelayEnabled(true); + peer->onMessage(createRequest(numTransactions)); + + auto fee = peer->getCurrentFeeCharge(); + if (expectRejected) + { + BEAST_EXPECT(fee == resource::kFeeMalformedRequest); + BEAST_EXPECT(foundExpectedLog); + } + else + { + BEAST_EXPECT(!foundExpectedLog); + } + } + + void + run() override + { + auto const limit = reduce_relay::kMaxTxQueueSize; + testTransactionCountAccepted(limit + 1, true); + testTransactionCountAccepted(limit, false); + testTransactionCountAccepted(limit - 1, false); + } +}; + +BEAST_DEFINE_TESTSUITE(TMTransactions, overlay, xrpl); + +} // namespace xrpl::test diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index eddeb01897..422db53389 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1414,6 +1414,10 @@ PeerImp::handleTransaction( } catch (std::exception const& ex) { + if (fee_.fee < resource::kFeeInvalidData) + { + fee_.update(resource::kFeeInvalidData, "tx invalid"); + } JLOG(pJournal_.warn()) << "Transaction invalid: " << strHex(m->rawtransaction()) << ". Exception: " << ex.what(); } @@ -2859,6 +2863,13 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } + if (m->transactions_size() > reduce_relay::kMaxTxQueueSize) + { + JLOG(pJournal_.error()) << "TMTransactions: transaction list too large"; + fee_.update(resource::kFeeMalformedRequest, "Transaction list too large"); + return; + } + JLOG(pJournal_.trace()) << "received TMTransactions " << m->transactions_size(); overlay_.addTxMetrics(m->transactions_size()); From c0d0fd0d9798f39ce4e732dadf7a668bbc4b5534 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 8 Sep 2026 21:36:55 +0100 Subject: [PATCH 08/16] build: Add assert-enabled builds and packages --- .github/scripts/strategy-matrix/generate.py | 46 ++++- .github/scripts/strategy-matrix/linux.json | 13 ++ .github/workflows/on-pr.yml | 1 + .github/workflows/on-trigger.yml | 1 + .../reusable-package-test-install.yml | 103 +++++++++++ .github/workflows/reusable-package.yml | 161 ++++++------------ cmake/XrplPackaging.cmake | 8 +- docs/install.md | 23 ++- package/README.md | 106 ++++++++++-- package/build_pkg.py | 100 +++++++++-- package/debian/{control => control.in} | 5 +- package/debian/{xrpld.docs => docs} | 0 package/debian/{xrpld.links => links} | 0 package/debian/lintian-overrides.in | 6 + package/debian/rules | 41 ++++- package/debian/xrpld.lintian-overrides | 6 - package/rpm/xrpld.spec | 49 ++++-- 17 files changed, 504 insertions(+), 165 deletions(-) create mode 100644 .github/workflows/reusable-package-test-install.yml rename package/debian/{control => control.in} (93%) rename package/debian/{xrpld.docs => docs} (100%) rename package/debian/{xrpld.links => links} (100%) create mode 100644 package/debian/lintian-overrides.in delete mode 100644 package/debian/xrpld.lintian-overrides diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 65671dbd11..5528d6442e 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -15,6 +15,14 @@ _BASE_CMAKE_ARGS = [ "-Drust=ON", ] +# The package formats a config can be packaged as, each with its own +# install-test job in reusable-package.yml. +PACKAGE_TYPES = ("deb", "rpm") + +# The package name a variant suffixes, as build_pkg.py's BASE_NAME spells it: +# the two have to agree, or the artifact globs miss what was built. +BASE_NAME = "xrpld" + # Maps sanitizer names (as used in cmake) to short config-name suffixes. _SANITIZER_SUFFIX: dict[str, str] = { "address": "asan", @@ -62,10 +70,20 @@ def get_cmake_args(build_type: str, extra_args: str) -> str: class PackageConfig: """The 'package' map of a config whose binaries are also packaged.""" - type: str # "deb" or "rpm"; has to match what the image provides + type: str # has to match what the image provides # The packaging container image: a vanilla distro image, not the nix image # the config itself builds in. image: str + # A flavour of the package, named xrpld-, for a config whose + # binaries are not the plain release build. A variant needs no counterpart + # in the other format. + variant: str = "" + + def __post_init__(self) -> None: + assert self.type in PACKAGE_TYPES, ( + f"unsupported package type {self.type!r}: " + f"use one of {', '.join(PACKAGE_TYPES)}." + ) @dataclasses.dataclass @@ -178,6 +196,8 @@ class PackagingEntry: validator_keys_artifact_name: str image: str package_type: str # "deb" or "rpm"; drives the format-specific steps + package_variant: str # passed to build_pkg.py --variant; empty for xrpld + package_name: str # the name it builds under, which the artifact globs use # --------------------------------------------------------------------------- @@ -267,12 +287,32 @@ def expand_linux_packaging(linux: LinuxFile) -> list[PackagingEntry]: validator_keys_artifact_name=f"validator-keys-{name}", image=cfg.package.image, package_type=cfg.package.type, + package_variant=cfg.package.variant, + package_name=( + f"{BASE_NAME}-{cfg.package.variant}" + if cfg.package.variant + else BASE_NAME + ), ) ) return entries +def package_names_by_type(entries: list[PackagingEntry]) -> dict[str, list[str]]: + """The names of the packages in 'entries', keyed by format. + + Derived from the packaging matrix rather than listed again, so the packages + the install-test jobs look for are the packages that were built. + """ + return { + package_type: sorted( + {e.package_name for e in entries if e.package_type == package_type} + ) + for package_type in PACKAGE_TYPES + } + + def expand_platform_matrix(pf: PlatformFile, minimal: bool) -> list[MatrixEntry]: """Expand a PlatformFile (macOS or Windows) into matrix entries. @@ -341,6 +381,10 @@ if __name__ == "__main__": if args.packaging: matrix = expand_linux_packaging(LinuxFile.load(THIS_DIR / "linux.json")) + # One list per format, so each install-test job installs the packages its + # own format produced. + for package_type, names in package_names_by_type(matrix).items(): + print(f"{package_type}_package_names={json.dumps(names)}") else: if args.config in ("linux", None): matrix += expand_linux_matrix( diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index f2cddac488..a5a451ea94 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -76,6 +76,19 @@ "type": "deb", "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-49cdc10" } + }, + { + "compiler": ["gcc"], + "build_type": ["Release"], + "arch": ["amd64"], + "minimal": false, + "suffix": "assert", + "extra_cmake_args": "-Dvalidator_keys=ON -Dassert=ON", + "package": { + "type": "deb", + "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-49cdc10", + "variant": "assert" + } } ], diff --git a/.github/workflows/on-pr.yml b/.github/workflows/on-pr.yml index 933c7b8a54..25bbaa2cc9 100644 --- a/.github/workflows/on-pr.yml +++ b/.github/workflows/on-pr.yml @@ -85,6 +85,7 @@ jobs: .github/workflows/reusable-build-test.yml .github/workflows/reusable-check-autogen.yml .github/workflows/reusable-clang-tidy.yml + .github/workflows/reusable-package-test-install.yml .github/workflows/reusable-package.yml .github/workflows/reusable-rust.yml .github/workflows/reusable-strategy-matrix.yml diff --git a/.github/workflows/on-trigger.yml b/.github/workflows/on-trigger.yml index 2099f5f739..2bf73332d2 100644 --- a/.github/workflows/on-trigger.yml +++ b/.github/workflows/on-trigger.yml @@ -23,6 +23,7 @@ on: - ".github/workflows/reusable-build-test.yml" - ".github/workflows/reusable-check-autogen.yml" - ".github/workflows/reusable-clang-tidy.yml" + - ".github/workflows/reusable-package-test-install.yml" - ".github/workflows/reusable-package.yml" - ".github/workflows/reusable-rust.yml" - ".github/workflows/reusable-strategy-matrix.yml" diff --git a/.github/workflows/reusable-package-test-install.yml b/.github/workflows/reusable-package-test-install.yml new file mode 100644 index 0000000000..84db0b666d --- /dev/null +++ b/.github/workflows/reusable-package-test-install.yml @@ -0,0 +1,103 @@ +# Install one package format on every distro family it targets, one job per +# package name and image, and run the binaries there. Called once per format by +# reusable-package.yml, which owns the names and the image lists. +name: Install packages + +on: + workflow_call: + inputs: + package_type: + description: 'The package format to install ("deb" or "rpm").' + required: true + type: string + package_names: + description: "JSON array of package names built for this format." + required: true + type: string + images: + description: "JSON array of container images to install in." + required: true + type: string + +defaults: + run: + shell: bash + +env: + PACKAGE_DIR: packages + +jobs: + install: + strategy: + fail-fast: false + matrix: + package_name: ${{ fromJson(inputs.package_names) }} + image: ${{ fromJson(inputs.images) }} + name: "${{ matrix.package_name }} on ${{ matrix.image }}" + permissions: + contents: read + runs-on: ubuntu-latest + container: ${{ matrix.image }} + timeout-minutes: 5 + + steps: + # Every package lands in one directory; the step below picks its own, + # which keeps this independent of the artifact names. + - name: Download package artifacts + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: "*-pkg" + merge-multiple: true + path: ${{ env.PACKAGE_DIR }} + + - name: Find the package + id: find + env: + PACKAGE_NAME: ${{ matrix.package_name }} + PACKAGE_TYPE: ${{ inputs.package_type }} + run: | + # The version follows the name, separated by '_' in a DEB and '-' in an + # RPM. Requiring a digit after it is what keeps 'xrpld' from picking up + # another package, such as 'xrpld-assert'. + pattern="${PACKAGE_NAME}[_-][0-9]*.${PACKAGE_TYPE}" + package="$(find "${PACKAGE_DIR}" -type f -name "${pattern}" -print -quit)" + test -n "${package}" || { + echo "no ${pattern} found in ${PACKAGE_DIR}" >&2 + exit 1 + } + echo "package=${package}" >>"${GITHUB_OUTPUT}" + + - name: Install the DEB + if: ${{ inputs.package_type == 'deb' }} + env: + DEBIAN_FRONTEND: noninteractive + PACKAGE: ${{ steps.find.outputs.package }} + run: | + # Stock Debian and Ubuntu images carry no package lists, so apt has + # nothing to resolve the systemd dependency from until it fetches them. + apt-get update -qq + apt-get install -y "./${PACKAGE}" + + - name: Install the RPM + if: ${{ inputs.package_type == 'rpm' }} + env: + PACKAGE: ${{ steps.find.outputs.package }} + run: dnf install -y "./${PACKAGE}" + + - name: Run xrpld + run: xrpld --version + + - name: Run validator-keys + run: validator-keys --version + + - name: Run rippled, the legacy compatibility symlink + run: rippled --version + + - name: Check the service account + run: id xrpld + + - name: Check the state directory + run: test -d /var/lib/xrpld + + - name: Check the log directory + run: test -d /var/log/xrpld diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index 58adc53dc8..e7dd34b064 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -3,8 +3,10 @@ # # - 'package' builds and signs one format per config that carries a "package" # map in linux.json; that map names the container image and the format -# - 'test-install' installs what was built on a range of distros and runs the -# binaries there, so a package that cannot be installed never reaches Nexus +# - 'test-install-deb' and 'test-install-rpm' call +# reusable-package-test-install.yml to install what was built on a range of +# distros and run the binaries there, so a package that cannot be installed +# never reaches Nexus # - 'publish' uploads with the image's publish_pkg.py, doing a --dry-run # unless 'publish: true' # @@ -49,6 +51,8 @@ jobs: runs-on: ubuntu-latest outputs: matrix: ${{ steps.generate.outputs.matrix }} + deb_package_names: ${{ steps.generate.outputs.deb_package_names }} + rpm_package_names: ${{ steps.generate.outputs.rpm_package_names }} steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -107,6 +111,7 @@ jobs: - name: Build package env: PACKAGE_TYPE: ${{ matrix.package_type }} + PACKAGE_VARIANT: ${{ matrix.package_variant }} PKG_RELEASE: ${{ steps.release_info.outputs.pkg_release }} CHANNEL: ${{ steps.release_info.outputs.channel }} run: | @@ -114,6 +119,7 @@ jobs: --package-type "${PACKAGE_TYPE}" \ --build-dir "${BUILD_DIR}" \ --pkg-release "${PKG_RELEASE}" \ + --variant "${PACKAGE_VARIANT}" \ --channel "${CHANNEL}" # Before the upload, so the artifact, the tested package and the published @@ -125,14 +131,17 @@ jobs: run: ./package/sign_rpm.py --package-dir "${BUILD_DIR}" # Split from the debug symbols, which are an order of magnitude larger, so - # that test-install downloads only what it installs. + # that test-install downloads only what it installs. In the globs below the + # version follows the name, separated by '_' in a DEB and '-' in an RPM. A + # version starts with a digit and a longer name does not, so that one digit + # is what tells 'xrpld-3.4.1-...' from 'xrpld-assert-3.4.1-...'. - name: Upload package artifact uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: ${{ matrix.xrpld_artifact_name }}-pkg path: | - ${{ env.BUILD_DIR }}/debbuild/xrpld_*.deb - ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/xrpld-[0-9]*.rpm + ${{ env.BUILD_DIR }}/debbuild/${{ matrix.package_name }}_[0-9]*.deb + ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/${{ matrix.package_name }}-[0-9]*.rpm if-no-files-found: error - name: Upload debug symbol artifact @@ -140,112 +149,52 @@ jobs: with: name: ${{ matrix.xrpld_artifact_name }}-pkg-debug path: | - ${{ env.BUILD_DIR }}/debbuild/xrpld-dbgsym_*.deb - ${{ env.BUILD_DIR }}/debbuild/xrpld-dbgsym_*.ddeb - ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/xrpld-debuginfo-*.rpm + ${{ env.BUILD_DIR }}/debbuild/${{ matrix.package_name }}-dbgsym_[0-9]*.deb + ${{ env.BUILD_DIR }}/debbuild/${{ matrix.package_name }}-dbgsym_[0-9]*.ddeb + ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/${{ matrix.package_name }}-debuginfo-[0-9]*.rpm if-no-files-found: error - # Every distro family the packages target, oldest release first, so both ends - # of the dependency range they declare are exercised. - test-install: - needs: [package] - strategy: - fail-fast: false - matrix: - include: - - package_type: deb - image: debian:11 - - package_type: deb - image: debian:12 - - package_type: deb - image: debian:13 - - package_type: deb - image: ubuntu:20.04 - - package_type: deb - image: ubuntu:22.04 - - package_type: deb - image: ubuntu:24.04 - - package_type: deb - image: ubuntu:26.04 + # One call per format, so a variant packaged for one format is installed for + # that format alone. The images are every distro family that format targets, + # oldest release first, so both ends of the dependency range the packages + # declare are exercised. + test-install-deb: + needs: [generate-matrix, package] + name: install deb + uses: ./.github/workflows/reusable-package-test-install.yml + with: + package_type: deb + package_names: ${{ needs.generate-matrix.outputs.deb_package_names }} + images: | + [ + "debian:11", + "debian:12", + "debian:13", + "ubuntu:20.04", + "ubuntu:22.04", + "ubuntu:24.04", + "ubuntu:26.04" + ] - - package_type: rpm - image: almalinux:9 - - package_type: rpm - image: almalinux:10 - - package_type: rpm - image: rockylinux/rockylinux:9 - - package_type: rpm - image: rockylinux/rockylinux:10 - - package_type: rpm - image: registry.access.redhat.com/ubi9/ubi - - package_type: rpm - image: registry.access.redhat.com/ubi10/ubi - name: "install ${{ matrix.package_type }} on ${{ matrix.image }}" - permissions: - contents: read - runs-on: ubuntu-latest - container: ${{ matrix.image }} - timeout-minutes: 5 - - steps: - # Both formats land in one directory; the step below picks its own by - # extension, so this stays independent of the artifact names. - - name: Download package artifacts - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 - with: - pattern: "*-pkg" - merge-multiple: true - path: ${{ env.PACKAGE_DIR }} - - - name: Find the package - id: find - env: - PACKAGE_TYPE: ${{ matrix.package_type }} - run: | - package="$(find "${PACKAGE_DIR}" -type f -name "*.${PACKAGE_TYPE}" -print -quit)" - test -n "${package}" || { - echo "no .${PACKAGE_TYPE} found in ${PACKAGE_DIR}" >&2 - exit 1 - } - echo "package=${package}" >>"${GITHUB_OUTPUT}" - - - name: Install the DEB - if: ${{ matrix.package_type == 'deb' }} - env: - DEBIAN_FRONTEND: noninteractive - PACKAGE: ${{ steps.find.outputs.package }} - run: | - # Stock Debian and Ubuntu images carry no package lists, so apt has - # nothing to resolve the systemd dependency from until it fetches them. - apt-get update -qq - apt-get install -y "./${PACKAGE}" - - - name: Install the RPM - if: ${{ matrix.package_type == 'rpm' }} - env: - PACKAGE: ${{ steps.find.outputs.package }} - run: dnf install -y "./${PACKAGE}" - - - name: Run xrpld - run: xrpld --version - - - name: Run validator-keys - run: validator-keys --version - - - name: Run rippled, the legacy compatibility symlink - run: rippled --version - - - name: Check the service account - run: id xrpld - - - name: Check the state directory - run: test -d /var/lib/xrpld - - - name: Check the log directory - run: test -d /var/log/xrpld + test-install-rpm: + needs: [generate-matrix, package] + name: install rpm + uses: ./.github/workflows/reusable-package-test-install.yml + with: + package_type: rpm + package_names: ${{ needs.generate-matrix.outputs.rpm_package_names }} + images: | + [ + "almalinux:9", + "almalinux:10", + "rockylinux/rockylinux:9", + "rockylinux/rockylinux:10", + "registry.access.redhat.com/ubi9/ubi", + "registry.access.redhat.com/ubi10/ubi" + ] publish: - needs: [generate-matrix, package, test-install] + needs: [generate-matrix, package, test-install-deb, test-install-rpm] strategy: fail-fast: false matrix: ${{ fromJson(needs.generate-matrix.outputs.matrix) }} diff --git a/cmake/XrplPackaging.cmake b/cmake/XrplPackaging.cmake index e2f7029ad2..0fdeae0d0d 100644 --- a/cmake/XrplPackaging.cmake +++ b/cmake/XrplPackaging.cmake @@ -44,12 +44,18 @@ else() set(pkg_type rpm) endif() +# Unquoted below, so an empty value adds no argument at all. +set(pkg_variant_option "") +if(assert) + set(pkg_variant_option --variant=assert) +endif() + add_custom_target( package COMMAND ${CMAKE_SOURCE_DIR}/package/build_pkg.py --package-type=${pkg_type} --build-dir=${CMAKE_BINARY_DIR} --pkg-release=${pkg_release} - --channel=UNRELEASED + ${pkg_variant_option} --channel=UNRELEASED WORKING_DIRECTORY ${CMAKE_BINARY_DIR} DEPENDS xrpld validator-keys COMMENT "Building Linux ${pkg_type} package" diff --git a/docs/install.md b/docs/install.md index 4c52b587b6..34606fbe11 100644 --- a/docs/install.md +++ b/docs/install.md @@ -6,7 +6,8 @@ `xrpld` is published as DEB and RPM packages for 64-bit x86 Linux. Use APT on Debian-based distributions such as Debian and Ubuntu, -and YUM on Red Hat-based distributions such as RHEL, AlmaLinux, and Rocky Linux. +and DNF on Red Hat-based distributions such as RHEL, AlmaLinux, and Rocky Linux, +where `yum` is a symlink to `dnf`. To build from source instead, see [BUILD.md](../BUILD.md). ## Release channels @@ -81,7 +82,7 @@ wherever it appears in the repository configuration. sudo apt -y install xrpld ``` -### With the YUM package manager +### With the DNF package manager 1. Add the XRPL Foundation package-signing key: @@ -109,9 +110,23 @@ wherever it appears in the repository configuration. 3. Install the `xrpld` package: ```bash - sudo yum install -y xrpld + sudo dnf install -y xrpld ``` +### Optional: the assert-enabled build + +Every channel also carries `xrpld-assert` as a DEB, the same build with assertions +enabled, for diagnosing a problem on a non-production server. +It installs the same files as `xrpld` and replaces it, so install one or the other: + +```bash +sudo apt -y install xrpld-assert # APT removes xrpld itself +``` + +Switching stops the service, since it is a removal and an installation rather than an upgrade, +and APT starts it again. +Install `xrpld` the same way to switch back. + ## The xrpld service Both package managers install a systemd unit and enable it, so `xrpld` starts on boot. @@ -121,7 +136,7 @@ Check whether it is already running: systemctl status xrpld.service ``` -The APT packages start it immediately as well; the YUM packages do not, so start it yourself: +The DEB packages start it immediately as well; the RPM packages do not, so start it yourself: ```bash sudo systemctl start xrpld.service diff --git a/package/README.md b/package/README.md index 6e88309ecd..57db577029 100644 --- a/package/README.md +++ b/package/README.md @@ -15,7 +15,8 @@ package/ publish_pkg.py Uploads built packages to the XRPLF Nexus repositories (called by CI, and shipped in that image) rpm/ xrpld.spec RPM spec - debian/ Debian control files (control, rules, copyright, xrpld.docs, xrpld.links, xrpld.lintian-overrides, source/format) + debian/ Debian control files (control.in, lintian-overrides.in, rules, copyright, docs, links, source/format). + The `.in` files are templates rendered by `build_pkg.py`; `docs` and `links` are staged under the package name shared/ xrpld.service systemd unit file (used by both RPM and DEB) xrpld.sysusers sysusers.d config (used by both RPM and DEB) @@ -32,20 +33,74 @@ packaging job cannot drift apart. Today only `linux/amd64` is emitted. The map pins the full container image in `image` — edit that field to move to a new image and both CI and local builds pick it up — and names the format that image builds in `type`, which CI passes to `build_pkg.py` as `--package-type`; the two -have to stay in step. +have to stay in step. An optional `variant` names a flavour of the package (see +[Package variants](#package-variants)), and CI passes it as `--variant`. | Package type | Image (`configs.[].package.image` in `linux.json`) | Tools required | | ------------ | ---------------------------------------------------------- | -------------------------------------------------------------- | | RPM | `ghcr.io/xrplf/xrpld/packaging-rhel:sha-` | `rpmbuild`, `rpmsign` | | DEB | `ghcr.io/xrplf/xrpld/packaging-debian:sha-` | `dpkg-buildpackage`, debhelper with compat level 13, `lintian` | -To print the full packaging matrix (artifact names and images) for the current -`linux.json`: +To print the full packaging matrix (artifact names, images and package names) +for the current `linux.json`: ```bash ./.github/scripts/strategy-matrix/generate.py --packaging ``` +## Package variants + +A config whose binaries are not the plain release build cannot be packaged as +`xrpld`: both would carry the same name and version, so whichever published last +would win. It is packaged as a **variant** instead — `variant: "assert"` in its +`package` map, which CI passes to `build_pkg.py` as `--variant assert`, +producing `xrpld-assert`. What the build option itself does is a build concern, +not a packaging one; see the options table in [`BUILD.md`](../BUILD.md). + +A variant ships the same paths as `xrpld` — `/usr/bin/xrpld`, `/etc/xrpld`, +`xrpld.service`, `/etc/logrotate.d/xrpld` — differing only in the per-package +documentation directory, so it declares itself a stand-in for the plain package +rather than something installable next to it: `Conflicts`, `Replaces` and a +versioned `Provides: xrpld` on Debian, `Conflicts` and `Provides` on RPM. +Neither format declares `Obsoletes`, so `apt upgrade` and `dnf upgrade` keep an +installed flavour on its own flavour, and switching is always explicit: + +```bash +apt-get install xrpld-assert # apt removes the plain package itself +dnf swap xrpld xrpld-VARIANT # 'dnf install' alone stops at the conflict +``` + +Only the DEB packages carry a variant today — `xrpld-assert` comes from the +`debian` config alone, there being no call for an assert build on RHEL-based +distributions — but the RPM side works the same way if one is added. + +A switch is a removal plus an installation rather than an upgrade, so unlike a +version upgrade it stops the service: Debian's scriptlets start it again, while +on RPM the operator runs `systemctl start xrpld`. Configuration survives either +way, being conffiles on Debian and `%config(noreplace)` on RPM. + +`dnf` installs the replacement before erasing the old flavour, whose `%preun` +would leave `xrpld.service` disabled, so `%postun` re-applies the preset when +the unit file outlives the erase — which, since rpm keeps a file another +installed package owns, happens only during a swap. The cost is that a +deliberate `systemctl disable` is not carried across an RPM switch. + +The alternative is an `xrpld-common` package owning the unit, the sysusers and +tmpfiles snippets and the configuration, required by both flavours at an exact +version: nothing is erased mid-swap, so no scriptlet has to detect one. It is +not worth it for a single variant — it moves files out of the production +package, and a sanitizer flavour would likely need its own unit anyway, putting +the lifecycle back where it is now. + +Adding a variant is the flavour in `VARIANTS` in `build_pkg.py`, which is the +list `--variant` accepts, plus a config in `linux.json` with the CMake arguments +and a `package` map naming it, for one format or for both: `generate.py +--packaging` emits the package names per format, and the `test-install-deb` and +`test-install-rpm` jobs install what their own format produced. + +Operators switch between the flavours as described in +[`docs/install.md`](../docs/install.md#optional-the-assert-enabled-build). + ## Building packages ### Via CI @@ -56,9 +111,11 @@ Caller workflows (`on-pr.yml`, `on-tag.yml`, `on-trigger.yml`) call 1. `package` fans out one job per config carrying a `package` map, building and signing in that config's container, and uploading `-pkg` alongside `-pkg-debug` for the much larger debug symbols. -2. `test-install` installs `-pkg` in the container of every distro the - packages target and runs the binaries there, so one that cannot be installed - never reaches Nexus. +2. `test-install-deb` and `test-install-rpm` call + [`reusable-package-test-install.yml`](../.github/workflows/reusable-package-test-install.yml) + with their format's package names and distro images, installing each package + in the container of every distro that format targets and running the binaries + there, so one that cannot be installed never reaches Nexus. 3. `publish` uploads both artifacts, or lists what it would upload. The packaging script derives the package version from the downloaded binary's @@ -104,6 +161,9 @@ docker run --rm \ # build/rpmbuild/RPMS/x86_64/*.rpm ``` +Add `--variant assert` to package binaries built with `-Dassert=ON`; the package +is then named `xrpld-assert`. + ### Via CMake (host-side target) If you run CMake configure on a host that has `rpmbuild` or `dpkg-buildpackage` @@ -133,6 +193,9 @@ The package version is not a CMake input on this path: `build_pkg.py` derives it from the just-built `xrpld` binary's `xrpld --version` output. The package release defaults to 1 and is overridable with `-Dpkg_release=N`. +`-Dassert=ON` passes `--variant assert`, so such a build packages as +`xrpld-assert` without anything else being asked for. + ## Publishing packages Packages are published to the XRPLF repositories on Sonatype Nexus at @@ -147,6 +210,9 @@ the event, and `publish_pkg.py` maps that channel to its repositories: | push to `develop` | `xrpld --version` | `develop` | `deb-develop` | `rpm-develop-hosted` | | tag, non-public codebase | _any_ | `private` | `deb-private` | `rpm-private-hosted` | +A variant is published to the same channel under its own name, so +`xrpld-assert` never overwrites `xrpld`. + Only a tag names a channel — do not extend that to `develop`, where `BuildInfo.cpp`'s `versionString` moves through `-bN`, `-rcN` and even the final version during a release cycle, which would send develop builds into `stable`. @@ -160,7 +226,7 @@ the last, and the date and hash say which commit a package on `packages.xrplf.org` came from. Both reach the packaging scripts as arguments, so neither script derives anything itself. -Publishing is its own job, gated behind `test-install`, uploading from the same +Publishing is its own job, gated behind the install tests, uploading from the same image that built the packages with the `publish_pkg.py` shipped in it — the same copy other repositories run. Without `publish: true` the job is a `--dry-run`, listing the uploads it would make without needing credentials, so @@ -175,7 +241,7 @@ Nexus owns the repository metadata; nothing here indexes anything. Worth knowing - Each apt-hosted repository needs a distribution (ours use `any`) and a PGP signing keypair configured in Nexus, which rejects one created without a keypair. Nexus signs the apt metadata with it, never the packages. -- Hosted yum repositories cannot be signed by Nexus, so each `rpm--hosted` +- yum-hosted repositories cannot be signed by Nexus, so each `rpm--hosted` repository sits behind a `rpm-` yum group repository whose metadata Nexus signs. Uploads go to the hosted repository; clients point at the group and verify the metadata with `repo_gpgcheck=1`. Nexus never signs the RPMs @@ -244,6 +310,19 @@ pre-release ordering convention, so RPM filenames/NVRs begin with forms like `xrpld-3.2.0~b1-...` and `xrpld-3.2.0~rc1-...` instead of encoding pre-releases with an older `0..` RPM `Release` value. +`--variant` is the flavour of the package, empty by default and accepting only +the flavours in `VARIANTS`; see [Package variants](#package-variants). The RPM +path passes it to the spec as the `pkg_variant` macro, which suffixes `Name` and +adds the `Conflicts`/`Provides` pair. Debian control files have no conditionals, so the DEB path renders +`debian/control.in` and `debian/lintian-overrides.in` instead, substituting +`@PKG@` with the package name and `@VARIANT_FIELDS@` with the +`Conflicts`/`Replaces`/`Provides` block, empty for the plain package; a token +with no value fails the build rather than reaching dpkg. The files debhelper +keys by package name (`docs`, `links`, and the units) are staged under that same +name. The paths inside the package are unchanged either way, so `debian/rules` +reads its package name from `dh_listpackages` and names the unit, sysusers, +tmpfiles and logrotate files with `--name xrpld`. + The package format is `--package-type`, either `deb` or `rpm`. It is required, so a job never silently builds the wrong format for the image it runs in; the matching build tool still has to be on PATH. @@ -286,8 +365,13 @@ service restart. 1. Creates a staging source tree at `debbuild/source/` inside the build directory. 2. Stages the binaries, configs, `README.md`, `LICENSE.md`, and `validator-keys-LICENSE`. -3. Copies `package/debian/` control files into `debbuild/source/debian/`. -4. Copies shared service/sysusers/tmpfiles/logrotate into `debian/` where `dh_installsystemd`, `dh_installsysusers`, `dh_installtmpfiles` and `dh_installlogrotate` pick them up automatically. +3. Stages `package/debian/` into `debbuild/source/debian/`: the `.in` templates + are rendered, and the files debhelper keys by package name (`docs`, `links`, + `lintian-overrides`) are staged under the name being built. +4. Copies shared service/sysusers/tmpfiles/logrotate into `debian/` as + `.xrpld.*`, which `dh_installsystemd`, `dh_installsysusers`, + `dh_installtmpfiles` and `dh_installlogrotate` read because `debian/rules` + passes them `--name xrpld`. 5. Generates a minimal `debian/changelog` using `${pkg_version}-${PKG_RELEASE}`, where `pkg_version` is derived from the binary-reported `xrpld` version. 6. Runs `dpkg-buildpackage -b --no-sign -d` (`-d` skips the build-dependency check, since the binary is already built). `debian/rules` uses manual `install` commands. diff --git a/package/build_pkg.py b/package/build_pkg.py index 1aaf53d5ff..77ef8f3120 100755 --- a/package/build_pkg.py +++ b/package/build_pkg.py @@ -21,6 +21,14 @@ SRC_DIR = Path(__file__).resolve().parents[1] PRE_RELEASE = re.compile(r"^(b|rc)(0|[1-9][0-9]*)(\+.*)?$") +# The package name a variant suffixes, and the name every variant keeps for its +# on-disk paths (/usr/bin/xrpld, /etc/xrpld, xrpld.service). +BASE_NAME = "xrpld" + +# The flavours that can be built, '' being the plain xrpld package. A variant +# needs a config in linux.json to be built by CI; see package/README.md. +VARIANTS = ("", "assert") + # Files both packaging systems consume, staged under the same names. STAGED_FROM_BUILD = ("xrpld", "validator-keys", "validator-keys-LICENSE") STAGED_FROM_SRC = { @@ -31,6 +39,18 @@ STAGED_FROM_SRC = { } STAGED_UNITS = ("xrpld.service", "xrpld.sysusers", "xrpld.tmpfiles", "xrpld.logrotate") +# debian/ files debhelper keys by package name, staged as '.'. +DEBIAN_PKG_FILES = ("docs", "links") + +# Debian control files have no conditionals, so what makes a variant replace the +# plain package is rendered into control.in rather than written there. +DEB_VARIANT_FIELDS = """\ +Conflicts: xrpld +Replaces: xrpld +Provides: xrpld (= ${binary:Version})""" + +TOKEN = re.compile(r"@[A-Z_]+@") + def run(*command: object, cwd: Path | None = None) -> None: """Echo a command and run it.""" @@ -75,6 +95,28 @@ def package_version(reported: str) -> str: return version +def render(template: Path, dest: Path, values: dict[str, str]) -> None: + """Write template to dest with its @TOKEN@ placeholders substituted. + + A token left without a value fails the build rather than reaching dpkg. + """ + text = template.read_text() + for token, value in values.items(): + text = text.replace(f"@{token}@", value) + + missing = sorted(set(TOKEN.findall(text))) + assert not missing, f"{template}: no value for {', '.join(missing)}" + + # An empty value at the end of a stanza would otherwise leave a blank line, + # which is what ends a stanza. + dest.write_text(text.rstrip("\n") + "\n") + + +def package_name(variant: str) -> str: + """The binary package name for a variant: '' -> xrpld, 'assert' -> xrpld-assert.""" + return f"{BASE_NAME}-{variant}" if variant else BASE_NAME + + def read_version(xrpld: Path) -> str: """Read the version from the binary that is about to be packaged.""" fields = capture(xrpld, "--version").partition("\n")[0].split() @@ -135,17 +177,18 @@ def stage_common(build_dir: Path, dest: Path) -> None: shutil.copy2(SRC_DIR / source, dest / name) -def stage_units(dest: Path) -> None: +def stage_units(dest: Path, *, prefix: str = "") -> None: """Copy the systemd, sysusers, tmpfiles and logrotate files into dest. - Each format wants them somewhere else: rpmbuild reads them from SOURCES, - debhelper from debian/. + Each format wants them somewhere else: rpmbuild reads them from SOURCES by + path, debhelper from debian/ by package name -- hence 'prefix', which makes + the copies 'xrpld-assert.xrpld.service' and so on. """ for name in STAGED_UNITS: - shutil.copy2(SRC_DIR / "package" / "shared" / name, dest / name) + shutil.copy2(SRC_DIR / "package" / "shared" / name, dest / f"{prefix}{name}") -def build_rpm(build_dir: Path, *, version: str, pkg_release: str) -> None: +def build_rpm(build_dir: Path, *, version: str, pkg_release: str, variant: str) -> None: """Stage the spec and its sources, then build the binary RPMs.""" topdir = build_dir / "rpmbuild" for name in ("BUILD", "BUILDROOT", "RPMS", "SOURCES", "SPECS", "SRPMS"): @@ -156,6 +199,9 @@ def build_rpm(build_dir: Path, *, version: str, pkg_release: str) -> None: stage_common(build_dir, topdir / "SOURCES") stage_units(topdir / "SOURCES") + # The spec defaults it to nothing, so a plain build is unchanged. + variant_defines = ["--define", f"pkg_variant {variant}"] if variant else [] + run( "rpmbuild", "-bb", @@ -168,10 +214,29 @@ def build_rpm(build_dir: Path, *, version: str, pkg_release: str) -> None: # The image tracks the newest distro, but the packages target el9. "--define", "dist .el9", + *variant_defines, spec, ) +def stage_debian(dest: Path, name: str) -> None: + """Stage the debian directory for the package name being built.""" + source = SRC_DIR / "package" / "debian" + shutil.copytree( + source, dest, ignore=shutil.ignore_patterns("*.in", *DEBIAN_PKG_FILES) + ) + + values = { + "PKG": name, + "VARIANT_FIELDS": "" if name == BASE_NAME else DEB_VARIANT_FIELDS, + } + render(source / "control.in", dest / "control", values) + render(source / "lintian-overrides.in", dest / f"{name}.lintian-overrides", values) + + for suffix in DEBIAN_PKG_FILES: + shutil.copy2(source / suffix, dest / f"{name}.{suffix}") + + def build_deb( build_dir: Path, *, @@ -180,21 +245,23 @@ def build_deb( pkg_release: str, channel: str, epoch: int, + name: str, ) -> None: """Stage the debian directory and its sources, then build the binary DEBs.""" staging = build_dir / "debbuild" / "source" stage_common(build_dir, staging) - shutil.copytree(SRC_DIR / "package" / "debian", staging / "debian") + stage_debian(staging / "debian", name) - # debhelper picks these up from debian/ automatically. - stage_units(staging / "debian") + # Prefixed whether it is a variant's name or not: debian/rules names them + # explicitly either way. + stage_units(staging / "debian", prefix=f"{name}.") date = datetime.fromtimestamp(epoch, timezone.utc).strftime( "%a, %d %b %Y %H:%M:%S %z" ) # The leading spaces are significant to dpkg. changelog = textwrap.dedent(f"""\ - xrpld ({version}-{pkg_release}) {channel}; urgency=medium + {name} ({version}-{pkg_release}) {channel}; urgency=medium * Release {reported}. -- XRPL Foundation {date} @@ -223,6 +290,14 @@ def main() -> None: default="1", help="package release iteration (default: %(default)s)", ) + parser.add_argument( + "--variant", + default="", + choices=VARIANTS, + help="the flavour of the package to build: 'assert' produces " + "xrpld-assert, which ships the same paths as xrpld and replaces it " + "(default: the plain xrpld package)", + ) parser.add_argument( "--channel", required=True, @@ -234,6 +309,8 @@ def main() -> None: build_dir: Path = args.build_dir.resolve() pkg_release: str = args.pkg_release channel: str = args.channel + variant: str = args.variant + name = package_name(variant) assert build_dir.is_dir(), ( f"build directory not found: {build_dir}. Build the binaries before " @@ -253,6 +330,8 @@ def main() -> None: for tree in ("debbuild", "rpmbuild"): shutil.rmtree(build_dir / tree, ignore_errors=True) + print(f"Building {package_type} {name} {version}-{pkg_release}", flush=True) + if package_type == "deb": build_deb( build_dir, @@ -261,9 +340,10 @@ def main() -> None: pkg_release=pkg_release, channel=channel, epoch=epoch, + name=name, ) else: - build_rpm(build_dir, version=version, pkg_release=pkg_release) + build_rpm(build_dir, version=version, pkg_release=pkg_release, variant=variant) if __name__ == "__main__": diff --git a/package/debian/control b/package/debian/control.in similarity index 93% rename from package/debian/control rename to package/debian/control.in index 359f39f770..20486efc9a 100644 --- a/package/debian/control +++ b/package/debian/control.in @@ -1,4 +1,4 @@ -Source: xrpld +Source: @PKG@ Section: net Priority: optional Maintainer: XRPL Foundation @@ -11,7 +11,7 @@ Homepage: https://github.com/XRPLF/rippled Vcs-Git: https://github.com/XRPLF/rippled.git Vcs-Browser: https://github.com/XRPLF/rippled -Package: xrpld +Package: @PKG@ Architecture: any Depends: ${shlibs:Depends}, @@ -22,3 +22,4 @@ Description: XRP Ledger daemon transactions, and maintains the ledger database. This package also includes the validator-keys tool for validator key management. +@VARIANT_FIELDS@ diff --git a/package/debian/xrpld.docs b/package/debian/docs similarity index 100% rename from package/debian/xrpld.docs rename to package/debian/docs diff --git a/package/debian/xrpld.links b/package/debian/links similarity index 100% rename from package/debian/xrpld.links rename to package/debian/links diff --git a/package/debian/lintian-overrides.in b/package/debian/lintian-overrides.in new file mode 100644 index 0000000000..5e72a5ef6b --- /dev/null +++ b/package/debian/lintian-overrides.in @@ -0,0 +1,6 @@ +# The /usr/local/bin/rippled symlink is deliberate compatibility for pre-FHS +# layouts, so the Policy 9.1.2 tags it raises are expected. +# TODO: remove alongside debian/links after rippled fully deprecated. +@PKG@: dir-in-usr-local [usr/local/bin/] +@PKG@: file-in-usr-local [usr/local/bin/rippled] +@PKG@: file-in-unusual-dir [usr/local/bin/rippled] diff --git a/package/debian/rules b/package/debian/rules index dd6d1e66b9..bc12f54218 100755 --- a/package/debian/rules +++ b/package/debian/rules @@ -8,33 +8,58 @@ export DH_VERBOSE = 1 # the binaries actually run on. LIBC_MIN = 2.31 +# The binary package's name, which a variant build changes to e.g. xrpld-assert, +# and the directory debhelper expects its files staged in. +PKG := $(firstword $(shell dh_listpackages)) +PKG_DIR = debian/$(PKG) + +# The base name, which every package ships under whatever it is called itself. +BASE_NAME = xrpld + +# What build_pkg.py stages beside this directory, each installed under its own +# name. The binaries are also the ones checked against LIBC_MIN below. +BINARIES = $(BASE_NAME) validator-keys +CONFIGS = $(BASE_NAME).cfg validators.txt + %: dh $@ override_dh_auto_configure override_dh_auto_build override_dh_auto_test: @: +# The unit, sysusers, tmpfiles and logrotate files are named after the daemon +# rather than after the package, so a variant still ships xrpld.service and +# /etc/logrotate.d/xrpld. debhelper only reads debian/$(PKG).$(BASE_NAME).* when told +# the name. override_dh_installsystemd: - dh_installsystemd --no-stop-on-upgrade xrpld.service + dh_installsystemd --no-stop-on-upgrade --name $(BASE_NAME) # The tmpfiles snippet sets ownership to the xrpld user, so the sysusers snippet # has to be emitted first: run it early and make its own sequence slot a no-op. execute_before_dh_installtmpfiles: - dh_installsysusers + dh_installsysusers --name $(BASE_NAME) override_dh_installsysusers: +override_dh_installtmpfiles: + dh_installtmpfiles --name $(BASE_NAME) + +override_dh_installlogrotate: + dh_installlogrotate --name $(BASE_NAME) + override_dh_install: - install -D -m 0755 xrpld debian/xrpld/usr/bin/xrpld - install -D -m 0755 validator-keys debian/xrpld/usr/bin/validator-keys - install -D -m 0644 xrpld.cfg debian/xrpld/etc/xrpld/xrpld.cfg - install -D -m 0644 validators.txt debian/xrpld/etc/xrpld/validators.txt + for binary in $(BINARIES); do \ + install -D -m 0755 "$$binary" "$(PKG_DIR)/usr/bin/$$binary"; \ + done + for config in $(CONFIGS); do \ + install -D -m 0644 "$$config" "$(PKG_DIR)/etc/$(BASE_NAME)/$$config"; \ + done override_dh_shlibdeps: dh_shlibdeps # Guards against the toolchain moving past LIBC_MIN and the packages then # claiming a floor they do not meet. - for binary in xrpld validator-keys; do \ + for binary in $(BINARIES); do \ needed=$$(readelf --dyn-syms --wide $$binary \ | grep -o 'GLIBC_[0-9.]*' | sed 's/GLIBC_//' | sort -uV | tail -1); \ if [ -z "$$needed" ]; then \ @@ -46,7 +71,7 @@ override_dh_shlibdeps: exit 1; \ fi; \ done - sed -i 's/libc6 (>= [0-9.]*)/libc6 (>= $(LIBC_MIN))/' debian/xrpld.substvars + sed -i 's/libc6 (>= [0-9.]*)/libc6 (>= $(LIBC_MIN))/' debian/$(PKG).substvars override_dh_dwz: @: diff --git a/package/debian/xrpld.lintian-overrides b/package/debian/xrpld.lintian-overrides deleted file mode 100644 index a0b3f583ed..0000000000 --- a/package/debian/xrpld.lintian-overrides +++ /dev/null @@ -1,6 +0,0 @@ -# The /usr/local/bin/rippled symlink is deliberate compatibility for pre-FHS -# layouts, so the Policy 9.1.2 tags it raises are expected. -# TODO: remove alongside debian/xrpld.links after rippled fully deprecated. -xrpld: dir-in-usr-local [usr/local/bin/] -xrpld: file-in-usr-local [usr/local/bin/rippled] -xrpld: file-in-unusual-dir [usr/local/bin/rippled] diff --git a/package/rpm/xrpld.spec b/package/rpm/xrpld.spec index 5139cd54e5..45e7a78e42 100644 --- a/package/rpm/xrpld.spec +++ b/package/rpm/xrpld.spec @@ -6,10 +6,14 @@ %{error:pkg_release must be defined} %endif -Name: xrpld +# The base name, which every package ships under. A variant build +# (build_pkg.py --variant) only suffixes the package name, e.g. xrpld-assert. +%global base_name xrpld + +Name: %{base_name}%{?pkg_variant:-%{pkg_variant}} Version: %{pkg_version} Release: %{pkg_release}%{?dist} -Summary: XRP Ledger daemon +Summary: XRP Ledger daemon%{?pkg_variant: (%{pkg_variant} build)} License: ISC URL: https://github.com/XRPLF/rippled @@ -17,6 +21,12 @@ URL: https://github.com/XRPLF/rippled ExclusiveArch: x86_64 aarch64 BuildRequires: systemd-rpm-macros +# A variant owns the same paths, so it stands in for the plain package. +%if "%{?pkg_variant}" != "" +Conflicts: %{base_name} +Provides: %{base_name} = %{version}-%{release} +%endif + # These have to precede %%debug_package: it opens the debuginfo subpackage, and # any tag after it is silently dropped from the main package. %{?systemd_requires} @@ -52,22 +62,22 @@ management. : %install -install -Dm0755 %{_sourcedir}/xrpld %{buildroot}%{_bindir}/%{name} +install -Dm0755 %{_sourcedir}/xrpld %{buildroot}%{_bindir}/%{base_name} install -Dm0755 %{_sourcedir}/validator-keys %{buildroot}%{_bindir}/validator-keys -install -Dm0644 %{_sourcedir}/xrpld.cfg %{buildroot}%{_sysconfdir}/%{name}/xrpld.cfg -install -Dm0644 %{_sourcedir}/validators.txt %{buildroot}%{_sysconfdir}/%{name}/validators.txt +install -Dm0644 %{_sourcedir}/xrpld.cfg %{buildroot}%{_sysconfdir}/%{base_name}/xrpld.cfg +install -Dm0644 %{_sourcedir}/validators.txt %{buildroot}%{_sysconfdir}/%{base_name}/validators.txt # systemd units, sysusers, tmpfiles, preset install -Dm0644 %{_sourcedir}/xrpld.service %{buildroot}%{_unitdir}/xrpld.service install -Dm0644 %{_sourcedir}/xrpld.sysusers %{buildroot}%{_sysusersdir}/xrpld.conf install -Dm0644 %{_sourcedir}/xrpld.tmpfiles %{buildroot}%{_tmpfilesdir}/xrpld.conf install -d %{buildroot}%{_presetdir} -cat >%{buildroot}%{_presetdir}/50-xrpld.preset <<'EOF' +cat >%{buildroot}%{_presetdir}/50-%{base_name}.preset <<'EOF' enable xrpld.service EOF # Logrotate config -install -Dm0644 %{_sourcedir}/xrpld.logrotate %{buildroot}%{_sysconfdir}/logrotate.d/%{name} +install -Dm0644 %{_sourcedir}/xrpld.logrotate %{buildroot}%{_sysconfdir}/logrotate.d/%{base_name} # Docs install -Dm0644 %{_sourcedir}/LICENSE.md %{buildroot}%{_docdir}/%{name}/LICENSE.md @@ -78,13 +88,13 @@ install -Dm0644 %{_sourcedir}/validator-keys-LICENSE %{buildroot}%{_docdir}/%{na # Legacy compatibility for pre-FHS package layouts. # TODO: remove after rippled fully deprecated. install -d %{buildroot}/usr/local/bin -ln -s %{_bindir}/%{name} %{buildroot}/usr/local/bin/rippled +ln -s %{_bindir}/%{base_name} %{buildroot}/usr/local/bin/rippled %pre -%sysusers_create_package %{name} %{_sourcedir}/xrpld.sysusers +%sysusers_create_package %{base_name} %{_sourcedir}/xrpld.sysusers %post -%tmpfiles_create_package %{name} %{_sourcedir}/xrpld.tmpfiles +%tmpfiles_create_package %{base_name} %{_sourcedir}/xrpld.tmpfiles %systemd_post xrpld.service %preun @@ -92,6 +102,13 @@ ln -s %{_bindir}/%{name} %{buildroot}/usr/local/bin/rippled %postun %systemd_postun xrpld.service +# A flavour swap installs the replacement before erasing this package, so the +# %%preun above has just disabled a unit the replacement still owns. rpm keeps a +# file that another installed package owns, so the unit outliving our own erase +# means exactly that; a plain erase takes it with us and re-presets nothing. +if [ $1 -eq 0 ] && [ -f %{_unitdir}/xrpld.service ]; then + systemctl preset xrpld.service >/dev/null 2>&1 || : +fi %files %attr(0755,root,root) %dir %{_docdir}/%{name} @@ -99,18 +116,18 @@ ln -s %{_bindir}/%{name} %{buildroot}/usr/local/bin/rippled %license %{_docdir}/%{name}/validator-keys-LICENSE %doc %{_docdir}/%{name}/README.md -%attr(0755,root,root) %dir %{_sysconfdir}/%{name} +%attr(0755,root,root) %dir %{_sysconfdir}/%{base_name} -%{_bindir}/%{name} +%{_bindir}/%{base_name} %{_bindir}/validator-keys -%config(noreplace) %{_sysconfdir}/%{name}/xrpld.cfg -%config(noreplace) %{_sysconfdir}/%{name}/validators.txt -%config(noreplace) %{_sysconfdir}/logrotate.d/%{name} +%config(noreplace) %{_sysconfdir}/%{base_name}/xrpld.cfg +%config(noreplace) %{_sysconfdir}/%{base_name}/validators.txt +%config(noreplace) %{_sysconfdir}/logrotate.d/%{base_name} %{_unitdir}/xrpld.service -%attr(0644,root,root) %{_presetdir}/50-xrpld.preset +%attr(0644,root,root) %{_presetdir}/50-%{base_name}.preset %{_sysusersdir}/xrpld.conf %{_tmpfilesdir}/xrpld.conf %ghost %dir /var/lib/xrpld From 76da5d447577237734b4b7ca64b733f453f86c70 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 8 Sep 2026 23:40:59 +0100 Subject: [PATCH 09/16] build: Fix test installation on debian:11 due to EOL --- .../workflows/reusable-package-test-install.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.github/workflows/reusable-package-test-install.yml b/.github/workflows/reusable-package-test-install.yml index 84db0b666d..4f1b6d7cb5 100644 --- a/.github/workflows/reusable-package-test-install.yml +++ b/.github/workflows/reusable-package-test-install.yml @@ -67,6 +67,23 @@ jobs: } echo "package=${package}" >>"${GITHUB_OUTPUT}" + # Debian 11 went end-of-life on 2026-08-31 + # (https://www.debian.org/News/2026/20260831) and its packages are + # already partly gone from deb.debian.org, so switch to the + # snapshot.debian.org entries the image ships commented out in its + # sources.list: they are pinned to the snapshot the image was built + # from, so they serve every version it needs and never go away. + # Snapshots keep their original, long-passed Valid-Until, hence the + # disabled check; the retries absorb snapshot.debian.org's throttling. + - name: Switch Debian 11 to snapshot.debian.org + if: ${{ matrix.image == 'debian:11' }} + run: | + sed -i 's|^deb |# deb |; s|^# deb http://snapshot|deb http://snapshot|' /etc/apt/sources.list + printf '%s\n' \ + 'Acquire::Check-Valid-Until "false";' \ + 'Acquire::Retries "3";' \ + >/etc/apt/apt.conf.d/99snapshot + - name: Install the DEB if: ${{ inputs.package_type == 'deb' }} env: From 3e4e56d6bbedf659718524775b085dbb9638f148 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Wed, 9 Sep 2026 17:35:07 +0200 Subject: [PATCH 10/16] fix: Make calculateBaseFee exception-safe --- include/xrpl/tx/applySteps.h | 18 +++-- src/libxrpl/tx/applySteps.cpp | 50 ++++++++++--- .../tx/transactors/lending/LoanPay.cpp | 23 +++++- src/libxrpl/tx/transactors/system/Batch.cpp | 15 +++- src/xrpld/app/misc/NetworkOPs.cpp | 20 ++++-- src/xrpld/app/misc/TxQ.h | 5 +- src/xrpld/app/misc/detail/TxQ.cpp | 71 +++++++++++++++---- src/xrpld/rpc/detail/TransactionSign.cpp | 5 +- 8 files changed, 162 insertions(+), 45 deletions(-) diff --git a/include/xrpl/tx/applySteps.h b/include/xrpl/tx/applySteps.h index bd495481f2..0a1ae9fa3a 100644 --- a/include/xrpl/tx/applySteps.h +++ b/include/xrpl/tx/applySteps.h @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -393,16 +394,21 @@ preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, Open * * No validation is done or implied by this function. * - * Caller is responsible for handling any exceptions. - * Since none should be thrown, that will usually - * mean terminating. - * + * Callers do not expect this function to throw; exceptions from a transactor's + * `calculateBaseFee` are caught and reported as an error instead. * @param view The current open ledger. * @param tx The transaction to be checked. * - * @return The base fee. + * @return The base fee on success. Returns `std::unexpected(temUNKNOWN)` if the transaction + * type is not recognized, and `std::unexpected(tefEXCEPTION)` if the transactor's + * `calculateBaseFee` threw. + * + * @note Failure is reported as an error rather than a fee of zero because a + * zero (or default) fee would pass checkFee and let the transaction be + * applied for less than it owes. Callers that only need a fee hint may fall + * back to a default; callers deciding whether to apply should reject. */ -XRPAmount +[[nodiscard]] std::expected calculateBaseFee(ReadView const& view, STTx const& tx); /** diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 2c05c874d3..00ac9f9983 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -195,7 +196,12 @@ invokePreclaim(PreclaimContext const& ctx) }()) return preSigResult; - if (TER const result = T::checkFee(ctx, calculateBaseFee(ctx.view, ctx.tx))) + // We can't check the fee if we can't compute it, so reject. + auto const baseFee = calculateBaseFee(ctx.view, ctx.tx); + if (!baseFee) + return baseFee.error(); + + if (TER const result = T::checkFee(ctx, *baseFee)) return result; } @@ -223,13 +229,12 @@ invokePreclaim(PreclaimContext const& ctx) * * @param view The ledger view to use for fee calculation. * @param tx The transaction for which the base fee is to be calculated. - * @return The calculated base fee as an XRPAmount. + * @return The calculated base fee. Returns `std::unexpected(temUNKNOWN)` if the transaction + * type is not recognized, and `std::unexpected(tefEXCEPTION)` if the transactor's + * `calculateBaseFee` threw. * - * @throws std::exception If an error occurs during fee calculation, including - * but not limited to unknown transaction types or internal errors, the function - * logs an error and returns an XRPAmount of zero. */ -static XRPAmount +static std::expected invokeCalculateBaseFee(ReadView const& view, STTx const& tx) { try @@ -238,13 +243,25 @@ invokeCalculateBaseFee(ReadView const& view, STTx const& tx) return T::calculateBaseFee(view, tx); }); } - catch (UnknownTxnType const& e) + catch (UnknownTxnType const&) { // LCOV_EXCL_START UNREACHABLE("xrpl::invoke_calculateBaseFee : unknown transaction type"); - return XRPAmount{0}; + return std::unexpected(temUNKNOWN); // LCOV_EXCL_STOP } + catch (std::exception const& e) + { + JLOG(debugLog().error()) << "calculateBaseFee: " << tx.getTransactionID() + << " threw an exception: " << e.what(); + return std::unexpected(tefEXCEPTION); + } + catch (...) + { + JLOG(debugLog().error()) << "calculateBaseFee: " << tx.getTransactionID() + << " threw an unknown exception"; + return std::unexpected(tefEXCEPTION); + } } TxConsequences::TxConsequences(NotTEC pfResult) @@ -416,7 +433,7 @@ preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, Open } } -XRPAmount +std::expected calculateBaseFee(ReadView const& view, STTx const& tx) { return invokeCalculateBaseFee(view, tx); @@ -441,13 +458,26 @@ doApply(PreclaimResult const& preclaimResult, ServiceRegistry& registry, OpenVie { if (!preclaimResult.likelyToClaimFee) return {preclaimResult.ter, false}; + + // For any tx with a real account, preclaim already computed this fee + // successfully against this same view. + auto const baseFee = calculateBaseFee(view, preclaimResult.tx); + if (!baseFee) + { + // LCOV_EXCL_START + JLOG(preclaimResult.j.error()) + << "apply: could not compute base fee: " << transToken(baseFee.error()); + return {tefINTERNAL, false}; + // LCOV_EXCL_STOP + } + ApplyContext ctx( registry, view, preclaimResult.parentBatchId, preclaimResult.tx, preclaimResult.ter, - calculateBaseFee(view, preclaimResult.tx), + *baseFee, preclaimResult.flags, preclaimResult.j); return invokeApply(ctx); diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index 18886b2682..624c4d0a84 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -36,6 +36,15 @@ namespace xrpl { namespace { +// Returns true if the transaction's payment amount is malformed. A loan +// payment must be strictly positive: zero would move nothing, and a negative +// amount is not a payment at all. +bool +isPaymentAmountInvalid(STAmount const& amount) +{ + return amount <= beast::kZero; +} + // Returns the account's true, unclamped balance in `asset`, for use only in // fund-conservation checks. accountHolds(..., SpendableHandling::FullBalance) // cannot be used for this: for XRP it always defers to xrpLiquid, which @@ -81,7 +90,7 @@ LoanPay::preflight(PreflightContext const& ctx) if (ctx.tx[sfLoanID] == beast::kZero) return temINVALID; - if (ctx.tx[sfAmount] <= beast::kZero) + if (isPaymentAmountInvalid(ctx.tx[sfAmount])) return temBAD_AMOUNT; // The loan payment flags are all mutually exclusive. If more than one is @@ -103,10 +112,19 @@ LoanPay::preflight(PreflightContext const& ctx) XRPAmount LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) { + auto fixEnabled313 = view.rules().enabled(fixCleanup3_1_3); + auto fixEnabled340 = view.rules().enabled(fixCleanup3_4_0); + using namespace lending; auto const normalCost = Transactor::calculateBaseFee(view, tx); + if (fixEnabled340 && isPaymentAmountInvalid(tx[sfAmount])) + { + // Let preflight worry about the error for this + return normalCost; + } + if (tx.isFlag(tfLoanFullPayment) || tx.isFlag(tfLoanLatePayment)) { // The loan will be making one set of calculations for one full or late @@ -179,8 +197,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) static constexpr std::int64_t kMaxFeeIncrements = kLoanMaximumPaymentsPerTransaction / kLoanPaymentsPerFeeIncrement; - if (view.rules().enabled(fixCleanup3_1_3) && - amount >= regularPayment * kLoanMaximumPaymentsPerTransaction) + if (fixEnabled313 && amount >= regularPayment * kLoanMaximumPaymentsPerTransaction) { // The payment handler will never process more than // loanMaximumPaymentsPerTransaction payments (including overpayments), diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index ccb113e07b..dcd06453aa 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -73,14 +73,23 @@ Batch::calculateBaseFeeImpl(ReadView const& view, STTx const& tx) for (auto const& stx : tx.getBatchTransactions()) { auto const fee = xrpl::calculateBaseFee(view, *stx); - // LCOV_EXCL_START - if (txnFees > maxAmount - fee) + if (!fee) { + JLOG(debugLog().error()) + << "BatchTrace: base fee of inner transaction " << stx->getTransactionID() + << " could not be computed: " << transToken(fee.error()); + return std::nullopt; + } + + // LCOV_EXCL_START + if (txnFees > maxAmount - *fee) + { + UNREACHABLE("XRPAmount overflow in txnFees calculation"); JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in txnFees calculation."; return std::nullopt; } // LCOV_EXCL_STOP - txnFees += fee; + txnFees += *fee; } // Calculate the Signers/BatchSigners Fees diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index a440c9ad1c..0dc6c8dcac 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1886,11 +1886,21 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) if (validatedLedgerIndex) { - auto [fee, accountSeq, availableSeq] = - registry_.get().getTxQ().getTxRequiredFeeAndSeq( - *newOL, e.transaction->getSTransaction()); - e.transaction->setCurrentLedgerState( - *validatedLedgerIndex, fee, accountSeq, availableSeq); + auto maybeFeeAndSeq = registry_.get().getTxQ().getTxRequiredFeeAndSeq( + *newOL, e.transaction->getSTransaction()); + if (maybeFeeAndSeq.has_value()) + { + auto [fee, accountSeq, availableSeq] = *maybeFeeAndSeq; + e.transaction->setCurrentLedgerState( + *validatedLedgerIndex, fee, accountSeq, availableSeq); + } + else + { + JLOG(journal_.debug()) + << "Unable to compute current ledger state for tx " + << e.transaction->getID() << " in validated ledger " + << *validatedLedgerIndex << ": " << transToken(maybeFeeAndSeq.error()); + } } } } diff --git a/src/xrpld/app/misc/TxQ.h b/src/xrpld/app/misc/TxQ.h index 65b4e9778c..4e6e9edbac 100644 --- a/src/xrpld/app/misc/TxQ.h +++ b/src/xrpld/app/misc/TxQ.h @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -390,10 +391,10 @@ public: * and first available sequence for transaction * @param view current open ledger * @param tx the transaction - * @return minimum required fee, first sequence in the ledger + * @return minimum required fee or an error, first sequence in the ledger * and first available sequence */ - FeeAndSeq + std::expected getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr const& tx) const; /** diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index b9cfc1d65c..19dc351444 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -54,22 +55,29 @@ namespace xrpl { ////////////////////////////////////////////////////////////////////////// -static FeeLevel64 +/** + * Compute the fee level that a transaction pays. + * @return The fee level paid, or the error reported by `calculateBaseFee`. + */ +static std::expected getFeeLevelPaid(ReadView const& view, STTx const& tx) { - auto const [baseFee, effectiveFeePaid] = [&view, &tx]() { - XRPAmount const baseFee = calculateBaseFee(view, tx); + auto const computedBaseFee = calculateBaseFee(view, tx); + if (!computedBaseFee) + return std::unexpected(computedBaseFee.error()); + + auto const [baseFee, effectiveFeePaid] = [&view, &tx, fee = *computedBaseFee]() { XRPAmount const feePaid = tx[sfFee].xrp(); // If baseFee is 0 then the cost of a basic transaction is free, but we // need the effective fee level to be non-zero. - XRPAmount const mod = [&view, &tx, baseFee]() { - if (baseFee.signum() > 0) + XRPAmount const mod = [&view, &tx, fee]() { + if (fee.signum() > 0) return XRPAmount{0}; auto def = calculateDefaultBaseFee(view, tx); return def.signum() == 0 ? XRPAmount{1} : def; }(); - return std::pair{baseFee + mod, feePaid + mod}; + return std::pair{fee + mod, feePaid + mod}; }(); XRPL_ASSERT(baseFee.signum() > 0, "xrpl::getFeeLevelPaid : positive fee"); @@ -112,10 +120,20 @@ TxQ::FeeMetrics::update( auto const size = std::distance(txBegin, txEnd); feeLevels.reserve(size); std::for_each(txBegin, txEnd, [&](auto const& tx) { - feeLevels.push_back(getFeeLevelPaid(view, *tx.first)); + auto const maybeFeeLevel = getFeeLevelPaid(view, *tx.first); + if (maybeFeeLevel.has_value()) + { + feeLevels.push_back(*maybeFeeLevel); + } + else + { + // Excluded from the median sample below. + JLOG(j_.warn()) << "Unable to compute the fee level for a validated transaction " + << tx.first->getTransactionID() << " in ledger " << view.header().seq + << ": " << transToken(maybeFeeLevel.error()); + } }); std::ranges::sort(feeLevels); - XRPL_ASSERT(size == feeLevels.size(), "xrpl::TxQ::FeeMetrics::update : fee levels size"); JLOG((timeLeap ? j_.warn() : j_.debug())) << "Ledger " << view.header().seq << " has " << size << " transactions. " @@ -159,7 +177,10 @@ TxQ::FeeMetrics::update( txnsExpected_ = std::min(next, maximumTxnCount_.value_or(next)); } - if (size == 0) + // The median is taken over the transactions whose fee level could be + // computed, while txnsExpected_ above deliberately uses the full + // transaction count. + if (feeLevels.empty()) { escalationMultiplier_ = setup.minimumEscalationMultiplier; } @@ -169,8 +190,9 @@ TxQ::FeeMetrics::update( // evaluates to the middle element; for an even // number of elements, it will add the two elements // on either side of the "middle" and average them. + auto const count = feeLevels.size(); escalationMultiplier_ = - (feeLevels[size / 2] + feeLevels[(size - 1) / 2] + FeeLevel64{1}) / 2; + (feeLevels[count / 2] + feeLevels[(count - 1) / 2] + FeeLevel64{1}) / 2; escalationMultiplier_ = std::max(escalationMultiplier_, setup.minimumEscalationMultiplier); } JLOG(j_.debug()) << "Expected transactions updated to " << txnsExpected_ @@ -878,7 +900,14 @@ TxQ::apply( // We may need the base fee for multiple transactions or transaction // replacement, so just pull it up now. auto const metricsSnapshot = feeMetrics_.getSnapshot(); - auto const feeLevelPaid = getFeeLevelPaid(view, *tx); + auto const computedFeeLevelPaid = getFeeLevelPaid(view, *tx); + // Without a fee level there is no way to tell whether the transaction + // pays enough, so it can be neither applied nor queued. + if (!computedFeeLevelPaid.has_value()) + { + return {computedFeeLevelPaid.error(), false}; + } + FeeLevel64 const feeLevelPaid = *computedFeeLevelPaid; auto const requiredFeeLevel = getRequiredFeeLevel(view, flags, metricsSnapshot, lock); // Is there a blocker already in the account's queue? If so, don't @@ -1684,7 +1713,14 @@ TxQ::tryDirectApply( // If the transaction's fee is high enough we may be able to put the // transaction straight into the ledger. - FeeLevel64 const feeLevelPaid = getFeeLevelPaid(view, *tx); + auto const computedFeeLevelPaid = getFeeLevelPaid(view, *tx); + // The fee level is unknown, so the transaction cannot be applied here, + // and queueing it would only run into the same failure. Reject it. + if (!computedFeeLevelPaid.has_value()) + { + return ApplyResult{computedFeeLevelPaid.error(), false}; + } + FeeLevel64 const feeLevelPaid = *computedFeeLevelPaid; if (feeLevelPaid >= requiredFeeLevel) { @@ -1768,7 +1804,7 @@ TxQ::getMetrics(OpenView const& view) const return result; } -TxQ::FeeAndSeq +std::expected TxQ::getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr const& tx) const { auto const account = (*tx)[sfAccount]; @@ -1776,14 +1812,19 @@ TxQ::getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr co std::scoped_lock const lock(mutex_); auto const snapshot = feeMetrics_.getSnapshot(); - auto const baseFee = calculateBaseFee(view, *tx); + auto const maybeBaseFee = calculateBaseFee(view, *tx); + if (!maybeBaseFee.has_value()) + { + return std::unexpected(maybeBaseFee.error()); + } + auto const baseFee = *maybeBaseFee; auto const fee = FeeMetrics::scaleFeeLevel(snapshot, view); auto const sle = view.read(keylet::account(account)); std::uint32_t const accountSeq = sle ? (*sle)[sfSequence] : 0; std::uint32_t const availableSeq = nextQueuableSeqImpl(sle, lock).value(); - return { + return FeeAndSeq{ .fee = mulDiv(fee, baseFee, kBaseLevel) .value_or(XRPAmount(std::numeric_limits::max())), .accountSeq = accountSeq, diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 3e9f62214e..6ea879aa2c 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -887,7 +887,10 @@ getTxFee(Application const& app, Config const& config, json::Value tx) if (!passesLocalChecks(stTx, reason)) return config.fees.referenceFee; - return calculateBaseFee(*app.getOpenLedger().current(), stTx); + // This fee is only a suggestion returned to the caller, so fall back to + // the reference fee as the other failure paths in this function do. + return calculateBaseFee(*app.getOpenLedger().current(), stTx) + .value_or(config.fees.referenceFee); } catch (std::exception& e) { From ebd810b184e67b67cbb7cc8dac8b9298de66a168 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Thu, 10 Sep 2026 16:21:52 +0100 Subject: [PATCH 11/16] build: Add missing script to conan package --- conanfile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/conanfile.py b/conanfile.py index 0683a3779f..52d4da4286 100644 --- a/conanfile.py +++ b/conanfile.py @@ -149,6 +149,7 @@ class Xrpl(ConanFile): self.requires("xxhash/0.8.3", transitive_headers=True) exports_sources = ( + "bin/default-loader-path.sh", "CMakeLists.txt", "cfg/*", "cmake/*", From 8c594c7ed94373203c7acb7099a5a3b25ab25635 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 10 Sep 2026 18:39:33 +0100 Subject: [PATCH 12/16] fix: Skip CheckCash limit waiver for the issuer --- src/libxrpl/tx/transactors/check/CheckCash.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libxrpl/tx/transactors/check/CheckCash.cpp b/src/libxrpl/tx/transactors/check/CheckCash.cpp index 857f759752..f753f604f0 100644 --- a/src/libxrpl/tx/transactors/check/CheckCash.cpp +++ b/src/libxrpl/tx/transactors/check/CheckCash.cpp @@ -439,6 +439,12 @@ CheckCash::doApply() AccountID const& deliverIssuer = flowDeliver.getIssuer(); auto const err = flowDeliver.asset().visit( [&](Issue const& issue) -> std::optional { + // An issuer needs no holder-limit waiver to receive its own currency. + if (deliverIssuer == accountID_ && ctx_.view().rules().enabled(fixCleanup3_4_0)) + { + return std::nullopt; + } + // If a trust line does not exist yet create one. Issue const& trustLineIssue = issue; AccountID const truster = deliverIssuer == accountID_ ? srcId : accountID_; From a18839d92d40aaf6b0303f52ee9842b96ecba908 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:59:08 +0200 Subject: [PATCH 13/16] fix: Relax MPT authorize cap for LoanSet and VaultWithdraw --- src/libxrpl/tx/invariants/MPTInvariant.cpp | 59 ++++++---- .../app/invariants/InvariantsMPT_test.cpp | 69 ++++++++++++ src/test/app/lending/LoanSet_test.cpp | 102 ++++++++++++++++++ src/test/app/vault/VaultBugs_test.cpp | 12 +-- src/test/app/vault/VaultLifecycle_test.cpp | 80 ++++++++++++++ 5 files changed, 294 insertions(+), 28 deletions(-) diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index e38e8f2b93..46d1037acf 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -299,27 +299,46 @@ ValidMPTIssuance::finalize( return false; } } - else if (lendingProtocolEnabled && (mptokensCreated_ + mptokensDeleted_) > 1) + else { - JLOG(j.fatal()) << "Invariant failed: MPT authorize succeeded " - "but created/deleted bad number mptokens"; - return false; - } - else if (submittedByIssuer && (mptokensCreated_ > 0 || mptokensDeleted_ > 0)) - { - JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by issuer " - "succeeded but created/deleted mptokens"; - return false; - } - else if ( - !submittedByIssuer && hasPrivilege(tx, Privilege::MustAuthorizeMpt) && - (mptokensCreated_ + mptokensDeleted_ != 1)) - { - // if the holder submitted this tx, then a mptoken must be - // either created or deleted. - JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by holder " - "succeeded but created/deleted bad number of mptokens"; - return false; + // Cap on MPToken creates and deletes while featureLendingProtocol is enabled. + // - LoanSet: at most two creates and no deletes. + // - VaultWithdraw: at most one create and one delete. + // - Other MayAuthorizeMpt types: created + deleted <= 1. + // - MustAuthorizeMpt still requires exactly one create or delete below. + auto const mptokensExceedAuthorizeCap = [&] { + if (!lendingProtocolEnabled) + return false; + if (rules.enabled(fixCleanup3_4_0)) + { + if (txnType == ttLOAN_SET) + return mptokensDeleted_ != 0 || mptokensCreated_ > 2; + if (txnType == ttVAULT_WITHDRAW) + return mptokensCreated_ > 1 || mptokensDeleted_ > 1; + } + return (mptokensCreated_ + mptokensDeleted_) > 1; + }; + if (mptokensExceedAuthorizeCap()) + { + JLOG(j.fatal()) << "Invariant failed: MPT authorize succeeded " + "but created/deleted bad number mptokens"; + return false; + } + if (submittedByIssuer && (mptokensCreated_ > 0 || mptokensDeleted_ > 0)) + { + JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by issuer " + "succeeded but created/deleted mptokens"; + return false; + } + if (!submittedByIssuer && hasPrivilege(tx, Privilege::MustAuthorizeMpt) && + (mptokensCreated_ + mptokensDeleted_ != 1)) + { + // if the holder submitted this tx, then a mptoken must be + // either created or deleted. + JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by holder " + "succeeded but created/deleted bad number of mptokens"; + return false; + } } return true; diff --git a/src/test/app/invariants/InvariantsMPT_test.cpp b/src/test/app/invariants/InvariantsMPT_test.cpp index 4692463baa..91984f2021 100644 --- a/src/test/app/invariants/InvariantsMPT_test.cpp +++ b/src/test/app/invariants/InvariantsMPT_test.cpp @@ -967,6 +967,75 @@ class InvariantsMPT_test : public InvariantsBase }); } + // LoanSet / VaultWithdraw MayAuthorizeMpt caps (fixCleanup3_4_0): + // LoanSet allows at most two creates and no deletes; VaultWithdraw + // allows at most one of each. Fabricate one extra mutation so a + // too-loose cap would miss these. + { + auto const insertHolderTokens = + [](Account const& issuer, Account const& holder, ApplyContext& ac, int n) { + auto const sle = ac.view().peek(keylet::account(issuer.id())); + if (!sle) + return false; + auto seq = sle->getFieldU32(sfSequence); + for (int i = 0; i < n; ++i) + { + MPTIssue const mpt{makeMptID(seq + i, issuer)}; + auto sleNew = + std::make_shared(keylet::mptoken(mpt.getMptID(), holder)); + (*sleNew)[sfAccount] = holder.id(); + (*sleNew)[sfMPTokenIssuanceID] = mpt.getMptID(); + ac.view().insert(sleNew); + } + return true; + }; + + std::array, 2> const createOverCap{ + {{ttLOAN_SET, 3}, {ttVAULT_WITHDRAW, 2}}}; + for (auto const& [txnType, nTokens] : createOverCap) + { + doInvariantCheck( + {{"MPT authorize succeeded but created/deleted bad number mptokens"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + return insertHolderTokens(a1, a2, ac, nTokens); + }, + XRPAmount{}, + STTx{txnType, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}); + } + + MPTID id; + auto const precloseTwoHolders = [&id](Account const& a1, Account const& a2, Env& env) { + Account const gw("gw"); + env.fund(XRP(1'000), gw); + MPTTester const mpt({.env = env, .issuer = gw, .holders = {a1, a2}}); + id = mpt.issuanceID(); + return true; + }; + std::array, 2> const deleteOverCap{ + {{ttLOAN_SET, 1}, {ttVAULT_WITHDRAW, 2}}}; + for (auto const& [txnType, nTokens] : deleteOverCap) + { + doInvariantCheck( + {{"MPT authorize succeeded but created/deleted bad number mptokens"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + std::array const holders{a1, a2}; + for (int i = 0; i < nTokens; ++i) + { + auto sle = ac.view().peek(keylet::mptoken(id, holders[i])); + if (!sle) + return false; + ac.view().erase(sle); + } + return true; + }, + XRPAmount{}, + STTx{txnType, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + precloseTwoHolders); + } + } + // sfReferenceHolding can only be set on creation by VaultCreate. A // non-VaultCreate transaction that creates an MPTokenIssuance with // sfReferenceHolding present must trip the invariant. diff --git a/src/test/app/lending/LoanSet_test.cpp b/src/test/app/lending/LoanSet_test.cpp index 5eea6f83fe..469c1662ad 100644 --- a/src/test/app/lending/LoanSet_test.cpp +++ b/src/test/app/lending/LoanSet_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -597,6 +598,105 @@ private: nullptr); } + void + testLoanSetOriginationFeeTwoMptCreates(FeatureBitset features) + { + using namespace jtx; + using namespace loan; + + bool const fix340Enabled = features[fixCleanup3_4_0]; + testcase << "LoanSet: borrower and broker owner missing MPToken" + << (fix340Enabled ? "" : " pre-fixCleanup3_4_0"); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + Env env(*this, features); + env.fund(XRP(1'000'000), issuer, lender, borrower); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanTransfer | tfMPTCanLock}); + env.close(); + PrettyAsset const asset = mptt.issuanceID(); + mptt.authorize({.account = lender}); + mptt.authorize({.account = borrower}); + env.close(); + + env(pay(issuer, lender, asset(10'000'000))); + env.close(); + + auto const broker = createVaultAndBroker(env, asset, lender); + + // Delete borrower's asset MPToken. + mptt.authorize({.account = borrower, .flags = tfMPTUnauthorize}); + env.close(); + + // Pay out and delete the broker owner's asset MPToken. + auto const lenderMPToken = keylet::mptoken(mptt.issuanceID(), lender); + auto const sleLenderMPT = env.le(lenderMPToken); + if (!BEAST_EXPECT(sleLenderMPT)) + return; + env(pay(lender, issuer, asset(sleLenderMPT->at(sfMPTAmount)))); + env.close(); + mptt.authorize({.account = lender, .flags = tfMPTUnauthorize}); + env.close(); + + auto const borrowerMPToken = keylet::mptoken(mptt.issuanceID(), borrower); + auto const brokerKeylet = keylet::loanBroker(broker.brokerID); + auto const sleBrokerBefore = env.le(brokerKeylet); + if (!BEAST_EXPECT(sleBrokerBefore)) + return; + auto const loanSequence = sleBrokerBefore->at(sfLoanSequence); + auto const debtTotalBefore = sleBrokerBefore->at(sfDebtTotal); + auto const loanKeylet = keylet::loan(broker.brokerID, SeqProxy::rawSequence(loanSequence)); + + auto const sleVaultBefore = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(sleVaultBefore)) + return; + auto const assetsAvailableBefore = sleVaultBefore->at(sfAssetsAvailable); + + env(set(borrower, broker.brokerID, asset(1'000).value()), + kLoanOriginationFee(asset(1).value()), + kCounterparty(lender), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 5), + Ter{fix340Enabled ? TER{tesSUCCESS} : TER{tecINVARIANT_FAILED}}); + env.close(); + + auto const sleBorrowerAfter = env.le(borrowerMPToken); + auto const sleLenderAfter = env.le(lenderMPToken); + auto const sleLoanAfter = env.le(loanKeylet); + auto const sleBrokerAfter = env.le(brokerKeylet); + auto const sleVaultAfter = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(sleVaultAfter)) + return; + if (fix340Enabled) + { + if (!BEAST_EXPECT(sleBorrowerAfter && sleLenderAfter && sleLoanAfter && sleBrokerAfter)) + return; + BEAST_EXPECT(sleBorrowerAfter->at(sfMPTAmount) == 999); + BEAST_EXPECT(sleLenderAfter->at(sfMPTAmount) == 1); + BEAST_EXPECT(sleLoanAfter->at(sfPrincipalOutstanding) == Number{1'000}); + BEAST_EXPECT(sleBrokerAfter->at(sfLoanSequence) == loanSequence + 1); + BEAST_EXPECT( + sleVaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore - Number{1'000}); + } + else + { + // The whole transaction must roll back. + BEAST_EXPECT(!sleBorrowerAfter); + BEAST_EXPECT(!sleLenderAfter); + BEAST_EXPECT(!sleLoanAfter); + if (!BEAST_EXPECT(sleBrokerAfter)) + return; + BEAST_EXPECT(sleBrokerAfter->at(sfLoanSequence) == loanSequence); + BEAST_EXPECT(sleBrokerAfter->at(sfDebtTotal) == debtTotalBefore); + BEAST_EXPECT(sleVaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore); + } + } + // LoanSet in a closed-ended vault — phase gating and maturity bound. void testLoanSetClosedEnded() @@ -838,6 +938,8 @@ public: testLoanSetClosedEnded(); testLoanSetExistingLineAfterIssuerClearsDefaultRipple(); + testLoanSetOriginationFeeTwoMptCreates(all_); + testLoanSetOriginationFeeTwoMptCreates(all_ - fixCleanup3_4_0); } }; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index cc30bd6091..43f2b0354c 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1586,14 +1586,10 @@ private: // which for an integral MPT asset the destination check would reject if // it were reached. // - // ValidMPTIssuance is a separate checker and still runs. It only trips on - // the one arm that both creates and deletes an MPToken: Alice's last - // share with the asset MPToken missing, where addEmptyHolding creates the - // asset token while her share token is deleted (created + deleted > 1). - // Leftover shares with the token missing is create-only, and a last share - // with the token present is delete-only; neither exceeds one. Bob still - // owns shares throughout, so this is never the vault's final outstanding - // share. + // ValidMPTIssuance: pre-fixCleanup3_4_0, a VaultWithdraw that both + // creates and deletes an MPToken fails. Post-fixCleanup3_4_0 that is + // allowed. + // // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero // payout and zeroDeltaIsLegitimate lets the vault-delta and diff --git a/src/test/app/vault/VaultLifecycle_test.cpp b/src/test/app/vault/VaultLifecycle_test.cpp index ce91ca857a..8d7afe67f2 100644 --- a/src/test/app/vault/VaultLifecycle_test.cpp +++ b/src/test/app/vault/VaultLifecycle_test.cpp @@ -795,6 +795,86 @@ private: }, {.requireAuth = false}); + auto const redeemAllNoAssetMpt = [this](TER expected) { + return [this, expected]( + Env& env, + Account const&, + Account const& owner, + Account const& depositor, + Asset const& asset, + Vault& vault, + MPTTester& mptt) { + testcase << "MPT non-owner redeems all shares with no asset MPToken" + << (isTesSuccess(expected) ? "" : " pre-fixCleanup3_4_0"); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + + tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(1000)}); // all assets held by depositor + env(tx); + env.close(); + + auto const vaultSle = env.le(keylet); + if (!BEAST_EXPECT(vaultSle)) + return; + auto const shareMPTID = vaultSle->at(sfShareMPTID); + + // Depositor's asset MPToken balance is now zero; delete it. + mptt.authorize({.account = depositor, .flags = tfMPTUnauthorize}); + env.close(); + + auto const mptoken = keylet::mptoken(mptt.issuanceID(), depositor); + + auto const shareKeylet = keylet::mptoken(shareMPTID, depositor.id()); + auto const sleShareBefore = env.le(shareKeylet); + if (!BEAST_EXPECT(sleShareBefore)) + return; + auto const shareAmountBefore = sleShareBefore->at(sfMPTAmount); + auto const assetsTotalBefore = vaultSle->at(sfAssetsTotal); + auto const assetsAvailableBefore = vaultSle->at(sfAssetsAvailable); + + // Redeeming ALL shares in one transaction both erases the + // now-empty share MPToken and re-creates the asset MPToken. + tx = vault.withdraw( + {.depositor = depositor, .id = keylet.key, .amount = asset(1000)}); + env(tx, Ter{expected}); + env.close(); + + auto const sleAsset = env.le(mptoken); + auto const sleShare = env.le(shareKeylet); + auto const vaultAfter = env.le(keylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + if (isTesSuccess(expected)) + { + if (!BEAST_EXPECT(sleAsset)) + return; + BEAST_EXPECT(sleAsset->at(sfMPTAmount) == 1000); + BEAST_EXPECT(!sleShare); + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == beast::kZero); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == beast::kZero); + } + else + { + BEAST_EXPECT(!sleAsset); + if (!BEAST_EXPECT(sleShare)) + return; + BEAST_EXPECT(sleShare->at(sfMPTAmount) == shareAmountBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore); + } + }; + }; + + testCase(redeemAllNoAssetMpt(tesSUCCESS), {.requireAuth = false}); + testCase( + redeemAllNoAssetMpt(tecINVARIANT_FAILED), + {.requireAuth = false, .features = testableAmendments() - fixCleanup3_4_0}); + auto const [acctReserve, incReserve] = [this]() -> std::pair { Env const env{*this, testableAmendments()}; return { From 00eeb0a005ef2f37490963f2ea485b3c1330acdc Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:52:04 +0100 Subject: [PATCH 14/16] fix: Reject variable-length prefixes the encoder cannot write --- include/xrpl/protocol/STValidation.h | 7 + include/xrpl/protocol/Serializer.h | 192 ++++++++++++++++++++++++-- src/libxrpl/protocol/STValidation.cpp | 43 +++++- src/libxrpl/protocol/Serializer.cpp | 143 +++++++++++-------- 4 files changed, 314 insertions(+), 71 deletions(-) diff --git a/include/xrpl/protocol/STValidation.h b/include/xrpl/protocol/STValidation.h index 8101b27341..f70e971f87 100644 --- a/include/xrpl/protocol/STValidation.h +++ b/include/xrpl/protocol/STValidation.h @@ -124,6 +124,13 @@ public: [[nodiscard]] NodeID const& getNodeID() const noexcept; + /** + * Whether this validation carries a good signature. + * + * Reports false if the signature cannot be checked at all, so a caller + * cannot tell that apart from a bad signature. Either way the validation is + * unusable, and the reason is logged. Only a computed answer is remembered. + */ [[nodiscard]] bool isValid() const noexcept; diff --git a/include/xrpl/protocol/Serializer.h b/include/xrpl/protocol/Serializer.h index c1ea5c16ba..997199629a 100644 --- a/include/xrpl/protocol/Serializer.h +++ b/include/xrpl/protocol/Serializer.h @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -25,6 +26,101 @@ private: Blob data_; public: + /** + * A header is never longer than this. The encoder fills a buffer of this + * size and writes only the bytes it used. + */ + static constexpr int kMaxNumberOfBytesInHeader = 3; + + // A field whose size varies is stored as a header holding its length, then + // the field data. The header is 1, 2 or 3 bytes long. Nothing outside it says + // which, so the decoder reads the first byte and its value says how long the + // header is: + // + // 0 ... 192 kMin/kMaxValueOfFirstByteFor1ByteHeader + // 193 ... 240 kMin/kMaxValueOfFirstByteFor2ByteHeader + // 241 ... 254 kMin/kMaxValueOfFirstByteFor3ByteHeader + // 255 belongs to no header + // + // Each range starts one past the end of the range before it. + + static constexpr int kMinValueOfFirstByteFor1ByteHeader = 0; + static constexpr int kMaxValueOfFirstByteFor1ByteHeader = 192; + + static constexpr int kMinValueOfFirstByteFor2ByteHeader = + kMaxValueOfFirstByteFor1ByteHeader + 1; + static constexpr int kMaxValueOfFirstByteFor2ByteHeader = 240; + + static constexpr int kMinValueOfFirstByteFor3ByteHeader = + kMaxValueOfFirstByteFor2ByteHeader + 1; + + static constexpr int kMaxValueOfFirstByteFor3ByteHeader = 254; + + // A length x too big for one byte is split across the header. For 2 bytes: + // + // first byte = 193 + (x - 193) / 256 + // second byte = (x - 193) % 256 + // + // so 300 is stored as 193, 107. For 3 bytes it is the same, from 241, with + // the remainder split across two bytes: 20,000 is stored as 241, 29, 95. + + static constexpr int kNumberOfValuesInOneByte = 256; + static constexpr int kNumberOfValuesInTwoBytes = + kNumberOfValuesInOneByte * kNumberOfValuesInOneByte; + + // Each header length therefore covers a range of field lengths: + // + // 0 ... 192 kMin/kMaxValueOfLengthFor1ByteHeader + // 193 ... 12,480 kMin/kMaxValueOfLengthFor2ByteHeader + // 12,481 ... 918,744 kMin/kMaxValueOfLengthFor3ByteHeader + // + // The encoder always uses the shortest header that fits. + + /** + * A 1 byte header holds the length in the byte itself, so both ends of + * this range are the same numbers as the first byte's own range. + */ + static constexpr int kMinValueOfLengthFor1ByteHeader = kMinValueOfFirstByteFor1ByteHeader; + static constexpr int kMaxValueOfLengthFor1ByteHeader = kMaxValueOfFirstByteFor1ByteHeader; + + static constexpr int kMinValueOfLengthFor2ByteHeader = kMaxValueOfLengthFor1ByteHeader + 1; + + /** + * 48 values of the first byte mean a 2 byte header, and each of them covers + * 256 lengths. The 48 is worked out from the two range ends above, so it + * stays right if either of them changes. + */ + static constexpr int kMaxValueOfLengthFor2ByteHeader = kMinValueOfLengthFor2ByteHeader + + ((kMaxValueOfFirstByteFor2ByteHeader - kMaxValueOfFirstByteFor1ByteHeader) * + kNumberOfValuesInOneByte) - + 1; + + static constexpr int kMinValueOfLengthFor3ByteHeader = kMaxValueOfLengthFor2ByteHeader + 1; + + /** + * 14 values of the first byte mean a 3 byte header, and each of them covers + * 65,536 lengths. Counted the same way, that gives the largest length any + * header can state. + * + * Nothing is accepted or rejected against this. The assertion below uses it + * to check that every length the encoder writes is one a header can state. + */ + static constexpr int kMaxRepresentableLength = kMinValueOfLengthFor3ByteHeader + + ((kMaxValueOfFirstByteFor3ByteHeader - kMaxValueOfFirstByteFor2ByteHeader) * + kNumberOfValuesInTwoBytes) - + 1; + + /** + * The largest length the encoder will write. This is the one number here + * that is picked rather than worked out. The decoder accepts nothing above + * it, so both sides agree on the same set of lengths. + */ + static constexpr int kMaxValueOfLengthFor3ByteHeader = 918744; + + static_assert( + kMaxValueOfLengthFor3ByteHeader <= kMaxRepresentableLength, + "a length the encoder writes must be one a header can state"); + explicit Serializer(int n = 256) { data_.reserve(n); @@ -61,7 +157,7 @@ public: // assemble functions int - add8(unsigned char i); + add8(unsigned char byteValue); int add16(std::uint16_t i); @@ -270,18 +366,90 @@ public: return v.data_ == data_; } + /** + * Works out how long a header is, from its first byte. + * + * Each overload of decodeVLLength below reads one header length, so call + * this first to learn which of them to call. + * + * @param firstByte First byte of the header, as read from the stream. + * @return How many bytes the whole header takes, counting firstByte: 1, 2 + * or 3. + * @throws std::overflow_error if firstByte is the one value that starts no + * header. + */ static int - decodeLengthLength(int b1); + decodeLengthLength(std::byte firstByte); + + /** + * Reads the field length out of a 1 byte header. + * + * @param firstByte The single header byte, which is the length itself. + * @return Field length in bytes, from kMinValueOfLengthFor1ByteHeader to + * kMaxValueOfLengthFor1ByteHeader. + * @throws std::overflow_error if firstByte is big enough to mean a longer + * header, in which case it is not a length by itself. + */ static int - decodeVLLength(int b1); + decodeVLLength(std::byte firstByte); + + /** + * Reads the field length out of a 2 byte header. + * + * @param firstByte First header byte. Its value means a 2 byte header, and + * how far it sits into that range gives the top part of the length. + * @param secondByte Second header byte, holding the rest of the length. + * @return Field length in bytes, from kMinValueOfLengthFor2ByteHeader to + * kMaxValueOfLengthFor2ByteHeader. + * @throws std::overflow_error if firstByte is outside the range that means + * a 2 byte header. + */ static int - decodeVLLength(int b1, int b2); + decodeVLLength(std::byte firstByte, std::byte secondByte); + + /** + * Reads the field length out of a 3 byte header. + * + * @param firstByte First header byte. Its value means a 3 byte header, and + * how far it sits into that range gives the top part of the length. + * @param secondByte Second header byte, holding the middle part of the + * length. + * @param thirdByte Third header byte, holding the low part. + * @return Field length in bytes, from kMinValueOfLengthFor3ByteHeader to + * kMaxValueOfLengthFor3ByteHeader. + * @throws std::overflow_error if firstByte is outside the range that means + * a 3 byte header, or if the three bytes together state a length above + * kMaxValueOfLengthFor3ByteHeader, which the encoder would not write back. + */ static int - decodeVLLength(int b1, int b2, int b3); + decodeVLLength(std::byte firstByte, std::byte secondByte, std::byte thirdByte); private: + /** + * Works out how many bytes the header needs for the given length. + * + * This deliberately repeats the width choice addEncoded makes, so that + * addVL's assertion can compare the two. It has no other caller; do not + * reach for it as a utility. + * + * @param length Field length in bytes. + * @return How many header bytes it needs: 1, 2 or 3. + * @throws std::overflow_error if length is negative, or above + * kMaxValueOfLengthFor3ByteHeader. + */ static int - encodeLengthLength(int length); // length to encode length + encodeLengthLength(int length); + + /** + * Appends the length header for a field of the given length. + * + * The field's own data is not written; the caller appends it next. + * + * @param length Field length in bytes. + * @return Offset within this Serializer at which the header was written. + * @throws std::overflow_error if length is negative, or above + * kMaxValueOfLengthFor3ByteHeader. + */ int addEncoded(int length); }; @@ -390,9 +558,15 @@ public: void getFieldID(int& type, int& name); - // Returns the size of the VL if the - // next object is a VL. Advances the iterator - // to the beginning of the VL. + /** + * Reads the length header at the read position and steps past it. + * + * @return Field length in bytes. The iterator is left on the first byte of + * the field data. + * @throws std::overflow_error if the header states a length the encoder could + * not have written. + * @throws std::runtime_error if the data runs out before the header does. + */ int getVLDataLength(); diff --git a/src/libxrpl/protocol/STValidation.cpp b/src/libxrpl/protocol/STValidation.cpp index 1656aad3a2..cf02fdacf7 100644 --- a/src/libxrpl/protocol/STValidation.cpp +++ b/src/libxrpl/protocol/STValidation.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include +#include #include namespace xrpl { @@ -104,11 +106,42 @@ STValidation::isValid() const noexcept publicKeyType(getSignerPublic()) == KeyType::Secp256k1, "xrpl::STValidation::isValid : valid key type"); - valid_ = verifyDigest( - getSignerPublic(), - getSigningHash(), - makeSlice(getFieldVL(sfSignature)), - (getFlags() & kVfFullyCanonicalSig) != 0u); + // Log that the signature was never checked, so an operator does not + // read this as a bad key. The log is guarded because it can throw too. + auto reportUncheckable = [this](char const* reason) noexcept { + try + { + JLOG(debugLog().error()) + << "Cannot check the signature of the validation for ledger " << getLedgerHash() + << ": " << reason; + } + catch (...) // NOLINT(bugprone-empty-catch) + { + // Nothing can be reported when reporting is what failed. + } + }; + + // The signing hash re-serializes the fields, which can fail. This + // function is noexcept, so report the validation as invalid instead of + // throwing. valid_ stays unset, so a later call checks again. + try + { + valid_ = verifyDigest( + getSignerPublic(), + getSigningHash(), + makeSlice(getFieldVL(sfSignature)), + (getFlags() & kVfFullyCanonicalSig) != 0u); + } + catch (std::exception const& e) + { + reportUncheckable(e.what()); + return false; + } + catch (...) + { + reportUncheckable("unknown exception"); + return false; + } } return valid_.value(); diff --git a/src/libxrpl/protocol/Serializer.cpp b/src/libxrpl/protocol/Serializer.cpp index 80ecdee6c8..7f6fe625c2 100644 --- a/src/libxrpl/protocol/Serializer.cpp +++ b/src/libxrpl/protocol/Serializer.cpp @@ -143,10 +143,10 @@ Serializer::addFieldID(int type, int name) } int -Serializer::add8(unsigned char byte) +Serializer::add8(unsigned char byteValue) { int const ret = data_.size(); - data_.push_back(byte); + data_.push_back(byteValue); return ret; } @@ -210,109 +210,138 @@ Serializer::addVL(void const* ptr, int len) int Serializer::addEncoded(int length) { - std::array bytes{}; + // Without this, a negative length would fall into the 1 byte case below and + // be cast to a first byte no header uses. A size too big for int arrives + // here negative as well, since callers pass sizes through this parameter. + if (length < kMinValueOfLengthFor1ByteHeader) + Throw("addEncoded: length is negative or did not fit in an int"); + + std::array bytes{}; int numBytes = 0; - if (length <= 192) + if (length <= kMaxValueOfLengthFor1ByteHeader) { - bytes[0] = static_cast(length); + bytes[0] = static_cast(length); numBytes = 1; } - else if (length <= 12480) + else if (length <= kMaxValueOfLengthFor2ByteHeader) { - length -= 193; - bytes[0] = 193 + static_cast(length >> 8); - bytes[1] = static_cast(length & 0xff); + // Count from the smallest length a 2 byte header covers. + int const offset = length - kMinValueOfLengthFor2ByteHeader; + bytes[0] = static_cast( + kMinValueOfFirstByteFor2ByteHeader + (offset / kNumberOfValuesInOneByte)); + bytes[1] = static_cast(offset % kNumberOfValuesInOneByte); numBytes = 2; } - else if (length <= 918744) + else if (length <= kMaxValueOfLengthFor3ByteHeader) { - length -= 12481; - bytes[0] = 241 + static_cast(length >> 16); - bytes[1] = static_cast((length >> 8) & 0xff); - bytes[2] = static_cast(length & 0xff); + int const offset = length - kMinValueOfLengthFor3ByteHeader; + bytes[0] = static_cast( + kMinValueOfFirstByteFor3ByteHeader + (offset / kNumberOfValuesInTwoBytes)); + bytes[1] = + static_cast((offset / kNumberOfValuesInOneByte) % kNumberOfValuesInOneByte); + bytes[2] = static_cast(offset % kNumberOfValuesInOneByte); numBytes = 3; } else { - Throw("lenlen"); + Throw("addEncoded: length is too large to encode"); } - return addRaw(&bytes[0], numBytes); + return addRaw(bytes.data(), numBytes); } int Serializer::encodeLengthLength(int length) { - if (length < 0) - Throw("len<0"); + if (length < kMinValueOfLengthFor1ByteHeader) + { + Throw( + "encodeLengthLength: length is negative or did not fit in an int"); + } - if (length <= 192) + if (length <= kMaxValueOfLengthFor1ByteHeader) return 1; - if (length <= 12480) + if (length <= kMaxValueOfLengthFor2ByteHeader) return 2; - if (length <= 918744) + if (length <= kMaxValueOfLengthFor3ByteHeader) return 3; - Throw("len>918744"); - return 0; // Silence compiler warning. + Throw("encodeLengthLength: length is too large to encode"); } int -Serializer::decodeLengthLength(int b1) +Serializer::decodeLengthLength(std::byte firstByte) { - if (b1 < 0) - Throw("b1<0"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 <= 192) + if (firstByteValue <= kMaxValueOfFirstByteFor1ByteHeader) return 1; - if (b1 <= 240) + if (firstByteValue <= kMaxValueOfFirstByteFor2ByteHeader) return 2; - if (b1 <= 254) + if (firstByteValue <= kMaxValueOfFirstByteFor3ByteHeader) return 3; - Throw("b1>254"); - return 0; // Silence compiler warning. + Throw("decodeLengthLength: first byte does not start any header"); } int -Serializer::decodeVLLength(int b1) +Serializer::decodeVLLength(std::byte firstByte) { - if (b1 < 0) - Throw("b1<0"); + int const length = std::to_integer(firstByte); - if (b1 > 254) - Throw("b1>254"); + // A bigger value means a longer header, so it is not a length by itself. + if (length > kMaxValueOfLengthFor1ByteHeader) + Throw("decodeVLLength 1 byte: first byte is not a length"); - return b1; + return length; } int -Serializer::decodeVLLength(int b1, int b2) +Serializer::decodeVLLength(std::byte firstByte, std::byte secondByte) { - if (b1 < 193) - Throw("b1<193"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 > 240) - Throw("b1>240"); + if (firstByteValue < kMinValueOfFirstByteFor2ByteHeader) + Throw("decodeVLLength 2 byte: first byte is below the range"); - return 193 + ((b1 - 193) * 256) + b2; + if (firstByteValue > kMaxValueOfFirstByteFor2ByteHeader) + Throw("decodeVLLength 2 byte: first byte is above the range"); + + // Both bytes are bounded by their own type, and the first one is bounded to + // the 2 byte range above, so this cannot leave the range the header covers. + return kMinValueOfLengthFor2ByteHeader + + ((firstByteValue - kMinValueOfFirstByteFor2ByteHeader) * kNumberOfValuesInOneByte) + + std::to_integer(secondByte); } int -Serializer::decodeVLLength(int b1, int b2, int b3) +Serializer::decodeVLLength(std::byte firstByte, std::byte secondByte, std::byte thirdByte) { - if (b1 < 241) - Throw("b1<241"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 > 254) - Throw("b1>254"); + if (firstByteValue < kMinValueOfFirstByteFor3ByteHeader) + Throw("decodeVLLength 3 byte: first byte is below the range"); - return 12481 + ((b1 - 241) * 65536) + (b2 * 256) + b3; + if (firstByteValue > kMaxValueOfFirstByteFor3ByteHeader) + Throw("decodeVLLength 3 byte: first byte is above the range"); + + int const length = kMinValueOfLengthFor3ByteHeader + + ((firstByteValue - kMinValueOfFirstByteFor3ByteHeader) * kNumberOfValuesInTwoBytes) + + (std::to_integer(secondByte) * kNumberOfValuesInOneByte) + + std::to_integer(thirdByte); + + // A 3 byte header reaches further than kMaxValueOfLengthFor3ByteHeader, which + // is as far as the encoder goes. Refuse the rest, so every length accepted + // here is one that can be written back. + if (length > kMaxValueOfLengthFor3ByteHeader) + Throw("decodeVLLength 3 byte: length is too large to re-encode"); + + return length; } //------------------------------------------------------------------------------ @@ -471,24 +500,24 @@ SerialIter::getRaw(int size) int SerialIter::getVLDataLength() { - int const b1 = get8(); + std::byte const firstByte{get8()}; int datLen = 0; - int const lenLen = Serializer::decodeLengthLength(b1); + int const lenLen = Serializer::decodeLengthLength(firstByte); if (lenLen == 1) { - datLen = Serializer::decodeVLLength(b1); + datLen = Serializer::decodeVLLength(firstByte); } else if (lenLen == 2) { - int const b2 = get8(); - datLen = Serializer::decodeVLLength(b1, b2); + std::byte const secondByte{get8()}; + datLen = Serializer::decodeVLLength(firstByte, secondByte); } else { XRPL_ASSERT(lenLen == 3, "xrpl::SerialIter::getVLDataLength : lenLen is 3"); - int const b2 = get8(); - int const b3 = get8(); - datLen = Serializer::decodeVLLength(b1, b2, b3); + std::byte const secondByte{get8()}; + std::byte const thirdByte{get8()}; + datLen = Serializer::decodeVLLength(firstByte, secondByte, thirdByte); } return datLen; } From c8e767afa4f8d4dc795ef74afe3dda636361888b Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Mon, 14 Sep 2026 17:24:15 -0400 Subject: [PATCH 15/16] fix: Reject PaymentBurn payments that cross zero balance --- .../tx/transactors/payment/Payment.cpp | 25 +++++- src/test/app/Delegate_test.cpp | 83 +++++++++++++++++++ 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index c4c2f9227b..d2da173345 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -339,17 +339,36 @@ Payment::checkGranularSemantics( bool const accountIsHolder = accountIsLow ? rawBalance > beast::kZero : rawBalance < beast::kZero; + bool const mayIssue = + heldGranularPermissions.contains(PaymentMint) && destLimit > beast::kZero; + // PaymentMint requires the destination to be the holder and the account to be the // issuer. destLimit > 0: destination is willing to hold account's IOUs (account is the // issuer). !accountIsHolder: DirectStepI will issue, not redeem. - if (heldGranularPermissions.contains(PaymentMint) && destLimit > beast::kZero && - !accountIsHolder) + if (mayIssue && !accountIsHolder) return tesSUCCESS; // PaymentBurn requires the source account to be the holder and the destination to be // the issuer. accountIsHolder: DirectStepI will redeem, not issue. if (heldGranularPermissions.contains(PaymentBurn) && accountIsHolder) - return tesSUCCESS; + { + if (view.rules().enabled(fixCleanup3_4_0)) + { + // Redeeming stops at the balance held; beyond that the payment engine + // crosses zero and issues the account's own IOUs, which is a mint. So with + // only PaymentBurn we must check the amount against the balance held. The + // granular template forbids sfPaths, tfPartialPayment and a cross-asset + // sfSendMax, so this is a single direct step, sfAmount is what the + // trustline is debited. + STAmount const held = accountIsLow ? rawBalance : -rawBalance; + if (dstAmount <= held || mayIssue) + return tesSUCCESS; + } + else + { + return tesSUCCESS; + } + } return terNO_DELEGATE_PERMISSION; }); diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 3ff90c2a8f..5414269333 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1144,6 +1144,88 @@ class Delegate_test : public beast::unit_test::Suite env.require(Balance(gw, aliceUSD(-20))); } + // PaymentBurn must not exceed the balance the account holds. Redeeming past + // zero makes the payment engine issue the account's own IOUs, which is a mint. + { + Env env(*this, features); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + auto const gwUSD = gw["USD"]; + auto const aliceUSD = alice["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + env.trust(gwUSD(200), alice); + env.close(); + + env(pay(gw, alice, gwUSD(50))); + env.close(); + env.require(Balance(alice, gwUSD(50))); + + // gw accepts alice-issued USD, so the engine has issuing liquidity + // available once the trustline reaches zero. + env(trust(gw, aliceUSD(200))); + env.close(); + + env(delegate::set(alice, bob, {"PaymentBurn"})); + env.close(); + + if (!features[fixCleanup3_4_0]) + { + // Pre-fixCleanup3_4_0: the balance direction alone authorizes the payment, so it + // redeems alice's 50 and then mints 50 alice-issued USD. + env(pay(alice, gw, gwUSD(100)), delegate::As(bob)); + env.require(Balance(alice, gwUSD(-50))); + env.require(Balance(gw, aliceUSD(50))); + } + else + { + // Post-fixCleanup3_4_0: Rejected because it exceeds what alice holds. + env(pay(alice, gw, gwUSD(100)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(alice, gwUSD(50))); + env.require(Balance(gw, aliceUSD(-50))); + + // Allowed because it is less than what alice holds. + env(pay(alice, gw, gwUSD(20)), delegate::As(bob)); + env.require(Balance(alice, gwUSD(30))); + env.close(); + + // Exactly what alice holds: allowed, and settles at zero. + env(pay(alice, gw, gwUSD(30)), delegate::As(bob)); + env.require(Balance(alice, gwUSD(0))); + env.close(); + + // Nothing left to burn: rejected. + env(pay(alice, gw, gwUSD(1)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(gw, aliceUSD(0))); + } + } + + // A delegate holding both PaymentMint and PaymentBurn may cross zero. + { + Env env(*this, features); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + auto const gwUSD = gw["USD"]; + auto const aliceUSD = alice["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + env.trust(gwUSD(200), alice); + env.close(); + + env(pay(gw, alice, gwUSD(50))); + env(trust(gw, aliceUSD(200))); + env.close(); + + env(delegate::set(alice, bob, {"PaymentBurn", "PaymentMint"})); + env.close(); + + env(pay(alice, gw, gwUSD(100)), delegate::As(bob)); + env.require(Balance(alice, gwUSD(-50))); + env.require(Balance(gw, aliceUSD(50))); + } + // Test invalid fields or flags not allowed in granular permission template { Env env(*this, features); @@ -2916,6 +2998,7 @@ class Delegate_test : public beast::unit_test::Suite testAccountDelete(); testDelegateTransaction(); testPaymentGranular(all); + testPaymentGranular(all - fixCleanup3_4_0); testTrustSetGranular(); testAccountSetGranular(); testMPTokenIssuanceSetGranular(); From 4a4fded2eba11427c48ce3f24d9c1aea5e7a9d17 Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 16 Sep 2026 12:10:08 -0400 Subject: [PATCH 16/16] chore: Bump version to 3.4.0 --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index bf67defa3b..a8fd555992 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.4.0-rc1" +char const* const versionString = "3.4.0" // clang-format on ;