From a0074f83d35f7fec4532d48f8ad3837d1ddc311e Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Fri, 14 Aug 2026 10:06:49 +0000 Subject: [PATCH 01/13] build: Fix versioned tools for exec wrappers (#8027) --- nix/packages.nix | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/nix/packages.nix b/nix/packages.nix index 0623ff51b9..c7972c9843 100644 --- a/nix/packages.nix +++ b/nix/packages.nix @@ -50,6 +50,9 @@ let # environment (the plain stdenv compiler in the dev shell, the custom-glibc # wrappers in ci-env.nix), so those callers pass their own `package`; the # clang tooling is environment-independent and is linked in commonPackages. + # + # Exec wrappers, not symlinks: the nixpkgs clang-tools wrapper dispatches on + # `$(basename $0)-unwrapped`, which a suffixed symlink turns into a dead path. mkVersionedToolLinks = { name, @@ -57,12 +60,15 @@ let version, tools, }: - pkgs.linkFarm "${name}-${toString version}-versioned-links" ( - map (tool: { - name = "bin/${tool}-${toString version}"; - path = "${package}/bin/${tool}"; - }) tools - ); + pkgs.symlinkJoin { + name = "${name}-${toString version}-versioned-links"; + paths = map ( + tool: + pkgs.writeShellScriptBin "${tool}-${toString version}" '' + exec "${package}/bin/${tool}" "$@" + '' + ) tools; + }; # The cc-wrapper doesn't re-export gcov, but coverage tooling (gcovr) needs a # gcov that exactly matches the compiler. Surface it from a gcc `cc` output. From d34aa37b3c9e7d2a3e71c15a009680fa7279c284 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 14 Aug 2026 13:49:08 +0000 Subject: [PATCH 02/13] refactor: Use std::format instead of boost::format where it fits (#7996) Co-authored-by: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Co-authored-by: Cursor Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Co-authored-by: Ayaz Salikhov --- include/xrpl/basics/StringUtilities.h | 1 - include/xrpl/net/HTTPClientSSLContext.h | 8 +- include/xrpl/rdb/DBInit.h | 29 +++- include/xrpl/server/Wallet.h | 4 + src/libxrpl/protocol/STLedgerEntry.cpp | 5 +- src/libxrpl/protocol/STTx.cpp | 17 +- src/libxrpl/protocol/STXChainBridge.cpp | 16 +- src/libxrpl/server/Vacuum.cpp | 4 +- src/libxrpl/server/Wallet.cpp | 11 +- src/test/app/AMMCalc_test.cpp | 4 +- src/test/core/Config_test.cpp | 63 ++++--- src/test/rpc/ServerInfo_test.cpp | 16 +- src/tests/libxrpl/protocol/STXChainBridge.cpp | 60 +++++++ src/xrpld/app/misc/Transaction.h | 4 + src/xrpld/app/misc/detail/WorkSSL.cpp | 4 +- src/xrpld/app/misc/detail/WorkSSL.h | 1 - src/xrpld/app/rdb/backend/detail/Node.cpp | 161 ++++++++++-------- src/xrpld/core/detail/Config.cpp | 11 +- src/xrpld/rpc/detail/RPCHelpers.cpp | 9 +- .../rpc/handlers/account/AccountInfo.cpp | 5 +- .../rpc/handlers/orderbook/BookOffers.cpp | 35 ++-- 21 files changed, 286 insertions(+), 182 deletions(-) create mode 100644 src/tests/libxrpl/protocol/STXChainBridge.cpp diff --git a/include/xrpl/basics/StringUtilities.h b/include/xrpl/basics/StringUtilities.h index d606613c65..e3b91c2f25 100644 --- a/include/xrpl/basics/StringUtilities.h +++ b/include/xrpl/basics/StringUtilities.h @@ -2,7 +2,6 @@ #include -#include #include #include diff --git a/include/xrpl/net/HTTPClientSSLContext.h b/include/xrpl/net/HTTPClientSSLContext.h index 51b50a084c..43467faa89 100644 --- a/include/xrpl/net/HTTPClientSSLContext.h +++ b/include/xrpl/net/HTTPClientSSLContext.h @@ -8,11 +8,11 @@ #include #include #include -#include #include #include +#include #include #include #include @@ -38,8 +38,8 @@ public: if (ec && sslVerifyDir.empty()) { - Throw(boost::str( - boost::format("Failed to set_default_verify_paths: %s") % ec.message())); + Throw( + std::format("Failed to set_default_verify_paths: {}", ec.message())); } } else @@ -54,7 +54,7 @@ public: if (ec) { Throw( - boost::str(boost::format("Failed to add verify path: %s") % ec.message())); + std::format("Failed to add verify path: {}", ec.message())); } } } diff --git a/include/xrpl/rdb/DBInit.h b/include/xrpl/rdb/DBInit.h index 10b04905f2..e6e7e87b6b 100644 --- a/include/xrpl/rdb/DBInit.h +++ b/include/xrpl/rdb/DBInit.h @@ -2,6 +2,9 @@ #include #include +#include +#include +#include namespace xrpl { @@ -9,9 +12,29 @@ namespace xrpl { // These pragmas are built at startup and applied to all database // connections, unless otherwise noted. -inline constexpr char const* kCommonDbPragmaJournal{"PRAGMA journal_mode=%s;"}; -inline constexpr char const* kCommonDbPragmaSync{"PRAGMA synchronous=%s;"}; -inline constexpr char const* kCommonDbPragmaTemp{"PRAGMA temp_store=%s;"}; +// +// They are exposed as functions rather than as format-string constants so +// that the un-substituted template can never reach sqlite: an unrecognized +// pragma value is silently ignored, so forgetting to interpolate would +// leave the setting at its default instead of failing loudly. +[[nodiscard]] inline std::string +commonDbPragmaJournal(std::string_view journalMode) +{ + return std::format("PRAGMA journal_mode={};", journalMode); +} + +[[nodiscard]] inline std::string +commonDbPragmaSync(std::string_view synchronous) +{ + return std::format("PRAGMA synchronous={};", synchronous); +} + +[[nodiscard]] inline std::string +commonDbPragmaTemp(std::string_view tempStore) +{ + return std::format("PRAGMA temp_store={};", tempStore); +} + // A warning will be logged if any lower-safety sqlite tuning settings // are used and at least this much ledger history is configured. This // includes full history nodes. This is because such a large amount of diff --git a/include/xrpl/server/Wallet.h b/include/xrpl/server/Wallet.h index ed8378989f..95486cc468 100644 --- a/include/xrpl/server/Wallet.h +++ b/include/xrpl/server/Wallet.h @@ -10,6 +10,10 @@ #include #include +// boost::optional (not std::optional) appears in the declarations below, +// because SOCI's into()/use() bindings only support boost::optional. +#include + #include #include #include diff --git a/src/libxrpl/protocol/STLedgerEntry.cpp b/src/libxrpl/protocol/STLedgerEntry.cpp index 8c5c5b5eae..9ee8d030ff 100644 --- a/src/libxrpl/protocol/STLedgerEntry.cpp +++ b/src/libxrpl/protocol/STLedgerEntry.cpp @@ -18,12 +18,11 @@ #include #include -#include - #include #include #include #include +#include #include #include #include @@ -111,7 +110,7 @@ STLedgerEntry::getSType() const std::string STLedgerEntry::getText() const { - return str(boost::format("{ %s, %s }") % to_string(key_) % STObject::getText()); + return std::format("{{ {}, {} }}", to_string(key_), STObject::getText()); } json::Value diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 7f1e19ea12..ce672b515d 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -33,13 +33,13 @@ #include #include -#include #include #include #include #include #include +#include #include #include #include @@ -399,16 +399,21 @@ STTx::getMetaSQL( TxnSql status, std::string const& escapedMetaData) const { - static boost::format const kBfTrans("('%s', '%s', '%s', '%d', '%d', '%c', %s, %s)"); std::string rTxn = sqlBlobLiteral(rawTxn.peekData()); auto format = TxFormats::getInstance().findByType(txType_); XRPL_ASSERT(format, "xrpl::STTx::getMetaSQL : non-null type format"); - return str( - boost::format(kBfTrans) % to_string(getTransactionID()) % format->getName() % - toBase58(getAccountID(sfAccount)) % getFieldU32(sfSequence) % inLedger % - safeCast(status) % rTxn % escapedMetaData); + return std::format( + "('{}', '{}', '{}', '{}', '{}', '{}', {}, {})", + to_string(getTransactionID()), + format->getName(), + toBase58(getAccountID(sfAccount)), + getFieldU32(sfSequence), + inLedger, + safeCast(status), + rTxn, + escapedMetaData); } static std::expected diff --git a/src/libxrpl/protocol/STXChainBridge.cpp b/src/libxrpl/protocol/STXChainBridge.cpp index 005c9ccbce..f9f1fd1dcc 100644 --- a/src/libxrpl/protocol/STXChainBridge.cpp +++ b/src/libxrpl/protocol/STXChainBridge.cpp @@ -11,9 +11,8 @@ #include #include -#include - #include +#include #include #include #include @@ -141,10 +140,15 @@ STXChainBridge::getJson(JsonOptions jo) const std::string STXChainBridge::getText() const { - return str( - boost::format("{ %s = %s, %s = %s, %s = %s, %s = %s }") % sfLockingChainDoor.getName() % - lockingChainDoor_.getText() % sfLockingChainIssue.getName() % lockingChainIssue_.getText() % - sfIssuingChainDoor.getName() % issuingChainDoor_.getText() % sfIssuingChainIssue.getName() % + return std::format( + "{{ {} = {}, {} = {}, {} = {}, {} = {} }}", + sfLockingChainDoor.getName(), + lockingChainDoor_.getText(), + sfLockingChainIssue.getName(), + lockingChainIssue_.getText(), + sfIssuingChainDoor.getName(), + issuingChainDoor_.getText(), + sfIssuingChainIssue.getName(), issuingChainIssue_.getText()); } diff --git a/src/libxrpl/server/Vacuum.cpp b/src/libxrpl/server/Vacuum.cpp index df768d509a..c952e722b8 100644 --- a/src/libxrpl/server/Vacuum.cpp +++ b/src/libxrpl/server/Vacuum.cpp @@ -5,8 +5,6 @@ #include #include -#include // IWYU pragma: keep - #include #include @@ -40,7 +38,7 @@ doVacuumDB(DatabaseCon::Setup const& setup, beast::Journal j) // Only the most trivial databases will fit in memory on typical // (recommended) hardware. Force temp files to be written to disk // regardless of the config settings. - session << boost::format(kCommonDbPragmaTemp) % "file"; + session << commonDbPragmaTemp("file"); session << "PRAGMA page_size;", soci::into(pageSize); std::cout << "VACUUM beginning. page_size: " << pageSize << std::endl; diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index 42ac80ef3f..56d0db67d4 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -16,7 +16,6 @@ #include #include -#include #include // IWYU pragma: keep #include // IWYU pragma: keep @@ -30,6 +29,7 @@ #include #include +#include #include #include #include @@ -172,11 +172,10 @@ getNodeIdentity(soci::session& session) // If a valid identity wasn't found, we randomly generate a new one: auto [newpublicKey, newsecretKey] = randomKeyPair(KeyType::Secp256k1); - session << str( - boost::format( - "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " - "VALUES ('%s','%s');") % - toBase58(TokenType::NodePublic, newpublicKey) % + session << std::format( + "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " + "VALUES ('{}','{}');", + toBase58(TokenType::NodePublic, newpublicKey), toBase58(TokenType::NodePrivate, newsecretKey)); return {newpublicKey, newsecretKey}; diff --git a/src/test/app/AMMCalc_test.cpp b/src/test/app/AMMCalc_test.cpp index 74080e669c..23f251d57a 100644 --- a/src/test/app/AMMCalc_test.cpp +++ b/src/test/app/AMMCalc_test.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -188,8 +189,7 @@ class AMMCalc_test : public beast::unit_test::Suite static std::string toString(STAmount const& a) { - return (boost::format("%s/%s") % a.getText() % ::xrpl::to_string(a.get().currency)) - .str(); + return std::format("{}/{}", a.getText(), ::xrpl::to_string(a.get().currency)); } static STAmount diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index dec6393010..5ed5ef4049 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -10,8 +10,6 @@ #include // IWYU pragma: keep #include -#include // IWYU pragma: keep -#include #include #include @@ -20,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -36,7 +35,7 @@ namespace detail { std::string configContents(std::string const& dbPath, std::string const& validatorsFile) { - static boost::format kConfigContentsTemplate(R"xrpldConfig( + static constexpr char const* kConfigContentsTemplate = R"xrpldConfig( [server] port_rpc port_peer @@ -83,9 +82,9 @@ cache_mb=256 file_size_mb=8 file_size_mult=2 -%1% +{} -%2% +{} # This needs to be an absolute directory reference, not a relative one. # Modify this value as required. @@ -106,7 +105,7 @@ r.ripple.com 51235 # Turn down default logging to save disk space in the long run. # Valid values here are trace, debug, info, warning, error, and fatal [rpc_startup] -{ "command": "log_level", "severity": "warning" } +{{ "command": "log_level", "severity": "warning" }} # Defaults to 1 ("yes") so that certificates will be validated. To allow the use # of self-signed certificates for development or internal use, set to 0 ("no"). @@ -115,12 +114,12 @@ r.ripple.com 51235 [sqdb] backend=sqlite -)xrpldConfig"); +)xrpldConfig"; std::string dbPathSection = dbPath.empty() ? "" : "[database_path]\n" + dbPath; std::string valFileSection = validatorsFile.empty() ? "" : "[validators_file]\n" + validatorsFile; - return boost::str(kConfigContentsTemplate % dbPathSection % valFileSection); + return std::format(kConfigContentsTemplate, dbPathSection, valFileSection); } /** @@ -427,7 +426,7 @@ port_wss_admin using namespace std::filesystem; { - boost::format cc("[database_path]\n%1%\n"); + constexpr char const* cc = "[database_path]\n{}\n"; auto const cwd = current_path(); path const dataDirRel("test_data_dir"); @@ -435,13 +434,13 @@ port_wss_admin { // Dummy test - do we get back what we put in Config c; - c.loadFromString(boost::str(cc % dataDirAbs.string())); + c.loadFromString(std::format(cc, dataDirAbs.string())); BEAST_EXPECT(c.legacy(Sections::kDatabasePath) == dataDirAbs.string()); } { // Rel paths should convert to abs paths Config c; - c.loadFromString(boost::str(cc % dataDirRel.string())); + c.loadFromString(std::format(cc, dataDirRel.string())); BEAST_EXPECT(c.legacy(Sections::kDatabasePath) == dataDirAbs.string()); } { @@ -508,20 +507,20 @@ port_wss_admin { Config c; - static boost::format kConfigTemplate(R"xrpldConfig( + static constexpr char const* kConfigTemplate = R"xrpldConfig( [validation_seed] -%1% +{} [validator_token] -%2% -)xrpldConfig"); +{} +)xrpldConfig"; std::string error; auto const expectedError = "Cannot have both [validation_seed] " "and [validator_token] config sections"; try { - c.loadFromString(boost::str(kConfigTemplate % validationSeed % token)); + c.loadFromString(std::format(kConfigTemplate, validationSeed, token)); } catch (std::runtime_error const& e) { @@ -604,7 +603,7 @@ main using namespace std::filesystem; { // load should throw for missing specified validators file - boost::format cc("[validators_file]\n%1%\n"); + constexpr char const* cc = "[validators_file]\n{}\n"; std::string error; std::string const missingPath = "/no/way/this/path/exists"; auto const expectedError = @@ -612,7 +611,7 @@ main try { Config c; - c.loadFromString(boost::str(cc % missingPath)); + c.loadFromString(std::format(cc, missingPath)); } catch (std::runtime_error const& e) { @@ -624,14 +623,14 @@ main // load should throw for invalid [validators_file] detail::ValidatorsTxtGuard const vtg(*this, "test_cfg", "validators.cfg"); path const invalidFile = current_path() / vtg.subdir(); - boost::format cc("[validators_file]\n%1%\n"); + constexpr char const* cc = "[validators_file]\n{}\n"; std::string error; auto const expectedError = "Invalid file specified in [validators_file]: " + invalidFile.string(); try { Config c; - c.loadFromString(boost::str(cc % invalidFile.string())); + c.loadFromString(std::format(cc, invalidFile.string())); } catch (std::runtime_error const& e) { @@ -829,8 +828,8 @@ trust-these-validators.gov detail::ValidatorsTxtGuard const vtg(*this, "test_cfg", "validators.cfg"); BEAST_EXPECT(vtg.validatorsFileExists()); Config c; - boost::format cc("[validators_file]\n%1%\n"); - c.loadFromString(boost::str(cc % vtg.validatorsFile())); + constexpr char const* cc = "[validators_file]\n{}\n"; + c.loadFromString(std::format(cc, vtg.validatorsFile())); BEAST_EXPECT(c.legacy(Sections::kValidatorsFile) == vtg.validatorsFile()); BEAST_EXPECT(c.section(Sections::kValidators).values().size() == 8); BEAST_EXPECT(c.section(Sections::kValidatorListSites).values().size() == 2); @@ -909,9 +908,9 @@ trust-these-validators.gov { // load validators from both config and validators file - boost::format cc(R"xrpldConfig( + constexpr char const* cc = R"xrpldConfig( [validators_file] -%1% +{} [validators] n949f75evCHwgyP4fPVgaHqNHxUVN15PsJEZ3B3HnXPcPjcZAoy7 @@ -930,11 +929,11 @@ trust-these-validators.gov [validator_list_keys] 021A99A537FDEBC34E4FCA03B39BEADD04299BB19E85097EC92B15A3518801E566 -)xrpldConfig"); +)xrpldConfig"; detail::ValidatorsTxtGuard const vtg(*this, "test_cfg", "validators.cfg"); BEAST_EXPECT(vtg.validatorsFileExists()); Config c; - c.loadFromString(boost::str(cc % vtg.validatorsFile())); + c.loadFromString(std::format(cc, vtg.validatorsFile())); BEAST_EXPECT(c.legacy(Sections::kValidatorsFile) == vtg.validatorsFile()); BEAST_EXPECT(c.section(Sections::kValidators).values().size() == 15); BEAST_EXPECT(c.section(Sections::kValidatorListSites).values().size() == 4); @@ -945,13 +944,13 @@ trust-these-validators.gov { // load should throw if [validator_list_threshold] is present both // in xrpld.cfg and validators file - boost::format cc(R"xrpldConfig( + constexpr char const* cc = R"xrpldConfig( [validators_file] -%1% +{} [validator_list_threshold] 1 -)xrpldConfig"); +)xrpldConfig"; std::string error; detail::ValidatorsTxtGuard const vtg(*this, "test_cfg", "validators.cfg"); BEAST_EXPECT(vtg.validatorsFileExists()); @@ -961,7 +960,7 @@ trust-these-validators.gov try { Config c; - c.loadFromString(boost::str(cc % vtg.validatorsFile())); + c.loadFromString(std::format(cc, vtg.validatorsFile())); fail(); } catch (std::runtime_error const& e) @@ -975,7 +974,7 @@ trust-these-validators.gov // [validator_list_keys] are missing from xrpld.cfg and // validators file Config const c; - boost::format cc("[validators_file]\n%1%\n"); + constexpr char const* cc = "[validators_file]\n{}\n"; std::string error; detail::ValidatorsTxtGuard const vtg(*this, "test_cfg", "validators.cfg"); BEAST_EXPECT(vtg.validatorsFileExists()); @@ -988,7 +987,7 @@ trust-these-validators.gov try { Config c2; - c2.loadFromString(boost::str(cc % vtg.validatorsFile())); + c2.loadFromString(std::format(cc, vtg.validatorsFile())); } catch (std::runtime_error const& e) { diff --git a/src/test/rpc/ServerInfo_test.cpp b/src/test/rpc/ServerInfo_test.cpp index 52a1e6cdb0..100ae0e49b 100644 --- a/src/test/rpc/ServerInfo_test.cpp +++ b/src/test/rpc/ServerInfo_test.cpp @@ -9,8 +9,7 @@ #include #include -#include - +#include #include namespace xrpl::test { @@ -36,12 +35,13 @@ public: makeValidatorConfig() { auto p = std::make_unique(); - boost::format toLoad(R"xrpldConfig( + auto const toLoad = std::format( + R"xrpldConfig( [validator_token] -%1% +{} [validators] -%2% +{} [port_grpc] ip = 0.0.0.0 @@ -52,9 +52,11 @@ ip = 0.0.0.0 port = 50052 protocol = wss2 admin = 127.0.0.1 -)xrpldConfig"); +)xrpldConfig", + validator_data::kToken, + validator_data::kPublicKey); - p->loadFromString(boost::str(toLoad % validator_data::kToken % validator_data::kPublicKey)); + p->loadFromString(toLoad); setupConfigForUnitTests(*p); diff --git a/src/tests/libxrpl/protocol/STXChainBridge.cpp b/src/tests/libxrpl/protocol/STXChainBridge.cpp new file mode 100644 index 0000000000..f4e6e60cc9 --- /dev/null +++ b/src/tests/libxrpl/protocol/STXChainBridge.cpp @@ -0,0 +1,60 @@ +#include + +#include +#include +#include + +#include + +#include +#include + +using namespace xrpl; + +namespace { + +// Built from raw bytes rather than base58 so the test does not depend on +// hand-computed checksums. +AccountID +account(std::string_view hex) +{ + AccountID id; + EXPECT_TRUE(id.parseHex(hex)); + return id; +} + +} // namespace + +// getText() builds its string from eight substitutions of the same type, so a +// transposed pair would still compile and still type check. Pin the output so +// the field/value pairing is actually verified. +TEST(STXChainBridge, getTextPairsEachFieldWithItsValue) +{ + auto const lockingDoor = account("0102030405060708090A0B0C0D0E0F1011121314"); + auto const issuingDoor = account("14131211100F0E0D0C0B0A090807060504030201"); + + auto const lockingIssue = xrpIssue(); + Issue const issuingIssue{toCurrency("USD"), issuingDoor}; + + STXChainBridge const bridge{lockingDoor, lockingIssue, issuingDoor, issuingIssue}; + + std::string const expected = "{ LockingChainDoor = " + toBase58(lockingDoor) + + ", LockingChainIssue = " + lockingIssue.getText() + + ", IssuingChainDoor = " + toBase58(issuingDoor) + + ", IssuingChainIssue = " + issuingIssue.getText() + " }"; + + EXPECT_EQ(bridge.getText(), expected); +} + +TEST(STXChainBridge, getTextOnADefaultBridge) +{ + STXChainBridge const bridge; + auto const text = bridge.getText(); + + // The outer braces are literal, and the four field names appear in + // declaration order regardless of the values. + EXPECT_TRUE(text.starts_with("{ LockingChainDoor = ")); + EXPECT_TRUE(text.ends_with(" }")); + EXPECT_LT(text.find("LockingChainIssue"), text.find("IssuingChainDoor")); + EXPECT_LT(text.find("IssuingChainDoor"), text.find("IssuingChainIssue")); +} diff --git a/src/xrpld/app/misc/Transaction.h b/src/xrpld/app/misc/Transaction.h index b6b6d1a8d5..61951fbb59 100644 --- a/src/xrpld/app/misc/Transaction.h +++ b/src/xrpld/app/misc/Transaction.h @@ -15,6 +15,10 @@ #include #include +// boost::optional (not std::optional) appears in the declarations below, +// because SOCI's into()/use() bindings only support boost::optional. +#include + #include #include #include diff --git a/src/xrpld/app/misc/detail/WorkSSL.cpp b/src/xrpld/app/misc/detail/WorkSSL.cpp index e8d24b55d6..48231b147e 100644 --- a/src/xrpld/app/misc/detail/WorkSSL.cpp +++ b/src/xrpld/app/misc/detail/WorkSSL.cpp @@ -10,8 +10,8 @@ #include #include #include -#include +#include #include #include @@ -38,7 +38,7 @@ WorkSSL::WorkSSL( { auto ec = context_.preConnectVerify(stream_, host_); if (ec) - Throw(boost::str(boost::format("preConnectVerify: %s") % ec.message())); + Throw(std::format("preConnectVerify: {}", ec.message())); } void diff --git a/src/xrpld/app/misc/detail/WorkSSL.h b/src/xrpld/app/misc/detail/WorkSSL.h index d4b3b9ff25..e4b7586054 100644 --- a/src/xrpld/app/misc/detail/WorkSSL.h +++ b/src/xrpld/app/misc/detail/WorkSSL.h @@ -7,7 +7,6 @@ #include #include -#include #include #include diff --git a/src/xrpld/app/rdb/backend/detail/Node.cpp b/src/xrpld/app/rdb/backend/detail/Node.cpp index ff57087ec5..be4c5d29e5 100644 --- a/src/xrpld/app/rdb/backend/detail/Node.cpp +++ b/src/xrpld/app/rdb/backend/detail/Node.cpp @@ -40,7 +40,6 @@ #include #include -#include #include // IWYU pragma: keep #include @@ -58,6 +57,7 @@ #include #include #include +#include #include #include #include @@ -109,18 +109,16 @@ makeLedgerDBs( // ledger database auto lgr{std::make_unique( setup, kLgrDbName, setup.lgrPragma, kLgrDbInit, checkpointerSetup, j)}; - lgr->getSession() << boost::str( - boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config.getValueFor(SizedItem::LgrDbCache))); + lgr->getSession() << std::format( + "PRAGMA cache_size=-{};", kilobytes(config.getValueFor(SizedItem::LgrDbCache))); if (config.useTxTables()) { // transaction database auto tx{std::make_unique( setup, kTxDbName, setup.txPragma, kTxDbInit, checkpointerSetup, j)}; - tx->getSession() << boost::str( - boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config.getValueFor(SizedItem::TxnDbCache))); + tx->getSession() << std::format( + "PRAGMA cache_size=-{};", kilobytes(config.getValueFor(SizedItem::TxnDbCache))); if (!setup.standAlone || setup.startUp == StartUpType::Load || setup.startUp == StartUpType::LoadFile || setup.startUp == StartUpType::Replay) @@ -280,15 +278,17 @@ saveValidatedLedger( } { - static boost::format kDeleteLedger("DELETE FROM Ledgers WHERE LedgerSeq = %u;"); - static boost::format kDeleteTranS1("DELETE FROM Transactions WHERE LedgerSeq = %u;"); - static boost::format kDeleteTranS2("DELETE FROM AccountTransactions WHERE LedgerSeq = %u;"); - static boost::format kDeleteAcctTrans( - "DELETE FROM AccountTransactions WHERE TransID = '%s';"); + static constexpr char const* kDeleteLedger = "DELETE FROM Ledgers WHERE LedgerSeq = {};"; + static constexpr char const* kDeleteTranS1 = + "DELETE FROM Transactions WHERE LedgerSeq = {};"; + static constexpr char const* kDeleteTranS2 = + "DELETE FROM AccountTransactions WHERE LedgerSeq = {};"; + static constexpr char const* kDeleteAcctTrans = + "DELETE FROM AccountTransactions WHERE TransID = '{}';"; { auto db = ldgDB.checkoutDb(); - *db << boost::str(kDeleteLedger % seq); + *db << std::format(kDeleteLedger, seq); } if (app.config().useTxTables()) @@ -305,19 +305,19 @@ saveValidatedLedger( soci::transaction tr(*db); - *db << boost::str(kDeleteTranS1 % seq); - *db << boost::str(kDeleteTranS2 % seq); + *db << std::format(kDeleteTranS1, seq); + *db << std::format(kDeleteTranS2, seq); std::string const ledgerSeq(std::to_string(seq)); for (auto const& acceptedLedgerTx : *aLedger) { - uint256 transactionID = acceptedLedgerTx->getTransactionID(); + uint256 const transactionID = acceptedLedgerTx->getTransactionID(); std::string const txnId(to_string(transactionID)); std::string const txnSeq(std::to_string(acceptedLedgerTx->getTxnSeq())); - *db << boost::str(kDeleteAcctTrans % transactionID); + *db << std::format(kDeleteAcctTrans, txnId); auto const& accts = acceptedLedgerTx->getAffected(); @@ -629,11 +629,11 @@ getHashesByIndex(soci::session& session, LedgerIndex minSeq, LedgerIndex maxSeq, std::pair>, int> getTxHistory(soci::session& session, Application& app, LedgerIndex startIndex, int quantity) { - std::string const sql = boost::str( - boost::format( - "SELECT LedgerSeq, Status, RawTxn " - "FROM Transactions ORDER BY LedgerSeq DESC LIMIT %u,%u;") % - startIndex % quantity); + std::string const sql = std::format( + "SELECT LedgerSeq, Status, RawTxn " + "FROM Transactions ORDER BY LedgerSeq DESC LIMIT {},{};", + startIndex, + quantity); std::vector> txs; int total = 0; @@ -730,41 +730,50 @@ transactionsSQL( if (options.ledgerRange.max != 0u) { - maxClause = boost::str( - boost::format("AND AccountTransactions.LedgerSeq <= '%u'") % options.ledgerRange.max); + maxClause = + std::format("AND AccountTransactions.LedgerSeq <= '{}'", options.ledgerRange.max); } if (options.ledgerRange.min != 0u) { - minClause = boost::str( - boost::format("AND AccountTransactions.LedgerSeq >= '%u'") % options.ledgerRange.min); + minClause = + std::format("AND AccountTransactions.LedgerSeq >= '{}'", options.ledgerRange.min); } std::string sql; if (count) { - sql = boost::str( - boost::format( - "SELECT %s FROM AccountTransactions " - "WHERE Account = '%s' %s %s LIMIT %u, %u;") % - selection % toBase58(options.account) % maxClause % minClause % options.offset % + sql = std::format( + "SELECT {} FROM AccountTransactions " + "WHERE Account = '{}' {} {} LIMIT {}, {};", + selection, + toBase58(options.account), + maxClause, + minClause, + options.offset, numberOfResults); } else { - sql = boost::str( - boost::format( - "SELECT %s FROM " - "AccountTransactions INNER JOIN Transactions " - "ON Transactions.TransID = AccountTransactions.TransID " - "WHERE Account = '%s' %s %s " - "ORDER BY AccountTransactions.LedgerSeq %s, " - "AccountTransactions.TxnSeq %s, AccountTransactions.TransID %s " - "LIMIT %u, %u;") % - selection % toBase58(options.account) % maxClause % minClause % - (descending ? "DESC" : "ASC") % (descending ? "DESC" : "ASC") % - (descending ? "DESC" : "ASC") % options.offset % numberOfResults); + char const* const order = descending ? "DESC" : "ASC"; + sql = std::format( + "SELECT {} FROM " + "AccountTransactions INNER JOIN Transactions " + "ON Transactions.TransID = AccountTransactions.TransID " + "WHERE Account = '{}' {} {} " + "ORDER BY AccountTransactions.LedgerSeq {}, " + "AccountTransactions.TxnSeq {}, AccountTransactions.TransID {} " + "LIMIT {}, {};", + selection, + toBase58(options.account), + maxClause, + minClause, + order, + order, + order, + options.offset, + numberOfResults); } JLOG(j.trace()) << "txSQL query: " << sql; return sql; @@ -1105,14 +1114,6 @@ accountTxPage( std::optional newmarker; - static std::string const kPrefix( - R"(SELECT AccountTransactions.LedgerSeq,AccountTransactions.TxnSeq, - Status,RawTxn,TxnMeta - FROM AccountTransactions INNER JOIN Transactions - ON Transactions.TransID = AccountTransactions.TransID - AND AccountTransactions.Account = '%s' WHERE - )"); - std::string sql; // SQL's BETWEEN uses a closed interval ([a,b]) @@ -1121,13 +1122,22 @@ accountTxPage( if (findLedger == 0) { - sql = boost::str( - boost::format(kPrefix + R"(AccountTransactions.LedgerSeq BETWEEN %u AND %u - ORDER BY AccountTransactions.LedgerSeq %s, - AccountTransactions.TxnSeq %s - LIMIT %u;)") % - toBase58(options.account) % options.ledgerRange.min % options.ledgerRange.max % order % - order % queryLimit); + sql = std::format( + R"(SELECT AccountTransactions.LedgerSeq,AccountTransactions.TxnSeq, + Status,RawTxn,TxnMeta + FROM AccountTransactions INNER JOIN Transactions + ON Transactions.TransID = AccountTransactions.TransID + AND AccountTransactions.Account = '{}' WHERE + AccountTransactions.LedgerSeq BETWEEN {} AND {} + ORDER BY AccountTransactions.LedgerSeq {}, + AccountTransactions.TxnSeq {} + LIMIT {};)", + toBase58(options.account), + options.ledgerRange.min, + options.ledgerRange.max, + order, + order, + queryLimit); } else { @@ -1136,27 +1146,34 @@ accountTxPage( std::uint32_t const maxLedger = forward ? options.ledgerRange.max : findLedger - 1; auto b58acct = toBase58(options.account); - sql = boost::str( - boost::format( - R"(SELECT AccountTransactions.LedgerSeq,AccountTransactions.TxnSeq, + sql = std::format( + R"(SELECT AccountTransactions.LedgerSeq,AccountTransactions.TxnSeq, Status,RawTxn,TxnMeta FROM AccountTransactions, Transactions WHERE (AccountTransactions.TransID = Transactions.TransID AND - AccountTransactions.Account = '%s' AND - AccountTransactions.LedgerSeq BETWEEN %u AND %u) + AccountTransactions.Account = '{}' AND + AccountTransactions.LedgerSeq BETWEEN {} AND {}) UNION SELECT AccountTransactions.LedgerSeq,AccountTransactions.TxnSeq,Status,RawTxn,TxnMeta FROM AccountTransactions, Transactions WHERE (AccountTransactions.TransID = Transactions.TransID AND - AccountTransactions.Account = '%s' AND - AccountTransactions.LedgerSeq = %u AND - AccountTransactions.TxnSeq %s %u) - ORDER BY AccountTransactions.LedgerSeq %s, - AccountTransactions.TxnSeq %s - LIMIT %u; - )") % - b58acct % minLedger % maxLedger % b58acct % findLedger % compare % findSeq % order % - order % queryLimit); + AccountTransactions.Account = '{}' AND + AccountTransactions.LedgerSeq = {} AND + AccountTransactions.TxnSeq {} {}) + ORDER BY AccountTransactions.LedgerSeq {}, + AccountTransactions.TxnSeq {} + LIMIT {}; + )", + b58acct, + minLedger, + maxLedger, + b58acct, + findLedger, + compare, + findSeq, + order, + order, + queryLimit); } { diff --git a/src/xrpld/core/detail/Config.cpp b/src/xrpld/core/detail/Config.cpp index efe4ab1cc9..3ff62c9b64 100644 --- a/src/xrpld/core/detail/Config.cpp +++ b/src/xrpld/core/detail/Config.cpp @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include // IWYU pragma: keep @@ -34,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -400,7 +400,7 @@ Config::setup(std::string const& strConf, bool bQuiet, bool bSilent, bool bStand std::filesystem::create_directories(dataDir, ec); if (ec) - Throw(boost::str(boost::format("Can not create %s") % dataDir)); + Throw(std::format("Can not create {}", dataDir.string())); legacy(Sections::kDatabasePath, std::filesystem::absolute(dataDir).string()); } @@ -1315,8 +1315,7 @@ setupDatabaseCon(Config const& c, std::optional j) boost::iequals(journalMode, "truncate") || boost::iequals(journalMode, "persist") || boost::iequals(journalMode, "wal")) { - result->emplace_back( - boost::str(boost::format(kCommonDbPragmaJournal) % journalMode)); + result->emplace_back(commonDbPragmaJournal(journalMode)); } else { @@ -1337,7 +1336,7 @@ setupDatabaseCon(Config const& c, std::optional j) if (higherRisk || boost::iequals(synchronous, "normal") || boost::iequals(synchronous, "full") || boost::iequals(synchronous, "extra")) { - result->emplace_back(boost::str(boost::format(kCommonDbPragmaSync) % synchronous)); + result->emplace_back(commonDbPragmaSync(synchronous)); } else { @@ -1358,7 +1357,7 @@ setupDatabaseCon(Config const& c, std::optional j) if (higherRisk || boost::iequals(tempStore, "default") || boost::iequals(tempStore, "file")) { - result->emplace_back(boost::str(boost::format(kCommonDbPragmaTemp) % tempStore)); + result->emplace_back(commonDbPragmaTemp(tempStore)); } else { diff --git a/src/xrpld/rpc/detail/RPCHelpers.cpp b/src/xrpld/rpc/detail/RPCHelpers.cpp index 4fa0fab6f7..321f8f5a3c 100644 --- a/src/xrpld/rpc/detail/RPCHelpers.cpp +++ b/src/xrpld/rpc/detail/RPCHelpers.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -424,7 +425,7 @@ parseSubUnsubJson( if (jv.isMember(jss::mpt_issuance_id) && (jv.isMember(jss::currency) || jv.isMember(jss::issuer))) { - JLOG(j.info()) << boost::format("Bad %s currency or MPT.") % name.cStr(); + JLOG(j.info()) << std::format("Bad {} currency or MPT.", name.cStr()); return RpcInvalidParams; } @@ -435,7 +436,7 @@ parseSubUnsubJson( if (!jv.isMember(jss::currency) || !toCurrency(issue.currency, jv[jss::currency].asString())) { - JLOG(j.info()) << boost::format("Bad %s currency.") % name.cStr(); + JLOG(j.info()) << std::format("Bad {} currency.", name.cStr()); return assetError; } @@ -445,7 +446,7 @@ parseSubUnsubJson( // Don't allow illegal issuers. || (!issue.currency != !issue.account) || noAccount() == issue.account) { - JLOG(j.info()) << boost::format("Bad %s issuer.") % name.cStr(); + JLOG(j.info()) << std::format("Bad {} issuer.", name.cStr()); return issuerError; } asset = issue; @@ -459,7 +460,7 @@ parseSubUnsubJson( } else { - JLOG(j.info()) << boost::format("Neither %s currency or MPT is present.") % name.cStr(); + JLOG(j.info()) << std::format("Neither {} currency or MPT is present.", name.cStr()); return assetError; } diff --git a/src/xrpld/rpc/handlers/account/AccountInfo.cpp b/src/xrpld/rpc/handlers/account/AccountInfo.cpp index eed4e4cfe3..6b244af1a9 100644 --- a/src/xrpld/rpc/handlers/account/AccountInfo.cpp +++ b/src/xrpld/rpc/handlers/account/AccountInfo.cpp @@ -23,10 +23,9 @@ #include #include -#include - #include #include +#include #include #include #include @@ -60,7 +59,7 @@ injectSLE(json::Value& jv, SLE const& sle) md5 = toLower(md5); // VFALCO TODO Give a name to this constant and move it // to a more visible location. - jv[jss::urlgravatar] = str(boost::format("https://www.gravatar.com/avatar/%s") % md5); + jv[jss::urlgravatar] = std::format("https://www.gravatar.com/avatar/{}", md5); } } diff --git a/src/xrpld/rpc/handlers/orderbook/BookOffers.cpp b/src/xrpld/rpc/handlers/orderbook/BookOffers.cpp index ae539a59f3..219c29d53a 100644 --- a/src/xrpld/rpc/handlers/orderbook/BookOffers.cpp +++ b/src/xrpld/rpc/handlers/orderbook/BookOffers.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -32,7 +33,7 @@ validateTakerJSON(json::Value const& taker, json::StaticString const& name) { if (!taker.isMember(jss::currency) && !taker.isMember(jss::mpt_issuance_id)) { - return rpc::missingFieldError((boost::format("%s.currency") % name.cStr()).str()); + return rpc::missingFieldError(std::format("{}.currency", name.cStr())); } if (taker.isMember(jss::mpt_issuance_id) && @@ -44,8 +45,7 @@ validateTakerJSON(json::Value const& taker, json::StaticString const& name) if ((taker.isMember(jss::currency) && !taker[jss::currency].isString()) || (taker.isMember(jss::mpt_issuance_id) && !taker[jss::mpt_issuance_id].isString())) { - return rpc::expectedFieldError( - (boost::format("%s.currency") % name.cStr()).str(), "string"); + return rpc::expectedFieldError(std::format("{}.currency", name.cStr()), "string"); } return std::nullopt; @@ -70,10 +70,9 @@ parseTakerAssetJSON( if (!toCurrency(issue.currency, taker[jss::currency].asString())) { - JLOG(j.info()) << boost::format("Bad %s currency.") % name.cStr(); + JLOG(j.info()) << std::format("Bad {} currency.", name.cStr()); return rpc::makeError( - assetError, - (boost::format("Invalid field '%s.currency', bad currency.") % name.cStr()).str()); + assetError, std::format("Invalid field '{}.currency', bad currency.", name.cStr())); } asset = issue; } @@ -83,8 +82,7 @@ parseTakerAssetJSON( if (!mptid.parseHex(taker[jss::mpt_issuance_id].asString())) { return rpc::makeError( - assetError, - (boost::format("Invalid field '%s.mpt_issuance_id'") % name.cStr()).str()); + assetError, std::format("Invalid field '{}.mpt_issuance_id'", name.cStr())); } asset = mptid; } @@ -113,24 +111,21 @@ parseTakerIssuerJSON( { if (!taker[jss::issuer].isString()) { - return rpc::expectedFieldError( - (boost::format("%s.issuer") % name.cStr()).str(), "string"); + return rpc::expectedFieldError(std::format("{}.issuer", name.cStr()), "string"); } if (!toIssuer(issue.account, taker[jss::issuer].asString())) { return rpc::makeError( issuerError, - (boost::format("Invalid field '%s.issuer', bad issuer.") % name.cStr()).str()); + std::format("Invalid field '{}.issuer', bad issuer.", name.cStr())); } if (issue.account == noAccount()) { return rpc::makeError( issuerError, - (boost::format("Invalid field '%s.issuer', bad issuer account one.") % - name.cStr()) - .str()); + std::format("Invalid field '{}.issuer', bad issuer account one.", name.cStr())); } } else @@ -142,19 +137,17 @@ parseTakerIssuerJSON( { return rpc::makeError( issuerError, - (boost::format( - "Unneeded field '%s.issuer' for XRP currency " - "specification.") % - name.cStr()) - .str()); + std::format( + "Unneeded field '{}.issuer' for XRP currency " + "specification.", + name.cStr())); } if (!isXRP(issue.currency) && isXRP(issue.account)) { return rpc::makeError( issuerError, - (boost::format("Invalid field '%s.issuer', expected non-XRP issuer.") % name.cStr()) - .str()); + std::format("Invalid field '{}.issuer', expected non-XRP issuer.", name.cStr())); } } From bd87edfc75f1ff4ee8e117e05cf37f20380fba20 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Fri, 14 Aug 2026 14:07:55 +0000 Subject: [PATCH 03/13] test: Check versioned tools in check-tools & print nicely (#8030) --- .cspell.config.yaml | 1 + .github/scripts/strategy-matrix/linux.json | 2 +- .github/workflows/publish-docs.yml | 2 +- .github/workflows/reusable-clang-tidy.yml | 2 +- .github/workflows/reusable-upload-recipe.yml | 2 +- bin/check-tools.sh | 66 +++++-- nix/check-tools/README.md | 13 +- nix/check-tools/macos.txt | 170 ++++++++++++---- nix/check-tools/nix-ubuntu-amd64.txt | 198 +++++++++++++++---- nix/check-tools/nix-ubuntu-arm64.txt | 198 +++++++++++++++---- 10 files changed, 511 insertions(+), 143 deletions(-) diff --git a/.cspell.config.yaml b/.cspell.config.yaml index bb763e9935..ec9f87cfdd 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -7,6 +7,7 @@ ignorePaths: - cmake/** - LICENSE.md - .clang-tidy + - nix/check-tools/*.txt # generated, and full of Nix store hashes language: en allowCompoundWords: true # TODO (#6334) ignoreRandomStrings: true diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index 33146cff3b..97163fb8ce 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -1,5 +1,5 @@ { - "image_tag": "sha-fecfc0c", + "image_tag": "sha-a0074f8", "configs": { "ubuntu": [ { diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index a3e096315c..6e973a251d 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -41,7 +41,7 @@ env: jobs: build: runs-on: ubuntu-latest - container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-fecfc0c + container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-a0074f8 steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/.github/workflows/reusable-clang-tidy.yml b/.github/workflows/reusable-clang-tidy.yml index f1fdc0569a..2049b1ce55 100644 --- a/.github/workflows/reusable-clang-tidy.yml +++ b/.github/workflows/reusable-clang-tidy.yml @@ -34,7 +34,7 @@ jobs: needs: [determine-files] if: ${{ needs.determine-files.outputs.cpp_changed_files != '' || needs.determine-files.outputs.need_full_run == 'true' }} runs-on: ["self-hosted", "Linux", "X64", "heavy"] - container: "ghcr.io/xrplf/xrpld/nix-debian:sha-fecfc0c" + container: "ghcr.io/xrplf/xrpld/nix-debian:sha-a0074f8" permissions: contents: read issues: write diff --git a/.github/workflows/reusable-upload-recipe.yml b/.github/workflows/reusable-upload-recipe.yml index b4ab638dee..a8d35fadad 100644 --- a/.github/workflows/reusable-upload-recipe.yml +++ b/.github/workflows/reusable-upload-recipe.yml @@ -40,7 +40,7 @@ defaults: jobs: upload: runs-on: ubuntu-latest - container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-fecfc0c + container: ghcr.io/xrplf/xrpld/nix-ubuntu:sha-a0074f8 env: REMOTE_NAME: ${{ inputs.remote_name }} CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.remote_username }} diff --git a/bin/check-tools.sh b/bin/check-tools.sh index e230302742..8273375428 100755 --- a/bin/check-tools.sh +++ b/bin/check-tools.sh @@ -15,10 +15,14 @@ # - Windows: the core build tools only (CMake, Conan, Git, Python). # MSVC is expected to be provided separately and is not checked here. # -# Some tools (clang-format, doxygen, gcovr, gh, git-cliff, gpg, pre-commit, -# run-clang-tidy) are present in our Linux CI images and in local development -# setups, but not in the macOS CI environment. They are checked everywhere -# except when running in CI on macOS. +# Some tools (clang-format, clang-tidy, doxygen, gcovr, gh, git-cliff, gpg, +# pre-commit, run-clang-tidy) are present in our Linux CI images and in local +# development setups, but not in the macOS CI environment. They are checked +# everywhere except when running in CI on macOS. +# +# Tools that Nix also exposes under a version-suffixed name (`clang-tidy-22`, +# `g++-15`, ...) are probed under both names: a suffixed name can break while +# the plain one still works (see mkVersionedToolLinks in nix/packages.nix). # # Environment variables: # CI if set, skip the tools above when on macOS. @@ -26,14 +30,27 @@ set -uo pipefail +# Version suffixes of the Nix tool links, tracking nix/packages.nix. +gcc_version=15 +llvm_version=22 + missing=() checked=0 +# tool_path +# Fully resolved path of a tool, so the snapshots record which derivation +# provides it. Prints nothing when it isn't on PATH. +tool_path() { + local path + path="$(command -v "$1" 2>/dev/null)" || return 0 + readlink -f "${path}" 2>/dev/null || printf '%s' "${path}" +} + # check [probe-command...] # Runs the probe (default: " --version"), capturing both stdout and -# stderr, and prints one aligned line: the status, the name, and the first -# non-blank line of the probe output (its version). Records as missing -# if the command is not found or exits non-zero. +# stderr, and prints three lines: the status and name, the first non-blank line +# of the probe output (its version, or the error when it failed), and the tool's +# resolved path. Records as missing if it is not found or exits non-zero. check() { local name="$1" shift @@ -43,14 +60,17 @@ check() { fi checked=$((checked + 1)) - local output version + local output version path + path="$(tool_path "${name}")" if output="$("${probe[@]}" 2>&1)"; then - version="$(printf '%s\n' "${output}" | grep -m1 '[^[:space:]]' || true)" - printf ' [ ok ] %-20s %s\n' "${name}" "${version}" + printf ' ✅ %s\n' "${name}" else - printf ' [MISS] %s\n' "${name}" + printf ' ❌ %s\n' "${name}" missing+=("${name}") fi + version="$(printf '%s\n' "${output}" | grep -m1 '[^[:space:]]' || true)" + printf ' %s\n' "${version:-(no output)}" + printf ' %s\n' "${path:-(not found)}" } case "$(uname -s)" in @@ -82,7 +102,9 @@ if [ "${os}" = "linux" ] || [ "${os}" = "macos" ]; then echo "Development tooling:" check ccache check clang + check "clang-${llvm_version}" check clang++ + check "clang++-${llvm_version}" check ClangBuildAnalyzer check curl check file @@ -101,7 +123,14 @@ if [ "${os}" = "linux" ] || [ "${os}" = "macos" ]; then # setups, but not in the macOS CI environment. So check them everywhere # except when running in CI on macOS. if [ "${os}" = "linux" ] || [ -z "${CI:-}" ]; then + check clang-apply-replacements + check "clang-apply-replacements-${llvm_version}" check clang-format + check "clang-format-${llvm_version}" + # clang-tidy leads --version with the LLVM banner, not the version. + tidy_probe="--version | grep -m1 -oE 'LLVM version [0-9.]+'" + check clang-tidy sh -c "clang-tidy ${tidy_probe}" + check "clang-tidy-${llvm_version}" sh -c "clang-tidy-${llvm_version} ${tidy_probe}" check dot check doxygen check gcovr @@ -112,6 +141,7 @@ if [ "${os}" = "linux" ] || [ "${os}" = "macos" ]; then # pre-commit, or its alternative implementation prek check pre-commit sh -c 'pre-commit --version || prek --version' check run-clang-tidy run-clang-tidy --help + check "run-clang-tidy-${llvm_version}" "run-clang-tidy-${llvm_version}" --help fi fi @@ -126,7 +156,7 @@ if [ "${os}" = "linux" ] || [ "${os}" = "macos" ]; then check cargo-audit cargo audit --version check cargo-llvm-cov cargo llvm-cov --version check cargo-nextest cargo nextest --version - check clippy clippy-driver --version + check clippy-driver check rust-analyzer check rustc check rustfmt @@ -138,7 +168,11 @@ if [ "${os}" = "linux" ]; then echo echo "GCC toolchain:" check gcc + check "gcc-${gcc_version}" check g++ + check "g++-${gcc_version}" + check cpp + check "cpp-${gcc_version}" check gcov echo @@ -163,9 +197,9 @@ else checked=$((checked + 1)) tmp_clone="$(mktemp -d)" if git clone --depth 1 https://github.com/XRPLF/actions.git "${tmp_clone}/actions" >/dev/null 2>&1; then - printf ' [ ok ] git clone over HTTPS\n' + printf ' ✅ git clone over HTTPS\n' else - printf ' [MISS] git clone over HTTPS\n' + printf ' ❌ git clone over HTTPS\n' missing+=("git-https-clone") fi rm -rf "${tmp_clone}" @@ -173,9 +207,9 @@ fi echo if [ "${#missing[@]}" -eq 0 ]; then - echo "All ${checked} checked tools are present and runnable." + echo "✅ All ${checked} checked tools are present and runnable." else - echo "Missing or non-functional tools (${#missing[@]} of ${checked}):" >&2 + echo "❌ Missing or non-functional tools (${#missing[@]} of ${checked}):" >&2 for tool in "${missing[@]}"; do echo " - ${tool}" >&2 done diff --git a/nix/check-tools/README.md b/nix/check-tools/README.md index 5b7538f2ca..f23b2dcc21 100644 --- a/nix/check-tools/README.md +++ b/nix/check-tools/README.md @@ -1,7 +1,8 @@ # check-tools snapshots These files capture the output of [`bin/check-tools.sh`](../../bin/check-tools.sh) -— the versions of the development tooling — in each Nix environment: +— the version and resolved store path of each development tool — in each Nix +environment: | File | Environment | | ---------------------- | ------------------------------------ | @@ -17,9 +18,13 @@ So if you change the environment (bump the image tag in and commit the affected snapshots. Each snapshot is `check-tools.sh` stdout with the git-clone connectivity check -skipped (`CHECK_TOOLS_SKIP_CLONE=1`), so it contains only deterministic version -data. On macOS the dev-shell greeting that `nix develop` prints first is dropped -with `sed -n '/^Detected OS:/,$p'`. +skipped (`CHECK_TOOLS_SKIP_CLONE=1`), so it is deterministic for a given +environment. On macOS the dev-shell greeting that `nix develop` prints first is +dropped with `sed -n '/^Detected OS:/,$p'`. + +The store paths carry their derivation hash, so they change whenever a tool is +rebuilt — a `flake.lock` update generally rewrites most of them even when no +version moves. That is deliberate: it makes tooling changes visible in review. ## Regenerating diff --git a/nix/check-tools/macos.txt b/nix/check-tools/macos.txt index 93cc926181..8e99aa28e4 100644 --- a/nix/check-tools/macos.txt +++ b/nix/check-tools/macos.txt @@ -1,47 +1,143 @@ Detected OS: macos (Darwin arm64) Core build tools: - [ ok ] cmake cmake version 4.1.2 - [ ok ] conan Conan version 2.28.1 - [ ok ] git git version 2.54.0 - [ ok ] python3 Python 3.13.13 + ✅ cmake + cmake version 4.1.2 + /nix/store/gvabsb4yqb5xsqzqph54rijnn4zpihnp-cmake-4.1.2/bin/cmake + ✅ conan + Conan version 2.28.1 + /nix/store/9jiyxmkpwmn6dcqs0765s83riw3l5ail-conan-2.28.1/bin/conan + ✅ git + git version 2.54.0 + /nix/store/a14yxcqvv9x2l9mllgpirzhvz93pgprg-git-2.54.0/bin/git + ✅ python3 + Python 3.13.13 + /nix/store/ygxqin6ydzjfawywqpp5pal8wv6sf5bh-python3-3.13.13/bin/python3.13 Development tooling: - [ ok ] ccache ccache version 4.13.6 - [ ok ] clang clang version 22.1.7 - [ ok ] clang++ clang version 22.1.7 - [ ok ] ClangBuildAnalyzer ClangBuildAnalyzer 1.6.0 - [ ok ] curl curl 8.20.0 (aarch64-apple-darwin25.3.0) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 - [ ok ] file file-5.47 - [ ok ] less less 692 (PCRE2 regular expressions) - [ ok ] make GNU Make 4.4.1 - [ ok ] netstat present - [ ok ] ninja 1.13.2 - [ ok ] perl v5.42.0 - [ ok ] pkg-config 0.29.2 - [ ok ] vim VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) - [ ok ] zip Zip 3.0 - [ ok ] clang-format clang-format version 22.1.7 - [ ok ] dot dot - graphviz version 12.2.1 (0) - [ ok ] doxygen 1.16.1 - [ ok ] gcovr gcovr 8.4 - [ ok ] gh gh version 2.94.0 (nixpkgs) - [ ok ] git-cliff git-cliff 2.13.1 - [ ok ] git-lfs git-lfs/3.7.1 (3.7.1; darwin arm64; go 1.26.3) - [ ok ] gpg gpg (GnuPG) 2.4.9 - [ ok ] pre-commit pre-commit 4.5.1 - [ ok ] run-clang-tidy usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + ✅ ccache + ccache version 4.13.6 + /nix/store/57davyvs6p6dkrl3svzwg1ph18wsy4cz-ccache-4.13.6/bin/ccache + ✅ clang + clang version 22.1.7 + /nix/store/192glrb2cldvziyf3378mzjqbzx3ih4g-clang-wrapper-22.1.7/bin/clang + ✅ clang-22 + clang version 22.1.7 + /nix/store/rbap7zqq7mw00fyqa02p5rj7gqjp4w5i-clang-22/bin/clang-22 + ✅ clang++ + clang version 22.1.7 + /nix/store/192glrb2cldvziyf3378mzjqbzx3ih4g-clang-wrapper-22.1.7/bin/clang++ + ✅ clang++-22 + clang version 22.1.7 + /nix/store/v9haf787f7bcz0mq1sad4bpyx21pj6li-clang++-22/bin/clang++-22 + ✅ ClangBuildAnalyzer + ClangBuildAnalyzer 1.6.0 + /nix/store/4l50ds9fa2mkvh7wg8qzrlbmjs12sb8l-clangbuildanalyzer-1.6.0/bin/ClangBuildAnalyzer + ✅ curl + curl 8.20.0 (aarch64-apple-darwin25.3.0) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 + /nix/store/kclq0czaxvsgh4ym9ld7b6iwy50l1snk-curl-8.20.0-bin/bin/curl + ✅ file + file-5.47 + /nix/store/dax63li7wwcbqxxkkgzc4g2rx7d4w86x-file-5.47/bin/file + ✅ less + less 692 (PCRE2 regular expressions) + /nix/store/lvr16y75r1pdxpdv0aph5ak2yd0hkvqm-less-692/bin/less + ✅ make + GNU Make 4.4.1 + /nix/store/8wwiw8pwyhrkzyq28hqzxfl4z84lks81-gnumake-4.4.1/bin/make + ✅ netstat + present + /nix/store/qsd1kzqb0ahrk433vmyl245gp623j19s-network_cmds-730.80.3/bin/netstat + ✅ ninja + 1.13.2 + /nix/store/bqykhrblarkj4fl0hz2mf8ngwfv6x6bz-ninja-1.13.2/bin/ninja + ✅ perl + v5.42.0 + /nix/store/js13ri9fvm0ajk1fpd3acigys2a9whdv-perl-5.42.0/bin/perl + ✅ pkg-config + 0.29.2 + /nix/store/lzrwr375jqhhbca116kja96xf1md83l8-pkg-config-wrapper-0.29.2/bin/pkg-config + ✅ vim + VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) + /nix/store/6vbkykg92w603c0sw3mkk7p7mfaawbns-vim-9.2.0389/bin/vim + ✅ zip + Zip 3.0 + /nix/store/z6ph729vcakbvz3wh8ln1wk6mi06w487-zip-3.0/bin/zip + ✅ clang-apply-replacements + clang-apply-replacements version 22.1.7 + /nix/store/vzyyjf3cm1hbj9wcr2qcb66x6j98zpy7-clang-tools-22.1.7/bin/clang-apply-replacements + ✅ clang-apply-replacements-22 + clang-apply-replacements version 22.1.7 + /nix/store/m3ii69rca4077lf4wlk7m3jcag1fs577-clang-apply-replacements-22/bin/clang-apply-replacements-22 + ✅ clang-format + clang-format version 22.1.7 + /nix/store/vzyyjf3cm1hbj9wcr2qcb66x6j98zpy7-clang-tools-22.1.7/bin/clang-format + ✅ clang-format-22 + clang-format version 22.1.7 + /nix/store/4fawqy6ngqcsqd2ygyyzm93q0xy3f5gs-clang-format-22/bin/clang-format-22 + ✅ clang-tidy + LLVM version 22.1.7 + /nix/store/vzyyjf3cm1hbj9wcr2qcb66x6j98zpy7-clang-tools-22.1.7/bin/clang-tidy + ✅ clang-tidy-22 + LLVM version 22.1.7 + /nix/store/jqw4280saixaxxihwdba9ldm2fsm6dr3-clang-tidy-22/bin/clang-tidy-22 + ✅ dot + dot - graphviz version 12.2.1 (0) + /nix/store/ijb4fbnqa6wzlpqnhb6q9knqpf7qqn5z-graphviz-12.2.1/bin/dot + ✅ doxygen + 1.16.1 + /nix/store/kbryjdpq9jizjb0ws0nzbf2h2ymbdiwm-doxygen-1.16.1/bin/doxygen + ✅ gcovr + gcovr 8.4 + /nix/store/wn8jiyh9p0bybs96s4163qp3k8vfmczx-python3.13-gcovr-8.4/bin/gcovr + ✅ gh + gh version 2.94.0 (nixpkgs) + /nix/store/fhnpw0hs0gjms1ha6ap02jq7rx13gkbp-gh-2.94.0/bin/gh + ✅ git-cliff + git-cliff 2.13.1 + /nix/store/cy0wwhgxa7yvrz97zydbq6sqmixc90fq-git-cliff-2.13.1/bin/git-cliff + ✅ git-lfs + git-lfs/3.7.1 (3.7.1; darwin arm64; go 1.26.3) + /nix/store/k9r7zjfjplqa4d5s71cqvf2iv73jd9mc-git-lfs-3.7.1/bin/git-lfs + ✅ gpg + gpg (GnuPG) 2.4.9 + /nix/store/cgh6iwzz5jgx9z5whka4vgj210i6npc6-gnupg-2.4.9/bin/gpg + ✅ pre-commit + pre-commit 4.5.1 + /nix/store/z3cca68620w0w10f090szgzdnmh1waf2-pre-commit-4.5.1/bin/pre-commit + ✅ run-clang-tidy + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/4x28x911z2f9y7adqlh3qspp4a16dig7-run-clang-tidy/bin/run-clang-tidy + ✅ run-clang-tidy-22 + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/x8iymrh76sk5q91ryg5pa7i32s6gfh34-run-clang-tidy-22/bin/run-clang-tidy-22 Rust toolchain: - [ ok ] cargo cargo 1.95.0 (f2d3ce0bd 2026-03-21) - [ ok ] cargo-audit cargo-audit-audit 0.22.1 - [ ok ] cargo-llvm-cov cargo-llvm-cov 0.8.5 - [ ok ] cargo-nextest cargo-nextest 0.9.137 - [ ok ] clippy clippy 0.1.95 (59807616e1 2026-04-14) - [ ok ] rust-analyzer rust-analyzer 1.95.0 (59807616 2026-04-14) - [ ok ] rustc rustc 1.95.0 (59807616e 2026-04-14) - [ ok ] rustfmt rustfmt 1.9.0-stable (59807616e1 2026-04-14) + ✅ cargo + cargo 1.95.0 (f2d3ce0bd 2026-03-21) + /nix/store/92vz1f4kislnj58j1pr1788l688py6f0-rust-minimal-1.95.0/bin/cargo + ✅ cargo-audit + cargo-audit-audit 0.22.1 + /nix/store/snwkga2f5gyf404h7mmp9wriwxb8v65f-cargo-audit-0.22.1/bin/cargo-audit + ✅ cargo-llvm-cov + cargo-llvm-cov 0.8.5 + /nix/store/fpiqdh91gwyxalqp409ynm0s0g086w7w-cargo-llvm-cov-0.8.5/bin/cargo-llvm-cov + ✅ cargo-nextest + cargo-nextest 0.9.137 + /nix/store/ylz7m947mhkgsp6i7611id3s3gcd58nq-cargo-nextest-0.9.137/bin/cargo-nextest + ✅ clippy-driver + clippy 0.1.95 (59807616e1 2026-04-14) + /nix/store/92vz1f4kislnj58j1pr1788l688py6f0-rust-minimal-1.95.0/bin/clippy-driver + ✅ rust-analyzer + rust-analyzer 1.95.0 (59807616 2026-04-14) + /nix/store/jqvjap2727r9cjpr25fkw5glv2kbxrdx-rust-analyzer-preview-1.95.0-aarch64-apple-darwin/bin/rust-analyzer + ✅ rustc + rustc 1.95.0 (59807616e 2026-04-14) + /nix/store/92vz1f4kislnj58j1pr1788l688py6f0-rust-minimal-1.95.0/bin/rustc + ✅ rustfmt + rustfmt 1.9.0-stable (59807616e1 2026-04-14) + /nix/store/03x750yj6fakl7shbhicpnkxiwqxjrrs-rustfmt-preview-1.95.0-aarch64-apple-darwin/bin/rustfmt Skipping git-over-HTTPS check (CHECK_TOOLS_SKIP_CLONE is set). -All 36 checked tools are present and runnable. +✅ All 44 checked tools are present and runnable. diff --git a/nix/check-tools/nix-ubuntu-amd64.txt b/nix/check-tools/nix-ubuntu-amd64.txt index b922cca4a8..a5857c93f1 100644 --- a/nix/check-tools/nix-ubuntu-amd64.txt +++ b/nix/check-tools/nix-ubuntu-amd64.txt @@ -1,55 +1,171 @@ Detected OS: linux (Linux x86_64) Core build tools: - [ ok ] cmake cmake version 4.1.2 - [ ok ] conan Conan version 2.28.1 - [ ok ] git git version 2.54.0 - [ ok ] python3 Python 3.13.13 + ✅ cmake + cmake version 4.1.2 + /nix/store/r9941n32g4wyvggz2703dlplbdq8a6rd-cmake-4.1.2/bin/cmake + ✅ conan + Conan version 2.28.1 + /nix/store/lxny9y4jvjdws7hgz1mygvb7hjrpmna5-conan-2.28.1/bin/conan + ✅ git + git version 2.54.0 + /nix/store/bcnisk3ydfgv26v2gw3zlky24g00yww2-git-2.54.0/bin/git + ✅ python3 + Python 3.13.13 + /nix/store/60m4rxhg2fldqaak400c0lry96ijrzqn-python3-3.13.13/bin/python3.13 Development tooling: - [ ok ] ccache ccache version 4.13.6 - [ ok ] clang clang version 22.1.7 - [ ok ] clang++ clang version 22.1.7 - [ ok ] ClangBuildAnalyzer ClangBuildAnalyzer 1.6.0 - [ ok ] curl curl 8.20.0 (x86_64-pc-linux-gnu) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 - [ ok ] file file-5.47 - [ ok ] less less 692 (PCRE2 regular expressions) - [ ok ] make GNU Make 4.4.1 - [ ok ] netstat net-tools 2.10 - [ ok ] ninja 1.13.2 - [ ok ] perl v5.42.0 - [ ok ] pkg-config 0.29.2 - [ ok ] vim VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) - [ ok ] zip Zip 3.0 - [ ok ] clang-format clang-format version 22.1.7 - [ ok ] dot dot - graphviz version 12.2.1 (0) - [ ok ] doxygen 1.16.1 - [ ok ] gcovr gcovr 8.4 - [ ok ] gh gh version 2.94.0 (nixpkgs) - [ ok ] git-cliff git-cliff 2.13.1 - [ ok ] git-lfs git-lfs/3.7.1 (3.7.1; linux amd64; go 1.26.3) - [ ok ] gpg gpg (GnuPG) 2.4.9 - [ ok ] pre-commit pre-commit 4.5.1 - [ ok ] run-clang-tidy usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + ✅ ccache + ccache version 4.13.6 + /nix/store/c9wwl7s5i6rsfwvf4v0xbbmzx5m6jgfr-ccache-4.13.6/bin/ccache + ✅ clang + clang version 22.1.7 + /nix/store/ff0hrp9r9i3pa5arkdw0sgmzp8d576qi-clang-wrapper-22.1.7/bin/clang + ✅ clang-22 + clang version 22.1.7 + /nix/store/dagc2rq44gfbr7w7yvvqca3yqpc9gqbq-clang-22/bin/clang-22 + ✅ clang++ + clang version 22.1.7 + /nix/store/ff0hrp9r9i3pa5arkdw0sgmzp8d576qi-clang-wrapper-22.1.7/bin/clang++ + ✅ clang++-22 + clang version 22.1.7 + /nix/store/l5m8clin1npl605wdkd8mr18ggxww3z4-clang++-22/bin/clang++-22 + ✅ ClangBuildAnalyzer + ClangBuildAnalyzer 1.6.0 + /nix/store/bshlmn8fqw55nsnm581xqlfbahfkykxx-clangbuildanalyzer-1.6.0/bin/ClangBuildAnalyzer + ✅ curl + curl 8.20.0 (x86_64-pc-linux-gnu) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 + /nix/store/zbwymrp4lcfjc4kkk0n4779v0kjjz58z-curl-8.20.0-bin/bin/curl + ✅ file + file-5.47 + /nix/store/bizyfqdw0h67wzqmp10knmf9s2pqahdb-file-5.47/bin/file + ✅ less + less 692 (PCRE2 regular expressions) + /nix/store/c6bacbn93qg4a7g9n4czww8rg24dvysr-less-692/bin/less + ✅ make + GNU Make 4.4.1 + /nix/store/d3bwqm6bymhy3pdgbvf7vxjqfp31m3j1-gnumake-4.4.1/bin/make + ✅ netstat + net-tools 2.10 + /nix/store/jmyzqvgflnswmws7rnxx6g3zbj680xvd-net-tools-2.10/bin/netstat + ✅ ninja + 1.13.2 + /nix/store/7a235m7crqbb4h49sak20fqxpw3n7hr0-ninja-1.13.2/bin/ninja + ✅ perl + v5.42.0 + /nix/store/6plwsm6pkq79yjv4xvy8csk2pd4hzr67-perl-5.42.0/bin/perl + ✅ pkg-config + 0.29.2 + /nix/store/1m05k7xgfnw6jc21xxk5681ni3ar97wf-pkg-config-wrapper-0.29.2/bin/pkg-config + ✅ vim + VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) + /nix/store/hvyqx52g4g2fxhgpans3fksjj6lmlyaw-vim-9.2.0389/bin/vim + ✅ zip + Zip 3.0 + /nix/store/qnd2ag67hrjj0b6vbmisdshf50r6s72n-zip-3.0/bin/zip + ✅ clang-apply-replacements + clang-apply-replacements version 22.1.7 + /nix/store/4zp1rjpj2xijrv4kqpwsy3ixwb2r6nlk-clang-tools-22.1.7/bin/clang-apply-replacements + ✅ clang-apply-replacements-22 + clang-apply-replacements version 22.1.7 + /nix/store/py2wihg0a96qcppv4hjmww547xabr0fb-clang-apply-replacements-22/bin/clang-apply-replacements-22 + ✅ clang-format + clang-format version 22.1.7 + /nix/store/4zp1rjpj2xijrv4kqpwsy3ixwb2r6nlk-clang-tools-22.1.7/bin/clang-format + ✅ clang-format-22 + clang-format version 22.1.7 + /nix/store/kz820ccifjlwqnwqjsx7kbiajrgsmbrh-clang-format-22/bin/clang-format-22 + ✅ clang-tidy + LLVM version 22.1.7 + /nix/store/4zp1rjpj2xijrv4kqpwsy3ixwb2r6nlk-clang-tools-22.1.7/bin/clang-tidy + ✅ clang-tidy-22 + LLVM version 22.1.7 + /nix/store/gdrkvpw846lkyzh8y9p3zx50g6ml2v84-clang-tidy-22/bin/clang-tidy-22 + ✅ dot + dot - graphviz version 12.2.1 (0) + /nix/store/12rgns2296s4qcja778gvcbx61z77rc4-graphviz-12.2.1/bin/dot + ✅ doxygen + 1.16.1 + /nix/store/k0vzr5lvgq1byraknzwvk51wcgpnsrkh-doxygen-1.16.1/bin/doxygen + ✅ gcovr + gcovr 8.4 + /nix/store/iyzi7fpyclqrha054adnizvif02lg49x-python3.13-gcovr-8.4/bin/gcovr + ✅ gh + gh version 2.94.0 (nixpkgs) + /nix/store/pidh15szlsb1vc41xdsa3xbdghdazvby-gh-2.94.0/bin/gh + ✅ git-cliff + git-cliff 2.13.1 + /nix/store/1q851fs62shgjhc03fxxdkpzxdjg7k11-git-cliff-2.13.1/bin/git-cliff + ✅ git-lfs + git-lfs/3.7.1 (3.7.1; linux amd64; go 1.26.3) + /nix/store/6ljwpal7b1756708m33vj0crpral7mvl-git-lfs-3.7.1/bin/git-lfs + ✅ gpg + gpg (GnuPG) 2.4.9 + /nix/store/wx7vk8babxkgy813r70yc67vcwnmagbx-gnupg-2.4.9/bin/gpg + ✅ pre-commit + pre-commit 4.5.1 + /nix/store/bj6i9vl34cij5h0r165y40hrjqak0bmz-pre-commit-4.5.1/bin/pre-commit + ✅ run-clang-tidy + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/sbg911hs9dbclrzlp04br3iyfpgnaj6r-run-clang-tidy/bin/run-clang-tidy + ✅ run-clang-tidy-22 + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/n8yak1ap308gvi7gmrniw0ybsx80fjws-run-clang-tidy-22/bin/run-clang-tidy-22 Rust toolchain: - [ ok ] cargo cargo 1.95.0 (f2d3ce0bd 2026-03-21) - [ ok ] cargo-audit cargo-audit-audit 0.22.1 - [ ok ] cargo-llvm-cov cargo-llvm-cov 0.8.5 - [ ok ] cargo-nextest cargo-nextest 0.9.137 - [ ok ] clippy clippy 0.1.95 (59807616e1 2026-04-14) - [ ok ] rust-analyzer rust-analyzer 1.95.0 (5980761 2026-04-14) - [ ok ] rustc rustc 1.95.0 (59807616e 2026-04-14) - [ ok ] rustfmt rustfmt 1.9.0-stable (59807616e1 2026-04-14) + ✅ cargo + cargo 1.95.0 (f2d3ce0bd 2026-03-21) + /nix/store/85qbwr3vzfs58m7ywnjblz105p8ahbrv-cargo-1.95.0-x86_64-unknown-linux-gnu/bin/cargo + ✅ cargo-audit + cargo-audit-audit 0.22.1 + /nix/store/2w9if868piw98xz057sz97jnjvf7hnvf-cargo-audit-0.22.1/bin/cargo-audit + ✅ cargo-llvm-cov + cargo-llvm-cov 0.8.5 + /nix/store/jjpdf1l6izz6607a346ykra9sndzaw7h-cargo-llvm-cov-0.8.5/bin/cargo-llvm-cov + ✅ cargo-nextest + cargo-nextest 0.9.137 + /nix/store/jhkr7gwyrchkml33gyns9cy0yn7b57qc-cargo-nextest-0.9.137/bin/cargo-nextest + ✅ clippy-driver + clippy 0.1.95 (59807616e1 2026-04-14) + /nix/store/bnvg9nmdq4g98dd9v3r6nvjg5h2rr8i7-rust-minimal-1.95.0/bin/clippy-driver + ✅ rust-analyzer + rust-analyzer 1.95.0 (5980761 2026-04-14) + /nix/store/i3cnpngfwa3k4jn431pl6ji1r4qmxky9-rust-analyzer-preview-1.95.0-x86_64-unknown-linux-gnu/bin/rust-analyzer + ✅ rustc + rustc 1.95.0 (59807616e 2026-04-14) + /nix/store/bnvg9nmdq4g98dd9v3r6nvjg5h2rr8i7-rust-minimal-1.95.0/bin/rustc + ✅ rustfmt + rustfmt 1.9.0-stable (59807616e1 2026-04-14) + /nix/store/366hhk2dgwxmnf4hgrj4b8llhjr3hf0i-rustfmt-preview-1.95.0-x86_64-unknown-linux-gnu/bin/rustfmt GCC toolchain: - [ ok ] gcc gcc (GCC) 15.2.0 - [ ok ] g++ g++ (GCC) 15.2.0 - [ ok ] gcov gcov (GCC) 15.2.0 + ✅ gcc + gcc (GCC) 15.2.0 + /nix/store/3dd6y3pq00i3r85l45jvz63wjya403nl-gcc-wrapper-15.2.0/bin/gcc + ✅ gcc-15 + gcc (GCC) 15.2.0 + /nix/store/d6iri2s6bzqq5ac3fg25j6hgnn1lz44f-gcc-15/bin/gcc-15 + ✅ g++ + g++ (GCC) 15.2.0 + /nix/store/3dd6y3pq00i3r85l45jvz63wjya403nl-gcc-wrapper-15.2.0/bin/g++ + ✅ g++-15 + g++ (GCC) 15.2.0 + /nix/store/gm3msmmxq055lm9gprkfjj9d2gdz1mpg-g++-15/bin/g++-15 + ✅ cpp + cpp (GCC) 15.2.0 + /nix/store/3dd6y3pq00i3r85l45jvz63wjya403nl-gcc-wrapper-15.2.0/bin/cpp + ✅ cpp-15 + cpp (GCC) 15.2.0 + /nix/store/bn3gmn0m7g4gn2i0yml46fljc7mghiq5-cpp-15/bin/cpp-15 + ✅ gcov + gcov (GCC) 15.2.0 + /nix/store/xvv5sm5i8x0ks6ypfkzl7c4j9srnxz7k-gcc-15.2.0/bin/gcov Mold: - [ ok ] mold mold 2.41.0 (compatible with GNU ld) + ✅ mold + mold 2.41.0 (compatible with GNU ld) + /nix/store/2w6fpgxjzzyqmd25wzplm23dfa49a0p2-mold-unwrapped-wrapper-2.41.0/bin/mold Skipping git-over-HTTPS check (CHECK_TOOLS_SKIP_CLONE is set). -All 40 checked tools are present and runnable. +✅ All 52 checked tools are present and runnable. diff --git a/nix/check-tools/nix-ubuntu-arm64.txt b/nix/check-tools/nix-ubuntu-arm64.txt index 5267839682..820c6de086 100644 --- a/nix/check-tools/nix-ubuntu-arm64.txt +++ b/nix/check-tools/nix-ubuntu-arm64.txt @@ -1,55 +1,171 @@ Detected OS: linux (Linux aarch64) Core build tools: - [ ok ] cmake cmake version 4.1.2 - [ ok ] conan Conan version 2.28.1 - [ ok ] git git version 2.54.0 - [ ok ] python3 Python 3.13.13 + ✅ cmake + cmake version 4.1.2 + /nix/store/nkcpxjifkambzlrwh27a8igvhnbchibg-cmake-4.1.2/bin/cmake + ✅ conan + Conan version 2.28.1 + /nix/store/8i2gyqgc00xvxg9xm6y7n0ilncdv8imw-conan-2.28.1/bin/conan + ✅ git + git version 2.54.0 + /nix/store/ixp98f9avf8ikpdrmp40cj33g0dazyp9-git-2.54.0/bin/git + ✅ python3 + Python 3.13.13 + /nix/store/lqn6mbgzzdrqq2qkwddcmxj9z6amdd86-python3-3.13.13/bin/python3.13 Development tooling: - [ ok ] ccache ccache version 4.13.6 - [ ok ] clang clang version 22.1.7 - [ ok ] clang++ clang version 22.1.7 - [ ok ] ClangBuildAnalyzer ClangBuildAnalyzer 1.6.0 - [ ok ] curl curl 8.20.0 (aarch64-unknown-linux-gnu) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 - [ ok ] file file-5.47 - [ ok ] less less 692 (PCRE2 regular expressions) - [ ok ] make GNU Make 4.4.1 - [ ok ] netstat net-tools 2.10 - [ ok ] ninja 1.13.2 - [ ok ] perl v5.42.0 - [ ok ] pkg-config 0.29.2 - [ ok ] vim VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) - [ ok ] zip Zip 3.0 - [ ok ] clang-format clang-format version 22.1.7 - [ ok ] dot dot - graphviz version 12.2.1 (0) - [ ok ] doxygen 1.16.1 - [ ok ] gcovr gcovr 8.4 - [ ok ] gh gh version 2.94.0 (nixpkgs) - [ ok ] git-cliff git-cliff 2.13.1 - [ ok ] git-lfs git-lfs/3.7.1 (3.7.1; linux arm64; go 1.26.3) - [ ok ] gpg gpg (GnuPG) 2.4.9 - [ ok ] pre-commit pre-commit 4.5.1 - [ ok ] run-clang-tidy usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + ✅ ccache + ccache version 4.13.6 + /nix/store/2q39xi2kbi04ibga7635f2sl148d1mzv-ccache-4.13.6/bin/ccache + ✅ clang + clang version 22.1.7 + /nix/store/xjqffrq9i7la058s9865ig71l9sp1ys5-clang-wrapper-22.1.7/bin/clang + ✅ clang-22 + clang version 22.1.7 + /nix/store/vcf6ilfwn57828hwzyp6zlyr24j9j6yw-clang-22/bin/clang-22 + ✅ clang++ + clang version 22.1.7 + /nix/store/xjqffrq9i7la058s9865ig71l9sp1ys5-clang-wrapper-22.1.7/bin/clang++ + ✅ clang++-22 + clang version 22.1.7 + /nix/store/xby0f6gamr7m27zp5cndsvghbp9lgb3c-clang++-22/bin/clang++-22 + ✅ ClangBuildAnalyzer + ClangBuildAnalyzer 1.6.0 + /nix/store/h893hd4q1bb6ily2lby5dzyfrrzd2nvj-clangbuildanalyzer-1.6.0/bin/ClangBuildAnalyzer + ✅ curl + curl 8.20.0 (aarch64-unknown-linux-gnu) libcurl/8.20.0 OpenSSL/3.6.2 zlib/1.3.2 libssh2/1.11.1 nghttp2/1.69.0 mit-krb5/1.22.1 + /nix/store/i1s0lqwlrmjd2dxzgy2p84cxqqsb0bmk-curl-8.20.0-bin/bin/curl + ✅ file + file-5.47 + /nix/store/dx973zg9km2w9albsib2vw9wyvacfrlw-file-5.47/bin/file + ✅ less + less 692 (PCRE2 regular expressions) + /nix/store/1blb3s7hhsr77wqi598m6k1qkfp3ms0w-less-692/bin/less + ✅ make + GNU Make 4.4.1 + /nix/store/9ngw1ippk25jjj5fjxv36xbp6iq7rxdx-gnumake-4.4.1/bin/make + ✅ netstat + net-tools 2.10 + /nix/store/7vdsz21f0s499s5yyqzp5s4676q4yxdd-net-tools-2.10/bin/netstat + ✅ ninja + 1.13.2 + /nix/store/8ksx98gsbn5lmlizcmw57yd4sg0k2p58-ninja-1.13.2/bin/ninja + ✅ perl + v5.42.0 + /nix/store/5wnly69vv1i3y97al4v3xrqymf9hlzgq-perl-5.42.0/bin/perl + ✅ pkg-config + 0.29.2 + /nix/store/c7vwy0gl1q0agl2h22gi0m9dg7xxad2l-pkg-config-wrapper-0.29.2/bin/pkg-config + ✅ vim + VIM - Vi IMproved 9.2 (2026 Feb 14, compiled Jan 01 1980 00:00:00) + /nix/store/v8c7pvx26irvy9k5sbwd183cyvckzzb3-vim-9.2.0389/bin/vim + ✅ zip + Zip 3.0 + /nix/store/5mh19mvbv9ym2sm9vymyyaac5l2cj2jq-zip-3.0/bin/zip + ✅ clang-apply-replacements + clang-apply-replacements version 22.1.7 + /nix/store/s53p2m776iqaz7acgr5csgpsd18w15h7-clang-tools-22.1.7/bin/clang-apply-replacements + ✅ clang-apply-replacements-22 + clang-apply-replacements version 22.1.7 + /nix/store/bg4kn8z81hk7b9284rjqvr51wpfjqc24-clang-apply-replacements-22/bin/clang-apply-replacements-22 + ✅ clang-format + clang-format version 22.1.7 + /nix/store/s53p2m776iqaz7acgr5csgpsd18w15h7-clang-tools-22.1.7/bin/clang-format + ✅ clang-format-22 + clang-format version 22.1.7 + /nix/store/79v57mzcw8ng8kl7p961ck08ymhp31v7-clang-format-22/bin/clang-format-22 + ✅ clang-tidy + LLVM version 22.1.7 + /nix/store/s53p2m776iqaz7acgr5csgpsd18w15h7-clang-tools-22.1.7/bin/clang-tidy + ✅ clang-tidy-22 + LLVM version 22.1.7 + /nix/store/wdyd6cb9z1lyi37lbzvwldgcc7yv1n5c-clang-tidy-22/bin/clang-tidy-22 + ✅ dot + dot - graphviz version 12.2.1 (0) + /nix/store/58rrk4yzwpmyxvl8cqm18h3dhv24zf00-graphviz-12.2.1/bin/dot + ✅ doxygen + 1.16.1 + /nix/store/hq32kzwpl89wgr49iq0gmqn9r5n072zq-doxygen-1.16.1/bin/doxygen + ✅ gcovr + gcovr 8.4 + /nix/store/sml3xbbfhhlhk6h7jnlg19pdbx9b764b-python3.13-gcovr-8.4/bin/gcovr + ✅ gh + gh version 2.94.0 (nixpkgs) + /nix/store/7hh2qi0gj2ifbxbl56cjzbiyfc379bji-gh-2.94.0/bin/gh + ✅ git-cliff + git-cliff 2.13.1 + /nix/store/bidn3pz53yd6qlg711917xx0q10hqmqv-git-cliff-2.13.1/bin/git-cliff + ✅ git-lfs + git-lfs/3.7.1 (3.7.1; linux arm64; go 1.26.3) + /nix/store/4rsklvkbac5bayy0zv12kxyvspi4sshd-git-lfs-3.7.1/bin/git-lfs + ✅ gpg + gpg (GnuPG) 2.4.9 + /nix/store/ka4i8zz5ni3rzqnzcxbfvwr95fk8pn6q-gnupg-2.4.9/bin/gpg + ✅ pre-commit + pre-commit 4.5.1 + /nix/store/n981w6hjfar2l81kxbxs2wxl64vwa5kj-pre-commit-4.5.1/bin/pre-commit + ✅ run-clang-tidy + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/4z2fyklg78klallr7x9j02kz92hnxp4m-run-clang-tidy/bin/run-clang-tidy + ✅ run-clang-tidy-22 + usage: run-clang-tidy [-h] [-allow-enabling-alpha-checkers] + /nix/store/f8m0p9ad40brp9ahy4i0h27kqjkya1j9-run-clang-tidy-22/bin/run-clang-tidy-22 Rust toolchain: - [ ok ] cargo cargo 1.95.0 (f2d3ce0bd 2026-03-21) - [ ok ] cargo-audit cargo-audit-audit 0.22.1 - [ ok ] cargo-llvm-cov cargo-llvm-cov 0.8.5 - [ ok ] cargo-nextest cargo-nextest 0.9.137 - [ ok ] clippy clippy 0.1.95 (59807616e1 2026-04-14) - [ ok ] rust-analyzer rust-analyzer 1.95.0 (5980761 2026-04-14) - [ ok ] rustc rustc 1.95.0 (59807616e 2026-04-14) - [ ok ] rustfmt rustfmt 1.9.0-stable (59807616e1 2026-04-14) + ✅ cargo + cargo 1.95.0 (f2d3ce0bd 2026-03-21) + /nix/store/yw1rs50s6qpsw0zyl7j3dpm18swbl0ag-cargo-1.95.0-aarch64-unknown-linux-gnu/bin/cargo + ✅ cargo-audit + cargo-audit-audit 0.22.1 + /nix/store/9rxbrn9aa2r1z96186s69pc7vzizyfch-cargo-audit-0.22.1/bin/cargo-audit + ✅ cargo-llvm-cov + cargo-llvm-cov 0.8.5 + /nix/store/vwjsi159n89szrx4yh5pc3jlf2gp4fld-cargo-llvm-cov-0.8.5/bin/cargo-llvm-cov + ✅ cargo-nextest + cargo-nextest 0.9.137 + /nix/store/qb6bcg2fjvm3r9s9j98nmffmf9xwh45s-cargo-nextest-0.9.137/bin/cargo-nextest + ✅ clippy-driver + clippy 0.1.95 (59807616e1 2026-04-14) + /nix/store/nz4qv12pf16c092qr9hh4dsn0fzf47da-rust-minimal-1.95.0/bin/clippy-driver + ✅ rust-analyzer + rust-analyzer 1.95.0 (5980761 2026-04-14) + /nix/store/m1rn67sqfz8s44idcxqallg680ifk71r-rust-analyzer-preview-1.95.0-aarch64-unknown-linux-gnu/bin/rust-analyzer + ✅ rustc + rustc 1.95.0 (59807616e 2026-04-14) + /nix/store/nz4qv12pf16c092qr9hh4dsn0fzf47da-rust-minimal-1.95.0/bin/rustc + ✅ rustfmt + rustfmt 1.9.0-stable (59807616e1 2026-04-14) + /nix/store/jidfsprj2820glyzjn54ldn3j1fmz8c5-rustfmt-preview-1.95.0-aarch64-unknown-linux-gnu/bin/rustfmt GCC toolchain: - [ ok ] gcc gcc (GCC) 15.2.0 - [ ok ] g++ g++ (GCC) 15.2.0 - [ ok ] gcov gcov (GCC) 15.2.0 + ✅ gcc + gcc (GCC) 15.2.0 + /nix/store/rn6svg593xsmn8qcjzk8x9pa1i62c4kb-gcc-wrapper-15.2.0/bin/gcc + ✅ gcc-15 + gcc (GCC) 15.2.0 + /nix/store/h489d1rmjisfbxh5kmsb0a7c35j8qsdf-gcc-15/bin/gcc-15 + ✅ g++ + g++ (GCC) 15.2.0 + /nix/store/rn6svg593xsmn8qcjzk8x9pa1i62c4kb-gcc-wrapper-15.2.0/bin/g++ + ✅ g++-15 + g++ (GCC) 15.2.0 + /nix/store/9ywmhz8bmzknrn3pn84g46z8hj3vrmw5-g++-15/bin/g++-15 + ✅ cpp + cpp (GCC) 15.2.0 + /nix/store/rn6svg593xsmn8qcjzk8x9pa1i62c4kb-gcc-wrapper-15.2.0/bin/cpp + ✅ cpp-15 + cpp (GCC) 15.2.0 + /nix/store/vmjilh1b830qz9yh0a1jj5ads0jxizdk-cpp-15/bin/cpp-15 + ✅ gcov + gcov (GCC) 15.2.0 + /nix/store/rmwf5hpi1y2m1wpnfvlxmrhksm4djk2j-gcc-15.2.0/bin/gcov Mold: - [ ok ] mold mold 2.41.0 (compatible with GNU ld) + ✅ mold + mold 2.41.0 (compatible with GNU ld) + /nix/store/f5qh5a0bx1dslmnf5n5gx0s6aljbswq3-mold-unwrapped-wrapper-2.41.0/bin/mold Skipping git-over-HTTPS check (CHECK_TOOLS_SKIP_CLONE is set). -All 40 checked tools are present and runnable. +✅ All 52 checked tools are present and runnable. From 2adffaef724f0180ffc44fb0a91c6bb854a2ebaf Mon Sep 17 00:00:00 2001 From: Bart Date: Fri, 14 Aug 2026 15:36:47 +0000 Subject: [PATCH 04/13] refactor: Remove support for protocol version 2.1 (#7432) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 --- include/xrpl/proto/xrpl.proto | 15 +- src/test/app/ValidatorList_test.cpp | 238 +++++-------------- src/test/overlay/ProtocolVersion_test.cpp | 38 +-- src/test/overlay/compression_test.cpp | 30 --- src/xrpld/app/misc/ValidatorList.h | 13 - src/xrpld/app/misc/detail/ValidatorList.cpp | 167 +++---------- src/xrpld/overlay/Peer.h | 2 - src/xrpld/overlay/detail/Message.cpp | 1 - src/xrpld/overlay/detail/PeerImp.cpp | 38 +-- src/xrpld/overlay/detail/PeerImp.h | 2 - src/xrpld/overlay/detail/ProtocolMessage.h | 5 - src/xrpld/overlay/detail/ProtocolVersion.cpp | 1 - src/xrpld/overlay/detail/TrafficCount.cpp | 1 - 13 files changed, 114 insertions(+), 437 deletions(-) diff --git a/include/xrpl/proto/xrpl.proto b/include/xrpl/proto/xrpl.proto index b9cb94e668..644e099179 100644 --- a/include/xrpl/proto/xrpl.proto +++ b/include/xrpl/proto/xrpl.proto @@ -1,10 +1,10 @@ syntax = "proto2"; package protocol; -// Unused numbers in the list below may have been used previously. Please don't -// reassign them for reuse unless you are 100% certain that there won't be a -// conflict. Even if you're sure, it's probably best to assign a new type. enum MessageType { + // Previously used - don't reuse. + reserved 0 to 1, 4, 6 to 14, 16 to 29, 36 to 40, 43 to 54, 61 to 62; + mtMANIFESTS = 2; mtPING = 3; mtCLUSTER = 5; @@ -17,7 +17,6 @@ enum MessageType { mtHAVE_SET = 35; mtVALIDATION = 41; mtGET_OBJECTS = 42; - mtVALIDATOR_LIST = 54; mtSQUELCH = 55; mtVALIDATOR_LIST_COLLECTION = 56; mtPROOF_PATH_REQ = 57; @@ -162,14 +161,6 @@ message TMHaveTransactionSet { required bytes hash = 2; } -// Validator list (UNL) -message TMValidatorList { - required bytes manifest = 1; - required bytes blob = 2; - required bytes signature = 3; - required uint32 version = 4; -} - // Validator List v2 message ValidatorBlobInfo { optional bytes manifest = 1; diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 323c77c780..d2e6cb24aa 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -2253,8 +2253,7 @@ private: { testcase("Sha512 hashing"); // Tests that ValidatorList hash_append helpers with a single blob - // returns the same result as xrpl::Sha512Half used by the - // TMValidatorList protocol message handler + // return the same result as xrpl::Sha512Half std::string const manifest = "This is not really a manifest"; std::string const blob = "This is not really a blob"; std::string const signature = "This is not really a signature"; @@ -2275,17 +2274,6 @@ private: BEAST_EXPECT(global != sha512Half(blob, blobMap, version)); } - { - protocol::TMValidatorList msg1; - msg1.set_manifest(manifest); - msg1.set_blob(blob); - msg1.set_signature(signature); - msg1.set_version(version); - BEAST_EXPECT(global == sha512Half(msg1)); - msg1.set_signature(blob); - BEAST_EXPECT(global != sha512Half(msg1)); - } - { protocol::TMValidatorListCollection msg2; msg2.set_manifest(manifest); @@ -2323,19 +2311,7 @@ private: BEAST_EXPECT(!ec); return std::make_pair(header, buffers); }; - auto extractProtocolMessage1 = [this, &extractHeader](Message& message) { - auto [header, buffers] = extractHeader(message); - if (BEAST_EXPECT(header) && - BEAST_EXPECT(header->messageType == protocol::mtVALIDATOR_LIST)) - { - auto const msg = - detail::parseMessageContent(*header, buffers.data()); - BEAST_EXPECT(msg); - return msg; - } - return std::shared_ptr(); - }; - auto extractProtocolMessage2 = [this, &extractHeader](Message& message) { + auto extractProtocolMessage = [this, &extractHeader](Message& message) { auto [header, buffers] = extractHeader(message); if (BEAST_EXPECT(header) && BEAST_EXPECT(header->messageType == protocol::mtVALIDATOR_LIST_COLLECTION)) @@ -2347,92 +2323,55 @@ private: } return std::shared_ptr(); }; - auto verifyMessage = - [this, manifestCutoff, &extractProtocolMessage1, &extractProtocolMessage2]( - auto const version, - auto const& manifest, - auto const& blobInfos, - auto const& messages, - std::vector>> expectedInfo) { - BEAST_EXPECT(messages.size() == expectedInfo.size()); - auto msgIter = expectedInfo.begin(); - for (auto const& messageWithHash : messages) + auto verifyMessage = [this, manifestCutoff, &extractProtocolMessage]( + auto const version, + auto const& manifest, + auto const& blobInfos, + auto const& messages, + std::vector> expectedInfo) { + BEAST_EXPECT(messages.size() == expectedInfo.size()); + auto msgIter = expectedInfo.begin(); + for (auto const& messageWithHash : messages) + { + if (!BEAST_EXPECT(msgIter != expectedInfo.end())) + break; + if (!BEAST_EXPECT(messageWithHash.message)) + continue; + auto const& expectedSeqs = *msgIter; + auto seqIter = expectedSeqs.begin(); { - if (!BEAST_EXPECT(msgIter != expectedInfo.end())) - break; - if (!BEAST_EXPECT(messageWithHash.message)) - continue; - auto const& expectedSeqs = msgIter->second; - auto seqIter = expectedSeqs.begin(); - auto const size = - messageWithHash.message->getBuffer(compression::Compressed::Off).size(); - // This size is arbitrary, but shouldn't change - BEAST_EXPECT(size == msgIter->first); - if (expectedSeqs.size() == 1) + std::vector hashingBlobs; + hashingBlobs.reserve(expectedSeqs.size()); + + auto const msg = extractProtocolMessage(*messageWithHash.message); + if (BEAST_EXPECT(msg)) { - auto const msg = extractProtocolMessage1(*messageWithHash.message); - auto const expectedVersion = 1; - if (BEAST_EXPECT(msg)) + BEAST_EXPECT(msg->version() == version); + BEAST_EXPECT(msg->manifest() == manifest); + for (auto const& blobInfo : msg->blobs()) { - BEAST_EXPECT(msg->version() == expectedVersion); if (!BEAST_EXPECT(seqIter != expectedSeqs.end())) - continue; + break; auto const& expectedBlob = blobInfos.at(*seqIter); - BEAST_EXPECT((*seqIter < manifestCutoff) == !!expectedBlob.manifest); - auto const expectedManifest = - *seqIter < manifestCutoff && expectedBlob.manifest - ? *expectedBlob.manifest - : manifest; - BEAST_EXPECT(msg->manifest() == expectedManifest); - BEAST_EXPECT(msg->blob() == expectedBlob.blob); - BEAST_EXPECT(msg->signature() == expectedBlob.signature); + hashingBlobs.push_back(expectedBlob); + BEAST_EXPECT(blobInfo.has_manifest() == !!expectedBlob.manifest); + BEAST_EXPECT(blobInfo.has_manifest() == (*seqIter < manifestCutoff)); + + if (*seqIter < manifestCutoff) + BEAST_EXPECT(blobInfo.manifest() == *expectedBlob.manifest); + BEAST_EXPECT(blobInfo.blob() == expectedBlob.blob); + BEAST_EXPECT(blobInfo.signature() == expectedBlob.signature); ++seqIter; - BEAST_EXPECT(seqIter == expectedSeqs.end()); - - BEAST_EXPECT( - messageWithHash.hash == - sha512Half( - expectedManifest, - expectedBlob.blob, - expectedBlob.signature, - expectedVersion)); } + BEAST_EXPECT(seqIter == expectedSeqs.end()); } - else - { - std::vector hashingBlobs; - hashingBlobs.reserve(msgIter->second.size()); - - auto const msg = extractProtocolMessage2(*messageWithHash.message); - if (BEAST_EXPECT(msg)) - { - BEAST_EXPECT(msg->version() == version); - BEAST_EXPECT(msg->manifest() == manifest); - for (auto const& blobInfo : msg->blobs()) - { - if (!BEAST_EXPECT(seqIter != expectedSeqs.end())) - break; - auto const& expectedBlob = blobInfos.at(*seqIter); - hashingBlobs.push_back(expectedBlob); - BEAST_EXPECT(blobInfo.has_manifest() == !!expectedBlob.manifest); - BEAST_EXPECT( - blobInfo.has_manifest() == (*seqIter < manifestCutoff)); - - if (*seqIter < manifestCutoff) - BEAST_EXPECT(blobInfo.manifest() == *expectedBlob.manifest); - BEAST_EXPECT(blobInfo.blob() == expectedBlob.blob); - BEAST_EXPECT(blobInfo.signature() == expectedBlob.signature); - ++seqIter; - } - BEAST_EXPECT(seqIter == expectedSeqs.end()); - } - BEAST_EXPECT( - messageWithHash.hash == sha512Half(manifest, hashingBlobs, version)); - } - ++msgIter; + BEAST_EXPECT( + messageWithHash.hash == sha512Half(manifest, hashingBlobs, version)); } - BEAST_EXPECT(msgIter == expectedInfo.end()); - }; + ++msgIter; + } + BEAST_EXPECT(msgIter == expectedInfo.end()); + }; auto verifyBuildMessages = [this]( std::pair const& result, std::size_t expectedSequence, @@ -2471,66 +2410,10 @@ private: std::vector messages; - // Version 1 - - // This peer has a VL ahead of our "current" - verifyBuildMessages( - ValidatorList::buildValidatorListMessages( - 1, 8, maxSequence, version, manifest, blobInfos, messages), - 0, - 0); - BEAST_EXPECT(messages.empty()); - - // Don't repeat the work if messages is populated, even though the - // peerSequence provided indicates it should. Note that this - // situation is contrived for this test and should never happen in - // real code. - messages.emplace_back(); - verifyBuildMessages( - ValidatorList::buildValidatorListMessages( - 1, 3, maxSequence, version, manifest, blobInfos, messages), - 5, - 0); - BEAST_EXPECT(messages.size() == 1 && !messages.front().message); - - // Generate a version 1 message - messages.clear(); - verifyBuildMessages( - ValidatorList::buildValidatorListMessages( - 1, 3, maxSequence, version, manifest, blobInfos, messages), - 5, - 1); - if (BEAST_EXPECT(messages.size() == 1) && BEAST_EXPECT(messages.front().message)) - { - auto const& messageWithHash = messages.front(); - auto const msg = extractProtocolMessage1(*messageWithHash.message); - auto const size = - messageWithHash.message->getBuffer(compression::Compressed::Off).size(); - // This size is arbitrary, but shouldn't change - BEAST_EXPECT(size == 108); - auto const& expected = blobInfos.at(5); - if (BEAST_EXPECT(msg)) - { - BEAST_EXPECT(msg->version() == 1); - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - BEAST_EXPECT(msg->manifest() == *expected.manifest); - BEAST_EXPECT(msg->blob() == expected.blob); - BEAST_EXPECT(msg->signature() == expected.signature); - } - BEAST_EXPECT( - messageWithHash.hash == - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - sha512Half(*expected.manifest, expected.blob, expected.signature, 1)); - } - - // Version 2 - - messages.clear(); - // This peer has a VL ahead of us. verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, maxSequence * 2, maxSequence, version, manifest, blobInfos, messages), + maxSequence * 2, maxSequence, version, manifest, blobInfos, messages), 0, 0); BEAST_EXPECT(messages.empty()); @@ -2542,19 +2425,19 @@ private: messages.emplace_back(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 3, maxSequence, version, manifest, blobInfos, messages), + 3, maxSequence, version, manifest, blobInfos, messages), maxSequence, 0); BEAST_EXPECT(messages.size() == 1 && !messages.front().message); - // Generate a version 2 message. Don't send the current + // Generate a message. Don't send the current messages.clear(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 5, maxSequence, version, manifest, blobInfos, messages), + 5, maxSequence, version, manifest, blobInfos, messages), maxSequence, 4); - verifyMessage(version, manifest, blobInfos, messages, {{372, {6, 7, 10, 12}}}); + verifyMessage(version, manifest, blobInfos, messages, {{6, 7, 10, 12}}); // Test message splitting on size limits. @@ -2562,50 +2445,39 @@ private: messages.clear(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 5, maxSequence, version, manifest, blobInfos, messages, 300), + 5, maxSequence, version, manifest, blobInfos, messages, 300), maxSequence, 4); - verifyMessage(version, manifest, blobInfos, messages, {{212, {6, 7}}, {192, {10, 12}}}); + verifyMessage(version, manifest, blobInfos, messages, {{6, 7}, {10, 12}}); // Set a limit between the size of the two earlier messages so one // will split and the other won't messages.clear(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 5, maxSequence, version, manifest, blobInfos, messages, 200), + 5, maxSequence, version, manifest, blobInfos, messages, 200), maxSequence, 4); - verifyMessage( - version, manifest, blobInfos, messages, {{108, {6}}, {108, {7}}, {192, {10, 12}}}); + verifyMessage(version, manifest, blobInfos, messages, {{6}, {7}, {10, 12}}); // Set a limit so that all the VLs are sent individually messages.clear(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 5, maxSequence, version, manifest, blobInfos, messages, 150), + 5, maxSequence, version, manifest, blobInfos, messages, 150), maxSequence, 4); - verifyMessage( - version, - manifest, - blobInfos, - messages, - {{108, {6}}, {108, {7}}, {110, {10}}, {110, {12}}}); + verifyMessage(version, manifest, blobInfos, messages, {{6}, {7}, {10}, {12}}); // Set a limit smaller than some of the messages. Because single // messages send regardless, they will all still be sent messages.clear(); verifyBuildMessages( ValidatorList::buildValidatorListMessages( - 2, 5, maxSequence, version, manifest, blobInfos, messages, 108), + 5, maxSequence, version, manifest, blobInfos, messages, 108), maxSequence, 4); - verifyMessage( - version, - manifest, - blobInfos, - messages, - {{108, {6}}, {108, {7}}, {110, {10}}, {110, {12}}}); + verifyMessage(version, manifest, blobInfos, messages, {{6}, {7}, {10}, {12}}); } void diff --git a/src/test/overlay/ProtocolVersion_test.cpp b/src/test/overlay/ProtocolVersion_test.cpp index e31a574502..e7b63a34cb 100644 --- a/src/test/overlay/ProtocolVersion_test.cpp +++ b/src/test/overlay/ProtocolVersion_test.cpp @@ -33,22 +33,30 @@ public: void run() override { - testcase("Convert protocol version to string"); - BEAST_EXPECT(to_string(makeProtocol(1, 3)) == "XRPL/1.3"); - BEAST_EXPECT(to_string(makeProtocol(2, 0)) == "XRPL/2.0"); - BEAST_EXPECT(to_string(makeProtocol(2, 1)) == "XRPL/2.1"); - BEAST_EXPECT(to_string(makeProtocol(10, 10)) == "XRPL/10.10"); + { + testcase("Convert protocol version to string"); + + BEAST_EXPECT(to_string(makeProtocol(0, 0)) == "XRPL/0.0"); + BEAST_EXPECT(to_string(makeProtocol(0, 1)) == "XRPL/0.1"); + BEAST_EXPECT(to_string(makeProtocol(1, 3)) == "XRPL/1.3"); + BEAST_EXPECT(to_string(makeProtocol(2, 0)) == "XRPL/2.0"); + BEAST_EXPECT(to_string(makeProtocol(2, 1)) == "XRPL/2.1"); + BEAST_EXPECT(to_string(makeProtocol(10, 10)) == "XRPL/10.10"); + BEAST_EXPECT(to_string(makeProtocol(65535, 65535)) == "XRPL/65535.65535"); + } { testcase("Convert strings to protocol versions"); - // Empty string + // Invalid versions, either they do not parse as XRPL/N.M or are unsupported. check("", ""); + check("RTXP/1.1,RTXP/1.2,RTXP/1.3", ""); + check("XRPL/-2.1,XRPL/0.3,XRPL/2,XRPL/2.01,websocket", ""); - check("RTXP/1.1,RTXP/1.2,RTXP/1.3,XRPL/2.1,XRPL/2.0,/XRPL/3.0", "XRPL/2.0,XRPL/2.1"); - check("RTXP/0.9,RTXP/1.01,XRPL/0.3,XRPL/2.01,websocket", ""); + // Mixture of valid, duplicate, and invalid versions. + check("RTXP/1.3,XRPL/2.1,XRPL/2.0,/XRPL/3.0", "XRPL/2.0,XRPL/2.1"); check( - "XRPL/2.0,XRPL/2.0,XRPL/19.4,XRPL/7.89,XRPL/XRPL/3.0,XRPL/2.01", + "XRPL/2.0,XRPL/2.0,XRPL/19.4,XRPL/7.89,XRPL/XRPL/3.0,XRPL/2.01,XRPL/-65535.65535", "XRPL/2.0,XRPL/7.89,XRPL/19.4"); check( "XRPL/2.0,XRPL/3.0,XRPL/4,XRPL/,XRPL,OPT XRPL/2.2,XRPL/5.67", @@ -58,15 +66,17 @@ public: { testcase("Protocol version negotiation"); - BEAST_EXPECT(negotiateProtocolVersion("RTXP/1.2") == std::nullopt); + // Only the highest supported protocol version, if any, is returned. + BEAST_EXPECT(negotiateProtocolVersion("") == std::nullopt); + BEAST_EXPECT(negotiateProtocolVersion("XRPL/0.0") == std::nullopt); + BEAST_EXPECT(negotiateProtocolVersion("RTXP/1.2,XRPL/0.1") == std::nullopt); BEAST_EXPECT( - negotiateProtocolVersion("RTXP/1.2, XRPL/2.0, XRPL/2.1") == makeProtocol(2, 1)); + negotiateProtocolVersion("XRPL/999.999, XRPL/-2.2,WebSocket/1.0") == std::nullopt); BEAST_EXPECT(negotiateProtocolVersion("XRPL/2.2") == makeProtocol(2, 2)); BEAST_EXPECT( - negotiateProtocolVersion("RTXP/1.2, XRPL/2.3, XRPL/2.4, XRPL/999.999") == + negotiateProtocolVersion( + "RTXP/1.2, XRPL/2.1, XRPL/2.2, XRPL/2.3, XRPL/2.4, XRPL/999.999") == makeProtocol(2, 3)); - BEAST_EXPECT(negotiateProtocolVersion("XRPL/999.999, WebSocket/1.0") == std::nullopt); - BEAST_EXPECT(negotiateProtocolVersion("") == std::nullopt); } } }; diff --git a/src/test/overlay/compression_test.cpp b/src/test/overlay/compression_test.cpp index a583a3aeab..40dee96c75 100644 --- a/src/test/overlay/compression_test.cpp +++ b/src/test/overlay/compression_test.cpp @@ -292,33 +292,6 @@ public: return getObject; } - static std::shared_ptr - buildValidatorList() - { - auto list = std::make_shared(); - - auto master = randomKeyPair(KeyType::Ed25519); - auto signing = randomKeyPair(KeyType::Ed25519); - STObject st(sfGeneric); - st[sfSequence] = 0; - st[sfPublicKey] = std::get<0>(master); - st[sfSigningPubKey] = std::get<0>(signing); - st[sfDomain] = makeSlice(std::string("example.com")); - sign(st, HashPrefix::Manifest, KeyType::Ed25519, std::get<1>(master), sfMasterSignature); - sign(st, HashPrefix::Manifest, KeyType::Ed25519, std::get<1>(signing)); - Serializer s; - st.add(s); - list->set_manifest(s.data(), s.size()); - list->set_version(3); - STObject const signature(sfSignature); - xrpl::sign(st, HashPrefix::Manifest, KeyType::Ed25519, std::get<1>(signing)); - Serializer s1; - st.add(s1); - list->set_signature(s1.data(), s1.size()); - list->set_blob(strHex(s.slice())); - return list; - } - static std::shared_ptr buildValidatorListCollection() { @@ -359,7 +332,6 @@ public: protocol::TMGetLedger const getLedger; protocol::TMLedgerData const ledgerData; protocol::TMGetObjectByHash const getObject; - protocol::TMValidatorList const validatorList; protocol::TMValidatorListCollection const validatorListCollection; // 4.5KB @@ -386,8 +358,6 @@ public: doTest(buildLedgerData(500000, *logs), protocol::mtLEDGER_DATA, 100, "TMLedgerData500000"); // 7.7KB doTest(buildGetObjectByHash(), protocol::mtGET_OBJECTS, 4, "TMGetObjectByHash"); - // 895B - doTest(buildValidatorList(), protocol::mtVALIDATOR_LIST, 4, "TMValidatorList"); doTest( buildValidatorListCollection(), protocol::mtVALIDATOR_LIST_COLLECTION, diff --git a/src/xrpld/app/misc/ValidatorList.h b/src/xrpld/app/misc/ValidatorList.h index abec6cf4e0..4e001affe8 100644 --- a/src/xrpld/app/misc/ValidatorList.h +++ b/src/xrpld/app/misc/ValidatorList.h @@ -30,7 +30,6 @@ #include namespace protocol { -class TMValidatorList; class TMValidatorListCollection; } // namespace protocol @@ -371,9 +370,6 @@ public: static std::vector parseBlobs(std::uint32_t version, json::Value const& body); - static std::vector - parseBlobs(protocol::TMValidatorList const& body); - static std::vector parseBlobs(protocol::TMValidatorListCollection const& body); @@ -391,7 +387,6 @@ public: [[nodiscard]] static std::pair buildValidatorListMessages( - std::size_t messageVersion, std::uint64_t peerSequence, std::size_t maxSequence, std::uint32_t rawVersion, @@ -987,14 +982,6 @@ hash_append(Hasher& h, std::map const& blobs) namespace protocol { -template -void -hash_append(Hasher& h, TMValidatorList const& msg) -{ - using beast::hash_append; - hash_append(h, msg.manifest(), msg.blob(), msg.signature(), msg.version()); -} - template void hash_append(Hasher& h, TMValidatorListCollection const& msg) diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index 0ada8ed55f..f099ebf059 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -449,13 +449,6 @@ ValidatorList::parseBlobs(std::uint32_t version, json::Value const& body) } } -// static -std::vector -ValidatorList::parseBlobs(protocol::TMValidatorList const& body) -{ - return {{.blob = body.blob(), .signature = body.signature(), .manifest = {}}}; -} - // static std::vector ValidatorList::parseBlobs(protocol::TMValidatorListCollection const& body) @@ -476,7 +469,7 @@ ValidatorList::parseBlobs(protocol::TMValidatorListCollection const& body) } XRPL_ASSERT( result.size() == body.blobs_size(), - "xrpl::ValidatorList::parseBlobs(TMValidatorList) : result size " + "xrpl::ValidatorList::parseBlobs(TMValidatorListCollection) : result size " "match"); return result; } @@ -520,29 +513,6 @@ splitMessageParts( { if (end <= begin) return 0; - if (end - begin == 1) - { - protocol::TMValidatorList smallMsg; - smallMsg.set_version(1); - smallMsg.set_manifest(largeMsg.manifest()); - - auto const& blob = largeMsg.blobs(begin); - smallMsg.set_blob(blob.blob()); - smallMsg.set_signature(blob.signature()); - // This is only possible if "downgrading" a v2 UNL to v1. - if (blob.has_manifest()) - smallMsg.set_manifest(blob.manifest()); - - XRPL_ASSERT( - Message::totalSize(smallMsg) <= kMaximumMessageSize, - "xrpl::splitMessageParts : maximum message size"); - - messages.emplace_back( - std::make_shared(smallMsg, protocol::mtVALIDATOR_LIST), - sha512Half(smallMsg), - 1); - return messages.back().numVLs; - } std::optional smallMsg; smallMsg.emplace(); @@ -554,13 +524,29 @@ splitMessageParts( *smallMsg->add_blobs() = largeMsg.blobs(i); } - if (Message::totalSize(*smallMsg) > maxSize) + auto const size = Message::totalSize(*smallMsg); + + // Split until each message fits, but a single blob can't be split any + // further, so stop recursing at that point regardless of maxSize. + if (size > maxSize && end - begin > 1) { // free up the message space smallMsg.reset(); return splitMessage(messages, largeMsg, maxSize, begin, end); } + // An unsplittable blob is still bounded by the protocol limit: peers drop + // messages exceeding it on receipt, so don't waste the bandwidth. maxSize + // only ever tightens this (it defaults to kMaximumMessageSize), so a blob + // reaching here can exceed maxSize but never the protocol limit. + if (size > kMaximumMessageSize) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::splitMessageParts : maximum message size exceeded"); + return 0; + // LCOV_EXCL_STOP + } + messages.emplace_back( std::make_shared(*smallMsg, protocol::mtVALIDATOR_LIST_COLLECTION), sha512Half(*smallMsg), @@ -568,37 +554,6 @@ splitMessageParts( return messages.back().numVLs; } -// Build a v1 protocol message using only the current VL -std::size_t -buildValidatorListMessage( - std::vector& messages, - std::uint32_t rawVersion, - std::string const& rawManifest, - ValidatorBlobInfo const& currentBlob, - std::size_t maxSize) -{ - XRPL_ASSERT( - messages.empty(), - "xrpl::buildValidatorListMessage(ValidatorBlobInfo) : empty messages " - "input"); - protocol::TMValidatorList msg; - auto const manifest = currentBlob.manifest ? *currentBlob.manifest : rawManifest; - auto const version = 1; - msg.set_manifest(manifest); - msg.set_blob(currentBlob.blob); - msg.set_signature(currentBlob.signature); - // Override the version - msg.set_version(version); - - XRPL_ASSERT( - Message::totalSize(msg) <= kMaximumMessageSize, - "xrpl::buildValidatorListMessage(ValidatorBlobInfo) : maximum " - "message size"); - messages.emplace_back( - std::make_shared(msg, protocol::mtVALIDATOR_LIST), sha512Half(msg), 1); - return 1; -} - // Build a v2 protocol message using all the VLs with sequence larger than the // peer's std::size_t @@ -650,7 +605,6 @@ buildValidatorListMessage( // static std::pair ValidatorList::buildValidatorListMessages( - std::size_t messageVersion, std::uint64_t peerSequence, std::size_t maxSequence, std::uint32_t rawVersion, @@ -663,14 +617,12 @@ ValidatorList::buildValidatorListMessages( !blobInfos.empty(), "xrpl::ValidatorList::buildValidatorListMessages : empty messages " "input"); - auto const& [currentSeq, currentBlob] = *blobInfos.begin(); auto numVLs = std::accumulate( messages.begin(), messages.end(), 0, [](std::size_t total, MessageWithHash const& m) { return total + m.numVLs; }); - if (messageVersion == 2 && peerSequence < maxSequence) + if (peerSequence < maxSequence) { - // Version 2 if (messages.empty()) { numVLs = buildValidatorListMessage( @@ -678,36 +630,13 @@ ValidatorList::buildValidatorListMessages( if (messages.empty()) { // No message was generated. Create an empty placeholder so we - // dont' repeat the work later. + // don't repeat the work later. messages.emplace_back(); } } - // Don't send it next time. return {maxSequence, numVLs}; } - if (messageVersion == 1 && peerSequence < currentSeq) - { - // Version 1 - if (messages.empty()) - { - numVLs = buildValidatorListMessage( - messages, - rawVersion, - currentBlob.manifest ? *currentBlob.manifest : rawManifest, - currentBlob, - maxSize); - if (messages.empty()) - { - // No message was generated. Create an empty placeholder so we - // dont' repeat the work later. - messages.emplace_back(); - } - } - - // Don't send it next time. - return {currentSeq, numVLs}; - } return {0, 0}; } @@ -725,19 +654,8 @@ ValidatorList::sendValidatorList( HashRouter& hashRouter, beast::Journal j) { - std::size_t messageVersion = 0; - if (peer.supportsFeature(ProtocolFeature::ValidatorList2Propagation)) - { - messageVersion = 2; - } - else if (peer.supportsFeature(ProtocolFeature::ValidatorListPropagation)) - { - messageVersion = 1; - } - if (messageVersion == 0u) - return; auto const [newPeerSequence, numVLs] = buildValidatorListMessages( - messageVersion, peerSequence, maxSequence, rawVersion, rawManifest, blobInfos, messages); + peerSequence, maxSequence, rawVersion, rawManifest, blobInfos, messages); if (newPeerSequence != 0u) { XRPL_ASSERT( @@ -764,24 +682,11 @@ ValidatorList::sendValidatorList( "xrpl::ValidatorList::sendValidatorList : sent or one message"); if (sent) { - if (messageVersion > 1) - { - JLOG(j.debug()) << "Sent " << messages.size() - << " validator list collection(s) containing " << numVLs - << " validator list(s) for " << strHex(publisherKey) - << " with sequence range " << peerSequence << ", " - << newPeerSequence << " to " << peer.fingerprint(); - } - else - { - XRPL_ASSERT( - numVLs == 1, - "xrpl::ValidatorList::sendValidatorList : one validator " - "list"); - JLOG(j.debug()) << "Sent validator list for " << strHex(publisherKey) - << " with sequence " << newPeerSequence << " to " - << peer.fingerprint(); - } + JLOG(j.debug()) << "Sent " << messages.size() + << " validator list collection(s) containing " << numVLs + << " validator list(s) for " << strHex(publisherKey) + << " with sequence range " << peerSequence << ", " << newPeerSequence + << " to " << peer.fingerprint(); } } } @@ -856,16 +761,9 @@ ValidatorList::broadcastBlobs( if (toSkip) { - // We don't know what messages or message versions we're sending - // until we examine our peer's properties. Build the message(s) on - // demand, but reuse them when possible. - - // This will hold a v1 message with only the current VL if we have - // any peers that don't support v2 - std::vector messages1; - // This will hold v2 messages indexed by the peer's - // `publisherListSequence`. For each `publisherListSequence`, we'll - // only send the VLs with higher sequences. + // Build v2 messages on demand and reuse them when possible. Messages + // are indexed by the peer's `publisherListSequence`; for each sequence, + // we only send VLs with higher sequences. std::map> messages2; // If any peers are found that are worth considering, this list will // be built to hold info for all of the valid VLs. @@ -885,8 +783,6 @@ ValidatorList::broadcastBlobs( { if (blobInfos.empty()) buildBlobInfos(blobInfos, lists); - auto const v2 = - peer->supportsFeature(ProtocolFeature::ValidatorList2Propagation); sendValidatorList( *peer, peerSequence, @@ -895,11 +791,10 @@ ValidatorList::broadcastBlobs( lists.rawVersion, lists.rawManifest, blobInfos, - v2 ? messages2[peerSequence] : messages1, + messages2[peerSequence], hashRouter, j); - // Even if the peer doesn't support the messages, - // suppress it so it'll be ignored next time. + // Don't send it next time. hashRouter.addSuppressionPeer(hash, peer->id()); } } diff --git a/src/xrpld/overlay/Peer.h b/src/xrpld/overlay/Peer.h index 87750ed40e..6c4cf1dff1 100644 --- a/src/xrpld/overlay/Peer.h +++ b/src/xrpld/overlay/Peer.h @@ -20,8 +20,6 @@ class Charge; } // namespace resource enum class ProtocolFeature { - ValidatorListPropagation, - ValidatorList2Propagation, LedgerReplay, LedgerNodeDepth, }; diff --git a/src/xrpld/overlay/detail/Message.cpp b/src/xrpld/overlay/detail/Message.cpp index c6e0511515..a6af525620 100644 --- a/src/xrpld/overlay/detail/Message.cpp +++ b/src/xrpld/overlay/detail/Message.cpp @@ -82,7 +82,6 @@ Message::compress() case protocol::mtGET_LEDGER: case protocol::mtLEDGER_DATA: case protocol::mtGET_OBJECTS: - case protocol::mtVALIDATOR_LIST: case protocol::mtVALIDATOR_LIST_COLLECTION: case protocol::mtREPLAY_DELTA_RESPONSE: case protocol::mtTRANSACTIONS: diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 726002fce4..3f0b4453b8 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -542,10 +542,6 @@ PeerImp::supportsFeature(ProtocolFeature f) const { switch (f) { - case ProtocolFeature::ValidatorListPropagation: - return protocol_ >= makeProtocol(2, 1); - case ProtocolFeature::ValidatorList2Propagation: - return protocol_ >= makeProtocol(2, 2); case ProtocolFeature::LedgerNodeDepth: return protocol_ >= makeProtocol(2, 3); case ProtocolFeature::LedgerReplay: @@ -885,7 +881,7 @@ PeerImp::doProtocolStart() onReadMessage(error_code(), 0); // Send all the validator lists that have been loaded - if (inbound_ && supportsFeature(ProtocolFeature::ValidatorListPropagation)) + if (inbound_) { app_.getValidators().forEachAvailable( [&](std::string const& manifest, @@ -2422,43 +2418,11 @@ PeerImp::onValidatorListMessage( } } -void -PeerImp::onMessage(std::shared_ptr const& m) -{ - try - { - if (!supportsFeature(ProtocolFeature::ValidatorListPropagation)) - { - JLOG(pJournal_.debug()) << "ValidatorList: received validator list from peer using " - << "protocol version " << to_string(protocol_) - << " which shouldn't support this feature."; - fee_.update(resource::kFeeUselessData, "unsupported peer"); - return; - } - onValidatorListMessage( - "ValidatorList", m->manifest(), m->version(), ValidatorList::parseBlobs(*m)); - } - catch (std::exception const& e) - { - JLOG(pJournal_.warn()) << "ValidatorList: Exception, " << e.what(); - using namespace std::string_literals; - fee_.update(resource::kFeeInvalidData, e.what()); - } -} - void PeerImp::onMessage(std::shared_ptr const& m) { try { - if (!supportsFeature(ProtocolFeature::ValidatorList2Propagation)) - { - JLOG(pJournal_.debug()) << "ValidatorListCollection: received validator list from peer " - << "using protocol version " << to_string(protocol_) - << " which shouldn't support this feature."; - fee_.update(resource::kFeeUselessData, "unsupported peer"); - return; - } if (m->version() < 2) { JLOG(pJournal_.debug()) diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index 7078d6fb56..0f229bf9d8 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -623,8 +623,6 @@ public: void onMessage(std::shared_ptr const& m); void - onMessage(std::shared_ptr const& m); - void onMessage(std::shared_ptr const& m); void onMessage(std::shared_ptr const& m); diff --git a/src/xrpld/overlay/detail/ProtocolMessage.h b/src/xrpld/overlay/detail/ProtocolMessage.h index f7d5e26272..88f50e1e2e 100644 --- a/src/xrpld/overlay/detail/ProtocolMessage.h +++ b/src/xrpld/overlay/detail/ProtocolMessage.h @@ -71,8 +71,6 @@ protocolMessageName(int type) return "status"; case protocol::mtHAVE_SET: return "have_set"; - case protocol::mtVALIDATOR_LIST: - return "validator_list"; case protocol::mtVALIDATOR_LIST_COLLECTION: return "validator_list_collection"; case protocol::mtVALIDATION: @@ -424,9 +422,6 @@ invokeProtocolMessage(Buffers const& buffers, Handler& handler, std::size_t& hin case protocol::mtVALIDATION: success = detail::invoke(*header, buffers, handler); break; - case protocol::mtVALIDATOR_LIST: - success = detail::invoke(*header, buffers, handler); - break; case protocol::mtVALIDATOR_LIST_COLLECTION: success = detail::invoke(*header, buffers, handler); diff --git a/src/xrpld/overlay/detail/ProtocolVersion.cpp b/src/xrpld/overlay/detail/ProtocolVersion.cpp index 93d4fae156..1296041ad5 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.cpp +++ b/src/xrpld/overlay/detail/ProtocolVersion.cpp @@ -28,7 +28,6 @@ namespace xrpl { */ constexpr ProtocolVersion const kSupportedProtocolList[]{ - {2, 1}, {2, 2}, {2, 3}, }; diff --git a/src/xrpld/overlay/detail/TrafficCount.cpp b/src/xrpld/overlay/detail/TrafficCount.cpp index bdce9e68f0..90d5c0b4ff 100644 --- a/src/xrpld/overlay/detail/TrafficCount.cpp +++ b/src/xrpld/overlay/detail/TrafficCount.cpp @@ -14,7 +14,6 @@ std::unordered_map const kTypeLoo {protocol::mtMANIFESTS, TrafficCount::Category::Manifests}, {protocol::mtENDPOINTS, TrafficCount::Category::Overlay}, {protocol::mtTRANSACTION, TrafficCount::Category::Transaction}, - {protocol::mtVALIDATOR_LIST, TrafficCount::Category::Validatorlist}, {protocol::mtVALIDATOR_LIST_COLLECTION, TrafficCount::Category::Validatorlist}, {protocol::mtVALIDATION, TrafficCount::Category::Validation}, {protocol::mtPROPOSE_LEDGER, TrafficCount::Category::Proposal}, From 43d842926a8c7a154062a90d389e526be3e89d2e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 14 Aug 2026 20:18:33 +0000 Subject: [PATCH 05/13] refactor: Rewrite Transactor::operator() to early return (#8003) --- src/libxrpl/tx/Transactor.cpp | 136 ++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 64 deletions(-) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 5fc6942e20..594aa24940 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -1637,85 +1638,92 @@ Transactor::operator()() if (auto stream = j_.trace()) stream << "preclaim result: " << transToken(result); - bool applied = isTesSuccess(result); auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); + bool const canApply = std::invoke([&result, &fee, this] { + bool canApplyTmp = isTesSuccess(result); - if (ctx_.size() > kOversizeMetaDataCap) - result = tecOVERSIZE; + if (ctx_.size() > kOversizeMetaDataCap) + result = tecOVERSIZE; - if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) - { - // If the TapFailHard flag is set, a tec result - // must not do anything - ctx_.discard(); - applied = false; - } - else if ( - (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || - (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) - { - std::tie(result, fee, applied) = processPersistentChanges(result, fee); - } - - if (applied) - { - // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can - // proceed to apply the tx - result = checkInvariants(result, fee); - if (result == tecINVARIANT_FAILED) + if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) { - // Reset to fee-claim only - auto const resetResult = reset(fee); - if (!isTesSuccess(resetResult.first)) - result = resetResult.first; - - fee = resetResult.second; - - // Check invariants again to ensure the fee claiming doesn't violate - // invariants. After reset, only protocol invariants are re-checked. - // Transaction invariants are not meaningful here — the transaction's - // effects have been rolled back. - if (isTesSuccess(result) || isTecClaim(result)) - result = ctx_.checkInvariants(result, fee); + // If the TapFailHard flag is set, a tec result + // must not do anything + ctx_.discard(); + canApplyTmp = false; } + else if ( + (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || + (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) + { + // This is and must remain the only place where `canApplyTmp` can change from false to + // true. Changing from true to false is no problem. + std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); + } + return canApplyTmp; + }); - // We ran through the invariant checker, which can, in some cases, - // return a tef error code. Don't apply the transaction in that case. - if (!isTecClaim(result) && !isTesSuccess(result)) - applied = false; + auto const logger = [this]( + TER result, + bool canApply, + std::optional&& metadata = std::nullopt) -> ApplyResult { + JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); + return {result, canApply, std::move(metadata)}; + }; + + if (!canApply) + return logger(result, canApply); + + // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can + // proceed to apply the tx + result = checkInvariants(result, fee); + if (result == tecINVARIANT_FAILED) + { + // Reset to fee-claim only + auto const resetResult = reset(fee); + if (!isTesSuccess(resetResult.first)) + result = resetResult.first; + + fee = resetResult.second; + + // Check invariants again to ensure the fee claiming doesn't violate + // invariants. After reset, only protocol invariants are re-checked. + // Transaction invariants are not meaningful here — the transaction's + // effects have been rolled back. + if (isTesSuccess(result) || isTecClaim(result)) + result = ctx_.checkInvariants(result, fee); } + // We ran through the invariant checker, which can, in some cases, + // return a tef error code. Don't apply the transaction in that case. + if (!isTecClaim(result) && !isTesSuccess(result)) + return logger(result, false); + std::optional metadata; - if (applied) - { - // Transaction succeeded fully or (retries are not allowed and the - // transaction could claim a fee) - // The transactor and invariant checkers guarantee that this will - // *never* trigger but if it, somehow, happens, don't allow a tx - // that charges a negative fee. - if (fee < beast::kZero) - Throw("fee charged is negative!"); + // Transaction succeeded fully or (retries are not allowed and the + // transaction could claim a fee) - // Charge whatever fee they specified. The fee has already been - // deducted from the balance of the account that issued the - // transaction. We just need to account for it in the ledger - // header. - if (!view().open() && fee != beast::kZero) - ctx_.destroyXRP(fee); + // The transactor and invariant checkers guarantee that this will + // *never* trigger but if it, somehow, happens, don't allow a tx + // that charges a negative fee. + if (fee < beast::kZero) + Throw("fee charged is negative!"); - // Once we call apply, we will no longer be able to look at view() - metadata = ctx_.apply(result); - } + // Charge whatever fee they specified. The fee has already been + // deducted from the balance of the account that issued the + // transaction. We just need to account for it in the ledger + // header. + if (!view().open() && fee != beast::kZero) + ctx_.destroyXRP(fee); + + // Once we call apply, we will no longer be able to look at view() + metadata = ctx_.apply(result); if ((ctx_.flags() & TapDryRun) != 0u) - { - applied = false; - } + return logger(result, false, std::move(metadata)); - JLOG(j_.trace()) << (applied ? "applied " : "not applied ") << transToken(result); - - return {result, applied, metadata}; + return logger(result, canApply, std::move(metadata)); } } // namespace xrpl From 5337d028a2559bd75ec46b60b7a5539487d18d0a Mon Sep 17 00:00:00 2001 From: Bart Date: Mon, 17 Aug 2026 10:07:14 +0000 Subject: [PATCH 06/13] refactor: Use unsigned int for branch-related operations (#7938) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 --- include/xrpl/shamap/SHAMap.h | 35 +++++---- include/xrpl/shamap/SHAMapInnerNode.h | 26 +++---- include/xrpl/shamap/SHAMapNodeID.h | 4 +- include/xrpl/shamap/detail/TaggedPointer.h | 12 +-- include/xrpl/shamap/detail/TaggedPointer.ipp | 59 +++++++------- src/libxrpl/shamap/SHAMap.cpp | 77 +++++++++---------- src/libxrpl/shamap/SHAMapDelta.cpp | 22 +++--- src/libxrpl/shamap/SHAMapInnerNode.cpp | 69 ++++++++--------- src/libxrpl/shamap/SHAMapNodeID.cpp | 15 ++-- src/libxrpl/shamap/SHAMapSync.cpp | 58 ++++++++------ .../app/ledger/detail/LedgerNodeHelpers.cpp | 2 +- 11 files changed, 195 insertions(+), 184 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 97ab2e9f7a..05de33ddf3 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -484,31 +484,36 @@ private: // returns the first item at or below this node SHAMapLeafNode* - firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch = 0) const; + firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch = 0u) const; // returns the last item at or below this node SHAMapLeafNode* - lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch = kBranchFactor) const; + lastBelow( + SHAMapTreeNodePtr node, + SharedPtrNodeStack& stack, + unsigned int branch = kBranchFactor) const; + + // direction in which belowHelper scans an inner node's branches + enum class BelowDirection { First, Last }; // helper function for firstBelow and lastBelow SHAMapLeafNode* belowHelper( SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, - int branch, - std::tuple, std::function> const& loopParams) - const; + unsigned int branch, + BelowDirection direction) const; // Simple descent // Get a child of the specified node SHAMapTreeNode* - descend(SHAMapInnerNode*, int branch) const; + descend(SHAMapInnerNode*, unsigned int branch) const; SHAMapTreeNode* - descendThrow(SHAMapInnerNode*, int branch) const; + descendThrow(SHAMapInnerNode*, unsigned int branch) const; SHAMapTreeNodePtr - descend(SHAMapInnerNode&, int branch) const; + descend(SHAMapInnerNode&, unsigned int branch) const; SHAMapTreeNodePtr - descendThrow(SHAMapInnerNode&, int branch) const; + descendThrow(SHAMapInnerNode&, unsigned int branch) const; // Descend with filter // If pending, callback is called as if it called fetchNodeNT @@ -516,7 +521,7 @@ private: SHAMapTreeNode* descendAsync( SHAMapInnerNode* parent, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter, bool& pending, descendCallback&&) const; @@ -525,13 +530,13 @@ private: descend( SHAMapInnerNode* parent, SHAMapNodeID const& parentID, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter) const; // Non-storing // Does not hook the returned node to its parent SHAMapTreeNodePtr - descendNoStore(SHAMapInnerNode&, int branch) const; + descendNoStore(SHAMapInnerNode&, unsigned int branch) const; /** * If there is only one leaf below this node, get its contents @@ -581,8 +586,8 @@ private: using StackEntry = std::tuple< SHAMapInnerNode*, // pointer to the node SHAMapNodeID, // the node's ID - int, // while child we check first - int, // which child we check next + unsigned int, // which child we check first + unsigned int, // which child we check next bool>; // whether we've found any missing children yet // We explicitly choose to specify the use of std::deque here, because @@ -596,7 +601,7 @@ private: using DeferredNode = std::tuple< SHAMapInnerNode*, // parent node SHAMapNodeID, // parent node ID - int, // branch + unsigned int, // branch SHAMapTreeNodePtr>; // node int deferred; diff --git a/include/xrpl/shamap/SHAMapInnerNode.h b/include/xrpl/shamap/SHAMapInnerNode.h index 44d3bd6279..83d039172f 100644 --- a/include/xrpl/shamap/SHAMapInnerNode.h +++ b/include/xrpl/shamap/SHAMapInnerNode.h @@ -62,8 +62,8 @@ private: * * @param i index of the requested child */ - std::optional - getChildIndex(int i) const; + std::optional + getChildIndex(unsigned int i) const; /** * Call the `f` callback for all 16 (branchFactor) branches - even if @@ -125,28 +125,28 @@ public: isEmpty() const; bool - isEmptyBranch(int m) const; + isEmptyBranch(unsigned int branch) const; - int + unsigned int getBranchCount() const; SHAMapHash const& - getChildHash(int m) const; + getChildHash(unsigned int branch) const; void - setChild(int m, SHAMapTreeNodePtr child); + setChild(unsigned int branch, SHAMapTreeNodePtr child); void - shareChild(int m, SHAMapTreeNodePtr const& child); + shareChild(unsigned int branch, SHAMapTreeNodePtr const& child); SHAMapTreeNode* - getChildPointer(int branch); + getChildPointer(unsigned int branch); SHAMapTreeNodePtr - getChild(int branch); + getChild(unsigned int branch); SHAMapTreeNodePtr - canonicalizeChild(int branch, SHAMapTreeNodePtr node); + canonicalizeChild(unsigned int branch, SHAMapTreeNodePtr node); // sync functions bool @@ -190,12 +190,12 @@ SHAMapInnerNode::isEmpty() const } inline bool -SHAMapInnerNode::isEmptyBranch(int m) const +SHAMapInnerNode::isEmptyBranch(unsigned int branch) const { - return (isBranch_ & (1 << m)) == 0; + return (isBranch_ & (1u << branch)) == 0u; } -inline int +inline unsigned int SHAMapInnerNode::getBranchCount() const { return popcnt16(isBranch_); diff --git a/include/xrpl/shamap/SHAMapNodeID.h b/include/xrpl/shamap/SHAMapNodeID.h index 1189304aa7..fcd5a4d00e 100644 --- a/include/xrpl/shamap/SHAMapNodeID.h +++ b/include/xrpl/shamap/SHAMapNodeID.h @@ -53,7 +53,7 @@ public: } [[nodiscard]] SHAMapNodeID - getChildNodeID(unsigned int m) const; + getChildNodeID(unsigned int branch) const; /** * Create a SHAMapNodeID of a node with the depth of the node and @@ -64,7 +64,7 @@ public: * @return SHAMapNodeID of the node */ static SHAMapNodeID - createID(int depth, uint256 const& key); + createID(unsigned int depth, uint256 const& key); /** * Comparison operators diff --git a/include/xrpl/shamap/detail/TaggedPointer.h b/include/xrpl/shamap/detail/TaggedPointer.h index 509e6cc58d..705681be1d 100644 --- a/include/xrpl/shamap/detail/TaggedPointer.h +++ b/include/xrpl/shamap/detail/TaggedPointer.h @@ -219,11 +219,11 @@ public: * * @param i index of the requested child */ - [[nodiscard]] std::optional - getChildIndex(std::uint16_t isBranch, int i) const; + [[nodiscard]] std::optional + getChildIndex(std::uint16_t isBranch, unsigned int i) const; }; -[[nodiscard]] inline int +[[nodiscard]] inline unsigned int popcnt16(std::uint16_t a) { #if __cpp_lib_bitops @@ -234,11 +234,11 @@ popcnt16(std::uint16_t a) // fallback to table lookup static constexpr auto tbl = []() { std::array ret{}; - for (int i = 0; i != 256; ++i) + for (auto i = 0u; i != 256u; ++i) { - for (int j = 0; j != 8; ++j) + for (auto j = 0u; j != 8u; ++j) { - if (i & (1 << j)) + if (i & (1u << j)) ret[i]++; } } diff --git a/include/xrpl/shamap/detail/TaggedPointer.ipp b/include/xrpl/shamap/detail/TaggedPointer.ipp index 9275f3d15a..7db101b3cb 100644 --- a/include/xrpl/shamap/detail/TaggedPointer.ipp +++ b/include/xrpl/shamap/detail/TaggedPointer.ipp @@ -22,6 +22,11 @@ static_assert( static_assert( kBoundaries.back() == SHAMapInnerNode::kBranchFactor, "Last element of boundaries must be number of children in a dense array"); +static_assert( + kBoundaries.front() >= 1, + "TaggedPointer.ipp subtracts 1 from a numAllocated value derived from " + "kBoundaries, as an unsigned quantity, in several places; the smallest " + "boundary must stay non-zero or those subtractions underflow."); // Terminology: A chunk is the memory being allocated from a block. A block // contains multiple chunks. This is the terminology the boost documentation @@ -148,16 +153,16 @@ TaggedPointer::iterChildren(std::uint16_t isBranch, F&& f) const if (numAllocated == SHAMapInnerNode::kBranchFactor) { // dense case - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) f(hashes[i]); } else { // sparse case - int curHashI = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto curHashI = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(hashes[curHashI++]); } @@ -176,9 +181,9 @@ TaggedPointer::iterNonEmptyChildIndexes(std::uint16_t isBranch, F&& f) const if (capacity() == SHAMapInnerNode::kBranchFactor) { // dense case - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(i, i); } @@ -187,10 +192,10 @@ TaggedPointer::iterNonEmptyChildIndexes(std::uint16_t isBranch, F&& f) const else { // sparse case - int curHashI = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto curHashI = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(i, curHashI++); } @@ -216,14 +221,14 @@ TaggedPointer::destroyHashesAndChildren() deallocateArrays(tag, ptr); } -inline std::optional -TaggedPointer::getChildIndex(std::uint16_t isBranch, int i) const +inline std::optional +TaggedPointer::getChildIndex(std::uint16_t isBranch, unsigned int i) const { if (isDense()) return i; // Sparse case - if ((isBranch & (1 << i)) == 0) + if ((isBranch & (1u << i)) == 0u) { // Empty branch. Sparse children do not store empty branches return {}; @@ -273,10 +278,10 @@ inline TaggedPointer::TaggedPointer( *this = std::move(other); auto [srcDstNumAllocated, srcDstHashes, srcDstChildren] = getHashesAndChildren(); bool const srcDstIsDense = isDense(); - int srcDstIndex = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto srcDstIndex = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - auto const mask = (1 << i); + auto const mask = (1u << i); bool const inSrc = (srcBranches & mask) != 0; bool const inDst = (dstBranches & mask) != 0; if (inSrc && inDst) @@ -298,13 +303,13 @@ inline TaggedPointer::TaggedPointer( // sparse // need to shift all the elements to the left by // one - for (int c = srcDstIndex; c < srcDstNumAllocated - 1; ++c) + for (auto c = srcDstIndex; c + 1 < srcDstNumAllocated; ++c) { srcDstHashes[c] = srcDstHashes[c + 1]; srcDstChildren[c] = std::move(srcDstChildren[c + 1]); } - srcDstHashes[srcDstNumAllocated - 1].zero(); - srcDstChildren[srcDstNumAllocated - 1].reset(); + srcDstHashes[srcDstNumAllocated - 1u].zero(); + srcDstChildren[srcDstNumAllocated - 1u].reset(); // do not increment the index } } @@ -321,7 +326,7 @@ inline TaggedPointer::TaggedPointer( // sparse // need to create a hole by shifting all the elements to the // right by one - for (int c = srcDstNumAllocated - 1; c > srcDstIndex; --c) + for (auto c = srcDstNumAllocated - 1u; c > srcDstIndex; --c) { srcDstHashes[c] = srcDstHashes[c - 1]; srcDstChildren[c] = std::move(srcDstChildren[c - 1]); @@ -352,10 +357,10 @@ inline TaggedPointer::TaggedPointer( auto [srcNumAllocated, srcHashes, srcChildren] = src.getHashesAndChildren(); bool const srcIsDense = src.isDense(); bool const dstIsDense = dst.isDense(); - int srcIndex = 0, dstIndex = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto srcIndex = 0u, dstIndex = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - auto const mask = (1 << i); + auto const mask = (1u << i); bool const inSrc = (srcBranches & mask) != 0; bool const inDst = (dstBranches & mask) != 0; if (inSrc && inDst) @@ -409,7 +414,7 @@ inline TaggedPointer::TaggedPointer( !dstIsDense || dstIndex == dstNumAllocated, "xrpl::TaggedPointer::TaggedPointer(TaggedPointer&& ...) : " "non-sparse or valid sparse"); - for (int i = dstIndex; i < dstNumAllocated; ++i) + for (auto i = dstIndex; i < dstNumAllocated; ++i) { new (&dstHashes[i]) SHAMapHash{}; new (&dstChildren[i]) SHAMapTreeNodePtr{}; @@ -448,9 +453,9 @@ inline TaggedPointer::TaggedPointer( new (&newChildren[branchNum]) SHAMapTreeNodePtr{std::move(oldChildren[indexNum])}; }); // Run the constructors for the remaining elements - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if (((1 << i) & isBranch) != 0) + if (((1u << i) & isBranch) != 0u) continue; new (&newHashes[i]) SHAMapHash{}; new (&newChildren[i]) SHAMapTreeNodePtr{}; @@ -459,7 +464,7 @@ inline TaggedPointer::TaggedPointer( else { // new arrays are sparse, old arrays may be sparse or dense - int curCompressedIndex = 0; + auto curCompressedIndex = 0u; iterNonEmptyChildIndexes(isBranch, [&](auto branchNum, auto indexNum) { new (&newHashes[curCompressedIndex]) SHAMapHash{oldHashes[indexNum]}; new (&newChildren[curCompressedIndex]) @@ -467,7 +472,7 @@ inline TaggedPointer::TaggedPointer( ++curCompressedIndex; }); // Run the constructors for the remaining elements - for (int i = curCompressedIndex; i < newNumAllocated; ++i) + for (auto i = curCompressedIndex; i < newNumAllocated; ++i) { new (&newHashes[i]) SHAMapHash{}; new (&newChildren[i]) SHAMapTreeNodePtr{}; diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 2483e6f6e1..3fa8d66be0 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -116,8 +116,7 @@ SHAMap::dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNode stack.pop(); XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node"); - int const branch = selectBranch(nodeID, target); - XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::dirtyUp : valid branch"); + auto const branch = selectBranch(nodeID, target); node = unshareNode(std::move(node), nodeID); node->setChild(branch, std::move(child)); @@ -278,7 +277,7 @@ SHAMap::fetchNode(SHAMapHash const& hash) const } SHAMapTreeNode* -SHAMap::descendThrow(SHAMapInnerNode* parent, int branch) const +SHAMap::descendThrow(SHAMapInnerNode* parent, unsigned int branch) const { SHAMapTreeNode* ret = descend(parent, branch); // NOLINT(misc-const-correctness) @@ -289,7 +288,7 @@ SHAMap::descendThrow(SHAMapInnerNode* parent, int branch) const } SHAMapTreeNodePtr -SHAMap::descendThrow(SHAMapInnerNode& parent, int branch) const +SHAMap::descendThrow(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = descend(parent, branch); @@ -300,7 +299,7 @@ SHAMap::descendThrow(SHAMapInnerNode& parent, int branch) const } SHAMapTreeNode* -SHAMap::descend(SHAMapInnerNode* parent, int branch) const +SHAMap::descend(SHAMapInnerNode* parent, unsigned int branch) const { SHAMapTreeNode* ret = parent->getChildPointer(branch); // NOLINT(misc-const-correctness) if ((ret != nullptr) || !backed_) @@ -315,7 +314,7 @@ SHAMap::descend(SHAMapInnerNode* parent, int branch) const } SHAMapTreeNodePtr -SHAMap::descend(SHAMapInnerNode& parent, int branch) const +SHAMap::descend(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr node = parent.getChild(branch); if (node || !backed_) @@ -332,7 +331,7 @@ SHAMap::descend(SHAMapInnerNode& parent, int branch) const // Gets the node that would be hooked to this branch, // but doesn't hook it up. SHAMapTreeNodePtr -SHAMap::descendNoStore(SHAMapInnerNode& parent, int branch) const +SHAMap::descendNoStore(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = parent.getChild(branch); if (!ret && backed_) @@ -344,12 +343,11 @@ std::pair SHAMap::descend( SHAMapInnerNode* parent, SHAMapNodeID const& parentID, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter) const { XRPL_ASSERT(parent->isInner(), "xrpl::SHAMap::descend : valid parent input"); - XRPL_ASSERT( - (branch >= 0) && (branch < kBranchFactor), "xrpl::SHAMap::descend : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMap::descend : valid branch input"); XRPL_ASSERT( !parent->isEmptyBranch(branch), "xrpl::SHAMap::descend : parent branch is non-empty"); @@ -373,7 +371,7 @@ SHAMap::descend( SHAMapTreeNode* SHAMap::descendAsync( SHAMapInnerNode* parent, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter, bool& pending, descendCallback&& callback) const @@ -433,10 +431,9 @@ SHAMapLeafNode* SHAMap::belowHelper( SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, - int branch, - std::tuple, std::function> const& loopParams) const + unsigned int branch, + BelowDirection direction) const { - auto& [init, cmp, incr] = loopParams; if (node->isLeaf()) { auto n = intr_ptr::staticPointerCast(node); @@ -452,11 +449,16 @@ SHAMap::belowHelper( { stack.emplace(inner, stack.top().second.getChildNodeID(branch)); } - for (int i = init; cmp(i);) + // `scanned` counts how many branches of `inner` we have examined; the branch we look at is + // derived from it, so no index ever goes out of range. + for (auto scanned = 0u; scanned < kBranchFactor;) { - if (!inner->isEmptyBranch(i)) + auto const childBranch = + (direction == BelowDirection::Last) ? (kBranchFactor - 1u - scanned) : scanned; + + if (!inner->isEmptyBranch(childBranch)) { - node.adopt(descendThrow(inner.get(), i)); + node.adopt(descendThrow(inner.get(), childBranch)); XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack"); if (node->isLeaf()) { @@ -466,32 +468,24 @@ SHAMap::belowHelper( } inner = intr_ptr::staticPointerCast(node); stack.emplace(inner, stack.top().second.getChildNodeID(branch)); - i = init; // descend and reset loop + scanned = 0u; // descend and restart the scan on the new node } else { - incr(i); // scan next branch + ++scanned; // scan next branch } } return nullptr; } SHAMapLeafNode* -SHAMap::lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch) const +SHAMap::lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const { - auto init = kBranchFactor - 1; - auto cmp = [](int i) { return i >= 0; }; - auto incr = [](int& i) { --i; }; - - return belowHelper(node, stack, branch, {init, cmp, incr}); + return belowHelper(node, stack, branch, BelowDirection::Last); } SHAMapLeafNode* -SHAMap::firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch) const +SHAMap::firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const { - auto init = 0; - auto cmp = [](int i) { return i <= kBranchFactor; }; - auto incr = [](int& i) { ++i; }; - - return belowHelper(node, stack, branch, {init, cmp, incr}); + return belowHelper(node, stack, branch, BelowDirection::First); } static boost::intrusive_ptr const kNoItem; @@ -504,7 +498,7 @@ SHAMap::onlyBelow(SHAMapTreeNode* node) const { SHAMapTreeNode* nextNode = nullptr; auto inner = safeDowncast(node); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { @@ -650,8 +644,9 @@ SHAMap::lowerBound(uint256 const& id) const else { auto inner = intr_ptr::staticPointerCast(node); - for (int branch = selectBranch(nodeID, id) - 1; branch >= 0; --branch) + for (auto branch = selectBranch(nodeID, id); branch > 0u;) { + --branch; if (!inner->isEmptyBranch(branch)) { node = descendThrow(*inner, branch); @@ -715,7 +710,7 @@ SHAMap::delItem(uint256 const& id) { // we may have made this a node with 1 or 0 children // And, if so, we need to remove this branch - int const bc = node->getBranchCount(); + auto const bc = node->getBranchCount(); if (bc == 0) { // no children below this branch @@ -730,7 +725,7 @@ SHAMap::delItem(uint256 const& id) if (item) { - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -786,7 +781,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr { // easy case, we end on an inner node auto inner = intr_ptr::staticPointerCast(node); - int const branch = selectBranch(nodeID, tag); + auto const branch = selectBranch(nodeID, tag); XRPL_ASSERT( inner->isEmptyBranch(branch), "xrpl::SHAMap::addGiveItem : inner branch is empty"); inner->setChild(branch, makeTypedLeaf(type, std::move(item), cowid_)); @@ -802,7 +797,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr node = intr_ptr::makeShared(node->cowid()); - unsigned int b1 = 0, b2 = 0; + auto b1 = 0u, b2 = 0u; while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key()))) { @@ -1012,12 +1007,12 @@ SHAMap::walkSubTree(bool doWrite, NodeObjectType t) // Stack of {parent,index,child} pointers representing // inner nodes we are in the process of flushing - using StackEntry = std::pair, int>; + using StackEntry = std::pair, unsigned int>; std::stack> stack; node = preFlushNode(std::move(node)); - int pos = 0; + auto pos = 0u; // We can't flush an inner node until we flush its children while (true) @@ -1032,7 +1027,7 @@ SHAMap::walkSubTree(bool doWrite, NodeObjectType t) { // No need to do I/O. If the node isn't linked, // it can't need to be flushed - int const branch = pos; + auto const branch = pos; auto child = node->getChild(pos++); if (child && (child->cowid() != 0)) @@ -1126,7 +1121,7 @@ SHAMap::dump(bool hash) const if (node->isInner()) { auto inner = safeDowncast(node); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { diff --git a/src/libxrpl/shamap/SHAMapDelta.cpp b/src/libxrpl/shamap/SHAMapDelta.cpp index 8336ce5481..1306fe6990 100644 --- a/src/libxrpl/shamap/SHAMapDelta.cpp +++ b/src/libxrpl/shamap/SHAMapDelta.cpp @@ -54,7 +54,7 @@ SHAMap::walkBranch( { // This is an inner node, add all non-empty branches auto inner = safeDowncast(node); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) nodeStack.push({descendThrow(inner, i)}); @@ -205,7 +205,7 @@ SHAMap::compare(SHAMap const& otherMap, Delta& differences, int maxCount) const { auto ours = safeDowncast(ourNode); auto other = safeDowncast(otherNode); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (ours->getChildHash(i) != other->getChildHash(i)) { @@ -257,7 +257,7 @@ SHAMap::walkMap(std::vector& missingNodes, int maxMissing) co intr_ptr::SharedPtr const node = std::move(nodeStack.top()); nodeStack.pop(); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -286,27 +286,29 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis return false; using StackEntry = intr_ptr::SharedPtr; - std::array topChildren; + std::array topChildren; { auto const& innerRoot = intr_ptr::staticPointerCast(root_); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!innerRoot->isEmptyBranch(i)) topChildren[i] = descendNoStore(*innerRoot, i); } } std::vector workers; - workers.reserve(16); + workers.reserve(SHAMapInnerNode::kBranchFactor); std::vector exceptions; - exceptions.reserve(16); + exceptions.reserve(SHAMapInnerNode::kBranchFactor); - std::array>, 16> nodeStacks; + std::array>, SHAMapInnerNode::kBranchFactor> + nodeStacks; // This mutex is used inside the worker threads to protect `missingNodes` // and `maxMissing` from race conditions std::mutex m; - for (int rootChildIndex = 0; rootChildIndex < 16; ++rootChildIndex) + for (auto rootChildIndex = 0u; rootChildIndex < SHAMapInnerNode::kBranchFactor; + ++rootChildIndex) { auto const& child = topChildren[rootChildIndex]; if (!child || !child->isInner()) @@ -327,7 +329,7 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis XRPL_ASSERT(node, "xrpl::SHAMap::walkMapParallel : non-null node"); nodeStack.pop(); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (node->isEmptyBranch(i)) continue; diff --git a/src/libxrpl/shamap/SHAMapInnerNode.cpp b/src/libxrpl/shamap/SHAMapInnerNode.cpp index 74a0e4515f..bdd89388b2 100644 --- a/src/libxrpl/shamap/SHAMapInnerNode.cpp +++ b/src/libxrpl/shamap/SHAMapInnerNode.cpp @@ -63,8 +63,8 @@ SHAMapInnerNode::resizeChildArrays(std::uint8_t toAllocate) hashesAndChildren_ = TaggedPointer(std::move(hashesAndChildren_), isBranch_, toAllocate); } -std::optional -SHAMapInnerNode::getChildIndex(int i) const +std::optional +SHAMapInnerNode::getChildIndex(unsigned int i) const { return hashesAndChildren_.getChildIndex(isBranch_, i); } @@ -89,7 +89,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const if (thisIsSparse) { - int cloneChildIndex = 0; + auto cloneChildIndex = 0u; iterNonEmptyChildIndexes([&](auto branchNum, auto indexNum) { cloneHashes[cloneChildIndex++] = thisHashes[indexNum]; }); @@ -105,7 +105,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const if (thisIsSparse) { - int cloneChildIndex = 0; + auto cloneChildIndex = 0u; iterNonEmptyChildIndexes([&](auto branchNum, auto indexNum) { cloneChildren[cloneChildIndex++] = thisChildren[indexNum]; }); @@ -133,12 +133,12 @@ SHAMapInnerNode::makeFullInner(Slice data, SHAMapHash const& hash, bool hashVali auto hashes = ret->hashesAndChildren_.getHashes(); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { hashes[i].asUInt256() = si.getBitString<256>(); if (hashes[i].isNonZero()) - ret->isBranch_ |= (1 << i); + ret->isBranch_ |= (1u << i); } ret->resizeChildArrays(ret->getBranchCount()); @@ -182,7 +182,7 @@ SHAMapInnerNode::makeCompressedInner(Slice data) hashes[pos].asUInt256() = hash; if (hashes[pos].isNonZero()) - ret->isBranch_ |= (1 << pos); + ret->isBranch_ |= (1u << pos); } ret->resizeChildArrays(ret->getBranchCount()); @@ -267,20 +267,19 @@ SHAMapInnerNode::getString(SHAMapNodeID const& id) const // We are modifying an inner node void -SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) +SHAMapInnerNode::setChild(unsigned int branch, SHAMapTreeNodePtr child) { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), "xrpl::SHAMapInnerNode::setChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::setChild : valid branch input"); XRPL_ASSERT(cowid_, "xrpl::SHAMapInnerNode::setChild : nonzero cowid"); XRPL_ASSERT(child.get() != this, "xrpl::SHAMapInnerNode::setChild : valid child input"); auto const dstIsBranch = [&] { if (child) { - return isBranch_ | (1u << m); + return isBranch_ | (1u << branch); } - return isBranch_ & ~(1u << m); + return isBranch_ & ~(1u << branch); }(); auto const dstToAllocate = popcnt16(dstIsBranch); @@ -293,8 +292,8 @@ SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) if (child) { - auto const childIndex = - *getChildIndex(m); // NOLINT(bugprone-unchecked-optional-access) isBranch_ set above + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) isBranch_ set above + auto const childIndex = *getChildIndex(branch); auto [_, hashes, children] = hashesAndChildren_.getHashesAndChildren(); hashes[childIndex].zero(); children[childIndex] = std::move(child); @@ -309,25 +308,24 @@ SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) // finished modifying, now make shareable void -SHAMapInnerNode::shareChild(int m, SHAMapTreeNodePtr const& child) +SHAMapInnerNode::shareChild(unsigned int branch, SHAMapTreeNodePtr const& child) { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), "xrpl::SHAMapInnerNode::shareChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::shareChild : valid branch input"); XRPL_ASSERT(cowid_, "xrpl::SHAMapInnerNode::shareChild : nonzero cowid"); XRPL_ASSERT(child, "xrpl::SHAMapInnerNode::shareChild : non-null child input"); XRPL_ASSERT(child.get() != this, "xrpl::SHAMapInnerNode::shareChild : valid child input"); - XRPL_ASSERT(!isEmptyBranch(m), "xrpl::SHAMapInnerNode::shareChild : non-empty branch input"); + XRPL_ASSERT( + !isEmptyBranch(branch), "xrpl::SHAMapInnerNode::shareChild : non-empty branch input"); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) assert above - hashesAndChildren_.getChildren()[*getChildIndex(m)] = child; + hashesAndChildren_.getChildren()[*getChildIndex(branch)] = child; } SHAMapTreeNode* -SHAMapInnerNode::getChildPointer(int branch) +SHAMapInnerNode::getChildPointer(unsigned int branch) { XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::getChildPointer : valid branch input"); + branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChildPointer : valid branch input"); XRPL_ASSERT( !isEmptyBranch(branch), "xrpl::SHAMapInnerNode::getChildPointer : non-empty branch input"); @@ -340,11 +338,9 @@ SHAMapInnerNode::getChildPointer(int branch) } SHAMapTreeNodePtr -SHAMapInnerNode::getChild(int branch) +SHAMapInnerNode::getChild(unsigned int branch) { - XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::getChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChild : valid branch input"); XRPL_ASSERT(!isEmptyBranch(branch), "xrpl::SHAMapInnerNode::getChild : non-empty branch input"); auto const index = @@ -356,23 +352,20 @@ SHAMapInnerNode::getChild(int branch) } SHAMapHash const& -SHAMapInnerNode::getChildHash(int m) const +SHAMapInnerNode::getChildHash(unsigned int branch) const { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), - "xrpl::SHAMapInnerNode::getChildHash : valid branch input"); - if (auto const i = getChildIndex(m)) + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChildHash : valid branch input"); + if (auto const i = getChildIndex(branch)) return hashesAndChildren_.getHashes()[*i]; return kZeroShaMapHash; } SHAMapTreeNodePtr -SHAMapInnerNode::canonicalizeChild(int branch, SHAMapTreeNodePtr node) +SHAMapInnerNode::canonicalizeChild(unsigned int branch, SHAMapTreeNodePtr node) { XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::canonicalizeChild : valid branch input"); + branch < kBranchFactor, "xrpl::SHAMapInnerNode::canonicalizeChild : valid branch input"); XRPL_ASSERT(node != nullptr, "xrpl::SHAMapInnerNode::canonicalizeChild : valid node input"); XRPL_ASSERT( !isEmptyBranch(branch), @@ -410,7 +403,7 @@ SHAMapInnerNode::invariants(bool isRoot) const if (numAllocated != kBranchFactor) { auto const branchCount = getBranchCount(); - for (int i = 0; i < branchCount; ++i) + for (auto i = 0u; i < branchCount; ++i) { XRPL_ASSERT( hashes[i].isNonZero(), @@ -422,12 +415,12 @@ SHAMapInnerNode::invariants(bool isRoot) const } else { - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (hashes[i].isNonZero()) { XRPL_ASSERT( - (isBranch_ & (1 << i)), + (isBranch_ & (1u << i)), "xrpl::SHAMapInnerNode::invariants : valid branch when " "nonzero hash"); if (children[i] != nullptr) @@ -437,7 +430,7 @@ SHAMapInnerNode::invariants(bool isRoot) const else { XRPL_ASSERT( - (isBranch_ & (1 << i)) == 0, + (isBranch_ & (1u << i)) == 0u, "xrpl::SHAMapInnerNode::invariants : valid branch when " "zero hash"); } diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index a511fc038c..ecde22a63d 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -16,7 +16,7 @@ namespace xrpl { static uint256 const& depthMask(unsigned int depth) { - static constexpr auto kMaskSize = 65; + static constexpr auto kMaskSize = SHAMap::kLeafDepth + 1; struct MasksT { @@ -25,7 +25,7 @@ depthMask(unsigned int depth) MasksT() { uint256 selector; - for (int i = 0; i < kMaskSize - 1; i += 2) + for (auto i = 0u; i < kMaskSize - 1; i += 2) { entry[i] = selector; *(selector.begin() + (i / 2)) = 0xF0; @@ -60,10 +60,10 @@ SHAMapNodeID::getRawString() const } SHAMapNodeID -SHAMapNodeID::getChildNodeID(unsigned int m) const +SHAMapNodeID::getChildNodeID(unsigned int branch) const { XRPL_ASSERT( - m < SHAMap::kBranchFactor, "xrpl::SHAMapNodeID::getChildNodeID : valid branch input"); + branch < SHAMap::kBranchFactor, "xrpl::SHAMapNodeID::getChildNodeID : valid branch input"); // A SHAMap has exactly 65 levels, so nodes must not exceed that // depth; if they do, this breaks the invariant of never allowing @@ -83,7 +83,7 @@ SHAMapNodeID::getChildNodeID(unsigned int m) const Throw("Incorrect mask for " + to_string(*this)); SHAMapNodeID node{depth_ + 1, id_}; - node.id_.begin()[depth_ / 2] |= ((depth_ & 1) != 0u) ? m : (m << 4); + node.id_.begin()[depth_ / 2] |= ((depth_ & 1) != 0u) ? branch : (branch << 4); return node; } @@ -127,10 +127,9 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash) } SHAMapNodeID -SHAMapNodeID::createID(int depth, uint256 const& key) +SHAMapNodeID::createID(unsigned int depth, uint256 const& key) { - XRPL_ASSERT( - depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); + XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); return SHAMapNodeID(depth, key & depthMask(depth)); } diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index cbed6885c9..e6948ec3ac 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -54,15 +54,15 @@ SHAMap::visitNodes(std::function const& function) const if (!root_->isInner()) return; - using StackEntry = std::pair>; + using StackEntry = std::pair>; std::stack> stack; auto node = intr_ptr::staticPointerCast(root_); - int pos = 0; + auto pos = 0u; while (true) { - while (pos < 16) + while (pos < kBranchFactor) { if (!node->isEmptyBranch(pos)) { @@ -77,10 +77,10 @@ SHAMap::visitNodes(std::function const& function) const else { // If there are no more children, don't push this node - while ((pos != 15) && (node->isEmptyBranch(pos + 1))) + while ((pos != kBranchFactor - 1u) && (node->isEmptyBranch(pos + 1))) ++pos; - if (pos != 15) + if (pos != kBranchFactor - 1u) { // save next position to resume at stack.emplace(pos + 1, std::move(node)); @@ -144,7 +144,7 @@ SHAMap::visitDifferences( return; // 2) push non-matching child inner nodes - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -176,13 +176,13 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) { SHAMapInnerNode*& node = std::get<0>(se); SHAMapNodeID& nodeID = std::get<1>(se); - int& firstChild = std::get<2>(se); - int& currentChild = std::get<3>(se); + auto& firstChild = std::get<2>(se); + auto& currentChild = std::get<3>(se); bool& fullBelow = std::get<4>(se); - while (currentChild < 16) + while (currentChild < kBranchFactor) { - int const branch = (firstChild + currentChild++) % 16; + auto const branch = (firstChild + currentChild++) % kBranchFactor; if (node->isEmptyBranch(branch)) continue; @@ -262,7 +262,7 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn) int complete = 0; while (complete != mn.deferred) { - std::tuple deferredNode; + MissingNodes::DeferredNode deferredNode; { std::unique_lock lock{mn.deferLock}; @@ -423,7 +423,7 @@ SHAMap::getNodeFat( while ((node != nullptr) && node->isInner() && (nodeID.getDepth() < wanted.getDepth())) { - int const branch = selectBranch(nodeID, wanted.getNodeID()); + auto const branch = selectBranch(nodeID, wanted.getNodeID()); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; @@ -444,7 +444,7 @@ SHAMap::getNodeFat( return false; } - std::stack> stack; + std::stack> stack; stack.emplace(node, nodeID, depth); Serializer s(8192); @@ -464,12 +464,12 @@ SHAMap::getNodeFat( // We descend inner nodes with only a single child // without decrementing the depth auto inner = safeDowncast(node); - int const bc = inner->getBranchCount(); + auto const bc = inner->getBranchCount(); if ((depth > 0) || (bc == 1)) { // We need to process this node's children - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { @@ -575,8 +575,7 @@ SHAMap::addKnownNode( !safeDowncast(currNode)->isFullBelow(generation) && (currNodeID.getDepth() < nodeID.getDepth())) { - int const branch = selectBranch(currNodeID, nodeID.getNodeID()); - XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::addKnownNode : valid branch"); + auto const branch = selectBranch(currNodeID, nodeID.getNodeID()); auto inner = safeDowncast(currNode); if (inner->isEmptyBranch(branch)) { @@ -686,7 +685,7 @@ SHAMap::deepCompare(SHAMap& other) const return false; auto nodeInner = safeDowncast(node); auto otherInner = safeDowncast(otherNode); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (nodeInner->isEmptyBranch(i)) { @@ -725,7 +724,7 @@ SHAMap::hasInnerNode(SHAMapNodeID const& targetNodeID, SHAMapHash const& targetN while (node->isInner() && (nodeID.getDepth() < targetNodeID.getDepth())) { - int const branch = selectBranch(nodeID, targetNodeID.getNodeID()); + auto const branch = selectBranch(nodeID, targetNodeID.getNodeID()); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; @@ -751,7 +750,20 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const do { - int const branch = selectBranch(nodeID, tag); + // An inner node is only reachable here at a depth below kLeafDepth in a well-formed map, + // where the loop always finds a leaf first. A malformed map could still have an inner + // node claiming kLeafDepth, and getChildNodeID below throws in that case: reject rather + // than let the throw escape uncaught. Not reachable through any public entry point, + // since addKnownNode already marks such a map invalid, so no test can cover this. + if (nodeID.getDepth() >= kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::hasLeafNode : inner node at leaf depth"); + return false; + // LCOV_EXCL_STOP + } + + auto const branch = selectBranch(nodeID, tag); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; // Dead end, node must not be here @@ -803,7 +815,7 @@ SHAMap::getProofPath(uint256 const& key) const bool SHAMap::verifyProofPath(uint256 const& rootHash, uint256 const& key, std::vector const& path) { - if (path.empty() || path.size() > 65) + if (path.empty() || path.size() > kLeafDepth + 1u) return false; SHAMapHash hash{rootHash}; @@ -819,10 +831,10 @@ SHAMap::verifyProofPath(uint256 const& rootHash, uint256 const& key, std::vector if (node->getHash() != hash) return false; - auto depth = std::distance(path.rbegin(), rit); + auto const depth = std::distance(path.rbegin(), rit); if (node->isInner()) { - auto nodeId = SHAMapNodeID::createID(depth, key); + auto nodeId = SHAMapNodeID::createID(static_cast(depth), key); hash = safeDowncast(node.get()) ->getChildHash(selectBranch(nodeId, key)); } diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index 531dba59f9..abd669d446 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -75,7 +75,7 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& if (treeNode.isLeaf()) { auto const key = leafKey(treeNode); - auto const expectedID = SHAMapNodeID::createID(static_cast(nodeID->getDepth()), key); + auto const expectedID = SHAMapNodeID::createID(nodeID->getDepth(), key); SOMETIMES( nodeID->getNodeID() != expectedID.getNodeID(), "xrpl::getSHAMapNodeID : legacy leaf ID inconsistent with key"); From c49789086ad3b031cd527fa7ed2e687e81fdfd4a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 12:52:20 +0000 Subject: [PATCH 07/13] fix: Extend locked-MPToken unauthorize check to fixCleanup3_4_0 (#8004) --- .../tx/transactors/token/MPTokenAuthorize.cpp | 27 ++++----- src/test/app/MPToken_test.cpp | 58 ++++++++++++++++++- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp index 0aeb6f33d1..c19b8f64d7 100644 --- a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp +++ b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp @@ -37,6 +37,7 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) { auto const accountID = ctx.tx[sfAccount]; auto const holderID = ctx.tx[~sfHolder]; + auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); // if non-issuer account submits this tx, then they are trying either: // 1. Unauthorize/delete MPToken @@ -51,9 +52,8 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) // There is an edge case where all holders have zero balance, issuance // is legally destroyed, then outstanding MPT(s) are deleted afterwards. - // Thus, there is no need to check for the existence of the issuance if - // the MPT is being deleted with a zero balance. Check for unauthorize - // before fetching the MPTIssuance object. + // Thus, the unauthorize/delete path below does not require the issuance + // to exist when the MPT is being deleted with a zero balance. // if holder wants to delete/unauthorize a mpt if (ctx.tx.isFlag(tfMPTUnauthorize)) @@ -63,8 +63,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[sfMPTAmount] != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE @@ -73,21 +71,24 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[~sfLockedAmount].value_or(0) != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE return tecHAS_OBLIGATIONS; } - if (ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + if (ctx.view.rules().enabled(fixCleanup3_4_0)) + { + if (sleMptIssuance && sleMpt->isFlag(lsfMPTLocked)) + return tecNO_PERMISSION; + } + else if ( + ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + { return tecNO_PERMISSION; + } if (ctx.view.rules().enabled(featureConfidentialTransfer)) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - // if there still existing encrypted balances of MPT in // circulation if (sleMptIssuance && @@ -106,9 +107,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) } // Now test when the holder wants to hold/create/authorize a new MPT - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; @@ -126,7 +124,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if (!sleHolder) return tecNO_DST; - auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index b392dca758..7086adf743 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -789,7 +789,7 @@ class MPToken_test : public beast::unit_test::Suite // locks up bob's mptoken again mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); - if (!features[featureSingleAssetVault]) + if (!features[featureSingleAssetVault] && !features[fixCleanup3_4_0]) { // Delete bob's mptoken even though it is locked mptAlice.authorize({.account = bob, .flags = tfMPTUnauthorize}); @@ -7657,6 +7657,56 @@ class MPToken_test : public beast::unit_test::Suite 0, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION); } + void + testLockedMPTokenDestroyedIssuance(FeatureBitset features) + { + testcase("Locked MPToken with destroyed issuance"); + + using namespace test::jtx; + Account const alice("alice"); // issuer + Account const bob("bob"); // holder + + Env env{*this, features}; + env.fund(XRP(1'000), alice, bob); + env.close(); + MPTTester mptAlice( + {.env = env, .issuer = alice, .holders = {bob}, .flags = kMptDexFlags | tfMPTCanLock}); + + // alice locks bob's mptoken individually + mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); + + // alice destroys her issuance. This succeeds: MPTokenIssuanceDestroy + // only requires that the issuance has no outstanding balance; it does + // not require that all holder MPTokens have been deleted first. + mptAlice.destroy({.ownerCount = 0}); + + if (!features[featureSingleAssetVault] || features[fixCleanup3_4_0]) + { + // pre SAV or post Cleanup340 amendment: bob deletes the dangling locked MPToken + mptAlice.authorize({.account = bob, .holderCount = 0, .flags = tfMPTUnauthorize}); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + else + { + // bob cannot delete his locked MPToken, even though the issuance + // no longer exists. + mptAlice.authorize( + {.account = bob, .flags = tfMPTUnauthorize, .err = tecNO_PERMISSION}); + + // and the lock can never be cleared, because unlocking + // requires the (destroyed) issuance + mptAlice.set( + {.account = alice, + .holder = bob, + .flags = tfMPTUnlock, + .err = tecOBJECT_NOT_FOUND}); + + // the dangling locked MPToken survives + BEAST_EXPECT(env.current()->exists(keylet::mptoken(mptAlice.issuanceID(), bob.id()))); + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + } + public: void run() override @@ -7703,7 +7753,9 @@ public: testSetValidation(all - featurePermissionedDomains); testSetValidation(all); + testSetEnabled(all - featureSingleAssetVault - fixCleanup3_4_0); testSetEnabled(all - featureSingleAssetVault); + testSetEnabled(all - fixCleanup3_4_0); testSetEnabled(all); // MPT clawback @@ -7770,6 +7822,10 @@ public: // Fixes testFixDoubleOwnerCount(all); + testLockedMPTokenDestroyedIssuance(all); + testLockedMPTokenDestroyedIssuance(all - fixCleanup3_4_0); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault - fixCleanup3_4_0); } }; From ca6121c5b34520f304796fdc1a57c2bac5806a83 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 20:58:46 +0000 Subject: [PATCH 08/13] feat: Enforce MPT CanTransfer on AMM LPTokens transfers (#7418) --- include/xrpl/ledger/View.h | 20 +++ src/libxrpl/ledger/View.cpp | 27 ++++ src/libxrpl/ledger/helpers/TokenHelpers.cpp | 9 ++ src/libxrpl/tx/paths/DirectStep.cpp | 11 +- src/test/app/LPTokenTransfer_test.cpp | 135 ++++++++++++++++++++ 5 files changed, 200 insertions(+), 2 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index e8b4a932d0..0893612bac 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -85,6 +85,26 @@ isLPTokenFrozen( Asset const& asset, Asset const& asset2); +/** + * Check whether an AMM LPToken may be transferred between @p from and @p to. + * + * @p lpTokenIssuer is the issuer of the LPToken being moved. If it is not an + * AMM account the token is not an LPToken and the transfer is unconditionally + * permitted. Otherwise, for each MPT pool asset of that AMM, canTransfer() must + * permit the transfer (which exempts the MPT issuer). Non-MPT pool assets are + * always transferable by this check, so it is implicitly gated by + * featureMPTokensV2 (MPTs can only be AMM pool assets once V2 is enabled). + * + * @return tesSUCCESS if permitted, otherwise the canTransfer() failure code + * (e.g. tecNO_AUTH) of the first MPT pool asset that disallows it. + */ +[[nodiscard]] TER +canTransferLPToken( + ReadView const& view, + AccountID const& from, + AccountID const& to, + AccountID const& lpTokenIssuer); + // Return the list of enabled amendments [[nodiscard]] std::set getEnabledAmendments(ReadView const& view); diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 2dd70e2950..0544771973 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -138,6 +138,33 @@ isLPTokenFrozen( return isFrozen(view, account, asset) || isFrozen(view, account, asset2); } +TER +canTransferLPToken( + ReadView const& view, + AccountID const& from, + AccountID const& to, + AccountID const& lpTokenIssuer) +{ + // Only AMM-issued LPTokens are subject to this check. The LPToken's issuer + // is the AMM account; if it is not an AMM, this is not an LPToken. + auto const sleIssuer = view.read(keylet::account(lpTokenIssuer)); + if (!sleIssuer || !sleIssuer->isFieldPresent(sfAMMID)) + return tesSUCCESS; + + auto const sleAmm = view.read(keylet::amm((*sleIssuer)[sfAMMID])); + if (!sleAmm) + return tecINTERNAL; // LCOV_EXCL_LINE + + auto const transferable = [&](Asset const& a) -> TER { + if (!a.holds()) + return tesSUCCESS; + return canTransfer(view, a.get(), from, to); + }; + if (auto const err = transferable((*sleAmm)[sfAsset]); !isTesSuccess(err)) + return err; + return transferable((*sleAmm)[sfAsset2]); +} + bool areCompatible( ReadView const& validLedger, diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 79e10cdf79..9e3452ccae 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -309,6 +309,15 @@ getLineIfUsable( } } } + + // An LPToken whose AMM pool contains an MPT that forbids transfers is not + // spendable. Issuer is the LPToken's AMM account; canTransferLPToken is + // a no-op for non-AMM issuers and non-MPT pool assets, so this is implicitly + // gated by featureMPTokensV2. + if (!isTesSuccess(canTransferLPToken(view, account, account, issuer))) + { + return nullptr; + } } return sle; diff --git a/src/libxrpl/tx/paths/DirectStep.cpp b/src/libxrpl/tx/paths/DirectStep.cpp index f8f12bd421..1854bd3632 100644 --- a/src/libxrpl/tx/paths/DirectStep.cpp +++ b/src/libxrpl/tx/paths/DirectStep.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -845,8 +846,14 @@ DirectStepI::check(StrandContext const& ctx) const // pure issue/redeem can't be frozen if (!(ctx.isLast && ctx.isFirst)) { - auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); - if (!isTesSuccess(ter)) + if (auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); !isTesSuccess(ter)) + return ter; + + // An LPToken redeemed against its AMM (dst_ is the LPToken issuer on + // this hop) cannot move if a pool asset is an MPT that forbids + // transfers between these accounts. A no-op unless dst_ is an AMM whose + // pool holds such an MPT (so it is implicitly gated by featureMPTokensV2). + if (auto const ter = canTransferLPToken(ctx.view, src_, dst_, dst_); !isTesSuccess(ter)) return ter; } diff --git a/src/test/app/LPTokenTransfer_test.cpp b/src/test/app/LPTokenTransfer_test.cpp index e30e37ed98..3e72094eb3 100644 --- a/src/test/app/LPTokenTransfer_test.cpp +++ b/src/test/app/LPTokenTransfer_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include // IWYU pragma: keep #include @@ -21,6 +22,8 @@ #include #include +#include + namespace xrpl::test { class LPTokenTransfer_test : public jtx::AMMTest @@ -433,6 +436,136 @@ class LPTokenTransfer_test : public jtx::AMMTest } } + void + testMPTCanTransferDirectStep(FeatureBitset features) + { + testcase("MPT CanTransfer DirectStep"); + + using namespace jtx; + + // An MPT can only be an AMM pool asset once featureMPTokensV2 is + // enabled, so this behavior is only meaningful when V2 is present, and + // is independent of fixFrozenLPTokenTransfer. + if (!features[featureMPTokensV2]) + return; + + // gw issues an MPT used as one of the AMM pool assets. gw (the MPT + // issuer) seeds the pool and hands LP tokens to alice. Transferring LP + // tokens between two non-issuer holders is only permitted when the + // pool MPT allows transfers (lsfMPTCanTransfer); issuer-involving + // transfers are always permitted. The check fires on the redeem step + // against the AMM account via canTransferLPToken(). + auto testLPTokenTransfer = [&](std::uint32_t mptFlags, bool poolXrpToBtc) { + Env env{*this, features}; + env.fund(XRP(30'000), gw_, alice_, bob_); + env.close(); + + // gw is the MPT issuer, so it may seed the pool regardless of + // whether the MPT permits third-party transfers. + MPT const btc = MPTTester( + {.env = env, .issuer = gw_, .holders = {alice_}, .pay = 1'000, .flags = mptFlags}); + + auto const asset1 = poolXrpToBtc ? XRP(10'000) : btc(10'000); + auto const asset2 = poolXrpToBtc ? btc(10'000) : XRP(10'000); + AMM const amm(env, gw_, asset1, asset2); + auto const lpIssue = amm.lptIssue(); + + env.trust(STAmount{lpIssue, 100'000}, alice_); + env.trust(STAmount{lpIssue, 100'000}, bob_); + env.close(); + + // Issuer-involving LP token transfer is always allowed (gw is the + // pool MPT's issuer), even when the MPT lacks CanTransfer. + env(pay(gw_, alice_, STAmount{lpIssue, 1'000})); + env.close(); + + // Transfer between two non-issuer holders is allowed only if the + // pool MPT has CanTransfer set; otherwise the redeem step against + // the AMM account blocks it with tecNO_AUTH. + if ((mptFlags & tfMPTCanTransfer) != 0u) + { + env(pay(alice_, bob_, STAmount{lpIssue, 100})); + } + else + { + env(pay(alice_, bob_, STAmount{lpIssue, 100}), Ter(tecNO_AUTH)); + } + env.close(); + }; + + // Pool MPT without CanTransfer blocks third-party LP token transfers. + testLPTokenTransfer(tfMPTCanTrade, true); + testLPTokenTransfer(tfMPTCanTrade, false); + + // Pool MPT with CanTransfer allows them. + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, true); + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, false); + } + + void + testMPTCanTransferOffer(FeatureBitset features) + { + testcase("MPT CanTransfer Offer"); + + using namespace jtx; + + if (!features[featureMPTokensV2]) + return; + + // Parity with frozen LP tokens for the order book: a non-transferable + // pool MPT makes the LP token un-spendable (canTransferLPToken zeroes + // the spendable balance in accountHolds, just as isLPTokenFrozen does), + // so an offer to sell it cannot be funded - the same tecUNFUNDED_OFFER + // outcome as freezing a pool asset (see testOfferCreation). + auto testLPTokenTransfer = [&](std::uint32_t mptFlags, bool poolXrpToBtc) { + Env env{*this, features}; + env.fund(XRP(30'000), gw_, carol_); + env.close(); + + MPT const btc = MPTTester( + {.env = env, .issuer = gw_, .holders = {carol_}, .pay = 1'000, .flags = mptFlags}); + + auto const asset1 = poolXrpToBtc ? XRP(10'000) : btc(10'000); + auto const asset2 = poolXrpToBtc ? btc(10'000) : XRP(10'000); + AMM const amm(env, gw_, asset1, asset2); + auto const lpIssue = amm.lptIssue(); + + env.trust(STAmount{lpIssue, 100'000}, carol_); + env.close(); + + // gw (the pool MPT issuer) seeds carol_ with LP tokens; issuer + // involving transfers are always allowed. + env(pay(gw_, carol_, STAmount{lpIssue, 1'000})); + env.close(); + + // carol_ tries to create an offer to sell the LP token. + if ((mptFlags & tfMPTCanTransfer) != 0u) + { + env(offer(carol_, XRP(10), STAmount{lpIssue, 10}), Txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol_, 1)); + } + else + { + // Non-transferable pool MPT => LP token un-spendable => the + // sell offer is unfunded, just as if a pool asset were frozen. + env(offer(carol_, XRP(10), STAmount{lpIssue, 10}), + Txflags(tfPassive), + Ter(tecUNFUNDED_OFFER)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol_, 0)); + } + }; + + // Pool MPT without CanTransfer: LP token sell offer is unfunded. + testLPTokenTransfer(tfMPTCanTrade, true); + testLPTokenTransfer(tfMPTCanTrade, false); + + // Pool MPT with CanTransfer: LP token sell offer is created. + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, true); + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, false); + } + public: void run() override @@ -447,6 +580,8 @@ public: testOfferCrossing(features); testCheck(features); testNFTOffers(features); + testMPTCanTransferDirectStep(features); + testMPTCanTransferOffer(features); } } }; From 1b226c8b2eb3d08b7018738adb1cddc6f6768372 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 21:15:16 +0000 Subject: [PATCH 09/13] perf: Optimize MPT freeze checks to reduce redundant state reads (#7411) Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- include/xrpl/ledger/View.h | 7 ++ include/xrpl/ledger/helpers/MPTokenHelpers.h | 35 ++++++++++ src/libxrpl/ledger/View.cpp | 69 +++++++++++++++---- src/libxrpl/ledger/helpers/AMMHelpers.cpp | 3 +- src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | 64 +++++++++++++++-- src/libxrpl/ledger/helpers/TokenHelpers.cpp | 2 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 2 +- .../tx/transactors/escrow/EscrowCreate.cpp | 4 +- .../tx/transactors/escrow/EscrowFinish.cpp | 2 +- src/test/app/AMMMPT_test.cpp | 54 +++++++++++++++ 10 files changed, 216 insertions(+), 26 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 0893612bac..f7fd5b5a8c 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -78,6 +78,13 @@ isVaultPseudoAccountFrozen( MPTIssue const& mptShare, std::uint8_t depth); +[[nodiscard]] bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth); + [[nodiscard]] bool isLPTokenFrozen( ReadView const& view, diff --git a/include/xrpl/ledger/helpers/MPTokenHelpers.h b/include/xrpl/ledger/helpers/MPTokenHelpers.h index 7babefd196..6d26cf3cbc 100644 --- a/include/xrpl/ledger/helpers/MPTokenHelpers.h +++ b/include/xrpl/ledger/helpers/MPTokenHelpers.h @@ -29,6 +29,9 @@ namespace xrpl { [[nodiscard]] bool isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue); +[[nodiscard]] bool +isGlobalFrozen(SLE const& issuanceSle); + /** * Returns true if @p account's MPToken for @p mptIssue carries the * individual-lock flag (lsfMPTLocked). @@ -40,9 +43,29 @@ isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue); * receive tokens — it combines isIndividualFrozen, isGlobalFrozen, and * isVaultPseudoAccountFrozen into a single complete check. */ + [[nodiscard]] bool isIndividualFrozen(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue); +[[nodiscard]] bool +isIndividualFrozen(SLE const& mptSle); + +/** + * Returns true if @p account cannot send or receive tokens of @p mptIssue + * because a freeze applies. This is the complete check callers should use + * before moving MPT value: it combines @ref isGlobalFrozen (issuance-level + * lock), @ref isIndividualFrozen (per-holder lock bit), and the transitive + * vault pseudo-account check (if @p mptIssue is a vault share, the underlying + * asset is checked, and so on recursively up to @c maxAssetCheckDepth). + * + * The @c SLE overload takes an already-loaded ltMPTOKEN or ltMPTOKEN_ISSUANCE + * ledger entry; for ltMPTOKEN it can skip the per-holder individual-lock lookup. + * @ref isAnyFrozen answers the same question for a set of accounts and returns true + * if the freeze applies to any of them. + * + * @param depth Current recursion depth for the vault-share walk. Callers + * outside this module should leave it at the default. + */ [[nodiscard]] bool isFrozen( ReadView const& view, @@ -50,6 +73,18 @@ isFrozen( MPTIssue const& mptIssue, std::uint8_t depth = 0); +/** + * SLE overload: pass an already-loaded ltMPTOKEN (holder row) or + * ltMPTOKEN_ISSUANCE to reuse it for the freeze checks and avoid re-reading + * the same object. For an ltMPTOKEN, @p sle is used directly for the + * individual-lock check and the issuance is read once for global-freeze and + * vault-pseudo-account. For an ltMPTOKEN_ISSUANCE, @p sle is used directly + * for global-freeze and vault-pseudo-account, and the caller's holder row is + * read for the individual-lock check. + */ +[[nodiscard]] bool +isFrozen(ReadView const& view, AccountID const& account, SLE const& sle, std::uint8_t depth = 0); + [[nodiscard]] bool isAnyFrozen( ReadView const& view, diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0544771973..e01ae2e492 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -61,12 +61,10 @@ hasExpired( : view.parentCloseTime() > boundary; } -bool -isVaultPseudoAccountFrozen( - ReadView const& view, - AccountID const& account, - MPTIssue const& mptShare, - std::uint8_t depth) +namespace { + +std::optional +checkVaultPseudoAccountFrozenPreconditions(ReadView const& view, std::uint8_t depth) { if (!view.rules().enabled(featureSingleAssetVault)) return false; @@ -74,26 +72,37 @@ isVaultPseudoAccountFrozen( if (depth >= kMaxAssetCheckDepth) { // LCOV_EXCL_START - UNREACHABLE("xrpl::View::isVaultPseudoAccountFrozen : reached asset check depth"); + UNREACHABLE( + "xrpl::View::checkVaultPseudoAccountFrozenPreconditions : reached asset check depth"); return true; // LCOV_EXCL_STOP } - auto const mptIssuance = view.read(keylet::mptokenIssuance(mptShare.getMptID())); - if (mptIssuance == nullptr) - return false; // zero MPToken won't block deletion of MPTokenIssuance + return std::nullopt; +} - auto const issuer = mptIssuance->getAccountID(sfIssuer); +bool +isVaultPseudoAccountFrozenForIssuance( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth) +{ + XRPL_ASSERT( + issuanceSle.getType() == ltMPTOKEN_ISSUANCE, + "xrpl::isVaultPseudoAccountFrozenForIssuance : MPTokenIssuance SLE"); + + auto const issuer = issuanceSle.getAccountID(sfIssuer); // Post-fixCleanup3_2_0: vault shares carry sfReferenceHolding pointing // to the vault pseudo's MPToken or RippleState for the underlying. // Read it to derive the underlying asset and recurse, skipping the // issuer-account-then-vault chain. Pre-amendment shares (no field) // fall back to the chain lookup below. - if (mptIssuance->isFieldPresent(sfReferenceHolding)) + if (issuanceSle.isFieldPresent(sfReferenceHolding)) { auto const sleHolding = - view.read(keylet::unchecked(mptIssuance->getFieldH256(sfReferenceHolding))); + view.read(keylet::unchecked(issuanceSle.getFieldH256(sfReferenceHolding))); if (!sleHolding) { // LCOV_EXCL_START @@ -102,7 +111,7 @@ isVaultPseudoAccountFrozen( // LCOV_EXCL_STOP } return isAnyFrozen( - view, {issuer, account}, assetOfHolding(*mptIssuance, *sleHolding), depth + 1); + view, {issuer, account}, assetOfHolding(issuanceSle, *sleHolding), depth + 1); } auto const mptIssuer = view.read(keylet::account(issuer)); @@ -128,6 +137,38 @@ isVaultPseudoAccountFrozen( return isAnyFrozen(view, {issuer, account}, vault->at(sfAsset), depth + 1); } +} // namespace + +bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth) +{ + if (auto const result = checkVaultPseudoAccountFrozenPreconditions(view, depth)) + return *result; + + return isVaultPseudoAccountFrozenForIssuance(view, account, issuanceSle, depth); +} + +bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + MPTIssue const& mptShare, + std::uint8_t depth) +{ + if (auto const result = checkVaultPseudoAccountFrozenPreconditions(view, depth)) + return *result; + + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptShare.getMptID())); + if (issuanceSle == nullptr) + return false; // zero MPToken won't block deletion of MPTokenIssuance + + return isVaultPseudoAccountFrozenForIssuance(view, account, *issuanceSle, depth); +} + bool isLPTokenFrozen( ReadView const& view, diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index df6d335085..fcad22d2d5 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -633,7 +634,7 @@ ammAccountHolds(ReadView const& view, AccountID const& ammAccountID, Asset const return asset.visit( [&](MPTIssue const& issue) { if (auto const sle = view.read(keylet::mptoken(issue, ammAccountID)); - sle && !isFrozen(view, ammAccountID, issue)) + sle && !isFrozen(view, ammAccountID, *sle)) return STAmount{issue, (*sle)[sfMPTAmount]}; return STAmount{asset}; }, diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index b239d0d3d1..73d5fdb1d5 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -42,18 +42,35 @@ bool isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue) { if (auto const sle = view.read(keylet::mptokenIssuance(mptIssue.getMptID()))) - return sle->isFlag(lsfMPTLocked); + return isGlobalFrozen(*sle); return false; } +bool +isGlobalFrozen(SLE const& issuanceSle) +{ + XRPL_ASSERT( + issuanceSle.getType() == ltMPTOKEN_ISSUANCE, "xrpl::isGlobalFrozen : MPTokenIssuance SLE"); + + return issuanceSle.isFlag(lsfMPTLocked); +} + bool isIndividualFrozen(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue) { if (auto const sle = view.read(keylet::mptoken(mptIssue.getMptID(), account))) - return sle->isFlag(lsfMPTLocked); + return isIndividualFrozen(*sle); return false; } +bool +isIndividualFrozen(SLE const& mptSle) +{ + XRPL_ASSERT(mptSle.getType() == ltMPTOKEN, "xrpl::isIndividualFrozen : MPToken SLE"); + + return mptSle.isFlag(lsfMPTLocked); +} + bool isFrozen( ReadView const& view, @@ -65,6 +82,34 @@ isFrozen( isVaultPseudoAccountFrozen(view, account, mptIssue, depth); } +bool +isFrozen(ReadView const& view, AccountID const& account, SLE const& sle, std::uint8_t depth) +{ + XRPL_ASSERT( + sle.getType() == ltMPTOKEN || sle.getType() == ltMPTOKEN_ISSUANCE, + "xrpl::isFrozen : MPToken or MPTokenIssuance SLE"); + + if (sle.getType() == ltMPTOKEN) + { + XRPL_ASSERT(sle[sfAccount] == account, "xrpl::isFrozen : valid MPToken holder"); + + MPTID const mptID = sle[sfMPTokenIssuanceID]; + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptID)); + + if ((issuanceSle && isGlobalFrozen(*issuanceSle)) || isIndividualFrozen(sle)) + return true; + + if (issuanceSle) + return isVaultPseudoAccountFrozen(view, account, *issuanceSle, depth); + + return isVaultPseudoAccountFrozen(view, account, MPTIssue{mptID}, depth); + } + + MPTIssue const mptIssue{sle[sfSequence], sle[sfIssuer]}; + return isGlobalFrozen(sle) || isIndividualFrozen(view, account, mptIssue) || + isVaultPseudoAccountFrozen(view, account, sle, depth); +} + [[nodiscard]] bool isAnyFrozen( ReadView const& view, @@ -72,7 +117,8 @@ isAnyFrozen( MPTIssue const& mptIssue, std::uint8_t depth) { - if (isGlobalFrozen(view, mptIssue)) + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptIssue.getMptID())); + if (issuanceSle && isGlobalFrozen(*issuanceSle)) return true; for (auto const& account : accounts) @@ -81,9 +127,15 @@ isAnyFrozen( return true; } - return std::ranges::any_of(accounts, [&](auto const& account) { - return isVaultPseudoAccountFrozen(view, account, mptIssue, depth); - }); + // Pass the issuance SLE when we have it to avoid re-reading it per account; + // otherwise defer to the MPTIssue overload, which handles a missing issuance. + auto const anyVaultFrozen = [&](auto const& shareOrIssuance) { + return std::ranges::any_of(accounts, [&](auto const& account) { + return isVaultPseudoAccountFrozen(view, account, shareOrIssuance, depth); + }); + }; + + return issuanceSle ? anyVaultFrozen(*issuanceSle) : anyVaultFrozen(mptIssue); } Rate diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 9e3452ccae..7ebfa64bcf 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -439,7 +439,7 @@ accountHolds( auto const sleMpt = view.read(keylet::mptoken(mptIssue.getMptID(), account)); if (!sleMpt || - (zeroIfFrozen == FreezeHandling::ZeroIfFrozen && isFrozen(view, account, mptIssue))) + (zeroIfFrozen == FreezeHandling::ZeroIfFrozen && isFrozen(view, account, *sleMpt))) { amount.clear(mptIssue); } diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 12ec078c82..d323718bd2 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -900,7 +900,7 @@ ValidMPTTransfer::finalize( // Check once: if any involved account is frozen, the whole issuance transfer is // considered frozen. Only need to check for frozen if there is a transfer of funds. if (!invalidTransfer && - (isFrozen(view, account, MPTIssue{mptID}) || + (isFrozen(view, account, *sleIssuance) || !isAuthorized(view, mptID, account, reqAuth))) { invalidTransfer = true; diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp index 212f9da075..0fe27fb3ba 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp @@ -304,11 +304,11 @@ escrowCreatePreclaimHelper( return ter; // If the issuer has frozen the account, return tecLOCKED - if (isFrozen(ctx.view, account, mptIssue)) + if (isFrozen(ctx.view, account, *sleIssuance)) return tecLOCKED; // If the issuer has frozen the destination, return tecLOCKED - if (isFrozen(ctx.view, dest, mptIssue)) + if (isFrozen(ctx.view, dest, *sleIssuance)) return tecLOCKED; // If the mpt cannot be transferred, return tecNO_AUTH diff --git a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp index 32f4d9ec48..aa352d5e98 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp @@ -186,7 +186,7 @@ escrowFinishPreclaimHelper( return ter; // If the issuer has frozen the destination, return tecLOCKED - if (isFrozen(ctx.view, dest, mptIssue)) + if (isFrozen(ctx.view, dest, *sleIssuance)) return tecLOCKED; return tesSUCCESS; diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 90a267f56f..bfd2d529b5 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -27,6 +27,8 @@ #include #include #include +#include +#include #include #include #include @@ -7421,6 +7423,57 @@ private: } } + void + testDanglingAMMMPTokenFreezeCheck() + { + testcase("Dangling AMM MPToken freeze check"); + + using namespace jtx; + FeatureBitset const all{testableAmendments()}; + + Env env(*this, all); + + env.fund(XRP(1'000), gw_, alice_); + MPTTester usd({.env = env, .issuer = gw_}); + MPTTester const btc({.env = env, .issuer = gw_}); + + AMM amm(env, gw_, usd(10'000), btc(10'000)); + for (auto i = 0; i < kMaxDeletableAmmTrustLines + 10; ++i) + { + Account const a{std::to_string(i)}; + env.fund(XRP(1'000), a); + env(trust(a, STAmount{amm.lptIssue(), 10'000})); + env.close(); + } + + // With too many LP-token trust lines to delete in one pass, the AMM + // remains in an empty state with zero-balance MPToken objects. + amm.withdrawAll(gw_); + BEAST_EXPECT(amm.ammExists()); + BEAST_EXPECT(amm.expectBalances(usd(0), btc(0), IOUAmount{0})); + + auto const ammToken = env.le(keylet::mptoken(usd.issuanceID(), amm.ammAccount())); + if (!BEAST_EXPECT(ammToken)) + return; + BEAST_EXPECT((*ammToken)[sfMPTAmount] == 0); + + usd.destroy(); + BEAST_EXPECT(env.le(keylet::mptokenIssuance(usd.issuanceID())) == nullptr); + BEAST_EXPECT(!isFrozen(*env.current(), amm.ammAccount(), *ammToken)); + // A Payment cannot cross this empty AMM because BookStep skips AMMs + // with zero LPTokenBalance. Probe the same ZeroIfFrozen balance read + // used by AMM accounting. + auto const balance = accountHolds( + *env.current(), + amm.ammAccount(), + MPTIssue{usd.issuanceID()}, + FreezeHandling::ZeroIfFrozen, + AuthHandling::IgnoreAuth, + env.journal); + + BEAST_EXPECT(balance == usd(0)); + } + void run() override { @@ -7461,6 +7514,7 @@ private: testDepositIntegralOverflowMPT(all); testDepositIntegralOverflowMPT(all - fixCleanup3_4_0); testWithdrawIntegralNoOverflowMPT(); + testDanglingAMMMPTokenFreezeCheck(); } }; From 820ca5b33201c67d290d5c16fa2419121ee76de0 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 17 Aug 2026 23:19:56 +0000 Subject: [PATCH 10/13] refactor: Convert boost::beast::string_view to std::string_view (#6306) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com> Co-authored-by: Mayukha Vadari Co-authored-by: Ayaz Salikhov Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> Co-authored-by: Mayukha Vadari Co-authored-by: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Co-authored-by: Cursor Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> --- include/xrpl/beast/rfc2616.h | 3 ++- include/xrpl/config/BasicConfig.h | 1 - include/xrpl/json/Output.h | 7 +++---- include/xrpl/server/detail/BaseWSPeer.h | 16 ++++++++-------- src/libxrpl/json/Writer.cpp | 5 +++-- src/xrpld/overlay/detail/ProtocolVersion.cpp | 5 +++-- src/xrpld/overlay/detail/ProtocolVersion.h | 7 +++---- src/xrpld/rpc/detail/ServerHandler.cpp | 6 +++--- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/include/xrpl/beast/rfc2616.h b/include/xrpl/beast/rfc2616.h index 0e061845fb..87d63b0260 100644 --- a/include/xrpl/beast/rfc2616.h +++ b/include/xrpl/beast/rfc2616.h @@ -11,6 +11,7 @@ #include #include #include +#include #include namespace beast::rfc2616 { @@ -186,7 +187,7 @@ splitCommas(FwdIt first, FwdIt last) template > Result -splitCommas(boost::beast::string_view const& s) +splitCommas(std::string_view s) { return splitCommas(s.begin(), s.end()); } diff --git a/include/xrpl/config/BasicConfig.h b/include/xrpl/config/BasicConfig.h index 607a0c3e5f..2278a0fa68 100644 --- a/include/xrpl/config/BasicConfig.h +++ b/include/xrpl/config/BasicConfig.h @@ -2,7 +2,6 @@ #include -#include #include #include diff --git a/include/xrpl/json/Output.h b/include/xrpl/json/Output.h index 53d453c277..f73bd38c77 100644 --- a/include/xrpl/json/Output.h +++ b/include/xrpl/json/Output.h @@ -1,20 +1,19 @@ #pragma once -#include - #include #include +#include namespace json { class Value; -using Output = std::function; +using Output = std::function; inline Output stringOutput(std::string& s) { - return [&](boost::beast::string_view const& b) { s.append(b.data(), b.size()); }; + return [&](std::string_view b) { s.append(b.data(), b.size()); }; } /** diff --git a/include/xrpl/server/detail/BaseWSPeer.h b/include/xrpl/server/detail/BaseWSPeer.h index b1670865bd..403d7f92ee 100644 --- a/include/xrpl/server/detail/BaseWSPeer.h +++ b/include/xrpl/server/detail/BaseWSPeer.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -62,8 +63,7 @@ private: bool pingActive_ = false; boost::beast::websocket::ping_data payload_; error_code ec_; - std::function - controlCallback_; + std::function controlCallback_; public: template @@ -151,7 +151,7 @@ protected: onPing(error_code const& ec); void - onPingPong(boost::beast::websocket::frame_type kind, boost::beast::string_view payload); + onPingPong(boost::beast::websocket::frame_type kind, std::string_view payload); void onTimer(error_code ec); @@ -189,9 +189,9 @@ BaseWSPeer::run() impl().ws_.set_option(port().pmdOptions); // Must manage the control callback memory outside of the `control_callback` // function - controlCallback_ = [this]( - boost::beast::websocket::frame_type kind, - boost::beast::string_view payload) { onPingPong(kind, payload); }; + controlCallback_ = [this](boost::beast::websocket::frame_type kind, std::string_view payload) { + onPingPong(kind, payload); + }; impl().ws_.control_callback(controlCallback_); startTimer(); closeOnTimer_ = true; @@ -430,11 +430,11 @@ template void BaseWSPeer::onPingPong( boost::beast::websocket::frame_type kind, - boost::beast::string_view payload) + std::string_view payload) { if (kind == boost::beast::websocket::frame_type::pong) { - boost::beast::string_view const p(payload_.begin()); + std::string_view const p(payload_.begin(), payload_.size()); if (payload == p) { closeOnTimer_ = false; diff --git a/src/libxrpl/json/Writer.cpp b/src/libxrpl/json/Writer.cpp index 4c922a0e33..c5ce4666ef 100644 --- a/src/libxrpl/json/Writer.cpp +++ b/src/libxrpl/json/Writer.cpp @@ -9,6 +9,7 @@ #include // IWYU pragma: keep #include #include +#include #include #include @@ -87,14 +88,14 @@ public: } void - output(boost::beast::string_view const& bytes) + output(std::string_view bytes) { markStarted(); output_(bytes); } void - stringOutput(boost::beast::string_view const& bytes) + stringOutput(std::string_view bytes) { markStarted(); std::size_t position = 0, writtenUntil = 0; diff --git a/src/xrpld/overlay/detail/ProtocolVersion.cpp b/src/xrpld/overlay/detail/ProtocolVersion.cpp index 1296041ad5..74dad61828 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.cpp +++ b/src/xrpld/overlay/detail/ProtocolVersion.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include namespace xrpl { @@ -52,7 +53,7 @@ to_string(ProtocolVersion const& p) } std::vector -parseProtocolVersions(boost::beast::string_view const& value) +parseProtocolVersions(std::string_view value) { static boost::regex const kRE( "^" // start of line @@ -119,7 +120,7 @@ negotiateProtocolVersion(std::vector const& versions) } std::optional -negotiateProtocolVersion(boost::beast::string_view const& versions) +negotiateProtocolVersion(std::string_view versions) { auto const them = parseProtocolVersions(versions); diff --git a/src/xrpld/overlay/detail/ProtocolVersion.h b/src/xrpld/overlay/detail/ProtocolVersion.h index b56871318a..5c05f63e2a 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.h +++ b/src/xrpld/overlay/detail/ProtocolVersion.h @@ -1,10 +1,9 @@ #pragma once -#include - #include #include #include +#include #include #include @@ -43,7 +42,7 @@ to_string(ProtocolVersion const& p); * no duplicates and will be sorted in ascending protocol order. */ std::vector -parseProtocolVersions(boost::beast::string_view const& s); +parseProtocolVersions(std::string_view s); /** * Given a list of supported protocol versions, choose the one we prefer. @@ -55,7 +54,7 @@ negotiateProtocolVersion(std::vector const& versions); * Given a list of supported protocol versions, choose the one we prefer. */ std::optional -negotiateProtocolVersion(boost::beast::string_view const& versions); +negotiateProtocolVersion(std::string_view versions); /** * The list of all the protocol versions we support. diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 827d8705fd..28e7eebd63 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -264,7 +264,7 @@ ServerHandler::onHandoff( static inline json::Output makeOutput(Session& session) { - return [&](boost::beast::string_view const& b) { session.write(b.data(), b.size()); }; + return [&](std::string_view b) { session.write(b.data(), b.size()); }; } static std::map @@ -564,11 +564,11 @@ ServerHandler::processSession( makeOutput(*session), coro, forwardedFor(session->request()), - [&] { + [&] -> std::string_view { auto const iter = session->request().find("X-User"); if (iter != session->request().end()) return iter->value(); - return boost::beast::string_view{}; + return {}; }()); if (beast::rfc2616::isKeepAlive(session->request())) From dd0edc19a05b62e7d3ed40d11222966a021ba4f4 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:09:33 +0000 Subject: [PATCH 11/13] fix: Conserve funds correctly when LoanPay fee payee is below reserve (#7843) --- .../tx/transactors/lending/LoanPay.cpp | 88 +++++++------- src/test/app/lending/LoanPay_test.cpp | 107 ++++++++++++++++++ 2 files changed, 146 insertions(+), 49 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index 4619540295..c5bfd8e9ee 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -9,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -33,6 +36,34 @@ namespace xrpl { +namespace { +// 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 +// subtracts the account's reserve, so a payee sitting below its own reserve +// would appear to receive nothing even though its raw ledger balance grew. +// That mismatch is exactly what a conservation check must not see. +STAmount +conservationBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j) +{ + if (isXRP(asset)) + { + auto const sle = view.read(keylet::account(id)); + if (!sle) + return STAmount{asset}; // LCOV_EXCL_LINE + return view.balanceHookIOU(id, xrpAccount(), sle->getFieldAmount(sfBalance)); + } + return accountHolds( + view, + id, + asset, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j, + SpendableHandling::FullBalance); +} +} // namespace + bool LoanPay::checkExtraFeatures(PreflightContext const& ctx) { @@ -581,34 +612,13 @@ LoanPay::doApply() } // These three values are used to check that funds are conserved after the transfers - auto const accountBalanceBefore = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceBefore = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceBefore = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); auto const brokerBalanceBefore = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, brokerPayee, asset, j_); if (totalPaidToVaultRounded != beast::kZero) { @@ -664,33 +674,13 @@ LoanPay::doApply() #endif // Check that funds are conserved - auto const accountBalanceAfter = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceAfter = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceAfter = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); - auto const brokerBalanceAfter = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); + auto const brokerBalanceAfter = accountID_ == brokerPayee + ? STAmount{asset, 0} + : conservationBalance(view, brokerPayee, asset, j_); auto const balanceScale = [&]() { // Find a reasonable scale to use for the balance comparisons. // diff --git a/src/test/app/lending/LoanPay_test.cpp b/src/test/app/lending/LoanPay_test.cpp index 9d840fe1bf..93d1671feb 100644 --- a/src/test/app/lending/LoanPay_test.cpp +++ b/src/test/app/lending/LoanPay_test.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -13,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -728,6 +730,110 @@ private: } } + void + testLoanPayFundsConservedPayeeBelowReserve(FeatureBitset features) + { + // Regression test: LoanPay::doApply's fund-conservation check used to + // read XRP balances via accountHolds(..., SpendableHandling:: + // FullBalance), which for XRP always defers to xrpLiquid (balance + // minus reserve, clamped at zero). When the broker fee landed on a + // payee sitting below its own reserve, that payee's clamped balance + // stayed zero and the fee vanished from the conservation sum, + // tripping "funds are conserved (with rounding)". + testcase("LoanPay funds conserved: broker fee payee below reserve"); + + using namespace jtx; + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + // Broker defaults match the fuzz workload: ManagementFeeRate = 100 + // tenth-bips. The service fee guarantees feePaid > 0 on the first + // regular payment. + BrokerParameters const brokerParams; + Number const serviceFeeValue{2}; + LoanParameters const loanParams{ + .account = borrower, + .counter = lender, + .principalRequest = 1000, + .serviceFee = serviceFeeValue, + .interest = TenthBips32{percentageToTenthBips(12)}, + .payTotal = 12, + .payInterval = 3600}; + + auto const loanOpt = + createLoan(env, AssetType::XRP, brokerParams, loanParams, issuer, lender, borrower); + if (BEAST_EXPECT(loanOpt); !loanOpt.has_value()) + return; + auto const& [broker, loanKeylet, brokerPseudo] = *loanOpt; + + auto const vaultPseudo = [&]() { + auto const vaultSle = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(vaultSle)) + return AccountID{}; + return vaultSle->at(sfAccount); + }(); + + // Raw AccountRoot balance, matching LoanPay::doApply's conservation + // check (not the reserve-clamped accountHolds()/xrpLiquid() value). + auto rawBalance = [&](AccountID const& id) -> STAmount { + auto const sle = env.le(keylet::account(id)); + if (!BEAST_EXPECT(sle)) + return STAmount{}; + return sle->getFieldAmount(sfBalance); + }; + auto lenderReserve = [&] { + return env.current()->fees().accountReserve(ownerCount(env, lender), 1); + }; + + STAmount const baseFee{env.current()->fees().base}; + + // Park the lender (broker owner, fee payee) exactly at its reserve, + // then burn part of the reserve with an oversized transaction fee. + // Fees are exempt from the reserve check, so the balance ends up + // below the reserve. + env(pay(lender, issuer, rawBalance(lender.id()) - lenderReserve() - baseFee)); + env(noop(lender), Fee(XRP(100))); + env.close(); + BEAST_EXPECT(env.balance(lender) < lenderReserve()); + + // First regular payment, exactly the amount due. + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const serviceFee = broker.asset(serviceFeeValue); + STAmount const roundedPeriodicPayment{ + broker.asset, + roundPeriodicPayment(broker.asset, state.periodicPayment, state.loanScale)}; + STAmount const totalDue = roundToScale( + roundedPeriodicPayment + serviceFee, state.loanScale, Number::RoundingMode::Upward); + + auto const borrowerBefore = rawBalance(borrower.id()); + auto const vaultBefore = rawBalance(vaultPseudo); + auto const lenderBefore = rawBalance(lender.id()); + + // Before the fix, this aborted inside LoanPay::doApply on + // XRPL_ASSERT_PARTS(goodRounding, "xrpl::LoanPay::doApply", "funds + // are conserved (with rounding)"). + env(loan::pay(borrower, loanKeylet.key, totalDue)); + env.close(); + + auto const borrowerAfter = rawBalance(borrower.id()); + auto const vaultAfter = rawBalance(vaultPseudo); + auto const lenderAfter = rawBalance(lender.id()); + + // The broker fee reached the lender's AccountRoot, even though the + // lender's balance remains below its reserve. + BEAST_EXPECT(lenderAfter > lenderBefore); + BEAST_EXPECT(lenderAfter < lenderReserve()); + + // Total funds conserved across the payer, vault, and fee payee. + BEAST_EXPECT( + borrowerBefore - baseFee + vaultBefore + lenderBefore == + borrowerAfter + vaultAfter + lenderAfter); + } + void runAmendmentIndependent() { @@ -741,6 +847,7 @@ private: #if LOAN_TODO testLoanPayLateFullPaymentBypassesPenalties(features); #endif + testLoanPayFundsConservedPayeeBelowReserve(features); testOverpaymentManagementFee(features); testDosLoanPay(features); testLoanNextPaymentDueDateOverflow(features); From ca39bff3c829add41d8191886450190e4d50e465 Mon Sep 17 00:00:00 2001 From: Bart Date: Tue, 18 Aug 2026 12:35:32 +0000 Subject: [PATCH 12/13] refactor: Add `SHAMapNodeID::isPrefixOf` (#7939) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- include/xrpl/shamap/SHAMapNodeID.h | 14 ++++++++++++++ src/libxrpl/shamap/SHAMapNodeID.cpp | 11 ++++++++--- src/libxrpl/shamap/SHAMapSync.cpp | 7 +++---- src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp | 5 ++--- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/include/xrpl/shamap/SHAMapNodeID.h b/include/xrpl/shamap/SHAMapNodeID.h index fcd5a4d00e..f35ba2d2a7 100644 --- a/include/xrpl/shamap/SHAMapNodeID.h +++ b/include/xrpl/shamap/SHAMapNodeID.h @@ -55,6 +55,20 @@ public: [[nodiscard]] SHAMapNodeID getChildNodeID(unsigned int branch) const; + /** + * Test whether this node ID lies on the path to the given leaf key + * + * A node at depth d identifies the tree path spelled by the first d + * nibbles of its key, so any leaf beneath it must agree on that prefix. + * A node ID that fails this test names a different subtree than the one + * it was built for. + * + * @param key the key of a leaf below this node + * @return whether this node ID is a prefix of the leaf key + */ + [[nodiscard]] bool + isPrefixOf(uint256 const& key) const; + /** * Create a SHAMapNodeID of a node with the depth of the node and * the key of a leaf diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index ecde22a63d..8fd7afe8fc 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -46,8 +46,7 @@ SHAMapNodeID::SHAMapNodeID(unsigned int depth, uint256 const& hash) : id_(hash), XRPL_ASSERT( depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::SHAMapNodeID : maximum depth input"); XRPL_ASSERT( - id_ == (id_ & depthMask(depth)), - "xrpl::SHAMapNodeID::SHAMapNodeID : hash and depth inputs do match"); + isPrefixOf(id_), "xrpl::SHAMapNodeID::SHAMapNodeID : hash and depth inputs do match"); } std::string @@ -79,7 +78,7 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const if (depth_ >= SHAMap::kLeafDepth) Throw("Request for child node ID of " + to_string(*this)); - if (id_ != (id_ & depthMask(depth_))) + if (!isPrefixOf(id_)) Throw("Incorrect mask for " + to_string(*this)); SHAMapNodeID node{depth_ + 1, id_}; @@ -87,6 +86,12 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const return node; } +bool +SHAMapNodeID::isPrefixOf(uint256 const& key) const +{ + return (key & depthMask(depth_)) == id_; +} + [[nodiscard]] std::optional deserializeSHAMapNodeID(void const* data, std::size_t size) { diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index e6948ec3ac..a12e524a5f 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -555,10 +555,9 @@ SHAMap::addKnownNode( { XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node"); XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node"); - XRPL_ASSERT( - !treeNode->isLeaf() || - SHAMapNodeID::createID(nodeID.getDepth(), leafKey(*treeNode)).getNodeID() == - nodeID.getNodeID(), + XRPL_ASSERT_IF( + treeNode->isLeaf(), + nodeID.isPrefixOf(leafKey(*treeNode)), "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID"); if (!isSynching()) diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index abd669d446..230c802022 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -75,11 +75,10 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& if (treeNode.isLeaf()) { auto const key = leafKey(treeNode); - auto const expectedID = SHAMapNodeID::createID(nodeID->getDepth(), key); SOMETIMES( - nodeID->getNodeID() != expectedID.getNodeID(), + !nodeID->isPrefixOf(key), "xrpl::getSHAMapNodeID : legacy leaf ID inconsistent with key"); - if (nodeID->getNodeID() != expectedID.getNodeID()) + if (!nodeID->isPrefixOf(key)) return std::nullopt; } From f5f47f1cf55960d318330d41dc4b9238451657a1 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 18 Aug 2026 15:03:45 +0000 Subject: [PATCH 13/13] chore: Publish debian/rpm packages from GitHub directly (#8031) --- .cspell.config.yaml | 3 + .github/actions/generate-version/action.yml | 44 -------- .github/actions/release-info/action.yml | 90 +++++++++++++++ .github/dependabot.yml | 2 +- .github/scripts/strategy-matrix/linux.json | 4 +- .github/workflows/on-pr.yml | 2 +- .github/workflows/on-tag.yml | 17 ++- .github/workflows/on-trigger.yml | 9 +- .../workflows/reusable-build-test-config.yml | 7 +- .github/workflows/reusable-package.yml | 49 ++++++-- .github/workflows/reusable-upload-recipe.yml | 12 +- package/README.md | 63 +++++++++-- package/build_pkg.sh | 32 +++--- package/publish_pkg.sh | 106 ++++++++++++++++++ 14 files changed, 343 insertions(+), 97 deletions(-) delete mode 100644 .github/actions/generate-version/action.yml create mode 100644 .github/actions/release-info/action.yml create mode 100755 package/publish_pkg.sh diff --git a/.cspell.config.yaml b/.cspell.config.yaml index ec9f87cfdd..e194ee21f8 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -250,6 +250,8 @@ words: - Raphson - rcflags - replayer + - repodata + - repomd - rerandomize - rerandomization - rerandomized @@ -290,6 +292,7 @@ words: - sles - soci - socidb + - Sonatype - sponsee - sponsees - SRPMS diff --git a/.github/actions/generate-version/action.yml b/.github/actions/generate-version/action.yml deleted file mode 100644 index 50b3166596..0000000000 --- a/.github/actions/generate-version/action.yml +++ /dev/null @@ -1,44 +0,0 @@ -name: Generate build version number -description: "Generate build version number." - -outputs: - version: - description: "The generated build version number." - value: ${{ steps.version.outputs.version }} - -runs: - using: composite - steps: - # When a tag is pushed, the version is used as-is. - - name: Generate version for tag event - if: ${{ startsWith(github.ref, 'refs/tags/') }} - shell: bash - env: - VERSION: ${{ github.ref_name }} - run: echo "VERSION=${VERSION}" >>"${GITHUB_ENV}" - - # When a tag is not pushed, then the version (e.g. 1.2.3-b0) is extracted - # from the BuildInfo.cpp file and the shortened commit hash appended to it. - # We use a plus sign instead of a hyphen because Conan recipe versions do - # not support two hyphens. - - name: Generate version for non-tag event - if: ${{ !startsWith(github.ref, 'refs/tags/') }} - shell: bash - run: | - echo 'Extracting version from BuildInfo.cpp.' - VERSION="$(cat src/libxrpl/protocol/BuildInfo.cpp | grep "versionString =" | awk -F '"' '{print $2}')" - if [[ -z "${VERSION}" ]]; then - echo 'Unable to extract version from BuildInfo.cpp.' - exit 1 - fi - - echo 'Appending shortened commit hash to version.' - SHA='${{ github.sha }}' - VERSION="${VERSION}+${SHA:0:7}" - - echo "VERSION=${VERSION}" >>"${GITHUB_ENV}" - - - name: Output version - id: version - shell: bash - run: echo "version=${VERSION}" >>"${GITHUB_OUTPUT}" diff --git a/.github/actions/release-info/action.yml b/.github/actions/release-info/action.yml new file mode 100644 index 0000000000..7f1061df93 --- /dev/null +++ b/.github/actions/release-info/action.yml @@ -0,0 +1,90 @@ +name: Release info +description: "Derive the version, release channel and package release number for this build." + +outputs: + version: + description: "The build version number." + value: ${{ steps.version.outputs.version }} + channel: + description: "The release channel this build belongs to." + value: ${{ steps.channel.outputs.channel }} + pkg_release: + description: "The package release number: 1 for a tag, the run number otherwise." + value: ${{ steps.pkg_release.outputs.pkg_release }} + +runs: + using: composite + steps: + # A tag names its own version. Anything else takes it from BuildInfo.cpp and + # appends the commit hash as build metadata, joined with a plus sign because a + # Conan version cannot contain two hyphens. + - name: Determine version + id: version + shell: bash + env: + IS_TAG: ${{ startsWith(github.ref, 'refs/tags/') }} + REF_NAME: ${{ github.ref_name }} + SHA: ${{ github.sha }} + run: | + if [[ "${IS_TAG}" == "true" ]]; then + version="${REF_NAME}" + else + version="$(awk -F'"' '/versionString =/ { print $2 }' src/libxrpl/protocol/BuildInfo.cpp)" + if [[ -z "${version}" ]]; then + echo "Unable to read versionString from BuildInfo.cpp." >&2 + exit 1 + fi + version="${version}+${SHA:0:7}" + fi + + echo "version=${version}" | tee -a "${GITHUB_OUTPUT}" + + # Only a tag says how mature a build is: a push is a develop build whatever + # its version, and a non-public codebase keeps its packages to itself. + - name: Determine release channel + id: channel + shell: bash + env: + IS_TAG: ${{ startsWith(github.ref, 'refs/tags/') }} + REF_NAME: ${{ github.ref_name }} + VISIBILITY: ${{ github.event.repository.visibility }} + run: | + pre_release="" + if [[ "${REF_NAME}" == *-* ]]; then + pre_release="${REF_NAME#*-}" + fi + + if [[ "${VISIBILITY}" != "public" ]]; then + channel=private + elif [[ "${IS_TAG}" != "true" ]]; then + channel=develop + elif [[ -z "${pre_release}" ]]; then + channel=stable + elif [[ "${pre_release}" =~ ^rc[0-9]+(\+.*)?$ ]]; then + channel=unstable + elif [[ "${pre_release}" =~ ^b(0|[1-9][0-9]*)(\+.*)?$ ]]; then + channel=experimental + else + echo "Unsupported pre-release in tag '${REF_NAME}'. Use bN or rcN." >&2 + exit 1 + fi + + echo "channel=${channel}" | tee -a "${GITHUB_OUTPUT}" + + # A tag is packaged once, so its release number is fixed at 1. Develop builds + # repeat the same version, so the run number is what makes each push an + # upgrade rather than a reinstall. + - name: Determine package release + id: pkg_release + shell: bash + env: + IS_TAG: ${{ startsWith(github.ref, 'refs/tags/') }} + RUN_NUMBER: ${{ github.run_number }} + run: | + if [[ "${IS_TAG}" == "true" ]]; then + pkg_release=1 + else + pkg_release="${RUN_NUMBER}" + fi + + echo "pkg_release=${pkg_release}" | tee -a "${GITHUB_OUTPUT}" diff --git a/.github/dependabot.yml b/.github/dependabot.yml index fcac44c44c..1ccbd61102 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -4,7 +4,7 @@ updates: directories: - / - .github/actions/build-deps/ - - .github/actions/generate-version/ + - .github/actions/release-info/ - .github/actions/set-compiler-env/ - .github/actions/setup-conan/ schedule: diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index 97163fb8ce..bd3446f599 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -92,7 +92,7 @@ "build_type": ["Release"], "arch": ["amd64"], "minimal": false, - "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-577d745" + "image": "ghcr.io/xrplf/xrpld/packaging-debian:sha-028ccea" } ], @@ -102,7 +102,7 @@ "build_type": ["Release"], "arch": ["amd64"], "minimal": false, - "image": "ghcr.io/xrplf/xrpld/packaging-rhel:sha-577d745" + "image": "ghcr.io/xrplf/xrpld/packaging-rhel:sha-028ccea" } ] } diff --git a/.github/workflows/on-pr.yml b/.github/workflows/on-pr.yml index 0a4e4b1f49..f14256b9e8 100644 --- a/.github/workflows/on-pr.yml +++ b/.github/workflows/on-pr.yml @@ -77,7 +77,7 @@ jobs: # Keep the paths below in sync with those in `on-trigger.yml`. .github/actions/build-deps/** - .github/actions/generate-version/** + .github/actions/release-info/** .github/actions/setup-conan/** .github/scripts/strategy-matrix/** .github/workflows/reusable-build-test-config.yml diff --git a/.github/workflows/on-tag.yml b/.github/workflows/on-tag.yml index abedc13d69..1c9fb414f2 100644 --- a/.github/workflows/on-tag.yml +++ b/.github/workflows/on-tag.yml @@ -1,5 +1,9 @@ -# This workflow uploads the libxrpl recipe to the Conan remote and builds -# release packages when a versioned tag is pushed. +# When a versioned tag is pushed, this workflow: +# +# - uploads the libxrpl recipe to the Conan remote +# - builds and tests the release binaries +# - builds the DEB and RPM packages +# - publishes those packages to the XRPLF package repositories name: Tag on: @@ -24,7 +28,7 @@ jobs: remote_password: ${{ secrets.NEXUS_REMOTE_PASSWORD }} build-test: - if: ${{ github.repository == 'XRPLF/rippled' }} + if: ${{ github.repository_owner == 'XRPLF' }} uses: ./.github/workflows/reusable-build-test.yml strategy: fail-fast: true @@ -37,6 +41,11 @@ jobs: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} package: - if: ${{ github.repository == 'XRPLF/rippled' }} + if: ${{ github.repository_owner == 'XRPLF' }} needs: build-test uses: ./.github/workflows/reusable-package.yml + with: + publish: true + secrets: + remote_username: ${{ secrets.NEXUS_REMOTE_USERNAME }} + remote_password: ${{ secrets.NEXUS_REMOTE_PASSWORD }} diff --git a/.github/workflows/on-trigger.yml b/.github/workflows/on-trigger.yml index 73f918d528..dcd14b7933 100644 --- a/.github/workflows/on-trigger.yml +++ b/.github/workflows/on-trigger.yml @@ -15,7 +15,7 @@ on: # Keep the paths below in sync with those in `on-pr.yml`. - ".github/actions/build-deps/**" - - ".github/actions/generate-version/**" + - ".github/actions/release-info/**" - ".github/actions/setup-conan/**" - ".github/scripts/strategy-matrix/**" - ".github/workflows/reusable-build-test-config.yml" @@ -108,3 +108,10 @@ jobs: package: needs: build-test uses: ./.github/workflows/reusable-package.yml + with: + # Packages are built on every trigger; only develop pushes in XRPLF/rippled + # publish them, matching upload-recipe above. + publish: ${{ github.repository == 'XRPLF/rippled' && github.event_name == 'push' && github.ref == 'refs/heads/develop' }} + secrets: + remote_username: ${{ secrets.NEXUS_REMOTE_USERNAME }} + remote_password: ${{ secrets.NEXUS_REMOTE_PASSWORD }} diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index d8550efc4c..7989d2c7f6 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -111,6 +111,9 @@ jobs: VOIDSTAR_ENABLED: ${{ contains(inputs.cmake_args, '-Dvoidstar=ON') }} VALIDATOR_KEYS_ENABLED: ${{ contains(inputs.cmake_args, '-Dvalidator_keys=ON') }} SANITIZERS_ENABLED: ${{ inputs.sanitizers != '' }} + # The binaries reusable-package.yml consumes. A private repository skips + # them except on a tag push, which is what produces its release packages. + PACKAGING_ARTIFACTS_ENABLED: ${{ github.event.repository.visibility == 'public' || startsWith(github.ref, 'refs/tags/') }} steps: - name: Cleanup workspace (macOS and Windows) if: ${{ runner.os == 'macOS' || runner.os == 'Windows' }} @@ -222,7 +225,7 @@ jobs: fi - name: Upload the binary (Linux) - if: ${{ github.event.repository.visibility == 'public' && runner.os == 'Linux' }} + if: ${{ env.PACKAGING_ARTIFACTS_ENABLED == 'true' && runner.os == 'Linux' }} uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: xrpld-${{ inputs.config_name }} @@ -236,7 +239,7 @@ jobs: run: ./validator-keys --unittest - name: Upload the validator-keys binary - if: ${{ github.event.repository.visibility == 'public' && env.VALIDATOR_KEYS_ENABLED == 'true' }} + if: ${{ env.PACKAGING_ARTIFACTS_ENABLED == 'true' && env.VALIDATOR_KEYS_ENABLED == 'true' }} uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: validator-keys-${{ inputs.config_name }} diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index b45cae52d9..430072b627 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -1,17 +1,34 @@ -# Build Linux packages (DEB and RPM) from pre-built binary artifacts (xrpld and -# validator-keys). Discovers which configurations to package from linux.json -# (configs in "package_configs") and fans out one job per distro. Only -# linux/amd64 is supported; the runner is hardcoded in the job below. +# Build Linux packages from the pre-built xrpld and validator-keys artifacts: +# +# - one job per distro, taken from "package_configs" in linux.json +# - each job runs in that distro's container, which is what decides DEB or RPM +# - with 'publish: true' a job also uploads what it built +# (see package/publish_pkg.sh) +# +# Only linux/amd64 is supported; the runner is hardcoded in the job below. name: Package on: workflow_call: inputs: - pkg_release: - description: "Package release number. Increment when repackaging the same executable." + publish: + description: "Whether to publish the packages after building them." + required: false + type: boolean + default: false + nexus_url: + description: "The base URL of the Nexus instance hosting the deb and rpm repositories." required: false type: string - default: "1" + default: https://packages.xrplf.org + + secrets: + remote_username: + description: "The username of a Nexus account with write access to the repositories." + required: false + remote_password: + description: "The password or token for that Nexus account." + required: false defaults: run: @@ -41,7 +58,7 @@ jobs: package: needs: [generate-matrix] - if: ${{ github.event.repository.visibility == 'public' }} + if: ${{ github.event.repository.visibility == 'public' || startsWith(github.ref, 'refs/tags/') }} strategy: fail-fast: false matrix: ${{ fromJson(needs.generate-matrix.outputs.matrix) }} @@ -71,9 +88,14 @@ jobs: - name: Make binaries executable run: chmod +x "${BUILD_DIR}/xrpld" "${BUILD_DIR}/validator-keys" + - name: Determine release info + id: release_info + uses: ./.github/actions/release-info + - name: Build package env: - PKG_RELEASE: ${{ inputs.pkg_release }} + PKG_RELEASE: ${{ steps.release_info.outputs.pkg_release }} + PKG_CHANNEL: ${{ steps.release_info.outputs.channel }} run: ./package/build_pkg.sh - name: Upload package artifact @@ -85,3 +107,12 @@ jobs: ${{ env.BUILD_DIR }}/debbuild/*.ddeb ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/*.rpm if-no-files-found: error + + - name: Publish package + if: ${{ inputs.publish }} + env: + CHANNEL: ${{ steps.release_info.outputs.channel }} + NEXUS_URL: ${{ inputs.nexus_url }} + NEXUS_USERNAME: ${{ secrets.remote_username }} + NEXUS_PASSWORD: ${{ secrets.remote_password }} + run: ./package/publish_pkg.sh "${CHANNEL}" "${BUILD_DIR}" diff --git a/.github/workflows/reusable-upload-recipe.yml b/.github/workflows/reusable-upload-recipe.yml index a8d35fadad..680d95fb97 100644 --- a/.github/workflows/reusable-upload-recipe.yml +++ b/.github/workflows/reusable-upload-recipe.yml @@ -49,9 +49,9 @@ jobs: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - name: Generate build version number - id: version - uses: ./.github/actions/generate-version + - name: Determine release info + id: release_info + uses: ./.github/actions/release-info - name: Set up Conan uses: ./.github/actions/setup-conan @@ -64,8 +64,8 @@ jobs: - name: Upload Conan recipe (version) run: | - conan export . --version=${{ steps.version.outputs.version }} - conan upload --confirm --check --remote="${REMOTE_NAME}" xrpl/${{ steps.version.outputs.version }} + conan export . --version=${{ steps.release_info.outputs.version }} + conan upload --confirm --check --remote="${REMOTE_NAME}" xrpl/${{ steps.release_info.outputs.version }} # When this workflow is triggered by a push event, it will always be when merging into the # 'develop' branch, see on-trigger.yml. @@ -92,4 +92,4 @@ jobs: conan upload --confirm --check --remote="${REMOTE_NAME}" xrpl/release outputs: - ref: xrpl/${{ steps.version.outputs.version }} + ref: xrpl/${{ steps.release_info.outputs.version }} diff --git a/package/README.md b/package/README.md index 887509b60b..4899ee203e 100644 --- a/package/README.md +++ b/package/README.md @@ -8,7 +8,8 @@ a build configured with `-Dvalidator_keys=ON`. ``` package/ - build_pkg.sh Staging and build script (called by the CMake `package` target and CI) + build_pkg.sh Staging and build script (called by the CMake `package` target and CI) + publish_pkg.sh Uploads built packages to the XRPLF Nexus repositories (called by CI) rpm/ xrpld.spec RPM spec debian/ Debian control files (control, rules, copyright, xrpld.docs, xrpld.links, source/format) @@ -87,7 +88,7 @@ docker run --rm \ ./package/build_pkg.sh --pkg-release "${PKG_RELEASE}" # Output: -# build/debbuild/*.deb (DEB + dbgsym .ddeb) +# build/debbuild/*.deb (DEB + dbgsym; Debian names both .deb) # build/rpmbuild/RPMS/x86_64/*.rpm ``` @@ -120,6 +121,50 @@ The package version is not a CMake input on this path: `build_pkg.sh` 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`. +## Publishing packages + +Packages are published to the XRPLF repositories on Sonatype Nexus at +`https://packages.xrplf.org`. The `release-info` action decides the channel from +the event, and `publish_pkg.sh` maps that channel to a repository pair: + +| Event | Version | Channel | DEB repository | RPM repository | +| ------------------------ | ----------------- | -------------- | ------------------ | ------------------ | +| tag | `X.Y.Z` | `stable` | `deb-stable` | `rpm-stable` | +| tag | `X.Y.Z-rcN` | `unstable` | `deb-unstable` | `rpm-unstable` | +| tag | `X.Y.Z-bN` | `experimental` | `deb-experimental` | `rpm-experimental` | +| push to `develop` | `xrpld --version` | `develop` | `deb-develop` | `rpm-develop` | +| tag, non-public codebase | _any_ | `private` | `deb-private` | `rpm-private` | + +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`. +Versions sort in row order, so moving to a more mature channel never downgrades. + +The action decides the package release number on the same split: a tag's version +is unique, so its packages are release 1, while develop repeats the same version +and takes `github.run_number` so each push supersedes the last. Both reach the +packaging scripts as arguments, so neither script derives anything itself. + +Publishing is the last step of each packaging job, uploading from the container +that built the packages. It runs when the caller passes `publish: true`: +`on-trigger.yml` for develop pushes in `XRPLF/rippled`, `on-tag.yml` for tags in +any `XRPLF` repository, `on-pr.yml` never. Both authenticate with the +`NEXUS_REMOTE_USERNAME` / `NEXUS_REMOTE_PASSWORD` secrets already used for the +Conan remote. + +Nexus owns the repository metadata; nothing here signs or indexes anything. Worth +knowing: + +- Each apt-hosted repository needs a distribution and a PGP signing keypair + configured in Nexus, which rejects one created without a keypair. +- yum metadata is rebuilt asynchronously, so a successful publish is not + immediately installable. +- Each job uploads only what it built, and uploads are not transactional, so a + failure can leave one format published alone. Re-running is safe: both the apt + POST and the yum PUT replace an existing asset. +- The `develop` repositories gain a package per push, so they need a cleanup + policy to stay bounded; tagged channels publish each version once. + ## How `build_pkg.sh` works `build_pkg.sh` derives the `xrpld` software version from @@ -151,10 +196,9 @@ With `PKG_RELEASE=1`, the package metadata becomes: | `3.2.0-b1` | `3.2.0~b1-1%{?dist}` | `3.2.0~b1-1` | | `3.2.0-rc1` | `3.2.0~rc1-1%{?dist}` | `3.2.0~rc1-1` | -The Debian changelog entry carries the repository component: final releases use -`stable`, `b0` builds, including `b0+metadata`, use `develop`, and `bN`/`rcN` -pre-releases use `unstable`. -Build metadata on a final release, such as `3.2.0+abc123`, is rejected. +The Debian changelog entry carries the channel passed as `--channel` +(`PKG_CHANNEL`), defaulting to `unstable`. An unsupported pre-release, and build +metadata on a final release such as `3.2.0+abc123`, are both rejected. The RPM path intentionally uses `~` in `Version`, matching the Debian pre-release ordering convention, so RPM filenames/NVRs begin with forms like @@ -209,17 +253,20 @@ service restart. 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. -7. Output: `debbuild/*.deb` and `debbuild/*.ddeb` (dbgsym package) +7. Output: `debbuild/*.deb`, the binary package and the `-dbgsym` package. + Debian gives dbgsym packages a `.deb` extension; only Ubuntu uses `.ddeb`. ## Post-build verification ```bash # DEB dpkg-deb -c debbuild/*.deb | grep -E 'systemd|sysusers|tmpfiles' -lintian -I debbuild/*.deb # RPM rpm -qlp rpmbuild/RPMS/x86_64/*.rpm + +# Optional, and not in the packaging image: apt-get install -y lintian +lintian -I debbuild/*.deb ``` ## Reproducibility diff --git a/package/build_pkg.sh b/package/build_pkg.sh index d853bf95b7..cca3be7248 100755 --- a/package/build_pkg.sh +++ b/package/build_pkg.sh @@ -16,6 +16,8 @@ Options (each can also be set via the env var shown): xrpld and validator-keys binaries [BUILD_DIR; default: ${PWD}/build] --pkg-release N package release iteration [PKG_RELEASE; default: 1] + --channel NAME release channel, written + to debian/changelog [PKG_CHANNEL; default: unstable] --source-date-epoch SECS reproducibility timestamp [SOURCE_DATE_EPOCH; latest git ctime; fallback: current time] -h, --help show this help and exit EOF @@ -32,6 +34,7 @@ need_arg() { SRC_DIR="${SRC_DIR:-}" BUILD_DIR="${BUILD_DIR:-}" PKG_RELEASE="${PKG_RELEASE:-1}" +PKG_CHANNEL="${PKG_CHANNEL:-unstable}" SOURCE_DATE_EPOCH="${SOURCE_DATE_EPOCH:-}" while [[ $# -gt 0 ]]; do @@ -51,6 +54,11 @@ while [[ $# -gt 0 ]]; do PKG_RELEASE="$2" shift 2 ;; + --channel) + need_arg "$@" + PKG_CHANNEL="$2" + shift 2 + ;; --source-date-epoch) need_arg "$@" SOURCE_DATE_EPOCH="$2" @@ -198,7 +206,6 @@ stage_common() { build_rpm() { local topdir="${BUILD_DIR}/rpmbuild" - rm -rf "${topdir}" mkdir -p "${topdir}"/{BUILD,BUILDROOT,RPMS,SOURCES,SPECS,SRPMS} cp "${SRC_DIR}/package/rpm/xrpld.spec" "${topdir}/SPECS/xrpld.spec" @@ -214,7 +221,6 @@ build_rpm() { build_deb() { local staging="${BUILD_DIR}/debbuild/source" - rm -rf "${staging}" mkdir -p "${staging}" stage_common "${staging}" @@ -225,25 +231,9 @@ build_deb() { cp "${staging}/xrpld.tmpfiles" "${staging}/debian/xrpld.tmpfiles" cp "${staging}/xrpld.logrotate" "${staging}/debian/xrpld.logrotate" - # Choose the Debian repository component for this package. - # 3.2.0 -> stable, *-b0[+metadata] -> develop, - # bN/rcN pre-releases -> unstable. - local deb_component - if [[ -z "${pre_release}" ]]; then - deb_component="stable" - elif [[ "${pre_release}" =~ ^b0(\+.*)?$ ]]; then - deb_component="develop" - elif [[ "${pre_release}" =~ ^(b[1-9][0-9]*|rc[0-9]+)(\+.*)?$ ]]; then - deb_component="unstable" - else - echo "build_pkg.sh: unsupported xrpld pre-release '${pre_release}'." >&2 - echo "Use bN or rcN, e.g. 3.2.0-b1 or 3.2.0-rc2." >&2 - exit 1 - fi - # Debian version is [~
]-.
     cat >"${staging}/debian/changelog" <  ${CHANGELOG_DATE}
@@ -255,4 +245,8 @@ EOF
     (cd "${staging}" && dpkg-buildpackage -b --no-sign -d)
 }
 
+# Remove both build directories, because a package left from an earlier build
+# would otherwise be picked up and published alongside this one.
+rm -rf "${BUILD_DIR}/debbuild" "${BUILD_DIR}/rpmbuild"
+
 "build_${pkg_type}"
diff --git a/package/publish_pkg.sh b/package/publish_pkg.sh
new file mode 100755
index 0000000000..be36b531de
--- /dev/null
+++ b/package/publish_pkg.sh
@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+# Publish the DEB and RPM packages built by build_pkg.sh to the XRPLF package
+# repositories on Sonatype Nexus.
+#
+# Usage: publish_pkg.sh  [package-dir]
+#
+#   channel      release channel, selecting the 'deb-' and
+#                'rpm-' repository pair
+#   package-dir  searched recursively for *.deb, *.ddeb and *.rpm ('build' by
+#                default)
+#
+# NEXUS_USERNAME and NEXUS_PASSWORD are required. NEXUS_URL overrides the target
+# instance, and DRY_RUN=1 lists the uploads without performing them.
+
+channel="${1:-}"
+pkg_dir="${2:-build}"
+nexus_url="${NEXUS_URL:-https://packages.xrplf.org}"
+
+if [[ -z "${channel}" ]]; then
+    echo "usage: publish_pkg.sh  [package-dir]" >&2
+    exit 2
+fi
+
+deb_repo="deb-${channel}"
+rpm_repo="rpm-${channel}"
+
+if [[ -z "${DRY_RUN:-}" ]]; then
+    : "${NEXUS_USERNAME:?is required}" "${NEXUS_PASSWORD:?is required}"
+fi
+
+# Deliberate curl choices:
+#
+#   - no --fail, which would hide the response body where Nexus explains what it
+#     rejected
+#   - no --location, since curl downgrades a redirected POST to GET and turns an
+#     upload into a no-op that still answers 200
+#   - credentials on stdin, to keep them out of the process list
+upload() {
+    local url="$1"
+    shift
+    [[ -z "${DRY_RUN:-}" ]] || return 0
+
+    local body code status=0
+    body="$(mktemp)"
+    code="$(
+        printf 'user = %s:%s\n' "${NEXUS_USERNAME}" "${NEXUS_PASSWORD}" |
+            curl \
+                --config - \
+                --silent \
+                --show-error \
+                --retry 3 \
+                --retry-delay 5 \
+                --retry-all-errors \
+                --output "${body}" \
+                --write-out '%{http_code}' \
+                "$@" \
+                "${url}"
+    )" || status=$?
+
+    if [[ ${status} -ne 0 || ! "${code}" =~ ^2[0-9][0-9]$ ]]; then
+        echo "publish_pkg.sh: upload failed (curl ${status}, HTTP ${code}): ${url}" >&2
+        cat "${body}" >&2
+        echo >&2
+        rm -f "${body}"
+        exit 1
+    fi
+
+    rm -f "${body}"
+}
+
+echo "Publishing ${pkg_dir} to ${deb_repo} and ${rpm_repo} on ${nexus_url}:"
+
+count=0
+while IFS= read -r -d '' file; do
+    name="${file##*/}"
+    case "${name}" in
+        # A raw body with a multipart Content-Type, POSTed to the repository root,
+        # is the documented upload for a hosted apt repository:
+        # https://help.sonatype.com/en/apt-repositories.html#deploying-packages-to-hosted-apt-repositories
+        *.deb | *.ddeb)
+            echo "  ${name} -> ${deb_repo}"
+            upload "${nexus_url}/repository/${deb_repo}/" \
+                --header 'Content-Type: multipart/form-data' \
+                --data-binary "@${file}"
+            ;;
+        # yum repositories are addressed by path; the arch comes from the name.
+        *.rpm)
+            arch="${name%.rpm}"
+            arch="${arch##*.}"
+            echo "  ${name} -> ${rpm_repo}/${arch}"
+            upload "${nexus_url}/repository/${rpm_repo}/${arch}/${name}" \
+                --upload-file "${file}"
+            ;;
+    esac
+    count=$((count + 1))
+done < <(find "${pkg_dir}" -type f \( -name '*.deb' -o -name '*.ddeb' -o -name '*.rpm' \) -print0)
+
+# Uploading nothing would otherwise look like a successful publish.
+if [[ ${count} -eq 0 ]]; then
+    echo "publish_pkg.sh: no packages found in ${pkg_dir}." >&2
+    exit 1
+fi
+
+echo "${count} package(s) ${DRY_RUN:+would be }published."