refactor: Use uint256 directly as key instead of void pointer - #6313
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the nodestore backend API to use uint256 const& directly instead of void const* for database fetch operations, eliminating unnecessary pointer conversions.
Changes:
- Updated the
Backendinterface to acceptuint256 const&instead ofvoid const*forfetch()andfetchBatch()methods - Modified all backend implementations (RocksDB, NuDB, Memory, Null) to use the new signature
- Updated all callers to pass hash objects directly instead of using
.data()or.begin() - Removed unnecessary pointer vector conversion code in
DatabaseNodeImp::fetchBatch() - Improved variable naming from "key" to "hash" in test files for better semantic clarity
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
include/xrpl/nodestore/Backend.h |
Updated virtual method signatures and documentation for fetch() and fetchBatch() to use uint256 const& |
src/libxrpl/nodestore/backend/RocksDBFactory.cpp |
Updated implementation to accept uint256 const&, using hash.data() for RocksDB Slice and DecodedBlob |
src/libxrpl/nodestore/backend/NullFactory.cpp |
Updated null backend stub implementation signatures |
src/libxrpl/nodestore/backend/NuDBFactory.cpp |
Updated implementation to accept uint256 const&, using hash.data() for NuDB API and DecodedBlob |
src/libxrpl/nodestore/backend/MemoryFactory.cpp |
Removed unnecessary fromVoid() conversion and updated signatures |
src/libxrpl/nodestore/DatabaseRotatingImp.cpp |
Updated caller to pass hash directly instead of hash.data() |
src/libxrpl/nodestore/DatabaseNodeImp.cpp |
Updated caller and removed redundant pointer vector creation in fetchBatch() |
src/test/nodestore/TestBase.h |
Updated test helper to pass hash directly and removed .cbegin() calls |
src/test/nodestore/Timing_test.cpp |
Updated test code to use "hash" variable naming and pass hash directly, removing .data() calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6313 +/- ##
=======================================
Coverage 79.8% 79.8%
=======================================
Files 848 848
Lines 67763 67757 -6
Branches 7578 7553 -25
=======================================
- Hits 54077 54074 -3
+ Misses 13686 13683 -3
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
vlntb
left a comment
vlntb
left a comment
There was a problem hiding this comment.
LGTM. The change improves type safety by replacing void const* with uint256 const&, follows East const notation, and eliminates unnecessary temporary vectors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
godexsoft
left a comment
godexsoft
left a comment
There was a problem hiding this comment.
Overall looks like a good idea. If you want to allow the old API then we could add overloads for each function to take the old void* and pass them to the new functions.
Definitely try to get rid of reinterpret_cast 👍
|
|
||
| rocksdb::ReadOptions const options; | ||
| rocksdb::Slice const slice(static_cast<char const*>(key), m_keyBytes); | ||
| rocksdb::Slice const slice(reinterpret_cast<char const*>(hash.data()), m_keyBytes); |
There was a problem hiding this comment.
Would be nice to avoid reinterpret_cast. It's only good as last resort.
There was a problem hiding this comment.
I changed this to std::bit_cast, as the to and from types are trivially copyable and have the same size.
- hash: is our custom
uint256, which is aliased to ourbase_uint<256>and wraps astd::array<std::uint32_t, WIDTH> data_;withWIDTH = 256/32 = 8. - hash.data(): calls
reinterpret_cast<const_pointer>(data_.data()), whereby it takes theuint32_t const*and casts it tounsigned char const*. - The
std::bit_cast<char const*>then convertsunsigned char const*to achar const*, as needed by RocksDB.
I built and tested the binary locally and was able to sync up with testnet using RocksDB. While this is not the ultimate test, it's a good sanity check.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| rocksdb::ReadOptions const options; | ||
| rocksdb::Slice const slice(static_cast<char const*>(key), m_keyBytes); | ||
| rocksdb::Slice const slice(std::bit_cast<char const*>(hash.data()), m_keyBytes); |
There was a problem hiding this comment.
The use of std::bit_cast here is inconsistent with the existing code in this file. Lines 335 and 337 use reinterpret_cast for similar pointer casts when creating rocksdb::Slice objects. For consistency with the established pattern in this codebase, consider using reinterpret_cast instead of std::bit_cast here.
| rocksdb::Slice const slice(std::bit_cast<char const*>(hash.data()), m_keyBytes); | |
| rocksdb::Slice const slice(reinterpret_cast<char const*>(hash.data()), m_keyBytes); |
| key, | ||
| [key, pno, &status](void const* data, std::size_t size) { | ||
| hash.data(), | ||
| [hash, pno, &status](void const* data, std::size_t size) { |
There was a problem hiding this comment.
The lambda now captures hash by value, which copies 32 bytes of data. Since the lambda is invoked synchronously within the db_.fetch call (as evidenced by the use of the status variable after the call returns), capturing hash by reference (&hash) would be more efficient and equally safe. Consider changing the capture to [&hash, pno, &status] to avoid the unnecessary copy.
| [hash, pno, &status](void const* data, std::size_t size) { | |
| [&hash, pno, &status](void const* data, std::size_t size) { |
…6313) This change replaces `void const*` by `uint256 const&` for database fetches. Object hashes are expressed using the `uint256` data type, and are converted to `void *` when calling the `fetch` or `fetchBatch` functions. However, in these fetch functions they are converted back to `uint256`, making the conversion process unnecessary. In a few cases the underlying pointer is needed, but that can then be easy obtained via `[hash variable].data()`.
High Level Overview of Change
This change replaces
void const*byuint256 const&for database fetches.Context of Change
Object hashes are expressed using the
uint256data type, and are converted tovoid *when calling thefetchorfetchBatchfunctions. However, in these fetch functions they are converted back touint256, making the conversion process unnecessary. In a few cases the underlying pointer is needed, but that can then be easy obtained via[hash variable].data().I tested this change with both NuDB and RocksDB using an existing database, and the binary was able to use those databases and proceed to sync with the network.
Type of Change
.gitignore, formatting, dropping support for older tooling)