Skip to content

Modularise RelationalDB#6224

Merged
bthomee merged 7 commits intodevelopfrom
a1q123456/modularise-relational-db
Feb 11, 2026
Merged

Modularise RelationalDB#6224
bthomee merged 7 commits intodevelopfrom
a1q123456/modularise-relational-db

Conversation

@a1q123456
Copy link
Copy Markdown
Contributor

@a1q123456 a1q123456 commented Jan 15, 2026

High Level Overview of Change

This PR modularise xrpld/rdb

Context of Change

The rdb module was not properly designed. We have 3 classes:

  1. The abstract class RelationalDB
  2. The abstract class SQLiteDatabase which inherits from RelationalDB and adds some more pure virtual methods
  3. The concrete class SQLiteDatabaseImp which inherits from SQLiteDatabase and implements all the methods.

The factory function RelationalDB::init method checks the config and only returns an instance of SQLiteDatabaseImp, we can see in the code base that we are doing dynamic_cast<SQLiteDatabase*>(app.getRelationalDb()) everywhere.

To address this problem and help modularise app/tx, the rdb module needs to be refactored and moved to libxrpl.

This PR is part of the modularisation of the transactors. See all PRs here:
#6222
#6223
#6224
#6225
#6226
#6227
#6228

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@a1q123456 a1q123456 requested review from a team, kuznetsss and vlntb January 15, 2026 13:30
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch 3 times, most recently from dd1a1a9 to cf67641 Compare January 15, 2026 14:19
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 15, 2026

Codecov Report

❌ Patch coverage is 78.65169% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.9%. Comparing base (ef28469) to head (86ba05b).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...rc/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp 69.8% 16 Missing ⚠️
src/xrpld/app/main/Application.cpp 50.0% 2 Missing ⚠️
src/xrpld/rpc/handlers/AccountTx.cpp 80.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6224   +/-   ##
=======================================
  Coverage     79.9%   79.9%           
=======================================
  Files          840     840           
  Lines        65522   65508   -14     
  Branches      7251    7250    -1     
=======================================
- Hits         52355   52353    -2     
+ Misses       13167   13155   -12     
Files with missing lines Coverage Δ
include/xrpl/rdb/RelationalDatabase.h 100.0% <100.0%> (ø)
src/xrpld/app/ledger/Ledger.cpp 82.4% <100.0%> (+0.1%) ⬆️
src/xrpld/app/ledger/detail/LedgerMaster.cpp 43.2% <ø> (ø)
src/xrpld/app/misc/SHAMapStoreImp.cpp 75.7% <100.0%> (+0.2%) ⬆️
src/xrpld/app/misc/Transaction.h 96.1% <ø> (ø)
src/xrpld/app/misc/detail/Transaction.cpp 83.3% <100.0%> (+1.1%) ⬆️
src/xrpld/app/rdb/backend/SQLiteDatabase.h 100.0% <100.0%> (ø)
src/xrpld/app/rdb/backend/detail/Node.cpp 58.6% <ø> (ø)
src/xrpld/overlay/detail/OverlayImpl.cpp 32.9% <ø> (ø)
src/xrpld/overlay/detail/PeerReservationTable.cpp 16.3% <ø> (ø)
... and 7 more

... and 8 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +53 to +54
std::uint32_t minLedger;
std::uint32_t maxLedger;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use LedgerRange?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just moved from src/xrpld/app/rdb/RelationalDatabase.h. I think we can have a separate PR for optimisations like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be fixed in RIPD-4880.

using AccountTx =
std::pair<std::shared_ptr<Transaction>, std::shared_ptr<TxMeta>>;
using AccountTxs = std::vector<AccountTx>;
using txnMetaLedgerType = std::tuple<Blob, Blob, std::uint32_t>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to avoid pairs and tuples if possible because they decrease readability

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be fixed in RIPD-4880.

closeTransactionDB() = 0;
};

template <class T, class C>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add concepts on T and C to improve compilation errors on misuse. Probably std::is_arithmetic and that std::is_convertible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be fixed in RIPD-4880.

@a1q123456 a1q123456 force-pushed the a1q123456/modularise-wallet-db-and-manifest branch from c9dbea1 to 8dc9483 Compare February 3, 2026 16:43
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch from 922752a to 17020f9 Compare February 3, 2026 17:20
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-wallet-db-and-manifest branch 2 times, most recently from 572f5b2 to 650dc4f Compare February 4, 2026 11:27
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch 2 times, most recently from eebaec1 to 860e22d Compare February 4, 2026 13:30
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-wallet-db-and-manifest branch from 0dd8b72 to b3f0361 Compare February 4, 2026 15:11
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch from 860e22d to 39f6e3a Compare February 4, 2026 15:16
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-wallet-db-and-manifest branch from b3f0361 to c0bbbb2 Compare February 4, 2026 15:30
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch from 39f6e3a to e436dff Compare February 4, 2026 15:37
Copy link
Copy Markdown
Contributor

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine from refactoring perspective except for the questions i left. I agree with @kuznetsss and there are more obvious things we should fix but i'd leave that for another pr.

auto const db = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase());
if (!db)
Throw<std::runtime_error>("Failed to get relational database");
auto& db = app.getRelationalDatabase();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we always guaranteed to have a db now? is there no way to start xrpld without a relational db? why was it possible before, do you know?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're always guaranteed to have a db even before the change. You can see from the code that we actually used to return a reference instead of a pointer.

The reason why we used to check if db is null is that we had to perform a dyanmic_cast on it, and the reason why we needed to dynamic_cast was the bad design.

We had 3 classes before the change, RelationDatabase, SQLiteDatabase, SQLiteDatabaseImp. The first two are abstract classes, and the last one is a concrete class. RelationDatabase defines some abstract methods, and then SQLiteDatabase inherits from it, adding more abstract methods, and finally, SQLiteDatabaseImp implements all the methods. We should just merge SQLiteDatabase into RelationDatabase because in my opinion, it makes no sense to have a SQLiteDatabase interface that adds some methods but hide that interface under the hood if the business logic needs those methods, and the name creates more confusion when putting it together with SQLiteDatabaseImp as SQLiteDatabase already sounds like the implementation of RelationDatabase.

Another funny thing is that RelationalDatabase::init checks if the rdb backend is sqlite, and it throws an exception if not, which further suggests that there's no chance to have anything other than SQLiteDatabase.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i see the reasoning. It makes more sense to store and return by interface in the service locator. This will allow us to add another implementation in the future, if we ever need one 👍

Base automatically changed from a1q123456/modularise-wallet-db-and-manifest to develop February 11, 2026 13:42
Copy link
Copy Markdown
Contributor

@kuznetsss kuznetsss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@a1q123456 a1q123456 added the Needs additional review PR requires at least one more code review approval before it can be merged label Feb 11, 2026
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@a1q123456 a1q123456 force-pushed the a1q123456/modularise-relational-db branch from 9dbe6b9 to 6ea2646 Compare February 11, 2026 15:02
Copy link
Copy Markdown
Contributor

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for modularization 👍

@a1q123456 a1q123456 removed the Needs additional review PR requires at least one more code review approval before it can be merged label Feb 11, 2026
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
@a1q123456 a1q123456 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 11, 2026
@bthomee bthomee enabled auto-merge (squash) February 11, 2026 16:03
@bthomee bthomee merged commit 9f17d10 into develop Feb 11, 2026
45 of 47 checks passed
@bthomee bthomee deleted the a1q123456/modularise-relational-db branch February 11, 2026 16:22
a1q123456 added a commit that referenced this pull request Feb 19, 2026
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
a1q123456 added a commit that referenced this pull request Feb 19, 2026
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
gregtatcam added a commit to gregtatcam/rippled that referenced this pull request Mar 7, 2026
* chore: Set ColumnLimit to 120 in clang-format (XRPLF#6288)

This change updates the ColumnLimit from 80 to 120, and applies clang-format to reformat the code.

* chore: Remove unnecessary `boost::system` requirement from conanfile (XRPLF#6290)

* chore: Add cmake-format pre-commit hook (XRPLF#6279)

This change adds `cmake-format` as. a pre-commit hook. The style file closely matches that in Clio, and they will be made to be equivalent over time. For now, some files have been excluded, as those need some manual adjustments, which will be done in future changes.

* chore: Format all cmake files without comments (XRPLF#6294)

* ci: Update hashes of XRPLF/actions (XRPLF#6316)

This updates the hashes of all XRPLF/actions to their latest versions.

* chore: Add upper-case match for ARM64 in CompilationEnv (XRPLF#6315)

* fix: Restore config changes that broke standalone mode (XRPLF#6301)

When support was added for `xrpld.cfg` in addition to `rippled.cfg` in XRPLF#6098, as part of an effort to rename occurrences of ripple(d) to xrpl(d), the clearing and creation of the data directory were modified for what, at the time, seemed to result in an equivalent code flow. This has turned out to not be true, which is why this change restores two modifications to `Config.cpp` that currently break running the binary in standalone mode.

* docs: Update API changelog, add APIv2+APIv3 version documentation (XRPLF#6308)

This change cleans up the `API-CHANGELOG.md` file. It moves the version-specific documentation to other files and fleshes out the changelog with all the API-related changes in each version.

* chore: Add .zed editor config directory to .gitignore (XRPLF#6317)

This change adds the project configuration directory to `.gitignore` for the `zed` editor. 

As per the [documentation](https://zed.dev/docs/remote-development?highlight=.zed#zed-settings), the project configuration files are stored in the `.zed` directory at the project root dir.

* fix: Deletes expired NFToken offers from ledger (XRPLF#5707)

This change introduces the `fixExpiredNFTokenOfferRemoval` amendment that allows expired offers to pass through `preclaim()` and be deleted in `doApply()`, following the same pattern used for expired credentials.

* refactor: Add ServiceRegistry to help modularization (XRPLF#6222)

Currently we're passing the `Application` object around, whereby the `Application` class acts more like a service registry that gives other classes access to other services. In order to allow modularization, we should replace `Application` with a service registry class so that modules depending on `Application` for other services can be moved easily. This change adds the `ServiceRegistry` class.

* chore: Remove unity builds (XRPLF#6300)

Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that ccache is available on all platforms, there is no need for unity builds anymore, as ccache stores compiled individual build objects for reuse. This change therefore removes the ability to make unity builds.

* refactor: Replace include guards by '#pragma once' (XRPLF#6322)

This change replaces all include guards in the `src/` and `include/` directories by `#pragma once`.

* chore: Remove unnecessary script (XRPLF#6326)

* chore: Update secp256k1 and openssl (XRPLF#6327)

* fix typo in LendingHelpers unit-test (XRPLF#6215)

* fix: Increment sequence when accepting new manifests (XRPLF#6059)

The `ManifestCache::applyManifest` function was returning early without incrementing `seq_`. `OverlayImpl `uses this sequence to identify/invalidate a cached `TMManifests` message, which is exchanged with peers on connection. Depending on network size, startup sequencing, and topology, this can cause syncing issues. This change therefore increments `seq_` when a new manifest is accepted.

* refactor: Update secp256k1 to 0.7.1 (XRPLF#6331)

The latest secp256k1 release, 0.7.1, contains bug fixes that we may benefit from, see https://github.com/bitcoin-core/secp256k1/blob/master/CHANGELOG.md.

* chore: Restore unity builds (XRPLF#6328)

In certain cases, such as when modifying headers used by many compilation units, performing a unity build is slower than when performing a regular build with `ccache` enabled. There is also a benefit to a unity build in that it can detect things such as macro redefinitions within the group of files that are compiled together as a unit. This change therefore restores the ability to perform unity builds. However, instead of running every configuration with and without unity enabled, it is now only enabled for a single configuration to maintain lower computational use.

As part of restoring the code, it became clear that currently two configurations have coverage enabled, since the check doesn't focus specifically on Debian Bookworm so it also applies to Debian Trixie. This has been fixed too in this change.

* perf: Remove unnecessary caches (XRPLF#5439)

This change removes the cache in `DatabaseNodeImp` and simplifies the caching logic in `SHAMapStoreImp`. As NuDB and RocksDB internally already use caches, additional caches in the code are not very valuable or may even be unnecessary, as also confirmed during preliminary performance analyses.

* chore: Remove CODEOWNERS (XRPLF#6337)

* test: Add file and line location to Env (XRPLF#6276)

This change uses `std::source_location` to output the file and line location of the call that triggered a failed transaction.

* refactor: Fix spelling issues in tests (XRPLF#6199)

This change removes the `src/tests` exception from the `cspell` config and fixes all the issues that arise as a result. No functionality/test change.

* refactor: Change main thread name to `xrpld-main` (XRPLF#6336)

This change builds on the thread-renaming PR (XRPLF#6212), by renaming the main thread name to reduce ambiguity in performance monitoring tools.

* fix: Update invariant checks for Permissioned Domains (XRPLF#6134)

* refactor: Modularize WalletDB and Manifest (XRPLF#6223)

This change modularizes the `WalletDB` and `Manifest`. Note that the wallet db has nothing to do with account wallets and it stores node configuration, which is why it depends on the manifest code.

* refactor: Modularize RelationalDB (XRPLF#6224)

The rdb module was not properly designed, which is fixed in this change. The module had three classes:
1) The abstract class `RelationalDB`.
2) The abstract class `SQLiteDatabase`, which inherited from `RelationalDB` and added some pure virtual methods.
3) The concrete class `SQLiteDatabaseImp`, which inherited from `SQLiteDatabase` and implemented all methods.

The updated code simplifies this as follows:
* The `SQLiteDatabaseImp` has become `SQLiteDatabase`, and
* The former `SQLiteDatabase `has merged with `RelationalDatabase`.

* chore: Fix `gcov` lib coverage build failure on macOS (XRPLF#6350)

For coverage builds, we try to link against the `gcov` library (specific to the environment). But as macOS doesn't have this library and thus doesn't have the coverage tools to generate reports, the coverage builds on that platform were failing on linking.

We actually don't need to explicitly force this linking, as the `CodeCoverage` file already has correct detection logic (currently on lines 177-193), which is invoked when the `--coverage` flag is provided:
* AppleClang: Uses `xcrun -f llvm-cov` to set `GCOV_TOOL="llvm-cov gcov"`.
* Clang: Finds `llvm-cov` to set `GCOV_TOOL="llvm-cov gcov"`.
* GCC: Finds `gcov` to set `GCOV_TOOL="gcov"`.
The `GCOV_TOOL` is then passed to `gcovr` on line 416, so the correct tool is used for processing coverage data.

This change therefore removes the `gcov` suffix from lines 473 and 475 in the `CodeCoverage.cmake` file.

* refactor: Modularize the NetworkOPs interface (XRPLF#6225)

This change moves the NetworkOPs interface into `libxrpl` and it leaves its implementation in `xrpld`.

* chore: Fix minor issues in comments (XRPLF#6346)

* refactor: Modularize `HashRouter`, `Conditions`, and `OrderBookDB` (XRPLF#6226)

This change modularizes additional components by moving code to `libxrpl`.

* chore: Update clang-format to 21.1.8 (XRPLF#6352)

* refactor: Decouple app/tx from `Application` and `Config` (XRPLF#6227)

This change decouples app/tx from `Application` and `Config` to clear the way to moving transactors to `libxrpl`.

* refactor: Modularize app/tx (XRPLF#6228)

* ci: Add clang tidy workflow to ci (XRPLF#6369)

* chore: Set cmake-format width to 100 (XRPLF#6386)

* chore: Set clang-format width to 100 in config file (XRPLF#6387)

* chore: Apply clang-format width 100 (XRPLF#6387)

* Fix tautological assertion (XRPLF#6393)

* ci: Add dependabot config (XRPLF#6379)

* ci: Build docs in PRs and in private repos (XRPLF#6400)

* ci: [DEPENDABOT] bump codecov/codecov-action from 5.4.3 to 5.5.2 (XRPLF#6398)

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5.4.3 to 5.5.2.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@18283e0...671740a)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-version: 5.5.2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix merge conflicts, format, spelling,
replace all include guards by #pragma once

* Fix non-unity build

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
Co-authored-by: Ed Hennis <ed@ripple.com>
Co-authored-by: Bart <bthomee@users.noreply.github.com>
Co-authored-by: Mayukha Vadari <mvadari@ripple.com>
Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jingchen <a1q123456@users.noreply.github.com>
Co-authored-by: Niq Dudfield <ndudfield@gmail.com>
Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com>
Co-authored-by: Olek <115580134+oleks-rip@users.noreply.github.com>
Co-authored-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Co-authored-by: nuxtreact <nuxtreact@outlook.com>
Co-authored-by: Sergey Kuznetsov <skuznetsov@ripple.com>
Co-authored-by: Ayaz Salikhov <asalikhov@ripple.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants