From 4d0ea8ae36778ae200178985a2318fb835cea8d3 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 30 Apr 2026 06:22:11 -0400 Subject: [PATCH 1/2] refactor: Apply various minor improvements and corrections (#7045) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- include/xrpl/protocol/detail/features.macro | 1 - src/libxrpl/beast/core/CurrentThreadName.cpp | 9 ++---- .../tx/transactors/nft/NFTokenAcceptOffer.cpp | 20 +++++------- src/test/app/NFToken_test.cpp | 32 +++++++++---------- src/test/rpc/LedgerEntry_test.cpp | 4 +-- src/xrpld/overlay/detail/PeerImp.cpp | 2 +- src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp | 2 +- 7 files changed, 31 insertions(+), 39 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index bad43dd6ed..8fbc28123f 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -19,7 +19,6 @@ XRPL_FIX (Cleanup3_2_0, Supported::no, VoteBehavior::DefaultNo XRPL_FEATURE(MPTokensV2, Supported::no, VoteBehavior::DefaultNo) XRPL_FIX (Security3_1_3, Supported::no, VoteBehavior::DefaultNo) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::no, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionDelegationV1_1, Supported::no, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/beast/core/CurrentThreadName.cpp b/src/libxrpl/beast/core/CurrentThreadName.cpp index ccd3df5f74..fd8a4764fa 100644 --- a/src/libxrpl/beast/core/CurrentThreadName.cpp +++ b/src/libxrpl/beast/core/CurrentThreadName.cpp @@ -81,12 +81,9 @@ setCurrentThreadNameImpl(std::string_view name) { // truncate and set the thread name. char boundedName[maxThreadNameLength + 1]; - std::snprintf( - boundedName, - sizeof(boundedName), - "%.*s", - static_cast(maxThreadNameLength), - name.data()); // NOLINT(bugprone-suspicious-stringview-data-usage) + auto const boundedSize = name.size() < maxThreadNameLength ? name.size() : maxThreadNameLength; + name.copy(boundedName, boundedSize); + boundedName[boundedSize] = '\0'; pthread_setname_np(pthread_self(), boundedName); diff --git a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp index 9de382733d..099b174778 100644 --- a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp +++ b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp @@ -68,15 +68,12 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration])) { - // Before fixExpiredNFTokenOfferRemoval amendment, expired - // offers caused tecEXPIRED in preclaim, leaving them on ledger - // forever. After the amendment, we allow expired offers to - // reach doApply() where they get deleted and tecEXPIRED is - // returned. - if (!ctx.view.rules().enabled(fixExpiredNFTokenOfferRemoval)) + // Before fixSecurity3_1_3 amendment, expired offers caused tecEXPIRED in preclaim, + // leaving them on ledger forever. After the amendment, we allow expired offers to + // reach doApply() where they get deleted and tecEXPIRED is returned. + if (!ctx.view.rules().enabled(fixSecurity3_1_3)) return {nullptr, tecEXPIRED}; - // Amendment enabled: return the expired offer to be handled in - // doApply + // Amendment enabled: return the expired offer to be handled in doApply. } if ((*offerSLE)[sfAmount].negative()) @@ -453,10 +450,9 @@ NFTokenAcceptOffer::doApply() auto bo = loadToken(ctx_.tx[~sfNFTokenBuyOffer]); auto so = loadToken(ctx_.tx[~sfNFTokenSellOffer]); - // With fixExpiredNFTokenOfferRemoval amendment, check for expired offers - // and delete them, returning tecEXPIRED. This ensures expired offers - // are properly cleaned up from the ledger. - if (view().rules().enabled(fixExpiredNFTokenOfferRemoval)) + // With fixSecurity3_1_3 amendment, check for expired offers and delete them, returning + // tecEXPIRED. This ensures expired offers are properly cleaned up from the ledger. + if (view().rules().enabled(fixSecurity3_1_3)) { bool foundExpired = false; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 40934cc095..a9252d2b5e 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -1096,10 +1096,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // The buy offer must not have expired. // NOTE: this is only a preclaim check with the - // fixExpiredNFTokenOfferRemoval amendment disabled. + // fixSecurity3_1_3 amendment disabled. env(token::acceptBuyOffer(alice, buyerExpOfferIndex), ter(tecEXPIRED)); env.close(); - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { buyerCount--; } @@ -1117,12 +1117,12 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // The sell offer must not have expired. // NOTE: this is only a preclaim check with the - // fixExpiredNFTokenOfferRemoval amendment disabled. + // fixSecurity3_1_3 amendment disabled. env(token::acceptSellOffer(buyer, aliceExpOfferIndex), ter(tecEXPIRED)); env.close(); // Alice's count is decremented by one when the expired offer is // removed. - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { aliceCount--; } @@ -3101,10 +3101,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // No one can accept an expired sell offer. env(token::acceptSellOffer(buyer, offer1), ter(tecEXPIRED)); - // With fixExpiredNFTokenOfferRemoval amendment, the first accept + // With fixSecurity3_1_3 amendment, the first accept // attempt deletes the expired offer. Without the amendment, // the offer remains and we can try to accept it again. - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { // After amendment: offer was deleted by first accept attempt minterCount--; @@ -3123,7 +3123,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (!features[fixExpiredNFTokenOfferRemoval]) + if (!features[fixSecurity3_1_3]) { // Before amendment: expired offer still exists and needs to be // cancelled @@ -3189,10 +3189,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // An expired buy offer cannot be accepted. env(token::acceptBuyOffer(minter, offer1), ter(tecEXPIRED)); - // With fixExpiredNFTokenOfferRemoval amendment, the first accept + // With fixSecurity3_1_3 amendment, the first accept // attempt deletes the expired offer. Without the amendment, // the offer remains and we can try to accept it again. - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { // After amendment: offer was deleted by first accept attempt buyerCount--; @@ -3211,7 +3211,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (!features[fixExpiredNFTokenOfferRemoval]) + if (!features[fixSecurity3_1_3]) { // Before amendment: expired offer still exists and can be // cancelled @@ -3288,7 +3288,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env(token::brokerOffers(issuer, buyOffer1, sellOffer1), ter(tecEXPIRED)); env.close(); - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { // With amendment: expired offers are deleted minterCount--; @@ -3298,7 +3298,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == minterCount); BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { // The buy offer was deleted, so no need to cancel it // The sell offer still exists, so we can cancel it @@ -3377,7 +3377,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); - if (features[fixExpiredNFTokenOfferRemoval]) + if (features[fixSecurity3_1_3]) { // After amendment: expired offers were deleted during broker // attempt @@ -3463,7 +3463,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // The expired offers are still in the ledger. BEAST_EXPECT(ownerCount(env, issuer) == 0); - if (!features[fixExpiredNFTokenOfferRemoval]) + if (!features[fixSecurity3_1_3]) { // Before amendment: expired offers still exist in ledger BEAST_EXPECT(ownerCount(env, minter) == 2); @@ -7190,7 +7190,7 @@ public: { testWithFeats( allFeatures - fixNFTokenReserve - featureNFTokenMintOffer - featureDynamicNFT - - fixExpiredNFTokenOfferRemoval); + fixSecurity3_1_3); } }; @@ -7227,7 +7227,7 @@ class NFTokenWOExpiredOfferRemoval_test : public NFTokenBaseUtil_test void run() override { - testWithFeats(allFeatures - fixExpiredNFTokenOfferRemoval); + testWithFeats(allFeatures - fixSecurity3_1_3); } }; diff --git a/src/test/rpc/LedgerEntry_test.cpp b/src/test/rpc/LedgerEntry_test.cpp index d208d718f4..d6e1ccc2ce 100644 --- a/src/test/rpc/LedgerEntry_test.cpp +++ b/src/test/rpc/LedgerEntry_test.cpp @@ -1201,7 +1201,7 @@ class LedgerEntry_test : public beast::unit_test::suite checkErrorValue( jrr[jss::result], "malformedAuthorizedCredentials", - "Invalid field 'authorized_credentials', not array."); + "Invalid field 'authorized_credentials', not array of objects."); } { @@ -1219,7 +1219,7 @@ class LedgerEntry_test : public beast::unit_test::suite checkErrorValue( jrr[jss::result], "malformedAuthorizedCredentials", - "Invalid field 'authorized_credentials', not array."); + "Invalid field 'authorized_credentials', not array of objects."); } { diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 01cad680ba..d90bbaff22 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -2639,7 +2639,7 @@ PeerImp::onMessage(std::shared_ptr const& m) { fee_.update( Resource::feeModerateBurdenPeer, - " Reply limit reached. Truncating reply."); + "Reply limit reached. Truncating reply."); break; } } diff --git a/src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp b/src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp index 4de75da1b0..fe59cf27f3 100644 --- a/src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp +++ b/src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp @@ -267,7 +267,7 @@ parseAuthorizeCredentials(Json::Value const& jv) if (!jo.isObject()) { return LedgerEntryHelpers::invalidFieldError( - "malformedAuthorizedCredentials", jss::authorized_credentials, "array"); + "malformedAuthorizedCredentials", jss::authorized_credentials, "array of objects"); } if (auto const value = LedgerEntryHelpers::hasRequired( From 6407f0fa52ca6322e1a0186d8dd9adf880b98150 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 30 Apr 2026 11:36:12 +0100 Subject: [PATCH 2/2] fix: Address code review comments regarding `boost::coroutine2` (#6977) Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- cmake/XrplInterface.cmake | 1 - conan/profiles/sanitizers | 7 +++++++ conanfile.py | 6 ------ include/xrpl/core/Coro.ipp | 12 +++++++----- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/cmake/XrplInterface.cmake b/cmake/XrplInterface.cmake index 7add613f5a..825cb63310 100644 --- a/cmake/XrplInterface.cmake +++ b/cmake/XrplInterface.cmake @@ -23,7 +23,6 @@ target_compile_definitions( BOOST_FILESYSTEM_NO_DEPRECATED > $<$>: - BOOST_COROUTINES2_NO_DEPRECATION_WARNING BOOST_BEAST_ALLOW_DEPRECATED BOOST_FILESYSTEM_DEPRECATED > diff --git a/conan/profiles/sanitizers b/conan/profiles/sanitizers index 6d37425f43..073fcf3072 100644 --- a/conan/profiles/sanitizers +++ b/conan/profiles/sanitizers @@ -82,5 +82,12 @@ tools.info.package_id:confs+=["tools.build:cxxflags", "tools.build:exelinkflags" boost/*:without_context=False # Boost stacktrace fails to build with some sanitizers boost/*:without_stacktrace=True + {% elif "thread" in sanitizers %} + # Build Boost.Context with ucontext backend for TSAN. fcontext (assembly) + # has no TSAN annotations, so without this the BOOST_USE_TSAN/BOOST_USE_UCONTEXT + # defines in [conf] would be ineffective. + boost/*:extra_b2_flags=context-impl=ucontext thread-sanitizer=on define=BOOST_USE_TSAN=1 + boost/*:without_context=False + boost/*:without_stacktrace=True {% endif %} {% endif %} diff --git a/conanfile.py b/conanfile.py index 4949516bfe..66de2e2d57 100644 --- a/conanfile.py +++ b/conanfile.py @@ -129,12 +129,6 @@ class Xrpl(ConanFile): if self.settings.compiler in ["clang", "gcc"]: self.options["boost"].without_cobalt = True - # Check if environment variable exists - if "SANITIZERS" in os.environ: - sanitizers = os.environ["SANITIZERS"] - if "address" in sanitizers.lower(): - self.default_options["fPIC"] = False - def requirements(self): self.requires("boost/1.90.0", force=True, transitive_headers=True) self.requires("date/3.0.4", transitive_headers=True) diff --git a/include/xrpl/core/Coro.ipp b/include/xrpl/core/Coro.ipp index 2343f805a5..e7461b1b85 100644 --- a/include/xrpl/core/Coro.ipp +++ b/include/xrpl/core/Coro.ipp @@ -2,17 +2,19 @@ namespace xrpl { +/// Coroutine stack size (1.5 MB). Increased from 1 MB because +/// ASAN-instrumented deep call stacks exceeded the original limit. +constexpr std::size_t coroStackSize = 1536 * 1024; + template JobQueue::Coro::Coro(Coro_create_t, JobQueue& jq, JobType type, std::string const& name, F&& f) : jq_(jq) , type_(type) , name_(name) , coro_( - // Stack size of 1MB wasn't sufficient for deep calls. ASAN tests flagged the issue. Hence - // increasing the size to 1.5MB. - boost::context::protected_fixedsize_stack(1536 * 1024), - [this, fn = std::forward(f)]( - boost::coroutines2::asymmetric_coroutine::push_type& do_yield) { + boost::context::protected_fixedsize_stack(coroStackSize), + [this, + fn = std::forward(f)](boost::coroutines2::coroutine::push_type& do_yield) { yield_ = &do_yield; yield(); fn(shared_from_this());