From d9b7a2688fd97e020e828ba76c87d9e042b8d4f1 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 17 Mar 2025 13:14:49 +0000 Subject: [PATCH 1/6] fix: Error message for ledger_entry rpc (#5344) Changes the error to `malformedAddress` for `permissioned_domain` in the `ledger_entry` rpc, when the account is not a string. This change makes it more clear to a user what is wrong with their request. --- src/test/rpc/LedgerRPC_test.cpp | 13 ++++++++++++- src/xrpld/rpc/handlers/LedgerEntry.cpp | 8 +++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 4a55a79eef..d4fd2fd392 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -3189,7 +3189,18 @@ class LedgerRPC_test : public beast::unit_test::suite params[jss::permissioned_domain][jss::account] = 1; params[jss::permissioned_domain][jss::seq] = seq; auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); - checkErrorValue(jrr[jss::result], "malformedRequest", ""); + checkErrorValue(jrr[jss::result], "malformedAddress", ""); + } + + { + // Fail, account is an object + Json::Value params; + params[jss::ledger_index] = jss::validated; + params[jss::permissioned_domain][jss::account] = + Json::Value{Json::ValueType::objectValue}; + params[jss::permissioned_domain][jss::seq] = seq; + auto const jrr = env.rpc("json", "ledger_entry", to_string(params)); + checkErrorValue(jrr[jss::result], "malformedAddress", ""); } { diff --git a/src/xrpld/rpc/handlers/LedgerEntry.cpp b/src/xrpld/rpc/handlers/LedgerEntry.cpp index 9839a08367..7da99b3a76 100644 --- a/src/xrpld/rpc/handlers/LedgerEntry.cpp +++ b/src/xrpld/rpc/handlers/LedgerEntry.cpp @@ -822,12 +822,18 @@ parsePermissionedDomains(Json::Value const& pd, Json::Value& jvResult) return std::nullopt; } - if (!pd.isMember(jss::account) || !pd[jss::account].isString()) + if (!pd.isMember(jss::account)) { jvResult[jss::error] = "malformedRequest"; return std::nullopt; } + if (!pd[jss::account].isString()) + { + jvResult[jss::error] = "malformedAddress"; + return std::nullopt; + } + if (!pd.isMember(jss::seq) || (pd[jss::seq].isInt() && pd[jss::seq].asInt() < 0) || (!pd[jss::seq].isInt() && !pd[jss::seq].isUInt())) From a7fb8ae91576c15ec15ab22162810194693cb646 Mon Sep 17 00:00:00 2001 From: cyan317 <120398799+cindyyan317@users.noreply.github.com> Date: Tue, 18 Mar 2025 12:21:33 +0000 Subject: [PATCH 2/6] fix: Handle invalid marker parameter in grpc call (#5317) The `end_marker` is used to limit the range of ledger entries to fetch. If `end_marker` is less than `marker`, a crash can occur. This change adds an additional check. --- src/xrpld/rpc/handlers/LedgerData.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/xrpld/rpc/handlers/LedgerData.cpp b/src/xrpld/rpc/handlers/LedgerData.cpp index 31baede65e..7bd50cc1e5 100644 --- a/src/xrpld/rpc/handlers/LedgerData.cpp +++ b/src/xrpld/rpc/handlers/LedgerData.cpp @@ -170,16 +170,23 @@ doLedgerDataGrpc( } auto e = ledger->sles.end(); - if (auto key = uint256::fromVoidChecked(request.end_marker())) + if (request.end_marker().size() != 0) { + auto const key = uint256::fromVoidChecked(request.end_marker()); + + if (!key) + return { + response, + {grpc::StatusCode::INVALID_ARGUMENT, "end marker malformed"}}; + + if (*key < startKey) + return { + response, + {grpc::StatusCode::INVALID_ARGUMENT, + "end marker out of range"}}; + e = ledger->sles.upper_bound(*key); } - else if (request.end_marker().size() != 0) - { - grpc::Status errorStatus{ - grpc::StatusCode::INVALID_ARGUMENT, "end marker malformed"}; - return {response, errorStatus}; - } int maxLimit = RPC::Tuning::pageLength(true); From bf504912a4a67f7935e49fc0d4ed3f94d7a1a5f6 Mon Sep 17 00:00:00 2001 From: Vlad <129996061+vvysokikh1@users.noreply.github.com> Date: Tue, 18 Mar 2025 13:03:03 +0000 Subject: [PATCH 3/6] fix: trust line RPC no ripple flag (#5345) The Trustline RPC `no_ripple` flag gets set depending on `lsfDefaultRipple` flag, which is not a flag of a trustline but of the account root. The `lsfDefaultRipple` flag does not provide any insight if this particular trust line has `lsfLowNoRipple` or `lsfHighNoRipple` flag set, so it should not be used here at all. This change simplifies the logic. --- src/test/rpc/AccountLines_test.cpp | 4 ++++ src/xrpld/app/paths/TrustLine.h | 6 ------ src/xrpld/rpc/handlers/AccountLines.cpp | 8 ++++---- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/test/rpc/AccountLines_test.cpp b/src/test/rpc/AccountLines_test.cpp index 79e4bf214f..6e6f0def19 100644 --- a/src/test/rpc/AccountLines_test.cpp +++ b/src/test/rpc/AccountLines_test.cpp @@ -250,6 +250,10 @@ public: gw1.human() + R"("})"); BEAST_EXPECT(lines[jss::result][jss::lines].isArray()); BEAST_EXPECT(lines[jss::result][jss::lines].size() == 26); + + // Check no ripple is not set for trustlines between alice and gw1 + auto const& line = lines[jss::result][jss::lines][0u]; + BEAST_EXPECT(!line[jss::no_ripple].isMember(jss::no_ripple)); } { // Use a malformed peer. diff --git a/src/xrpld/app/paths/TrustLine.h b/src/xrpld/app/paths/TrustLine.h index 37cdc750f3..efbe281f5e 100644 --- a/src/xrpld/app/paths/TrustLine.h +++ b/src/xrpld/app/paths/TrustLine.h @@ -101,12 +101,6 @@ public: return mFlags & (!mViewLowest ? lsfLowAuth : lsfHighAuth); } - bool - getDefaultRipple() const - { - return mFlags & lsfDefaultRipple; - } - bool getNoRipple() const { diff --git a/src/xrpld/rpc/handlers/AccountLines.cpp b/src/xrpld/rpc/handlers/AccountLines.cpp index 7afb00c8de..e921eee386 100644 --- a/src/xrpld/rpc/handlers/AccountLines.cpp +++ b/src/xrpld/rpc/handlers/AccountLines.cpp @@ -54,10 +54,10 @@ addLine(Json::Value& jsonLines, RPCTrustLine const& line) jPeer[jss::authorized] = true; if (line.getAuthPeer()) jPeer[jss::peer_authorized] = true; - if (line.getNoRipple() || !line.getDefaultRipple()) - jPeer[jss::no_ripple] = line.getNoRipple(); - if (line.getNoRipplePeer() || !line.getDefaultRipple()) - jPeer[jss::no_ripple_peer] = line.getNoRipplePeer(); + if (line.getNoRipple()) + jPeer[jss::no_ripple] = true; + if (line.getNoRipplePeer()) + jPeer[jss::no_ripple_peer] = true; if (line.getFreeze()) jPeer[jss::freeze] = true; if (line.getFreezePeer()) From d167d4864f9f5720021d650fd909f1c7e7f5f31d Mon Sep 17 00:00:00 2001 From: Bart Date: Tue, 18 Mar 2025 11:25:48 -0400 Subject: [PATCH 4/6] refactor: Updates Conan dependencies: RocksDB (#5335) Updates RocksDB to version 9.7.3, the latest version supported in Conan 1.x. A patch for 9.7.4 that fixes a memory leak is included. --- .github/actions/dependencies/action.yml | 6 +- .github/workflows/macos.yml | 1 - BUILD.md | 4 +- conanfile.py | 2 +- external/rocksdb/conandata.yml | 29 +- external/rocksdb/conanfile.py | 24 +- ...-0001-add-include-cstdint-for-gcc-13.patch | 30 -- .../6.29.5-0002-exclude-thirdparty.patch | 16 - .../patches/9.7.3-0001-memory-leak.patch | 319 ++++++++++++++++++ .../9.x.x-0001-exclude-thirdparty.patch | 30 ++ .../nodestore/backend/RocksDBFactory.cpp | 6 +- 11 files changed, 378 insertions(+), 89 deletions(-) delete mode 100644 external/rocksdb/patches/6.29.5-0001-add-include-cstdint-for-gcc-13.patch delete mode 100644 external/rocksdb/patches/6.29.5-0002-exclude-thirdparty.patch create mode 100644 external/rocksdb/patches/9.7.3-0001-memory-leak.patch create mode 100644 external/rocksdb/patches/9.x.x-0001-exclude-thirdparty.patch diff --git a/.github/actions/dependencies/action.yml b/.github/actions/dependencies/action.yml index d3c67e8668..afce1557d3 100644 --- a/.github/actions/dependencies/action.yml +++ b/.github/actions/dependencies/action.yml @@ -14,7 +14,7 @@ runs: run: | conan config set general.revisions_enabled=1 conan export external/snappy snappy/1.1.10@ - conan export external/rocksdb rocksdb/6.29.5@ + conan export external/rocksdb rocksdb/9.7.3@ conan export external/soci soci/4.0.3@ conan export external/nudb nudb/2.0.8@ - name: add Ripple Conan remote @@ -55,7 +55,3 @@ runs: --options xrpld=True \ --settings build_type=${{ inputs.configuration }} \ .. - - name: upload dependencies to remote - if: (steps.binaries.outputs.missing != '[]') && (steps.remote.outputs.outcome == 'success') - shell: bash - run: conan upload --remote ripple '*' --all --parallel --confirm diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index 36d9f0225a..566e3550e0 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -73,7 +73,6 @@ jobs: run : | conan profile new default --detect || true conan profile update settings.compiler.cppstd=20 default - conan profile update 'conf.tools.build:cxxflags+=["-DBOOST_ASIO_DISABLE_CONCEPTS"]' default - name: build dependencies uses: ./.github/actions/dependencies env: diff --git a/BUILD.md b/BUILD.md index 310c538584..fd985dce81 100644 --- a/BUILD.md +++ b/BUILD.md @@ -178,9 +178,9 @@ It does not override paths to dependencies when building with Visual Studio. ``` # Conan 1.x - conan export external/rocksdb rocksdb/6.29.5@ + conan export external/rocksdb rocksdb/9.7.3@ # Conan 2.x - conan export --version 6.29.5 external/rocksdb + conan export --version 9.7.3 external/rocksdb ``` Export our [Conan recipe for SOCI](./external/soci). diff --git a/conanfile.py b/conanfile.py index d4513068fb..a42c116ca2 100644 --- a/conanfile.py +++ b/conanfile.py @@ -106,7 +106,7 @@ class Xrpl(ConanFile): if self.options.jemalloc: self.requires('jemalloc/5.3.0') if self.options.rocksdb: - self.requires('rocksdb/6.29.5') + self.requires('rocksdb/9.7.3') exports_sources = ( 'CMakeLists.txt', diff --git a/external/rocksdb/conandata.yml b/external/rocksdb/conandata.yml index 86b42f79f0..7d7a575d98 100644 --- a/external/rocksdb/conandata.yml +++ b/external/rocksdb/conandata.yml @@ -1,27 +1,12 @@ sources: - "6.29.5": - url: "https://github.com/facebook/rocksdb/archive/refs/tags/v6.29.5.tar.gz" - sha256: "ddbf84791f0980c0bbce3902feb93a2c7006f6f53bfd798926143e31d4d756f0" - "6.27.3": - url: "https://github.com/facebook/rocksdb/archive/refs/tags/v6.27.3.tar.gz" - sha256: "ee29901749b9132692b26f0a6c1d693f47d1a9ed8e3771e60556afe80282bf58" - "6.20.3": - url: "https://github.com/facebook/rocksdb/archive/refs/tags/v6.20.3.tar.gz" - sha256: "c6502c7aae641b7e20fafa6c2b92273d935d2b7b2707135ebd9a67b092169dca" - "8.8.1": - url: "https://github.com/facebook/rocksdb/archive/refs/tags/v8.8.1.tar.gz" - sha256: "056c7e21ad8ae36b026ac3b94b9d6e0fcc60e1d937fc80330921e4181be5c36e" + "9.7.3": + url: "https://github.com/facebook/rocksdb/archive/refs/tags/v9.7.3.tar.gz" + sha256: "acfabb989cbfb5b5c4d23214819b059638193ec33dad2d88373c46448d16d38b" patches: - "6.29.5": - - patch_file: "patches/6.29.5-0001-add-include-cstdint-for-gcc-13.patch" - patch_description: "Fix build with gcc 13 by including cstdint" - patch_type: "portability" - patch_source: "https://github.com/facebook/rocksdb/pull/11118" - - patch_file: "patches/6.29.5-0002-exclude-thirdparty.patch" + "9.7.3": + - patch_file: "patches/9.x.x-0001-exclude-thirdparty.patch" patch_description: "Do not include thirdparty.inc" patch_type: "portability" - "6.27.3": - - patch_file: "patches/6.27.3-0001-add-include-cstdint-for-gcc-13.patch" - patch_description: "Fix build with gcc 13 by including cstdint" + - patch_file: "patches/9.7.3-0001-memory-leak.patch" + patch_description: "Fix a leak of obsolete blob files left open until DB::Close()" patch_type: "portability" - patch_source: "https://github.com/facebook/rocksdb/pull/11118" diff --git a/external/rocksdb/conanfile.py b/external/rocksdb/conanfile.py index 1c7853d814..8b85ce1540 100644 --- a/external/rocksdb/conanfile.py +++ b/external/rocksdb/conanfile.py @@ -15,10 +15,10 @@ required_conan_version = ">=1.53.0" class RocksDBConan(ConanFile): name = "rocksdb" - homepage = "https://github.com/facebook/rocksdb" + description = "A library that provides an embeddable, persistent key-value store for fast storage" license = ("GPL-2.0-only", "Apache-2.0") url = "https://github.com/conan-io/conan-center-index" - description = "A library that provides an embeddable, persistent key-value store for fast storage" + homepage = "https://github.com/facebook/rocksdb" topics = ("database", "leveldb", "facebook", "key-value") package_type = "library" settings = "os", "arch", "compiler", "build_type" @@ -58,12 +58,12 @@ class RocksDBConan(ConanFile): @property def _compilers_minimum_version(self): return {} if self._min_cppstd == "11" else { - "apple-clang": "10", - "clang": "7", - "gcc": "7", - "msvc": "191", - "Visual Studio": "15", - } + "apple-clang": "10", + "clang": "7", + "gcc": "7", + "msvc": "191", + "Visual Studio": "15", + } def export_sources(self): export_conandata_patches(self) @@ -115,9 +115,9 @@ class RocksDBConan(ConanFile): check_min_vs(self, "191") if self.version == "6.20.3" and \ - self.settings.os == "Linux" and \ - self.settings.compiler == "gcc" and \ - Version(self.settings.compiler.version) < "5": + self.settings.os == "Linux" and \ + self.settings.compiler == "gcc" and \ + Version(self.settings.compiler.version) < "5": raise ConanInvalidConfiguration("Rocksdb 6.20.3 is not compilable with gcc <5.") # See https://github.com/facebook/rocksdb/issues/3522 def source(self): @@ -163,6 +163,8 @@ class RocksDBConan(ConanFile): if self.options.with_jemalloc: deps.set_property("jemalloc", "cmake_file_name", "JeMalloc") deps.set_property("jemalloc", "cmake_target_name", "JeMalloc::JeMalloc") + if self.options.with_zstd: + deps.set_property("zstd", "cmake_target_name", "zstd::zstd") deps.generate() def build(self): diff --git a/external/rocksdb/patches/6.29.5-0001-add-include-cstdint-for-gcc-13.patch b/external/rocksdb/patches/6.29.5-0001-add-include-cstdint-for-gcc-13.patch deleted file mode 100644 index 05725bf2c9..0000000000 --- a/external/rocksdb/patches/6.29.5-0001-add-include-cstdint-for-gcc-13.patch +++ /dev/null @@ -1,30 +0,0 @@ ---- a/include/rocksdb/utilities/checkpoint.h -+++ b/include/rocksdb/utilities/checkpoint.h -@@ -8,6 +8,7 @@ - #pragma once - #ifndef ROCKSDB_LITE - -+#include - #include - #include - #include "rocksdb/status.h" ---- a/table/block_based/data_block_hash_index.h -+++ b/table/block_based/data_block_hash_index.h -@@ -5,6 +5,7 @@ - - #pragma once - -+#include - #include - #include - ---- a/util/string_util.h -+++ b/util/string_util.h -@@ -6,6 +6,7 @@ - - #pragma once - -+#include - #include - #include - #include diff --git a/external/rocksdb/patches/6.29.5-0002-exclude-thirdparty.patch b/external/rocksdb/patches/6.29.5-0002-exclude-thirdparty.patch deleted file mode 100644 index fb0dd0c46b..0000000000 --- a/external/rocksdb/patches/6.29.5-0002-exclude-thirdparty.patch +++ /dev/null @@ -1,16 +0,0 @@ -diff --git a/CMakeLists.txt b/CMakeLists.txt -index ec59d4491..35577c998 100644 ---- a/CMakeLists.txt -+++ b/CMakeLists.txt -@@ -101 +100,0 @@ if(MSVC) -- option(WITH_GFLAGS "build with GFlags" OFF) -@@ -103,2 +102,2 @@ if(MSVC) -- include(${CMAKE_CURRENT_SOURCE_DIR}/thirdparty.inc) --else() -+endif() -+ -@@ -117 +116 @@ else() -- if(MINGW) -+ if(MINGW OR MSVC) -@@ -183 +181,0 @@ else() --endif() diff --git a/external/rocksdb/patches/9.7.3-0001-memory-leak.patch b/external/rocksdb/patches/9.7.3-0001-memory-leak.patch new file mode 100644 index 0000000000..bb086e6cb2 --- /dev/null +++ b/external/rocksdb/patches/9.7.3-0001-memory-leak.patch @@ -0,0 +1,319 @@ +diff --git a/HISTORY.md b/HISTORY.md +index 36d472229..05ad1a202 100644 +--- a/HISTORY.md ++++ b/HISTORY.md +@@ -1,6 +1,10 @@ + # Rocksdb Change Log + > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt` + ++## 9.7.4 (10/31/2024) ++### Bug Fixes ++* Fix a leak of obsolete blob files left open until DB::Close(). This bug was introduced in version 9.4.0. ++ + ## 9.7.3 (10/16/2024) + ### Behavior Changes + * OPTIONS file to be loaded by remote worker is now preserved so that it does not get purged by the primary host. A similar technique as how we are preserving new SST files from getting purged is used for this. min_options_file_numbers_ is tracked like pending_outputs_ is tracked. +diff --git a/db/blob/blob_file_cache.cc b/db/blob/blob_file_cache.cc +index 5f340aadf..1b9faa238 100644 +--- a/db/blob/blob_file_cache.cc ++++ b/db/blob/blob_file_cache.cc +@@ -42,6 +42,7 @@ Status BlobFileCache::GetBlobFileReader( + assert(blob_file_reader); + assert(blob_file_reader->IsEmpty()); + ++ // NOTE: sharing same Cache with table_cache + const Slice key = GetSliceForKey(&blob_file_number); + + assert(cache_); +@@ -98,4 +99,13 @@ Status BlobFileCache::GetBlobFileReader( + return Status::OK(); + } + ++void BlobFileCache::Evict(uint64_t blob_file_number) { ++ // NOTE: sharing same Cache with table_cache ++ const Slice key = GetSliceForKey(&blob_file_number); ++ ++ assert(cache_); ++ ++ cache_.get()->Erase(key); ++} ++ + } // namespace ROCKSDB_NAMESPACE +diff --git a/db/blob/blob_file_cache.h b/db/blob/blob_file_cache.h +index 740e67ada..6858d012b 100644 +--- a/db/blob/blob_file_cache.h ++++ b/db/blob/blob_file_cache.h +@@ -36,6 +36,15 @@ class BlobFileCache { + uint64_t blob_file_number, + CacheHandleGuard* blob_file_reader); + ++ // Called when a blob file is obsolete to ensure it is removed from the cache ++ // to avoid effectively leaking the open file and assicated memory ++ void Evict(uint64_t blob_file_number); ++ ++ // Used to identify cache entries for blob files (not normally useful) ++ static const Cache::CacheItemHelper* GetHelper() { ++ return CacheInterface::GetBasicHelper(); ++ } ++ + private: + using CacheInterface = + BasicTypedCacheInterface; +diff --git a/db/column_family.h b/db/column_family.h +index e4b7adde8..86637736a 100644 +--- a/db/column_family.h ++++ b/db/column_family.h +@@ -401,6 +401,7 @@ class ColumnFamilyData { + SequenceNumber earliest_seq); + + TableCache* table_cache() const { return table_cache_.get(); } ++ BlobFileCache* blob_file_cache() const { return blob_file_cache_.get(); } + BlobSource* blob_source() const { return blob_source_.get(); } + + // See documentation in compaction_picker.h +diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc +index 261593423..06573ac2e 100644 +--- a/db/db_impl/db_impl.cc ++++ b/db/db_impl/db_impl.cc +@@ -659,8 +659,9 @@ Status DBImpl::CloseHelper() { + // We need to release them before the block cache is destroyed. The block + // cache may be destroyed inside versions_.reset(), when column family data + // list is destroyed, so leaving handles in table cache after +- // versions_.reset() may cause issues. +- // Here we clean all unreferenced handles in table cache. ++ // versions_.reset() may cause issues. Here we clean all unreferenced handles ++ // in table cache, and (for certain builds/conditions) assert that no obsolete ++ // files are hanging around unreferenced (leak) in the table/blob file cache. + // Now we assume all user queries have finished, so only version set itself + // can possibly hold the blocks from block cache. After releasing unreferenced + // handles here, only handles held by version set left and inside +@@ -668,6 +669,9 @@ Status DBImpl::CloseHelper() { + // time a handle is released, we erase it from the cache too. By doing that, + // we can guarantee that after versions_.reset(), table cache is empty + // so the cache can be safely destroyed. ++#ifndef NDEBUG ++ TEST_VerifyNoObsoleteFilesCached(/*db_mutex_already_held=*/true); ++#endif // !NDEBUG + table_cache_->EraseUnRefEntries(); + + for (auto& txn_entry : recovered_transactions_) { +@@ -3227,6 +3231,8 @@ Status DBImpl::MultiGetImpl( + s = Status::Aborted(); + break; + } ++ // This could be a long-running operation ++ ROCKSDB_THREAD_YIELD_HOOK(); + } + + // Post processing (decrement reference counts and record statistics) +diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h +index 5e4fa310b..ccc0abfa7 100644 +--- a/db/db_impl/db_impl.h ++++ b/db/db_impl/db_impl.h +@@ -1241,9 +1241,14 @@ class DBImpl : public DB { + static Status TEST_ValidateOptions(const DBOptions& db_options) { + return ValidateOptions(db_options); + } +- + #endif // NDEBUG + ++ // In certain configurations, verify that the table/blob file cache only ++ // contains entries for live files, to check for effective leaks of open ++ // files. This can only be called when purging of obsolete files has ++ // "settled," such as during parts of DB Close(). ++ void TEST_VerifyNoObsoleteFilesCached(bool db_mutex_already_held) const; ++ + // persist stats to column family "_persistent_stats" + void PersistStats(); + +diff --git a/db/db_impl/db_impl_debug.cc b/db/db_impl/db_impl_debug.cc +index 790a50d7a..67f5b4aaf 100644 +--- a/db/db_impl/db_impl_debug.cc ++++ b/db/db_impl/db_impl_debug.cc +@@ -9,6 +9,7 @@ + + #ifndef NDEBUG + ++#include "db/blob/blob_file_cache.h" + #include "db/column_family.h" + #include "db/db_impl/db_impl.h" + #include "db/error_handler.h" +@@ -328,5 +329,49 @@ size_t DBImpl::TEST_EstimateInMemoryStatsHistorySize() const { + InstrumentedMutexLock l(&const_cast(this)->stats_history_mutex_); + return EstimateInMemoryStatsHistorySize(); + } ++ ++void DBImpl::TEST_VerifyNoObsoleteFilesCached( ++ bool db_mutex_already_held) const { ++ // This check is somewhat expensive and obscure to make a part of every ++ // unit test in every build variety. Thus, we only enable it for ASAN builds. ++ if (!kMustFreeHeapAllocations) { ++ return; ++ } ++ ++ std::optional l; ++ if (db_mutex_already_held) { ++ mutex_.AssertHeld(); ++ } else { ++ l.emplace(&mutex_); ++ } ++ ++ std::vector live_files; ++ for (auto cfd : *versions_->GetColumnFamilySet()) { ++ if (cfd->IsDropped()) { ++ continue; ++ } ++ // Sneakily add both SST and blob files to the same list ++ cfd->current()->AddLiveFiles(&live_files, &live_files); ++ } ++ std::sort(live_files.begin(), live_files.end()); ++ ++ auto fn = [&live_files](const Slice& key, Cache::ObjectPtr, size_t, ++ const Cache::CacheItemHelper* helper) { ++ if (helper != BlobFileCache::GetHelper()) { ++ // Skip non-blob files for now ++ // FIXME: diagnose and fix the leaks of obsolete SST files revealed in ++ // unit tests. ++ return; ++ } ++ // See TableCache and BlobFileCache ++ assert(key.size() == sizeof(uint64_t)); ++ uint64_t file_number; ++ GetUnaligned(reinterpret_cast(key.data()), &file_number); ++ // Assert file is in sorted live_files ++ assert( ++ std::binary_search(live_files.begin(), live_files.end(), file_number)); ++ }; ++ table_cache_->ApplyToAllEntries(fn, {}); ++} + } // namespace ROCKSDB_NAMESPACE + #endif // NDEBUG +diff --git a/db/db_iter.cc b/db/db_iter.cc +index e02586377..bf4749eb9 100644 +--- a/db/db_iter.cc ++++ b/db/db_iter.cc +@@ -540,6 +540,8 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key, + } else { + iter_.Next(); + } ++ // This could be a long-running operation due to tombstones, etc. ++ ROCKSDB_THREAD_YIELD_HOOK(); + } while (iter_.Valid()); + + valid_ = false; +diff --git a/db/table_cache.cc b/db/table_cache.cc +index 71fc29c32..8a5be75e8 100644 +--- a/db/table_cache.cc ++++ b/db/table_cache.cc +@@ -164,6 +164,7 @@ Status TableCache::GetTableReader( + } + + Cache::Handle* TableCache::Lookup(Cache* cache, uint64_t file_number) { ++ // NOTE: sharing same Cache with BlobFileCache + Slice key = GetSliceForFileNumber(&file_number); + return cache->Lookup(key); + } +@@ -179,6 +180,7 @@ Status TableCache::FindTable( + size_t max_file_size_for_l0_meta_pin, Temperature file_temperature) { + PERF_TIMER_GUARD_WITH_CLOCK(find_table_nanos, ioptions_.clock); + uint64_t number = file_meta.fd.GetNumber(); ++ // NOTE: sharing same Cache with BlobFileCache + Slice key = GetSliceForFileNumber(&number); + *handle = cache_.Lookup(key); + TEST_SYNC_POINT_CALLBACK("TableCache::FindTable:0", +diff --git a/db/version_builder.cc b/db/version_builder.cc +index ed8ab8214..c98f53f42 100644 +--- a/db/version_builder.cc ++++ b/db/version_builder.cc +@@ -24,6 +24,7 @@ + #include + + #include "cache/cache_reservation_manager.h" ++#include "db/blob/blob_file_cache.h" + #include "db/blob/blob_file_meta.h" + #include "db/dbformat.h" + #include "db/internal_stats.h" +@@ -744,12 +745,9 @@ class VersionBuilder::Rep { + return Status::Corruption("VersionBuilder", oss.str()); + } + +- // Note: we use C++11 for now but in C++14, this could be done in a more +- // elegant way using generalized lambda capture. +- VersionSet* const vs = version_set_; +- const ImmutableCFOptions* const ioptions = ioptions_; +- +- auto deleter = [vs, ioptions](SharedBlobFileMetaData* shared_meta) { ++ auto deleter = [vs = version_set_, ioptions = ioptions_, ++ bc = cfd_ ? cfd_->blob_file_cache() ++ : nullptr](SharedBlobFileMetaData* shared_meta) { + if (vs) { + assert(ioptions); + assert(!ioptions->cf_paths.empty()); +@@ -758,6 +756,9 @@ class VersionBuilder::Rep { + vs->AddObsoleteBlobFile(shared_meta->GetBlobFileNumber(), + ioptions->cf_paths.front().path); + } ++ if (bc) { ++ bc->Evict(shared_meta->GetBlobFileNumber()); ++ } + + delete shared_meta; + }; +@@ -766,7 +767,7 @@ class VersionBuilder::Rep { + blob_file_number, blob_file_addition.GetTotalBlobCount(), + blob_file_addition.GetTotalBlobBytes(), + blob_file_addition.GetChecksumMethod(), +- blob_file_addition.GetChecksumValue(), deleter); ++ blob_file_addition.GetChecksumValue(), std::move(deleter)); + + mutable_blob_file_metas_.emplace( + blob_file_number, MutableBlobFileMetaData(std::move(shared_meta))); +diff --git a/db/version_set.h b/db/version_set.h +index 9336782b1..024f869e7 100644 +--- a/db/version_set.h ++++ b/db/version_set.h +@@ -1514,7 +1514,6 @@ class VersionSet { + void GetLiveFilesMetaData(std::vector* metadata); + + void AddObsoleteBlobFile(uint64_t blob_file_number, std::string path) { +- // TODO: Erase file from BlobFileCache? + obsolete_blob_files_.emplace_back(blob_file_number, std::move(path)); + } + +diff --git a/include/rocksdb/version.h b/include/rocksdb/version.h +index 2a19796b8..0afa2cab1 100644 +--- a/include/rocksdb/version.h ++++ b/include/rocksdb/version.h +@@ -13,7 +13,7 @@ + // minor or major version number planned for release. + #define ROCKSDB_MAJOR 9 + #define ROCKSDB_MINOR 7 +-#define ROCKSDB_PATCH 3 ++#define ROCKSDB_PATCH 4 + + // Do not use these. We made the mistake of declaring macros starting with + // double underscore. Now we have to live with our choice. We'll deprecate these +diff --git a/port/port.h b/port/port.h +index 13aa56d47..141716e5b 100644 +--- a/port/port.h ++++ b/port/port.h +@@ -19,3 +19,19 @@ + #elif defined(OS_WIN) + #include "port/win/port_win.h" + #endif ++ ++#ifdef OS_LINUX ++// A temporary hook into long-running RocksDB threads to support modifying their ++// priority etc. This should become a public API hook once the requirements ++// are better understood. ++extern "C" void RocksDbThreadYield() __attribute__((__weak__)); ++#define ROCKSDB_THREAD_YIELD_HOOK() \ ++ { \ ++ if (RocksDbThreadYield) { \ ++ RocksDbThreadYield(); \ ++ } \ ++ } ++#else ++#define ROCKSDB_THREAD_YIELD_HOOK() \ ++ {} ++#endif diff --git a/external/rocksdb/patches/9.x.x-0001-exclude-thirdparty.patch b/external/rocksdb/patches/9.x.x-0001-exclude-thirdparty.patch new file mode 100644 index 0000000000..7b5858bc1e --- /dev/null +++ b/external/rocksdb/patches/9.x.x-0001-exclude-thirdparty.patch @@ -0,0 +1,30 @@ +diff --git a/CMakeLists.txt b/CMakeLists.txt +index 93b884d..b715cb6 100644 +--- a/CMakeLists.txt ++++ b/CMakeLists.txt +@@ -106,14 +106,9 @@ endif() + include(CMakeDependentOption) + + if(MSVC) +- option(WITH_GFLAGS "build with GFlags" OFF) + option(WITH_XPRESS "build with windows built in compression" OFF) +- option(ROCKSDB_SKIP_THIRDPARTY "skip thirdparty.inc" OFF) +- +- if(NOT ROCKSDB_SKIP_THIRDPARTY) +- include(${CMAKE_CURRENT_SOURCE_DIR}/thirdparty.inc) +- endif() +-else() ++endif() ++if(TRUE) + if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD" AND NOT CMAKE_SYSTEM_NAME MATCHES "kFreeBSD") + # FreeBSD has jemalloc as default malloc + # but it does not have all the jemalloc files in include/... +@@ -126,7 +121,7 @@ else() + endif() + endif() + +- if(MINGW) ++ if(MSVC OR MINGW) + option(WITH_GFLAGS "build with GFlags" OFF) + else() + option(WITH_GFLAGS "build with GFlags" ON) diff --git a/src/xrpld/nodestore/backend/RocksDBFactory.cpp b/src/xrpld/nodestore/backend/RocksDBFactory.cpp index c6533e82d6..0e421cd6bd 100644 --- a/src/xrpld/nodestore/backend/RocksDBFactory.cpp +++ b/src/xrpld/nodestore/backend/RocksDBFactory.cpp @@ -191,8 +191,12 @@ public: if (keyValues.exists("bbt_options")) { + rocksdb::ConfigOptions config_options; auto const s = rocksdb::GetBlockBasedTableOptionsFromString( - table_options, get(keyValues, "bbt_options"), &table_options); + config_options, + table_options, + get(keyValues, "bbt_options"), + &table_options); if (!s.ok()) Throw( std::string("Unable to set RocksDB bbt_options: ") + From 345ddc7234bf5754b0f07ab171f29c3dbd741e54 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 18 Mar 2025 16:45:25 +0000 Subject: [PATCH 5/6] fix: Remove null pointer deref, just do abort (#5338) This change removes the existing undefined behavior from `LogicError`, so we can be certain that there will be always a stacktrace. De-referencing a null pointer is an old trick to generate `SIGSEGV`, which would typically also create a stacktrace. However it is also an undefined behaviour and compilers can do something else. A more robust way to create a stacktrace while crashing the program is to use `std::abort`, which we have also used in this location for a long time. If we combine the two, we might not get the expected behaviour - namely, the nullpointer deref followed by `std::abort`, as handled in certain compiler versions may not immediately cause a crash. We have observed stacktrace being wiped instead, and thread put in indeterminate state, then stacktrace created without any useful information. --- src/libxrpl/basics/contract.cpp | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/libxrpl/basics/contract.cpp b/src/libxrpl/basics/contract.cpp index b0cd882591..b5a7b3f368 100644 --- a/src/libxrpl/basics/contract.cpp +++ b/src/libxrpl/basics/contract.cpp @@ -27,19 +27,6 @@ namespace ripple { -namespace detail { - -[[noreturn]] void -accessViolation() noexcept -{ - // dereference memory location zero - int volatile* j = 0; - (void)*j; - std::abort(); -} - -} // namespace detail - void LogThrow(std::string const& title) { @@ -57,7 +44,7 @@ LogicError(std::string const& s) noexcept // here. // For the above reasons, we want this contract to stand out. UNREACHABLE("LogicError", {{"message", s}}); - detail::accessViolation(); + std::abort(); } } // namespace ripple From 7fe81fe62e27d7f14520d68cf717ac5627745c62 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 18 Mar 2025 17:26:08 +0000 Subject: [PATCH 6/6] chore: Add PR number to payload (#5310) This PR adds one more payload field to the libXRPL compatibility check workflow - the PR number itself. --- .github/workflows/libxrpl.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/libxrpl.yml b/.github/workflows/libxrpl.yml index fe4a2c3e22..2834595e3a 100644 --- a/.github/workflows/libxrpl.yml +++ b/.github/workflows/libxrpl.yml @@ -85,4 +85,5 @@ jobs: run: | gh api --method POST -H "Accept: application/vnd.github+json" -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/xrplf/clio/dispatches -f "event_type=check_libxrpl" \ - -F "client_payload[version]=${{ needs.publish.outputs.version }}@${{ needs.publish.outputs.channel }}" + -F "client_payload[version]=${{ needs.publish.outputs.version }}@${{ needs.publish.outputs.channel }}" \ + -F "client_payload[pr]=${{ github.event.pull_request.number }}"