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.9% 79.9% -0.0%
=========================================
Files 840 840
Lines 65508 65503 -5
Branches 7248 7269 +21
=========================================
- Hits 52354 52322 -32
- Misses 13154 13181 +27
... and 10 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.
3 participants
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)