From ed5d6f3e22e7806545ff299ed6329e3292d186fb Mon Sep 17 00:00:00 2001 From: Jingchen Date: Wed, 29 Oct 2025 14:16:37 +0000 Subject: [PATCH 01/10] feat: Add public key to log messages (#5678) To protect the identity of UNL validators, the IP addresses are redacted from the log messages sent to the common Grafana instance. However, without such identifying information it is challenging to debug issues. This change adds a node's public key to logs to improve our ability to debug issues. Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com> --- include/xrpl/protocol/PublicKey.h | 19 +++++ include/xrpl/resource/Consumer.h | 4 + include/xrpl/resource/detail/Entry.h | 5 +- src/libxrpl/resource/Consumer.cpp | 6 ++ src/test/app/LedgerReplay_test.cpp | 7 ++ src/test/overlay/reduce_relay_test.cpp | 7 ++ src/xrpld/app/misc/detail/ValidatorList.cpp | 7 +- src/xrpld/overlay/Peer.h | 2 + src/xrpld/overlay/detail/ConnectAttempt.cpp | 2 + src/xrpld/overlay/detail/OverlayImpl.cpp | 20 ++--- src/xrpld/overlay/detail/PeerImp.cpp | 85 +++++++++------------ src/xrpld/overlay/detail/PeerImp.h | 23 +++++- src/xrpld/peerfinder/detail/Logic.h | 72 ++++++++--------- src/xrpld/peerfinder/detail/SlotImp.h | 6 ++ 14 files changed, 162 insertions(+), 103 deletions(-) diff --git a/include/xrpl/protocol/PublicKey.h b/include/xrpl/protocol/PublicKey.h index 9bf01e5cda..a54f593fee 100644 --- a/include/xrpl/protocol/PublicKey.h +++ b/include/xrpl/protocol/PublicKey.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_PUBLICKEY_H_INCLUDED #include +#include #include #include #include @@ -264,6 +265,24 @@ calcNodeID(PublicKey const&); AccountID calcAccountID(PublicKey const& pk); +inline std::string +getFingerprint( + beast::IP::Endpoint const& address, + std::optional const& publicKey = std::nullopt, + std::optional const& id = std::nullopt) +{ + std::stringstream ss; + ss << "IP Address: " << address; + if (publicKey.has_value()) + { + ss << ", Public Key: " << toBase58(TokenType::NodePublic, *publicKey); + } + if (id.has_value()) + { + ss << ", Id: " << id.value(); + } + return ss.str(); +} } // namespace ripple //------------------------------------------------------------------------------ diff --git a/include/xrpl/resource/Consumer.h b/include/xrpl/resource/Consumer.h index 00d8a7b58b..cf6c25a205 100644 --- a/include/xrpl/resource/Consumer.h +++ b/include/xrpl/resource/Consumer.h @@ -21,6 +21,7 @@ #define RIPPLE_RESOURCE_CONSUMER_H_INCLUDED #include +#include #include #include @@ -87,6 +88,9 @@ public: Entry& entry(); + void + setPublicKey(PublicKey const& publicKey); + private: Logic* m_logic; Entry* m_entry; diff --git a/include/xrpl/resource/detail/Entry.h b/include/xrpl/resource/detail/Entry.h index 32ba77c6d3..a8002611ce 100644 --- a/include/xrpl/resource/detail/Entry.h +++ b/include/xrpl/resource/detail/Entry.h @@ -53,7 +53,7 @@ struct Entry : public beast::List::Node std::string to_string() const { - return key->address.to_string(); + return getFingerprint(key->address, publicKey); } /** @@ -82,6 +82,9 @@ struct Entry : public beast::List::Node return local_balance.add(charge, now) + remote_balance; } + // The public key of the peer + std::optional publicKey; + // Back pointer to the map key (bit of a hack here) Key const* key; diff --git a/src/libxrpl/resource/Consumer.cpp b/src/libxrpl/resource/Consumer.cpp index 71a84340cd..9ba3345f28 100644 --- a/src/libxrpl/resource/Consumer.cpp +++ b/src/libxrpl/resource/Consumer.cpp @@ -148,6 +148,12 @@ Consumer::entry() return *m_entry; } +void +Consumer::setPublicKey(PublicKey const& publicKey) +{ + m_entry->publicKey = publicKey; +} + std::ostream& operator<<(std::ostream& os, Consumer const& v) { diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index 88d944d789..e54f24d340 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -325,6 +325,13 @@ public: return false; } + std::string const& + fingerprint() const override + { + return fingerprint_; + } + + std::string fingerprint_; bool ledgerReplayEnabled_; PublicKey nodePublicKey_; }; diff --git a/src/test/overlay/reduce_relay_test.cpp b/src/test/overlay/reduce_relay_test.cpp index e53f53f2db..318eae21ec 100644 --- a/src/test/overlay/reduce_relay_test.cpp +++ b/src/test/overlay/reduce_relay_test.cpp @@ -475,6 +475,12 @@ public: return id_; } + std::string const& + fingerprint() const override + { + return fingerprint_; + } + static void resetId() { @@ -508,6 +514,7 @@ public: private: inline static id_t sid_ = 0; + std::string fingerprint_; id_t id_; Overlay& overlay_; reduce_relay::Squelch squelch_; diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index 92095b7211..926649808d 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -795,9 +795,7 @@ ValidatorList::sendValidatorList( << " validator list collection(s) containing " << numVLs << " validator list(s) for " << strHex(publisherKey) << " with sequence range " << peerSequence << ", " - << newPeerSequence << " to " - << peer.getRemoteAddress().to_string() << " [" << peer.id() - << "]"; + << newPeerSequence << " to " << peer.fingerprint(); else { XRPL_ASSERT( @@ -807,8 +805,7 @@ ValidatorList::sendValidatorList( JLOG(j.debug()) << "Sent validator list for " << strHex(publisherKey) << " with sequence " << newPeerSequence << " to " - << peer.getRemoteAddress().to_string() << " [" << peer.id() - << "]"; + << peer.fingerprint(); } } } diff --git a/src/xrpld/overlay/Peer.h b/src/xrpld/overlay/Peer.h index 9a5bd7136b..9a1c75c9b8 100644 --- a/src/xrpld/overlay/Peer.h +++ b/src/xrpld/overlay/Peer.h @@ -112,6 +112,8 @@ public: virtual void setPublisherListSequence(PublicKey const&, std::size_t const) = 0; + virtual std::string const& + fingerprint() const = 0; // // Ledger // diff --git a/src/xrpld/overlay/detail/ConnectAttempt.cpp b/src/xrpld/overlay/detail/ConnectAttempt.cpp index 15a3b91802..3d7f0abe01 100644 --- a/src/xrpld/overlay/detail/ConnectAttempt.cpp +++ b/src/xrpld/overlay/detail/ConnectAttempt.cpp @@ -589,6 +589,8 @@ ConnectAttempt::processResponse() remote_endpoint_.address(), app_); + usage_.setPublicKey(publicKey); + JLOG(journal_.debug()) << "Protocol: " << to_string(*negotiatedProtocol); JLOG(journal_.info()) diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index 8d295faace..025fcfabce 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -262,6 +262,8 @@ OverlayImpl::onHandoff( remote_endpoint.address(), app_); + consumer.setPublicKey(publicKey); + { // The node gets a reserved slot if it is in our cluster // or if it has a reservation. @@ -434,6 +436,9 @@ OverlayImpl::connect(beast::IP::Endpoint const& remote_endpoint) void OverlayImpl::add_active(std::shared_ptr const& peer) { + beast::WrappedSink sink{journal_.sink(), peer->prefix()}; + beast::Journal journal{sink}; + std::lock_guard lock(mutex_); { @@ -457,11 +462,7 @@ OverlayImpl::add_active(std::shared_ptr const& peer) list_.emplace(peer.get(), peer); - JLOG(journal_.debug()) << "activated " << peer->getRemoteAddress() << " (" - << peer->id() << ":" - << toBase58( - TokenType::NodePublic, peer->getNodePublic()) - << ")"; + JLOG(journal.debug()) << "activated"; // As we are not on the strand, run() must be called // while holding the lock, otherwise new I/O can be @@ -605,6 +606,9 @@ OverlayImpl::onWrite(beast::PropertyStream::Map& stream) void OverlayImpl::activate(std::shared_ptr const& peer) { + beast::WrappedSink sink{journal_.sink(), peer->prefix()}; + beast::Journal journal{sink}; + // Now track this peer { std::lock_guard lock(mutex_); @@ -618,11 +622,7 @@ OverlayImpl::activate(std::shared_ptr const& peer) (void)result.second; } - JLOG(journal_.debug()) << "activated " << peer->getRemoteAddress() << " (" - << peer->id() << ":" - << toBase58( - TokenType::NodePublic, peer->getNodePublic()) - << ")"; + JLOG(journal.debug()) << "activated"; // We just accepted this peer so we have non-zero active peers XRPL_ASSERT(size(), "ripple::OverlayImpl::activate : nonzero peers"); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 47d01eb7c5..41491a2ec2 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -82,8 +82,11 @@ PeerImp::PeerImp( : Child(overlay) , app_(app) , id_(id) - , sink_(app_.journal("Peer"), makePrefix(id)) - , p_sink_(app_.journal("Protocol"), makePrefix(id)) + , fingerprint_( + getFingerprint(slot->remote_endpoint(), publicKey, to_string(id))) + , prefix_(makePrefix(fingerprint_)) + , sink_(app_.journal("Peer"), prefix_) + , p_sink_(app_.journal("Protocol"), prefix_) , journal_(sink_) , p_journal_(p_sink_) , stream_ptr_(std::move(stream_ptr)) @@ -131,8 +134,7 @@ PeerImp::PeerImp( headers_, FEATURE_VPRR, app_.config().VP_REDUCE_RELAY_BASE_SQUELCH_ENABLE) - << " tx reduce-relay enabled " << txReduceRelayEnabled_ << " on " - << remote_address_ << " " << id_; + << " tx reduce-relay enabled " << txReduceRelayEnabled_; } PeerImp::~PeerImp() @@ -280,8 +282,7 @@ PeerImp::send(std::shared_ptr const& m) sink && (sendq_size % Tuning::sendQueueLogFreq) == 0) { std::string const n = name(); - sink << (n.empty() ? remote_address_.to_string() : n) - << " sendq: " << sendq_size; + sink << n << " sendq: " << sendq_size; } send_queue_.push(m); @@ -585,10 +586,7 @@ PeerImp::fail(std::string const& name, error_code ec) if (!socket_.is_open()) return; - JLOG(journal_.warn()) << name << " from " - << toBase58(TokenType::NodePublic, publicKey_) - << " at " << remote_address_.to_string() << ": " - << ec.message(); + JLOG(journal_.warn()) << name << ": " << ec.message(); shutdown(); } @@ -611,8 +609,7 @@ PeerImp::fail(std::string const& reason) if (journal_.active(beast::severities::kWarning)) { std::string const n = name(); - JLOG(journal_.warn()) << (n.empty() ? remote_address_.to_string() : n) - << " failed: " << reason; + JLOG(journal_.warn()) << n << " failed: " << reason; } shutdown(); @@ -730,6 +727,16 @@ PeerImp::setTimer(std::chrono::seconds interval) &PeerImp::onTimer, shared_from_this(), std::placeholders::_1))); } +//------------------------------------------------------------------------------ + +std::string +PeerImp::makePrefix(std::string const& fingerprint) +{ + std::stringstream ss; + ss << "[" << fingerprint << "] "; + return ss.str(); +} + void PeerImp::onTimer(error_code const& ec) { @@ -810,14 +817,6 @@ PeerImp::cancelTimer() noexcept } } -std::string -PeerImp::makePrefix(id_t id) -{ - std::stringstream ss; - ss << "[" << std::setfill('0') << std::setw(3) << id << "] "; - return ss.str(); -} - //------------------------------------------------------------------------------ void PeerImp::doAccept() @@ -826,7 +825,7 @@ PeerImp::doAccept() read_buffer_.size() == 0, "ripple::PeerImp::doAccept : empty read buffer"); - JLOG(journal_.debug()) << "doAccept: " << remote_address_; + JLOG(journal_.debug()) << "doAccept"; // a shutdown was initiated before the handshake, there is nothing to do if (shutdown_) @@ -840,8 +839,6 @@ PeerImp::doAccept() return fail("makeSharedValue: Unexpected failure"); JLOG(journal_.debug()) << "Protocol: " << to_string(protocol_); - JLOG(journal_.info()) << "Public Key: " - << toBase58(TokenType::NodePublic, publicKey_); if (auto member = app_.cluster().member(publicKey_)) { @@ -2143,8 +2140,7 @@ PeerImp::onValidatorListMessage( // ValidatorList class rules), so charge accordingly and skip processing. if (blobs.empty()) { - JLOG(p_journal_.warn()) << "Ignored malformed " << messageType - << " from peer " << remote_address_; + JLOG(p_journal_.warn()) << "Ignored malformed " << messageType; // This shouldn't ever happen with a well-behaved peer fee_.update(Resource::feeHeavyBurdenPeer, "no blobs"); return; @@ -2152,9 +2148,7 @@ PeerImp::onValidatorListMessage( auto const hash = sha512Half(manifest, blobs, version); - JLOG(p_journal_.debug()) - << "Received " << messageType << " from " << remote_address_.to_string() - << " (" << id_ << ")"; + JLOG(p_journal_.debug()) << "Received " << messageType; if (!app_.getHashRouter().addSuppressionPeer(hash, id_)) { @@ -2181,8 +2175,7 @@ PeerImp::onValidatorListMessage( << "Processed " << messageType << " version " << version << " from " << (applyResult.publisherKey ? strHex(*applyResult.publisherKey) : "unknown or invalid publisher") - << " from " << remote_address_.to_string() << " (" << id_ - << ") with best result " << to_string(applyResult.bestDisposition()); + << " with best result " << to_string(applyResult.bestDisposition()); // Act based on the best result switch (applyResult.bestDisposition()) @@ -2296,51 +2289,44 @@ PeerImp::onValidatorListMessage( // New list case ListDisposition::accepted: JLOG(p_journal_.debug()) - << "Applied " << count << " new " << messageType - << "(s) from peer " << remote_address_; + << "Applied " << count << " new " << messageType; break; // Newest list is expired, and that needs to be broadcast, too case ListDisposition::expired: JLOG(p_journal_.debug()) - << "Applied " << count << " expired " << messageType - << "(s) from peer " << remote_address_; + << "Applied " << count << " expired " << messageType; break; // Future list case ListDisposition::pending: JLOG(p_journal_.debug()) - << "Processed " << count << " future " << messageType - << "(s) from peer " << remote_address_; + << "Processed " << count << " future " << messageType; break; case ListDisposition::same_sequence: JLOG(p_journal_.warn()) << "Ignored " << count << " " << messageType - << "(s) with current sequence from peer " - << remote_address_; + << "(s) with current sequence"; break; case ListDisposition::known_sequence: JLOG(p_journal_.warn()) << "Ignored " << count << " " << messageType - << "(s) with future sequence from peer " << remote_address_; + << "(s) with future sequence"; break; case ListDisposition::stale: JLOG(p_journal_.warn()) - << "Ignored " << count << "stale " << messageType - << "(s) from peer " << remote_address_; + << "Ignored " << count << "stale " << messageType; break; case ListDisposition::untrusted: JLOG(p_journal_.warn()) - << "Ignored " << count << " untrusted " << messageType - << "(s) from peer " << remote_address_; + << "Ignored " << count << " untrusted " << messageType; break; case ListDisposition::unsupported_version: JLOG(p_journal_.warn()) << "Ignored " << count << "unsupported version " - << messageType << "(s) from peer " << remote_address_; + << messageType; break; case ListDisposition::invalid: JLOG(p_journal_.warn()) - << "Ignored " << count << "invalid " << messageType - << "(s) from peer " << remote_address_; + << "Ignored " << count << "invalid " << messageType; break; // LCOV_EXCL_START default: @@ -2374,8 +2360,7 @@ PeerImp::onMessage(std::shared_ptr const& m) } catch (std::exception const& e) { - JLOG(p_journal_.warn()) << "ValidatorList: Exception, " << e.what() - << " from peer " << remote_address_; + JLOG(p_journal_.warn()) << "ValidatorList: Exception, " << e.what(); using namespace std::string_literals; fee_.update(Resource::feeInvalidData, e.what()); } @@ -2414,8 +2399,8 @@ PeerImp::onMessage( } catch (std::exception const& e) { - JLOG(p_journal_.warn()) << "ValidatorListCollection: Exception, " - << e.what() << " from peer " << remote_address_; + JLOG(p_journal_.warn()) + << "ValidatorListCollection: Exception, " << e.what(); using namespace std::string_literals; fee_.update(Resource::feeInvalidData, e.what()); } diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index c2221c136d..5ce1ece950 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -134,6 +134,8 @@ private: Application& app_; id_t const id_; + std::string fingerprint_; + std::string prefix_; beast::WrappedSink sink_; beast::WrappedSink p_sink_; beast::Journal const journal_; @@ -635,7 +637,7 @@ private: cancelTimer() noexcept; static std::string - makePrefix(id_t id); + makePrefix(std::string const& fingerprint); void doAccept(); @@ -689,6 +691,18 @@ private: handleHaveTransactions( std::shared_ptr const& m); + std::string const& + fingerprint() const override + { + return fingerprint_; + } + + std::string const& + prefix() const + { + return prefix_; + } + public: //-------------------------------------------------------------------------- // @@ -832,8 +846,11 @@ PeerImp::PeerImp( : Child(overlay) , app_(app) , id_(id) - , sink_(app_.journal("Peer"), makePrefix(id)) - , p_sink_(app_.journal("Protocol"), makePrefix(id)) + , fingerprint_( + getFingerprint(slot->remote_endpoint(), publicKey, to_string(id_))) + , prefix_(makePrefix(fingerprint_)) + , sink_(app_.journal("Peer"), prefix_) + , p_sink_(app_.journal("Protocol"), prefix_) , journal_(sink_) , p_journal_(p_sink_) , stream_ptr_(std::move(stream_ptr)) diff --git a/src/xrpld/peerfinder/detail/Logic.h b/src/xrpld/peerfinder/detail/Logic.h index 74bec8431e..6a353b1f9a 100644 --- a/src/xrpld/peerfinder/detail/Logic.h +++ b/src/xrpld/peerfinder/detail/Logic.h @@ -35,6 +35,7 @@ #include #include #include +#include #include #include @@ -240,21 +241,23 @@ public: slot.checked = true; slot.connectivityCheckInProgress = false; + beast::WrappedSink sink{m_journal.sink(), slot.prefix()}; + beast::Journal journal{sink}; + if (ec) { // VFALCO TODO Should we retry depending on the error? slot.canAccept = false; - JLOG(m_journal.error()) - << beast::leftw(18) << "Logic testing " << iter->first - << " with error, " << ec.message(); + JLOG(journal.error()) << "Logic testing " << iter->first + << " with error, " << ec.message(); bootcache_.on_failure(checkedAddress); return; } slot.canAccept = true; slot.set_listening_port(checkedAddress.port()); - JLOG(m_journal.debug()) << beast::leftw(18) << "Logic testing " - << checkedAddress << " succeeded"; + JLOG(journal.debug()) + << "Logic testing " << checkedAddress << " succeeded"; } //-------------------------------------------------------------------------- @@ -359,9 +362,10 @@ public: SlotImp::ptr const& slot, beast::IP::Endpoint const& local_endpoint) { - JLOG(m_journal.trace()) - << beast::leftw(18) << "Logic connected" << slot->remote_endpoint() - << " on local " << local_endpoint; + beast::WrappedSink sink{m_journal.sink(), slot->prefix()}; + beast::Journal journal{sink}; + + JLOG(journal.trace()) << "Logic connected on local " << local_endpoint; std::lock_guard _(lock_); @@ -381,9 +385,7 @@ public: iter->second->local_endpoint() == slot->remote_endpoint(), "ripple::PeerFinder::Logic::onConnected : local and remote " "endpoints do match"); - JLOG(m_journal.warn()) - << beast::leftw(18) << "Logic dropping " - << slot->remote_endpoint() << " as self connect"; + JLOG(journal.warn()) << "Logic dropping as self connect"; return false; } } @@ -398,9 +400,12 @@ public: Result activate(SlotImp::ptr const& slot, PublicKey const& key, bool reserved) { - JLOG(m_journal.debug()) - << beast::leftw(18) << "Logic handshake " << slot->remote_endpoint() - << " with " << (reserved ? "reserved " : "") << "key " << key; + beast::WrappedSink sink{m_journal.sink(), slot->prefix()}; + beast::Journal journal{sink}; + + JLOG(journal.debug()) + << "Logic handshake " << slot->remote_endpoint() << " with " + << (reserved ? "reserved " : "") << "key " << key; std::lock_guard _(lock_); @@ -462,8 +467,7 @@ public: "missing from fixed_"); iter->second.success(m_clock.now()); - JLOG(m_journal.trace()) << beast::leftw(18) << "Logic fixed " - << slot->remote_endpoint() << " success"; + JLOG(journal.trace()) << "Logic fixed success"; } return Result::success; @@ -681,9 +685,10 @@ public: { SlotImp::ptr const& slot = t.slot(); auto const& list = t.list(); - JLOG(m_journal.trace()) - << beast::leftw(18) << "Logic sending " - << slot->remote_endpoint() << " with " << list.size() + beast::WrappedSink sink{m_journal.sink(), slot->prefix()}; + beast::Journal journal{sink}; + JLOG(journal.trace()) + << "Logic sending " << list.size() << ((list.size() == 1) ? " endpoint" : " endpoints"); result.push_back(std::make_pair(slot, list)); } @@ -788,6 +793,9 @@ public: void on_endpoints(SlotImp::ptr const& slot, Endpoints list) { + beast::WrappedSink sink{m_journal.sink(), slot->prefix()}; + beast::Journal journal{sink}; + // If we're sent too many endpoints, sample them at random: if (list.size() > Tuning::numberOfEndpointsMax) { @@ -795,10 +803,8 @@ public: list.resize(Tuning::numberOfEndpointsMax); } - JLOG(m_journal.trace()) - << beast::leftw(18) << "Endpoints from " << slot->remote_endpoint() - << " contained " << list.size() - << ((list.size() > 1) ? " entries" : " entry"); + JLOG(journal.trace()) << "Endpoints contained " << list.size() + << ((list.size() > 1) ? " entries" : " entry"); std::lock_guard _(lock_); @@ -835,9 +841,8 @@ public: { if (slot->connectivityCheckInProgress) { - JLOG(m_journal.debug()) - << beast::leftw(18) << "Logic testing " << ep.address - << " already in progress"; + JLOG(journal.debug()) << "Logic testing " << ep.address + << " already in progress"; continue; } @@ -934,6 +939,9 @@ public: remove(slot); + beast::WrappedSink sink{m_journal.sink(), slot->prefix()}; + beast::Journal journal{sink}; + // Mark fixed slot failure if (slot->fixed() && !slot->inbound() && slot->state() != Slot::active) { @@ -944,16 +952,14 @@ public: "missing from fixed_"); iter->second.failure(m_clock.now()); - JLOG(m_journal.debug()) << beast::leftw(18) << "Logic fixed " - << slot->remote_endpoint() << " failed"; + JLOG(journal.debug()) << "Logic fixed failed"; } // Do state specific bookkeeping switch (slot->state()) { case Slot::accept: - JLOG(m_journal.trace()) << beast::leftw(18) << "Logic accept " - << slot->remote_endpoint() << " failed"; + JLOG(journal.trace()) << "Logic accept failed"; break; case Slot::connect: @@ -967,13 +973,11 @@ public: break; case Slot::active: - JLOG(m_journal.trace()) << beast::leftw(18) << "Logic close " - << slot->remote_endpoint(); + JLOG(journal.trace()) << "Logic close"; break; case Slot::closing: - JLOG(m_journal.trace()) << beast::leftw(18) << "Logic finished " - << slot->remote_endpoint(); + JLOG(journal.trace()) << "Logic finished"; break; // LCOV_EXCL_START diff --git a/src/xrpld/peerfinder/detail/SlotImp.h b/src/xrpld/peerfinder/detail/SlotImp.h index e5d8b1e6c5..363fe018e4 100644 --- a/src/xrpld/peerfinder/detail/SlotImp.h +++ b/src/xrpld/peerfinder/detail/SlotImp.h @@ -91,6 +91,12 @@ public: return m_public_key; } + std::string + prefix() const + { + return "[" + getFingerprint(remote_endpoint(), public_key()) + "] "; + } + std::optional listening_port() const override { From bd3bc917f84779a73ba78ead283a25456551c92f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Wed, 29 Oct 2025 14:21:50 +0000 Subject: [PATCH 02/10] refactor: Retire fix1571 amendment (#5925) Amendments activated for more than 2 years can be retired. This change retires the fix1571 amendment. Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com> --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/Escrow_test.cpp | 99 +++++++--------- src/xrpld/app/tx/detail/Escrow.cpp | 120 ++++---------------- 3 files changed, 64 insertions(+), 157 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 6670b5fb0b..0aba334de6 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -109,7 +109,6 @@ XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYe XRPL_FIX (1578, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (1571, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) @@ -154,3 +153,4 @@ XRPL_RETIRE(fix1513) XRPL_RETIRE(fix1515) XRPL_RETIRE(fix1543) XRPL_RETIRE(fix1781) +XRPL_RETIRE(fix1571) diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index cea3a835a6..8d993f9162 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -294,74 +294,51 @@ struct Escrow_test : public beast::unit_test::suite } void - test1571(FeatureBitset features) + testRequiresConditionOrFinishAfter(FeatureBitset features) { using namespace jtx; using namespace std::chrono; - { - testcase("Implied Finish Time (without fix1571)"); + testcase("RequiresConditionOrFinishAfter"); - Env env(*this, testable_amendments() - fix1571); - auto const baseFee = env.current()->fees().base; - env.fund(XRP(5000), "alice", "bob", "carol"); - env.close(); + Env env(*this, features); + auto const baseFee = env.current()->fees().base; + env.fund(XRP(5000), "alice", "bob", "carol"); + env.close(); - // Creating an escrow without a finish time and finishing it - // is allowed without fix1571: - auto const seq1 = env.seq("alice"); - env(escrow::create("alice", "bob", XRP(100)), - escrow::cancel_time(env.now() + 1s), - fee(baseFee * 150)); - env.close(); - env(escrow::finish("carol", "alice", seq1), fee(baseFee * 150)); - BEAST_EXPECT(env.balance("bob") == XRP(5100)); + // Creating an escrow with only a cancel time is not allowed: + env(escrow::create("alice", "bob", XRP(100)), + escrow::cancel_time(env.now() + 90s), + fee(baseFee * 150), + ter(temMALFORMED)); - env.close(); + // Creating an escrow with only a cancel time and a condition is + // allowed: + auto const seq = env.seq("alice"); + env(escrow::create("alice", "bob", XRP(100)), + escrow::cancel_time(env.now() + 90s), + escrow::condition(escrow::cb1), + fee(baseFee * 150)); + env.close(); + env(escrow::finish("carol", "alice", seq), + escrow::condition(escrow::cb1), + escrow::fulfillment(escrow::fb1), + fee(baseFee * 150)); + BEAST_EXPECT(env.balance("bob") == XRP(5100)); - // Creating an escrow without a finish time and a condition is - // also allowed without fix1571: - auto const seq2 = env.seq("alice"); - env(escrow::create("alice", "bob", XRP(100)), - escrow::cancel_time(env.now() + 1s), - escrow::condition(escrow::cb1), - fee(baseFee * 150)); - env.close(); - env(escrow::finish("carol", "alice", seq2), - escrow::condition(escrow::cb1), - escrow::fulfillment(escrow::fb1), - fee(baseFee * 150)); - BEAST_EXPECT(env.balance("bob") == XRP(5200)); - } - - { - testcase("Implied Finish Time (with fix1571)"); - - Env env(*this, features); - auto const baseFee = env.current()->fees().base; - env.fund(XRP(5000), "alice", "bob", "carol"); - env.close(); - - // Creating an escrow with only a cancel time is not allowed: - env(escrow::create("alice", "bob", XRP(100)), - escrow::cancel_time(env.now() + 90s), - fee(baseFee * 150), - ter(temMALFORMED)); - - // Creating an escrow with only a cancel time and a condition is - // allowed: - auto const seq = env.seq("alice"); - env(escrow::create("alice", "bob", XRP(100)), - escrow::cancel_time(env.now() + 90s), - escrow::condition(escrow::cb1), - fee(baseFee * 150)); - env.close(); - env(escrow::finish("carol", "alice", seq), - escrow::condition(escrow::cb1), - escrow::fulfillment(escrow::fb1), - fee(baseFee * 150)); - BEAST_EXPECT(env.balance("bob") == XRP(5100)); - } + // Creating an escrow with only a cancel time and a finish time is + // allowed: + auto const seqFt = env.seq("alice"); + env(escrow::create("alice", "bob", XRP(100)), + escrow::finish_time(env.now()), // Set finish time to now so that + // we can call finish immediately. + escrow::cancel_time(env.now() + 50s), + fee(baseFee * 150)); + env.close(); + env(escrow::finish("carol", "alice", seqFt), fee(150 * baseFee)); + BEAST_EXPECT( + env.balance("bob") == + XRP(5200)); // 5100 (from last transaction) + 100 } void @@ -1708,7 +1685,7 @@ struct Escrow_test : public beast::unit_test::suite testTiming(features); testTags(features); testDisallowXRP(features); - test1571(features); + testRequiresConditionOrFinishAfter(features); testFails(features); testLockup(features); testEscrowConditions(features); diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index a5c44b1e2e..c73ef3f7a1 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -151,15 +151,12 @@ EscrowCreate::preflight(PreflightContext const& ctx) ctx.tx[sfCancelAfter] <= ctx.tx[sfFinishAfter]) return temBAD_EXPIRATION; - if (ctx.rules.enabled(fix1571)) - { - // In the absence of a FinishAfter, the escrow can be finished - // immediately, which can be confusing. When creating an escrow, - // we want to ensure that either a FinishAfter time is explicitly - // specified or a completion condition is attached. - if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition]) - return temMALFORMED; - } + // In the absence of a FinishAfter, the escrow can be finished + // immediately, which can be confusing. When creating an escrow, + // we want to ensure that either a FinishAfter time is explicitly + // specified or a completion condition is attached. + if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition]) + return temMALFORMED; if (auto const cb = ctx.tx[~sfCondition]) { @@ -446,41 +443,11 @@ EscrowCreate::doApply() { auto const closeTime = ctx_.view().info().parentCloseTime; - // Prior to fix1571, the cancel and finish times could be greater - // than or equal to the parent ledgers' close time. - // - // With fix1571, we require that they both be strictly greater - // than the parent ledgers' close time. - if (ctx_.view().rules().enabled(fix1571)) - { - if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter])) - return tecNO_PERMISSION; + if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter])) + return tecNO_PERMISSION; - if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter])) - return tecNO_PERMISSION; - } - else - { - // This is old code that needs to stay to support replaying old ledgers, - // but does not need to be covered by new tests. - // LCOV_EXCL_START - if (ctx_.tx[~sfCancelAfter]) - { - auto const cancelAfter = ctx_.tx[sfCancelAfter]; - - if (closeTime.time_since_epoch().count() >= cancelAfter) - return tecNO_PERMISSION; - } - - if (ctx_.tx[~sfFinishAfter]) - { - auto const finishAfter = ctx_.tx[sfFinishAfter]; - - if (closeTime.time_since_epoch().count() >= finishAfter) - return tecNO_PERMISSION; - } - // LCOV_EXCL_STOP - } + if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter])) + return tecNO_PERMISSION; auto const sle = ctx_.view().peek(keylet::account(account_)); if (!sle) @@ -1027,38 +994,16 @@ EscrowFinish::doApply() } // If a cancel time is present, a finish operation should only succeed prior - // to that time. fix1571 corrects a logic error in the check that would make - // a finish only succeed strictly after the cancel time. - if (ctx_.view().rules().enabled(fix1571)) - { - auto const now = ctx_.view().info().parentCloseTime; + // to that time. + auto const now = ctx_.view().info().parentCloseTime; - // Too soon: can't execute before the finish time - if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter])) - return tecNO_PERMISSION; + // Too soon: can't execute before the finish time + if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter])) + return tecNO_PERMISSION; - // Too late: can't execute after the cancel time - if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter])) - return tecNO_PERMISSION; - } - else - { - // This is old code that needs to stay to support replaying old ledgers, - // but does not need to be covered by new tests. - // LCOV_EXCL_START - // Too soon? - if ((*slep)[~sfFinishAfter] && - ctx_.view().info().parentCloseTime.time_since_epoch().count() <= - (*slep)[sfFinishAfter]) - return tecNO_PERMISSION; - - // Too late? - if ((*slep)[~sfCancelAfter] && - ctx_.view().info().parentCloseTime.time_since_epoch().count() <= - (*slep)[sfCancelAfter]) - return tecNO_PERMISSION; - // LCOV_EXCL_STOP - } + // Too late: can't execute after the cancel time + if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter])) + return tecNO_PERMISSION; // Check cryptocondition fulfillment { @@ -1315,30 +1260,15 @@ EscrowCancel::doApply() return tecNO_TARGET; } - if (ctx_.view().rules().enabled(fix1571)) - { - auto const now = ctx_.view().info().parentCloseTime; + auto const now = ctx_.view().info().parentCloseTime; - // No cancel time specified: can't execute at all. - if (!(*slep)[~sfCancelAfter]) - return tecNO_PERMISSION; + // No cancel time specified: can't execute at all. + if (!(*slep)[~sfCancelAfter]) + return tecNO_PERMISSION; - // Too soon: can't execute before the cancel time. - if (!after(now, (*slep)[sfCancelAfter])) - return tecNO_PERMISSION; - } - else - { - // This is old code that needs to stay to support replaying old ledgers, - // but does not need to be covered by new tests. - // LCOV_EXCL_START - // Too soon? - if (!(*slep)[~sfCancelAfter] || - ctx_.view().info().parentCloseTime.time_since_epoch().count() <= - (*slep)[sfCancelAfter]) - return tecNO_PERMISSION; - // LCOV_EXCL_STOP - } + // Too soon: can't execute before the cancel time. + if (!after(now, (*slep)[sfCancelAfter])) + return tecNO_PERMISSION; AccountID const account = (*slep)[sfAccount]; From efa917d9f32958ab9ebb2f0c284657a96901e922 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Wed, 29 Oct 2025 16:08:17 +0000 Subject: [PATCH 03/10] refactor: Retire fix1578 amendment (#5927) Amendments activated for more than 2 years can be retired. This change retires the fix1578 amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/AMMExtended_test.cpp | 188 +++++++++----------- src/test/app/Offer_test.cpp | 14 +- src/test/rpc/Feature_test.cpp | 1 - src/test/rpc/NoRipple_test.cpp | 135 +++++++------- src/xrpld/app/tx/detail/CreateOffer.cpp | 4 +- src/xrpld/app/tx/detail/SetTrust.cpp | 2 +- 7 files changed, 158 insertions(+), 188 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 0aba334de6..6d3da84f84 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -106,7 +106,6 @@ XRPL_FIX (CheckThreading, Supported::yes, VoteBehavior::DefaultYe XRPL_FIX (MasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (TakerDryOfferRemoval, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (1578, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) @@ -154,3 +153,4 @@ XRPL_RETIRE(fix1515) XRPL_RETIRE(fix1543) XRPL_RETIRE(fix1781) XRPL_RETIRE(fix1571) +XRPL_RETIRE(fix1578) diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index ee6988bd75..b0e810fce3 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -198,109 +198,100 @@ private: // Fill or Kill - unless we fully cross, just charge a fee and don't // place the offer on the books. But also clean up expired offers // that are discovered along the way. - // - // fix1578 changes the return code. Verify expected behavior - // without and with fix1578. - for (auto const& tweakedFeatures : - {features - fix1578, features | fix1578}) - { - testAMM( - [&](AMM& ammAlice, Env& env) { - // Order that can't be filled - TER const killedCode{ - tweakedFeatures[fix1578] ? TER{tecKILLED} - : TER{tesSUCCESS}}; - env(offer(carol, USD(100), XRP(100)), - txflags(tfFillOrKill), - ter(killedCode)); - env.close(); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'100), USD(10'000), ammAlice.tokens())); - // fee = AMM - BEAST_EXPECT(expectLedgerEntryRoot( - env, carol, XRP(30'000) - (txfee(env, 1)))); - BEAST_EXPECT(expectOffers(env, carol, 0)); - BEAST_EXPECT(expectHolding(env, carol, USD(30'000))); + testAMM( + [&](AMM& ammAlice, Env& env) { + // Order that can't be filled + TER const killedCode{TER{tecKILLED}}; + env(offer(carol, USD(100), XRP(100)), + txflags(tfFillOrKill), + ter(killedCode)); + env.close(); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'100), USD(10'000), ammAlice.tokens())); + // fee = AMM + BEAST_EXPECT(expectLedgerEntryRoot( + env, carol, XRP(30'000) - (txfee(env, 1)))); + BEAST_EXPECT(expectOffers(env, carol, 0)); + BEAST_EXPECT(expectHolding(env, carol, USD(30'000))); - // Order that can be filled - env(offer(carol, XRP(100), USD(100)), - txflags(tfFillOrKill), - ter(tesSUCCESS)); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), USD(10'100), ammAlice.tokens())); - BEAST_EXPECT(expectLedgerEntryRoot( - env, carol, XRP(30'000) + XRP(100) - txfee(env, 2))); - BEAST_EXPECT(expectHolding(env, carol, USD(29'900))); - BEAST_EXPECT(expectOffers(env, carol, 0)); - }, - {{XRP(10'100), USD(10'000)}}, - 0, - std::nullopt, - {tweakedFeatures}); + // Order that can be filled + env(offer(carol, XRP(100), USD(100)), + txflags(tfFillOrKill), + ter(tesSUCCESS)); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), USD(10'100), ammAlice.tokens())); + BEAST_EXPECT(expectLedgerEntryRoot( + env, carol, XRP(30'000) + XRP(100) - txfee(env, 2))); + BEAST_EXPECT(expectHolding(env, carol, USD(29'900))); + BEAST_EXPECT(expectOffers(env, carol, 0)); + }, + {{XRP(10'100), USD(10'000)}}, + 0, + std::nullopt, + {features}); - // Immediate or Cancel - cross as much as possible - // and add nothing on the books. - testAMM( - [&](AMM& ammAlice, Env& env) { - env(offer(carol, XRP(200), USD(200)), - txflags(tfImmediateOrCancel), - ter(tesSUCCESS)); + // Immediate or Cancel - cross as much as possible + // and add nothing on the books. + testAMM( + [&](AMM& ammAlice, Env& env) { + env(offer(carol, XRP(200), USD(200)), + txflags(tfImmediateOrCancel), + ter(tesSUCCESS)); - // AMM generates a synthetic offer of 100USD/100XRP - // to match the CLOB offer quality. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), USD(10'100), ammAlice.tokens())); - // +AMM - offer * fee - BEAST_EXPECT(expectLedgerEntryRoot( - env, carol, XRP(30'000) + XRP(100) - txfee(env, 1))); - // AMM - BEAST_EXPECT(expectHolding(env, carol, USD(29'900))); - BEAST_EXPECT(expectOffers(env, carol, 0)); - }, - {{XRP(10'100), USD(10'000)}}, - 0, - std::nullopt, - {tweakedFeatures}); + // AMM generates a synthetic offer of 100USD/100XRP + // to match the CLOB offer quality. + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), USD(10'100), ammAlice.tokens())); + // +AMM - offer * fee + BEAST_EXPECT(expectLedgerEntryRoot( + env, carol, XRP(30'000) + XRP(100) - txfee(env, 1))); + // AMM + BEAST_EXPECT(expectHolding(env, carol, USD(29'900))); + BEAST_EXPECT(expectOffers(env, carol, 0)); + }, + {{XRP(10'100), USD(10'000)}}, + 0, + std::nullopt, + {features}); - // tfPassive -- place the offer without crossing it. - testAMM( - [&](AMM& ammAlice, Env& env) { - // Carol creates a passive offer that could cross AMM. - // Carol's offer should stay in the ledger. - env(offer(carol, XRP(100), USD(100), tfPassive)); - env.close(); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'100), STAmount{USD, 10'000}, ammAlice.tokens())); - BEAST_EXPECT(expectOffers( - env, carol, 1, {{{XRP(100), STAmount{USD, 100}}}})); - }, - {{XRP(10'100), USD(10'000)}}, - 0, - std::nullopt, - {tweakedFeatures}); + // tfPassive -- place the offer without crossing it. + testAMM( + [&](AMM& ammAlice, Env& env) { + // Carol creates a passive offer that could cross AMM. + // Carol's offer should stay in the ledger. + env(offer(carol, XRP(100), USD(100), tfPassive)); + env.close(); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'100), STAmount{USD, 10'000}, ammAlice.tokens())); + BEAST_EXPECT(expectOffers( + env, carol, 1, {{{XRP(100), STAmount{USD, 100}}}})); + }, + {{XRP(10'100), USD(10'000)}}, + 0, + std::nullopt, + {features}); - // tfPassive -- cross only offers of better quality. - testAMM( - [&](AMM& ammAlice, Env& env) { - env(offer(alice, USD(110), XRP(100))); - env.close(); + // tfPassive -- cross only offers of better quality. + testAMM( + [&](AMM& ammAlice, Env& env) { + env(offer(alice, USD(110), XRP(100))); + env.close(); - // Carol creates a passive offer. That offer should cross - // AMM and leave Alice's offer untouched. - env(offer(carol, XRP(100), USD(100), tfPassive)); - env.close(); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'900), - STAmount{USD, UINT64_C(9'082'56880733945), -11}, - ammAlice.tokens())); - BEAST_EXPECT(expectOffers(env, carol, 0)); - BEAST_EXPECT(expectOffers(env, alice, 1)); - }, - {{XRP(11'000), USD(9'000)}}, - 0, - std::nullopt, - {tweakedFeatures}); - } + // Carol creates a passive offer. That offer should cross + // AMM and leave Alice's offer untouched. + env(offer(carol, XRP(100), USD(100), tfPassive)); + env.close(); + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'900), + STAmount{USD, UINT64_C(9'082'56880733945), -11}, + ammAlice.tokens())); + BEAST_EXPECT(expectOffers(env, carol, 0)); + BEAST_EXPECT(expectOffers(env, alice, 1)); + }, + {{XRP(11'000), USD(9'000)}}, + 0, + std::nullopt, + {features}); } void @@ -867,8 +858,7 @@ private: using namespace jtx; // Code returned if an offer is killed. - TER const killedCode{ - features[fix1578] ? TER{tecKILLED} : TER{tesSUCCESS}}; + TER const killedCode{TER{tecKILLED}}; { Env env{*this, features}; diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 06dcd6e53c..709d3b213b 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -846,13 +846,8 @@ public: // Fill or Kill - unless we fully cross, just charge a fee and don't // place the offer on the books. But also clean up expired offers // that are discovered along the way. - // - // fix1578 changes the return code. Verify expected behavior - // without and with fix1578. - for (auto const& tweakedFeatures : - {features - fix1578, features | fix1578}) { - Env env{*this, tweakedFeatures}; + Env env{*this, features}; auto const f = env.current()->fees().base; @@ -878,9 +873,7 @@ public: // Order that can't be filled but will remove bob's expired offer: { - TER const killedCode{ - tweakedFeatures[fix1578] ? TER{tecKILLED} - : TER{tesSUCCESS}}; + TER const killedCode{TER{tecKILLED}}; env(offer(alice, XRP(1000), USD(1000)), txflags(tfFillOrKill), ter(killedCode)); @@ -3008,8 +3001,7 @@ public: env.close(); // Code returned if an offer is killed. - TER const killedCode{ - features[fix1578] ? TER{tecKILLED} : TER{tesSUCCESS}}; + TER const killedCode{TER{tecKILLED}}; // bob offers XRP for USD. env(trust(bob, USD(200))); diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 84fc284b13..895988152c 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -143,7 +143,6 @@ class Feature_test : public beast::unit_test::suite featureToName(fixTrustLinesToSelf) == "fixTrustLinesToSelf"); BEAST_EXPECT(featureToName(featureFlow) == "Flow"); BEAST_EXPECT(featureToName(featureNegativeUNL) == "NegativeUNL"); - BEAST_EXPECT(featureToName(fix1578) == "fix1578"); BEAST_EXPECT( featureToName(fixTakerDryOfferRemoval) == "fixTakerDryOfferRemoval"); diff --git a/src/test/rpc/NoRipple_test.cpp b/src/test/rpc/NoRipple_test.cpp index 93457ada8c..0f2b4748b6 100644 --- a/src/test/rpc/NoRipple_test.cpp +++ b/src/test/rpc/NoRipple_test.cpp @@ -85,83 +85,74 @@ public: auto const bob = Account("bob"); auto const carol = Account("carol"); - // fix1578 changes the return code. Verify expected behavior - // without and with fix1578. - for (auto const& tweakedFeatures : - {features - fix1578, features | fix1578}) + Env env(*this, features); + + env.fund(XRP(10000), gw, alice, bob, carol); + env.close(); + + env.trust(alice["USD"](100), bob); + env.trust(bob["USD"](100), carol); + env.close(); + + // After this payment alice has a -50 USD balance with bob, and + // bob has a -50 USD balance with carol. So neither alice nor + // bob should be able to clear the noRipple flag. + env(pay(alice, carol, carol["USD"](50)), path(bob)); + env.close(); + + TER const terNeg{TER{tecNO_PERMISSION}}; + + env(trust(alice, bob["USD"](100), bob, tfSetNoRipple), ter(terNeg)); + env(trust(bob, carol["USD"](100), carol, tfSetNoRipple), ter(terNeg)); + env.close(); + + Json::Value params; + params[jss::source_account] = alice.human(); + params[jss::destination_account] = carol.human(); + params[jss::destination_amount] = [] { + Json::Value dest_amt; + dest_amt[jss::currency] = "USD"; + dest_amt[jss::value] = "1"; + dest_amt[jss::issuer] = Account("carol").human(); + return dest_amt; + }(); + + auto const resp = + env.rpc("json", "ripple_path_find", to_string(params)); + BEAST_EXPECT(resp[jss::result][jss::alternatives].size() == 1); + + auto getAccountLines = [&env](Account const& acct) { + auto const r = jtx::getAccountLines(env, acct); + return r[jss::lines]; + }; { - Env env(*this, tweakedFeatures); + auto const aliceLines = getAccountLines(alice); + BEAST_EXPECT(aliceLines.size() == 1); + BEAST_EXPECT(aliceLines[0u][jss::no_ripple].asBool() == false); - env.fund(XRP(10000), gw, alice, bob, carol); - env.close(); + auto const bobLines = getAccountLines(bob); + BEAST_EXPECT(bobLines.size() == 2); + BEAST_EXPECT(bobLines[0u][jss::no_ripple].asBool() == false); + BEAST_EXPECT(bobLines[1u][jss::no_ripple].asBool() == false); + } - env.trust(alice["USD"](100), bob); - env.trust(bob["USD"](100), carol); - env.close(); + // Now carol sends the 50 USD back to alice. Then alice and + // bob can set the noRipple flag. + env(pay(carol, alice, alice["USD"](50)), path(bob)); + env.close(); - // After this payment alice has a -50 USD balance with bob, and - // bob has a -50 USD balance with carol. So neither alice nor - // bob should be able to clear the noRipple flag. - env(pay(alice, carol, carol["USD"](50)), path(bob)); - env.close(); + env(trust(alice, bob["USD"](100), bob, tfSetNoRipple)); + env(trust(bob, carol["USD"](100), carol, tfSetNoRipple)); + env.close(); + { + auto const aliceLines = getAccountLines(alice); + BEAST_EXPECT(aliceLines.size() == 1); + BEAST_EXPECT(aliceLines[0u].isMember(jss::no_ripple)); - TER const terNeg{ - tweakedFeatures[fix1578] ? TER{tecNO_PERMISSION} - : TER{tesSUCCESS}}; - - env(trust(alice, bob["USD"](100), bob, tfSetNoRipple), ter(terNeg)); - env(trust(bob, carol["USD"](100), carol, tfSetNoRipple), - ter(terNeg)); - env.close(); - - Json::Value params; - params[jss::source_account] = alice.human(); - params[jss::destination_account] = carol.human(); - params[jss::destination_amount] = [] { - Json::Value dest_amt; - dest_amt[jss::currency] = "USD"; - dest_amt[jss::value] = "1"; - dest_amt[jss::issuer] = Account("carol").human(); - return dest_amt; - }(); - - auto const resp = - env.rpc("json", "ripple_path_find", to_string(params)); - BEAST_EXPECT(resp[jss::result][jss::alternatives].size() == 1); - - auto getAccountLines = [&env](Account const& acct) { - auto const r = jtx::getAccountLines(env, acct); - return r[jss::lines]; - }; - { - auto const aliceLines = getAccountLines(alice); - BEAST_EXPECT(aliceLines.size() == 1); - BEAST_EXPECT(aliceLines[0u][jss::no_ripple].asBool() == false); - - auto const bobLines = getAccountLines(bob); - BEAST_EXPECT(bobLines.size() == 2); - BEAST_EXPECT(bobLines[0u][jss::no_ripple].asBool() == false); - BEAST_EXPECT(bobLines[1u][jss::no_ripple].asBool() == false); - } - - // Now carol sends the 50 USD back to alice. Then alice and - // bob can set the noRipple flag. - env(pay(carol, alice, alice["USD"](50)), path(bob)); - env.close(); - - env(trust(alice, bob["USD"](100), bob, tfSetNoRipple)); - env(trust(bob, carol["USD"](100), carol, tfSetNoRipple)); - env.close(); - { - auto const aliceLines = getAccountLines(alice); - BEAST_EXPECT(aliceLines.size() == 1); - BEAST_EXPECT(aliceLines[0u].isMember(jss::no_ripple)); - - auto const bobLines = getAccountLines(bob); - BEAST_EXPECT(bobLines.size() == 2); - BEAST_EXPECT(bobLines[0u].isMember(jss::no_ripple_peer)); - BEAST_EXPECT(bobLines[1u].isMember(jss::no_ripple)); - } + auto const bobLines = getAccountLines(bob); + BEAST_EXPECT(bobLines.size() == 2); + BEAST_EXPECT(bobLines[0u].isMember(jss::no_ripple_peer)); + BEAST_EXPECT(bobLines[1u].isMember(jss::no_ripple)); } } diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index a503f913fa..6303918ef2 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/src/xrpld/app/tx/detail/CreateOffer.cpp @@ -795,9 +795,7 @@ CreateOffer::applyGuts(Sandbox& sb, Sandbox& sbCancel) if (bFillOrKill) { JLOG(j_.trace()) << "Fill or Kill: offer killed"; - if (sb.rules().enabled(fix1578)) - return {tecKILLED, false}; - return {tesSUCCESS, false}; + return {tecKILLED, false}; } // For 'immediate or cancel' offers, the amount remaining doesn't get diff --git a/src/xrpld/app/tx/detail/SetTrust.cpp b/src/xrpld/app/tx/detail/SetTrust.cpp index d881425960..59333f3077 100644 --- a/src/xrpld/app/tx/detail/SetTrust.cpp +++ b/src/xrpld/app/tx/detail/SetTrust.cpp @@ -576,7 +576,7 @@ SetTrust::doApply() if ((bHigh ? saHighBalance : saLowBalance) >= beast::zero) uFlagsOut |= (bHigh ? lsfHighNoRipple : lsfLowNoRipple); - else if (view().rules().enabled(fix1578)) + else // Cannot set noRipple on a negative balance. return tecNO_PERMISSION; } From 553fb5be3bab1fd1bf0553d4bed9d44191e14bf1 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Wed, 29 Oct 2025 16:36:51 +0000 Subject: [PATCH 04/10] refactor: Retire fixCheckThreading amendment (#5957) Amendments activated for more than 2 years can be retired. This change retires the fixCheckThreading amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/libxrpl/ledger/ApplyStateTable.cpp | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 6d3da84f84..aa759603e9 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -102,7 +102,6 @@ XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYe XRPL_FIX (QualityUpperBound, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (PayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (CheckThreading, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (MasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (TakerDryOfferRemoval, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) @@ -154,3 +153,4 @@ XRPL_RETIRE(fix1543) XRPL_RETIRE(fix1781) XRPL_RETIRE(fix1571) XRPL_RETIRE(fix1578) +XRPL_RETIRE(fixCheckThreading) diff --git a/src/libxrpl/ledger/ApplyStateTable.cpp b/src/libxrpl/ledger/ApplyStateTable.cpp index aaad056c58..99b76e2d11 100644 --- a/src/libxrpl/ledger/ApplyStateTable.cpp +++ b/src/libxrpl/ledger/ApplyStateTable.cpp @@ -682,12 +682,6 @@ ApplyStateTable::threadOwners( if (auto const optSleAcct{(*sle)[~sfAccount]}) threadTx(base, meta, *optSleAcct, mods, j); - // Don't thread a check's sfDestination unless the amendment is - // enabled - if (ledgerType == ltCHECK && - !base.rules().enabled(fixCheckThreading)) - break; - // If sfDestination is present, thread to that account if (auto const optSleDest{(*sle)[~sfDestination]}) threadTx(base, meta, *optSleDest, mods, j); From 48d38c1e2ca1d73612776f3a27eb472847429fa7 Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 29 Oct 2025 13:03:16 -0400 Subject: [PATCH 05/10] refactor: Sorts retired amendments to reduce conflicts (#5966) We are on an amendment retiring spree, but each change results in conflicts in `features.macro` because currently they all add the retired amendment to the end of the list. By sorting the list the number of conflicts should be reduced, making it easier to merge them. --- include/xrpl/protocol/detail/features.macro | 30 ++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index aa759603e9..d2ff03cab7 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -131,26 +131,26 @@ XRPL_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete) // pre-amendment code has been removed and the identifiers are deprecated. // All known amendments and amendments that may appear in a validated // ledger must be registered either here or above with the "active" amendments -XRPL_RETIRE(MultiSign) -XRPL_RETIRE(TrustSetAuth) -XRPL_RETIRE(FeeEscalation) -XRPL_RETIRE(PayChan) -XRPL_RETIRE(CryptoConditions) -XRPL_RETIRE(TickSize) -XRPL_RETIRE(fix1368) -XRPL_RETIRE(Escrow) -XRPL_RETIRE(fix1373) -XRPL_RETIRE(EnforceInvariants) -XRPL_RETIRE(SortedDirectories) XRPL_RETIRE(fix1201) +XRPL_RETIRE(fix1368) +XRPL_RETIRE(fix1373) XRPL_RETIRE(fix1512) -XRPL_RETIRE(fix1523) -XRPL_RETIRE(fix1528) -XRPL_RETIRE(FlowCross) XRPL_RETIRE(fix1513) XRPL_RETIRE(fix1515) +XRPL_RETIRE(fix1523) +XRPL_RETIRE(fix1528) XRPL_RETIRE(fix1543) -XRPL_RETIRE(fix1781) XRPL_RETIRE(fix1571) XRPL_RETIRE(fix1578) +XRPL_RETIRE(fix1781) XRPL_RETIRE(fixCheckThreading) +XRPL_RETIRE(CryptoConditions) +XRPL_RETIRE(Escrow) +XRPL_RETIRE(EnforceInvariants) +XRPL_RETIRE(FeeEscalation) +XRPL_RETIRE(FlowCross) +XRPL_RETIRE(MultiSign) +XRPL_RETIRE(PayChan) +XRPL_RETIRE(SortedDirectories) +XRPL_RETIRE(TickSize) +XRPL_RETIRE(TrustSetAuth) From 80a3ae6386b257bff2475ac8055b3422be707127 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Wed, 29 Oct 2025 17:34:06 +0000 Subject: [PATCH 06/10] refactor: Retire fixRmSmallIncreasedQOffers amendment (#5955) Amendments activated for more than 2 years can be retired. This change retires the fixRmSmallIncreasedQOffers amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/Offer_test.cpp | 141 +++++--------------- src/xrpld/app/tx/detail/OfferStream.cpp | 3 - 3 files changed, 38 insertions(+), 108 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index d2ff03cab7..b80b61fc8c 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -91,7 +91,6 @@ XRPL_FIX (TrustLinesToSelf, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(NonFungibleTokensV1_1, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(ExpandedSignerList, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(CheckCashMakesTrustLine, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FIX (RmSmallIncreasedQOffers, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (STAmountCanonicalize, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYes) @@ -144,6 +143,7 @@ XRPL_RETIRE(fix1571) XRPL_RETIRE(fix1578) XRPL_RETIRE(fix1781) XRPL_RETIRE(fixCheckThreading) +XRPL_RETIRE(fixRmSmallIncreasedQOffers) XRPL_RETIRE(CryptoConditions) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 709d3b213b..c279afd2a2 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -366,37 +366,22 @@ public: env(offer(alice, USD(1), aliceTakerGets)); env.close(); - if (features[fixRmSmallIncreasedQOffers]) + env.require( + offers(carol, 0), + balance( + carol, + initialCarolUSD)); // offer is removed but not taken + if (crossBothOffers) { env.require( - offers(carol, 0), - balance( - carol, - initialCarolUSD)); // offer is removed but not taken - if (crossBothOffers) - { - env.require( - offers(alice, 0), - balance(alice, USD(1))); // alice's offer is crossed - } - else - { - env.require( - offers(alice, 1), - balance( - alice, USD(0))); // alice's offer is not crossed - } + offers(alice, 0), + balance(alice, USD(1))); // alice's offer is crossed } else { env.require( offers(alice, 1), - offers(bob, 1), - offers(carol, 1), - balance(alice, USD(0)), - balance( - carol, - initialCarolUSD)); // offer is not crossed at all + balance(alice, USD(0))); // alice's offer is not crossed } } @@ -434,36 +419,19 @@ public: ter(expectedTer)); env.close(); - if (features[fixRmSmallIncreasedQOffers]) + if (expectedTer == tesSUCCESS) { - if (expectedTer == tesSUCCESS) - { - env.require(offers(carol, 0)); - env.require(balance( - carol, - initialCarolUSD)); // offer is removed but not taken - } - else - { - // TODO: Offers are not removed when payments fail - // If that is addressed, the test should show that carol's - // offer is removed but not taken, as in the other branch of - // this if statement - } + env.require(offers(carol, 0)); + env.require(balance( + carol, + initialCarolUSD)); // offer is removed but not taken } else { - if (partialPayment) - { - env.require(offers(carol, 0)); - env.require( - balance(carol, USD(0))); // offer is removed and taken - } - else - { - // offer is not removed or taken - BEAST_EXPECT(isOffer(env, carol, drops(1), USD(1))); - } + // TODO: Offers are not removed when payments fail + // If that is addressed, the test should show that carol's + // offer is removed but not taken, as in the other branch of + // this if statement } } } @@ -526,37 +494,22 @@ public: env(offer(alice, USD(1), aliceTakerGets)); env.close(); - if (features[fixRmSmallIncreasedQOffers]) + env.require( + offers(carol, 0), + balance( + carol, + initialCarolUSD)); // offer is removed but not taken + if (crossBothOffers) { env.require( - offers(carol, 0), - balance( - carol, - initialCarolUSD)); // offer is removed but not taken - if (crossBothOffers) - { - env.require( - offers(alice, 0), - balance(alice, USD(1))); // alice's offer is crossed - } - else - { - env.require( - offers(alice, 1), - balance( - alice, USD(0))); // alice's offer is not crossed - } + offers(alice, 0), + balance(alice, USD(1))); // alice's offer is crossed } else { env.require( offers(alice, 1), - offers(bob, 1), - offers(carol, 1), - balance(alice, USD(0)), - balance( - carol, - initialCarolUSD)); // offer is not crossed at all + balance(alice, USD(0))); // alice's offer is not crossed } } @@ -597,36 +550,19 @@ public: ter(expectedTer)); env.close(); - if (features[fixRmSmallIncreasedQOffers]) + if (expectedTer == tesSUCCESS) { - if (expectedTer == tesSUCCESS) - { - env.require(offers(carol, 0)); - env.require(balance( - carol, - initialCarolUSD)); // offer is removed but not taken - } - else - { - // TODO: Offers are not removed when payments fail - // If that is addressed, the test should show that carol's - // offer is removed but not taken, as in the other branch of - // this if statement - } + env.require(offers(carol, 0)); + env.require(balance( + carol, + initialCarolUSD)); // offer is removed but not taken } else { - if (partialPayment) - { - env.require(offers(carol, 0)); - env.require( - balance(carol, USD(0))); // offer is removed and taken - } - else - { - // offer is not removed or taken - BEAST_EXPECT(isOffer(env, carol, EUR(1), USD(2))); - } + // TODO: Offers are not removed when payments fail + // If that is addressed, the test should show that carol's + // offer is removed but not taken, as in the other branch of + // this if statement } } } @@ -5359,8 +5295,6 @@ public: using namespace jtx; static FeatureBitset const all{testable_amendments()}; static FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; - static FeatureBitset const rmSmallIncreasedQOffers{ - fixRmSmallIncreasedQOffers}; static FeatureBitset const immediateOfferKilled{ featureImmediateOfferKilled}; FeatureBitset const fillOrKill{fixFillOrKill}; @@ -5369,8 +5303,7 @@ public: static std::array const feats{ all - takerDryOffer - immediateOfferKilled - permDEX, all - immediateOfferKilled - permDEX, - all - rmSmallIncreasedQOffers - immediateOfferKilled - fillOrKill - - permDEX, + all - immediateOfferKilled - fillOrKill - permDEX, all - fillOrKill - permDEX, all - permDEX, all}; diff --git a/src/xrpld/app/tx/detail/OfferStream.cpp b/src/xrpld/app/tx/detail/OfferStream.cpp index 9d43e419d7..8f19c20cf5 100644 --- a/src/xrpld/app/tx/detail/OfferStream.cpp +++ b/src/xrpld/app/tx/detail/OfferStream.cpp @@ -158,9 +158,6 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const !std::is_same_v, "Cannot have XRP/XRP offers"); - if (!view_.rules().enabled(fixRmSmallIncreasedQOffers)) - return false; - // Consider removing the offer if: // o `TakerPays` is XRP (because of XRP drops granularity) or // o `TakerPays` and `TakerGets` are both IOU and `TakerPays`<`TakerGets` From f8b4f692f1cb3b971138b71912bcd2c2eb9ee4d5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Wed, 29 Oct 2025 18:17:50 +0000 Subject: [PATCH 07/10] refactor: Retire fixSTAmountCanonicalize code (#5956) Amendments activated for more than 2 years can be retired. This change retires the fixSTAmountCanonicalize amendment. --- include/xrpl/protocol/STAmount.h | 31 ----------- include/xrpl/protocol/detail/features.macro | 2 +- src/libxrpl/protocol/STAmount.cpp | 59 ++++++--------------- src/xrpld/app/misc/detail/TxQ.cpp | 2 - src/xrpld/app/tx/detail/Transactor.cpp | 5 +- src/xrpld/app/tx/detail/apply.cpp | 2 - 6 files changed, 18 insertions(+), 83 deletions(-) diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index f1e34463b6..ee68e33000 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -709,37 +709,6 @@ canAdd(STAmount const& amt1, STAmount const& amt2); bool canSubtract(STAmount const& amt1, STAmount const& amt2); -// Since `canonicalize` does not have access to a ledger, this is needed to put -// the low-level routine stAmountCanonicalize on an amendment switch. Only -// transactions need to use this switchover. Outside of a transaction it's safe -// to unconditionally use the new behavior. - -bool -getSTAmountCanonicalizeSwitchover(); - -void -setSTAmountCanonicalizeSwitchover(bool v); - -/** RAII class to set and restore the STAmount canonicalize switchover. - */ - -class STAmountSO -{ -public: - explicit STAmountSO(bool v) : saved_(getSTAmountCanonicalizeSwitchover()) - { - setSTAmountCanonicalizeSwitchover(v); - } - - ~STAmountSO() - { - setSTAmountCanonicalizeSwitchover(saved_); - } - -private: - bool saved_; -}; - } // namespace ripple //------------------------------------------------------------------------------ diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index b80b61fc8c..0c817829db 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -91,7 +91,6 @@ XRPL_FIX (TrustLinesToSelf, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(NonFungibleTokensV1_1, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(ExpandedSignerList, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(CheckCashMakesTrustLine, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FIX (STAmountCanonicalize, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes) @@ -144,6 +143,7 @@ XRPL_RETIRE(fix1578) XRPL_RETIRE(fix1781) XRPL_RETIRE(fixCheckThreading) XRPL_RETIRE(fixRmSmallIncreasedQOffers) +XRPL_RETIRE(fixSTAmountCanonicalize) XRPL_RETIRE(CryptoConditions) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 0c72244885..dfd1b3dfbe 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -68,29 +68,6 @@ namespace ripple { -namespace { - -// Use a static inside a function to help prevent order-of-initialzation issues -LocalValue& -getStaticSTAmountCanonicalizeSwitchover() -{ - static LocalValue r{true}; - return r; -} -} // namespace - -bool -getSTAmountCanonicalizeSwitchover() -{ - return *getStaticSTAmountCanonicalizeSwitchover(); -} - -void -setSTAmountCanonicalizeSwitchover(bool v) -{ - *getStaticSTAmountCanonicalizeSwitchover() = v; -} - static std::uint64_t const tenTo14 = 100000000000000ull; static std::uint64_t const tenTo14m1 = tenTo14 - 1; static std::uint64_t const tenTo17 = tenTo14 * 1000; @@ -884,18 +861,14 @@ STAmount::canonicalize() return; } - if (getSTAmountCanonicalizeSwitchover()) - { - // log(cMaxNativeN, 10) == 17 - if (native() && mOffset > 17) - Throw( - "Native currency amount out of range"); - // log(maxMPTokenAmount, 10) ~ 18.96 - if (mAsset.holds() && mOffset > 18) - Throw("MPT amount out of range"); - } + // log(cMaxNativeN, 10) == 17 + if (native() && mOffset > 17) + Throw("Native currency amount out of range"); + // log(maxMPTokenAmount, 10) ~ 18.96 + if (mAsset.holds() && mOffset > 18) + Throw("MPT amount out of range"); - if (getSTNumberSwitchover() && getSTAmountCanonicalizeSwitchover()) + if (getSTNumberSwitchover()) { Number num( mIsNegative ? -mValue : mValue, mOffset, Number::unchecked{}); @@ -919,16 +892,14 @@ STAmount::canonicalize() while (mOffset > 0) { - if (getSTAmountCanonicalizeSwitchover()) - { - // N.B. do not move the overflow check to after the - // multiplication - if (native() && mValue > cMaxNativeN) - Throw( - "Native currency amount out of range"); - else if (!native() && mValue > maxMPTokenAmount) - Throw("MPT amount out of range"); - } + // N.B. do not move the overflow check to after the + // multiplication + if (native() && mValue > cMaxNativeN) + Throw( + "Native currency amount out of range"); + else if (!native() && mValue > maxMPTokenAmount) + Throw("MPT amount out of range"); + mValue *= 10; --mOffset; } diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index 7c0a6f07e2..23a083fbe8 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -300,7 +300,6 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j) // If the rules or flags change, preflight again XRPL_ASSERT( pfresult, "ripple::TxQ::MaybeTx::apply : preflight result is set"); - STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; NumberSO stNumberSO{view.rules().enabled(fixUniversalNumber)}; if (pfresult->rules != view.rules() || pfresult->flags != flags) @@ -734,7 +733,6 @@ TxQ::apply( ApplyFlags flags, beast::Journal j) { - STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; NumberSO stNumberSO{view.rules().enabled(fixUniversalNumber)}; // See if the transaction is valid, properly formed, diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 2f62a142c0..6e3b56fe99 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -1163,9 +1163,8 @@ Transactor::operator()() { JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID(); - // raii classes for the current ledger rules. fixSTAmountCanonicalize and - // fixSTAmountCanonicalize predate the rulesGuard and should be replaced. - STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)}; + // raii classes for the current ledger rules. + // fixUniversalNumber predate the rulesGuard and should be replaced. NumberSO stNumberSO{view().rules().enabled(fixUniversalNumber)}; CurrentTransactionRulesGuard currentTransctionRulesGuard(view().rules()); diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index e2e0adae45..0ddaf0578e 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -138,9 +138,7 @@ template ApplyResult apply(Application& app, OpenView& view, PreflightChecks&& preflightChecks) { - STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)}; NumberSO stNumberSO{view.rules().enabled(fixUniversalNumber)}; - return doApply(preclaim(preflightChecks(), app, view), app, view); } From efd4c1b95d329c93dd76e7e3d6ae99337362240c Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Thu, 30 Oct 2025 12:49:44 +0000 Subject: [PATCH 08/10] chore: Use new prepare-runner (#5970) See: XRPLF/actions#19. --- .github/workflows/reusable-build.yml | 2 +- .github/workflows/upload-conan-deps.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reusable-build.yml b/.github/workflows/reusable-build.yml index d156f05394..a9d5fd7f7c 100644 --- a/.github/workflows/reusable-build.yml +++ b/.github/workflows/reusable-build.yml @@ -63,7 +63,7 @@ jobs: uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - name: Prepare runner - uses: XRPLF/actions/.github/actions/prepare-runner@638e0dc11ea230f91bd26622fb542116bb5254d5 + uses: XRPLF/actions/.github/actions/prepare-runner@99685816bb60a95a66852f212f382580e180df3a with: disable_ccache: false diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index aa3870ffcf..f6262a2f1f 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -66,7 +66,7 @@ jobs: uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - name: Prepare runner - uses: XRPLF/actions/.github/actions/prepare-runner@638e0dc11ea230f91bd26622fb542116bb5254d5 + uses: XRPLF/actions/.github/actions/prepare-runner@99685816bb60a95a66852f212f382580e180df3a with: disable_ccache: false From a10f42a3aa4e32943023adef7a13321823050cf3 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 30 Oct 2025 09:19:51 -0400 Subject: [PATCH 09/10] ci: Check whether test failures are caused by port exhaustion (#5938) This change adds an extra step to the CI test job that outputs network info, which may allow us to confirm whether random test failures are caused by port exhaustion. --- .github/workflows/reusable-test.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/reusable-test.yml b/.github/workflows/reusable-test.yml index 2127355f40..8d4a4a8d33 100644 --- a/.github/workflows/reusable-test.yml +++ b/.github/workflows/reusable-test.yml @@ -100,3 +100,12 @@ jobs: "$test_file" fi done + + - name: Debug failure (Linux) + if: ${{ failure() && runner.os == 'Linux' && inputs.run_tests }} + shell: bash + run: | + echo "IPv4 local port range:" + cat /proc/sys/net/ipv4/ip_local_port_range + echo "Netstat:" + netstat -an From 44e027e51614c10124ab42588c65b63b173c1bf9 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Thu, 30 Oct 2025 15:27:01 +0000 Subject: [PATCH 10/10] refactor: Retire fixTakerDryOfferRemoval amendment (#5958) Amendments activated for more than 2 years can be retired. This change retires the fixTakerDryOfferRemoval amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/Offer_test.cpp | 25 +++++---------------- src/test/rpc/Feature_test.cpp | 4 ++-- 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 0c817829db..6eb9c03267 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -101,7 +101,6 @@ XRPL_FIX (QualityUpperBound, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (PayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (MasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (TakerDryOfferRemoval, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes) @@ -144,6 +143,7 @@ XRPL_RETIRE(fix1781) XRPL_RETIRE(fixCheckThreading) XRPL_RETIRE(fixRmSmallIncreasedQOffers) XRPL_RETIRE(fixSTAmountCanonicalize) +XRPL_RETIRE(fixTakerDryOfferRemoval) XRPL_RETIRE(CryptoConditions) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index c279afd2a2..0dbcfccebd 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5294,14 +5294,12 @@ public: { using namespace jtx; static FeatureBitset const all{testable_amendments()}; - static FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; static FeatureBitset const immediateOfferKilled{ featureImmediateOfferKilled}; FeatureBitset const fillOrKill{fixFillOrKill}; FeatureBitset const permDEX{featurePermissionedDEX}; - static std::array const feats{ - all - takerDryOffer - immediateOfferKilled - permDEX, + static std::array const feats{ all - immediateOfferKilled - permDEX, all - immediateOfferKilled - fillOrKill - permDEX, all - fillOrKill - permDEX, @@ -5323,7 +5321,7 @@ public: } }; -class OfferWTakerDryOffer_test : public OfferBaseUtil_test +class OfferWOSmallQOffers_test : public OfferBaseUtil_test { void run() override @@ -5332,7 +5330,7 @@ class OfferWTakerDryOffer_test : public OfferBaseUtil_test } }; -class OfferWOSmallQOffers_test : public OfferBaseUtil_test +class OfferWOFillOrKill_test : public OfferBaseUtil_test { void run() override @@ -5341,7 +5339,7 @@ class OfferWOSmallQOffers_test : public OfferBaseUtil_test } }; -class OfferWOFillOrKill_test : public OfferBaseUtil_test +class OfferWOPermDEX_test : public OfferBaseUtil_test { void run() override @@ -5350,21 +5348,12 @@ class OfferWOFillOrKill_test : public OfferBaseUtil_test } }; -class OfferWOPermDEX_test : public OfferBaseUtil_test -{ - void - run() override - { - OfferBaseUtil_test::run(4); - } -}; - class OfferAllFeatures_test : public OfferBaseUtil_test { void run() override { - OfferBaseUtil_test::run(5, true); + OfferBaseUtil_test::run(4, true); } }; @@ -5376,7 +5365,6 @@ class Offer_manual_test : public OfferBaseUtil_test using namespace jtx; FeatureBitset const all{testable_amendments()}; FeatureBitset const immediateOfferKilled{featureImmediateOfferKilled}; - FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; FeatureBitset const fillOrKill{fixFillOrKill}; FeatureBitset const permDEX{featurePermissionedDEX}; @@ -5385,13 +5373,10 @@ class Offer_manual_test : public OfferBaseUtil_test testAll(all - fillOrKill - permDEX); testAll(all - permDEX); testAll(all); - - testAll(all - takerDryOffer - permDEX); } }; BEAST_DEFINE_TESTSUITE_PRIO(OfferBaseUtil, app, ripple, 2); -BEAST_DEFINE_TESTSUITE_PRIO(OfferWTakerDryOffer, app, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(OfferWOSmallQOffers, app, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(OfferWOFillOrKill, app, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(OfferWOPermDEX, app, ripple, 2); diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 895988152c..f01ee5b116 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -144,8 +144,8 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECT(featureToName(featureFlow) == "Flow"); BEAST_EXPECT(featureToName(featureNegativeUNL) == "NegativeUNL"); BEAST_EXPECT( - featureToName(fixTakerDryOfferRemoval) == - "fixTakerDryOfferRemoval"); + featureToName(fixIncludeKeyletFields) == "fixIncludeKeyletFields"); + BEAST_EXPECT(featureToName(featureTokenEscrow) == "TokenEscrow"); } void