refactor: Modularise ledger - #6536
Conversation
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>
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>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
…-generator Signed-off-by: JCW <a1q123456@users.noreply.github.com> # Conflicts: # cmake/XrplCore.cmake # cmake/XrplInstall.cmake
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>
Added more tests. 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>
…n the code base && Renamed the folder ledger_obejcts to ledger_entries to match the namespace
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>
| std::vector<TrustLine> | ||
| TrustLine::getItems(AccountID const& accountID, ReadView const& view, LineDirection direction) | ||
| { | ||
| std::vector<TrustLine> items; |
There was a problem hiding this comment.
Can we run reserve on items? Do we know the expected size in advance?
items.shrink_to_fit(); suggests we don't?
There was a problem hiding this comment.
we'll either make it slower or consuming more memory if we do so.
There are 3 options:
- we run forEachItem twice and we call makeItem inside - the first batch returns the size and the second batch adds items to the std::vector
- we run forEachItem twice and we duplicate the logic
- we reserve a larger value (i.e. sfOwnerCount)
The first approach is slower; The second approach requires us to duplicate the logic because we'll need line->getNoRipple() to work without line to be there (because we don't call makeItem); The last approach wastes memory.
There was a problem hiding this comment.
Have completely reverted this file.
| } | ||
|
|
||
| if (!loadLedger->assertSensible(journal("Ledger"))) | ||
| if (!loadLedger->isSensible()) |
There was a problem hiding this comment.
assertSensible means the ledger should be sensible, if not throw/exit. isSensible means, check if ledger is sensible. This replacement doesn't map 1-1. I am guessing, assertSensible was only checking and the decision on what to do was taken later anyway?
There was a problem hiding this comment.
This replacement doesn't map 1-1
No it doesn't map 1-1, and this is why I moved the logic related to exiting inside the if block so it behaves the same overall.
The reason is:
- The logic required for logging is tightly coupled with rpc and LedgerMaster (which is the central orchestrator and we don't want to modularise it now).
- I don't think it makes sense to have a method that exits the program and logs some error when it's corrupted (self diagnostics) in such a generic class. It feels more like something for the application.
pratikmankawde
left a comment
There was a problem hiding this comment.
Minor questions, otherwise looks good.
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
…se-ledger Signed-off-by: JCW <a1q123456@users.noreply.github.com> # Conflicts: # src/test/app/FeeVote_test.cpp # src/test/consensus/NegativeUNL_test.cpp # src/xrpld/app/paths/TrustLine.cpp # src/xrpld/rpc/handlers/AccountLines.cpp # src/xrpld/rpc/handlers/GatewayBalances.cpp # src/xrpld/rpc/handlers/NoRippleCheck.cpp
| } | ||
|
|
||
| std::shared_ptr<Ledger const> ret = loadByIndex(index, app_); | ||
| Rules const rules{app_.config().features}; |
There was a problem hiding this comment.
I see that in every call you need to wrap the config's features in a Rules instance. How much effort would it be to add a toRules() function - like the toFees() - to make it more intuitive?
There was a problem hiding this comment.
Hmm I just added it locally but it seems that it doesn't provide any benefits.
What we currently have is: Rules{app_.config().features}, and what it'll look like with toRules is toRules(app_.config().features).
It doesn't look too much different except it comes with more characters.
There was a problem hiding this comment.
Got it. I was more thinking along the lines of changing the std::unordered_set<uint256, beast::uhash<>> features data type to be a wrapper class that would then have a toRules function that could be called.
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>
Co-authored-by: Bart <bthomee@users.noreply.github.com>
There was a problem hiding this comment.
Went over the changes
Several correctness and safety issues flagged inline: unconditional finishWork on save failure (line 36), reference capture lifetime risk in async job (line 76), current-config rules/fees used for historical ledger loading (LedgerHistory.cpp:118), missing finishLoadByIndexOrHash in getLatestLedger (line 128), duplicated hash logic with no consistency enforcement (LedgerHeader.cpp:59), and unverified callers of the new isSensible() soft-return API (Ledger.h:330).
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
| uint256 | ||
| calculateLedgerHash(LedgerHeader const& info) | ||
| { | ||
| // VFALCO This has to match addRaw in View.h. |
There was a problem hiding this comment.
Duplicated hash logic with no enforcement — divergence from addRaw in View.h will silently break hash consistency. Consider extracting the shared serialization into a single helper called by both, or add a static_assert / test that cross-checks the two outputs.
There was a problem hiding this comment.
This is moved from the original file src/xrpld/app/paths/TrustLine.cpp
There was a problem hiding this comment.
Should this be tracked so it can be fixed in a follow-up PR?
There was a problem hiding this comment.
I think this can be an optimisation PR for future.
| } | ||
|
|
||
| ret = loadByHash(hash, app_); | ||
| Rules const rules{app_.config().features}; |
There was a problem hiding this comment.
Current-config rules/fees used to load a historical ledger — semantic mismatch if these influence validation during deserialization. Should these come from the ledger's own epoch rather than app_.config()?
There was a problem hiding this comment.
No. loadByHash requires the current rules and Ledger::setup() will overwrite rules later after it finds the ledger data.
|
|
||
| // Clients can now trust the database for | ||
| // information about this ledger sequence. | ||
| registry.pendingSaves().finishWork(seq); |
There was a problem hiding this comment.
finishWork called unconditionally — signals completion even when saveValidatedLedger failed. Move it inside a success branch:
if (res)
registry.pendingSaves().finishWork(seq);
return res;
There was a problem hiding this comment.
This is moved from the original file src/xrpld/app/paths/TrustLine.cpp
There was a problem hiding this comment.
Is this a valid issue that we need to follow-up on in a separate PR?
There was a problem hiding this comment.
It shouldn't be fixed in this PR for sure, but give me a moment, let me just double check if what AI says makes sense.
There was a problem hiding this comment.
I don't think it makes sense, and it's not a real issue.
After we fail to save the ledger, we're done with the ledger and we'll never retry. I don't think it makes sense to call finishWork conditionally.
| registry.getJobQueue().addJob( | ||
| isCurrent ? jtPUBLEDGER : jtPUBOLDLEDGER, | ||
| std::to_string(ledger->seq()), | ||
| [®istry, ledger, isCurrent]() { saveValidatedLedger(registry, ledger, isCurrent); })) |
There was a problem hiding this comment.
registry captured by reference in an async job — use-after-free if ServiceRegistry is destroyed before the job runs. Capture by pointer or document the lifetime guarantee explicitly:
| [®istry, ledger, isCurrent]() { saveValidatedLedger(registry, ledger, isCurrent); })) | |
| [®istry, ledger, isCurrent]() { saveValidatedLedger(registry, ledger, isCurrent); })) // NOLINT: registry must outlive all submitted jobs |
|
|
||
| bool | ||
| assertSensible(beast::Journal ledgerJ) const; | ||
| isSensible() const; |
There was a problem hiding this comment.
assertSensible previously crashed on invalid state; isSensible() silently returns false. Are all callers updated to check the return value and handle failure?
There was a problem hiding this comment.
Yes. Only Application.cpp calls it and I've updated the call site.
| return {loadLedgerHelper(*info, rules, fees, registry, true), info->seq, info->hash}; | ||
| } | ||
|
|
||
| std::shared_ptr<Ledger> |
There was a problem hiding this comment.
getLatestLedger skips finishLoadByIndexOrHash — the returned ledger won't have setImmutable()/setFull() called or the fees assertion checked. Is this intentional?
There was a problem hiding this comment.
This is moved from the original file src/xrpld/app/paths/TrustLine.cpp
There was a problem hiding this comment.
Same here, is this a valid issue and should we fix it in a follow-up PR?
There was a problem hiding this comment.
we'll call those functions in ApplicationImp::getLastFullLedger() so this is intentional.
| } | ||
|
|
||
| std::shared_ptr<Ledger> | ||
| loadLedgerHelper( |
There was a problem hiding this comment.
Lambda captures ServiceRegistry by reference in async job
In pendSaveValidated, the lambda passed to addJob captures registry by reference ([®istry, ledger, isCurrent]). If the job executes after the ServiceRegistry reference becomes invalid (e.g., during shutdown), this is a use-after-free. The original code had the same pattern with app, where Application is long-lived, but here the parameter type is a reference to an abstract interface — the lifetime guarantee is less explicit.
Suggested fix: If ServiceRegistry has a shared ownership model, capture a std::shared_ptr<ServiceRegistry> instead. Otherwise, document explicitly that ServiceRegistry must outlive all submitted jobs.
See: std::shared_ptr
There was a problem hiding this comment.
ServiceRegistry is never a shared pointer and it lives as long as the program lives
Signed-off-by: JCW <a1q123456@users.noreply.github.com> Co-authored-by: Bart <bthomee@users.noreply.github.com> Co-authored-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
High Level Overview of Change
This PR modularises ledger-related components by moving them from xrpld (server implementation) to libxrpl (core protocol library), making them available for reuse by external projects that depend on libxrpl.
Key changes:
Context of Change
This is part of an ongoing effort to modularise the rippled codebase by properly separating the reusable protocol library (libxrpl) from the server implementation (xrpld). The Ledger class is a fundamental type used throughout the codebase and should be part of the core library.
The TrustLine class has been rewritten to use the new ledger_entries::RippleState wrapper, providing a cleaner API that aligns with the code generation approach being introduced in the parent branch.
Path-finding components have been moved to rpc/detail as they are primarily used by RPC handlers and don't belong in the core ledger module.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Future Tasks
xrpl::Feesto make them more clear RIPD-5326