updates to book_changes RPC command #5096

ckeshava · 2024-08-10T05:07:51Z

High Level Overview of Change

Updates to the book_changes RPC command.
Pending clarification, I believe this PR also fixes #5033 and #5036 (improves the latency of the command)

Context of Change

accepts shortcut strings (current, closed, validated) as inputs to ledger_index parameter in the RPC command.
returns validated: true/false in the RPC output

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)

…er-strings

…ated" field

codecov · 2024-08-10T05:33:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.2%. Comparing base (9a6af9c) to head (75cb233).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5096     +/-   ##
=========================================
+ Coverage     76.1%   76.2%   +0.1%     
=========================================
  Files          760     760             
  Lines        61548   61550      +2     
  Branches      8160    8124     -36     
=========================================
+ Hits         46834   46884     +50     
+ Misses       14714   14666     -48

Files with missing lines	Coverage Δ
src/xrpld/rpc/BookChanges.h	`14.4% <100.0%> (+14.4%)`	⬆️
src/xrpld/rpc/handlers/BookOffers.cpp	`98.8% <100.0%> (+6.0%)`	⬆️

... and 9 files with indirect coverage changes

src/xrpld/rpc/BookChanges.h

…commit removes the includes for protocol, json_reader and json_writer files

Bronek · 2024-09-13T17:12:24Z

@godexsoft you might want to test this branch with clio.

Bronek

I love that it gets rid of getLedgerByContext call in a USER context, also the newly added unit tests seem to bump test coverage overall. Nice work !

ckeshava · 2024-09-13T18:24:32Z

@Bronek thanks for the quick review!

What do you mean by USER context? I feel the getLedgerByContext should only be used by the consensus mechanism (and Full History Nodes) to collect+store all ledgers. Its in-efficient for a tiny or medium resources validator to engage in acquisition of ledgers.

The getLedgerByContext method can also be removed in the ledger_request RPC call. I expect to see similar issues in that RPC call. Check my comment here: #5036 (comment)

godexsoft

I don't see anything that could impact Clio.
My only concern is whether the new code still does the same error handling as the old did.

godexsoft · 2024-09-16T01:36:19Z

src/xrpld/rpc/handlers/BookOffers.cpp


-    if (std::holds_alternative<Json::Value>(res))
-        return std::get<Json::Value>(res);
+    Json::Value result = RPC::lookupLedger(ledger, context);


Does this change still yield the same error from checking ledger_index etc.?

Can we do the same in LedgerRequest.cpp and eliminate getLedgerByContext() entirely?

@godexsoft the ledger_index handling (and errors) will be the same as in almost every other command, since the vast majority of commands use lookupLedger. However, since with this change, the ledger won't be requested from other nodes, if a ledger which is not in the local cache is requested, this method will fail with ledgerNotFound error (just like other methods do in such situation)

No; the ADMIN method ledger_request which, with this PR, will be the only method using getLedgerByContext(), and it needs to rely on this function in order to download the ledger from other nodes.

Bronek · 2024-09-16T14:05:21Z

What do you mean by USER context? I feel the getLedgerByContext should only be used by the consensus mechanism (and Full History Nodes) to collect+store all ledgers. Its in-efficient for a tiny or medium resources validator to engage in acquisition of ledgers.

USER methods are accessible to users, ADMIN methods are accessible to admins only (i.e. they require a password and are bound to specific ip/port). The purpose of getLedgerByContext, as exposed by ledger_request is to allow downloading ledgers from other nodes, which is what InboundLedgers::acquire does. This has uses outside of consensus, e.g. replaying transactions. This is nothing to do with node size, and it will be slow by definition.

The getLedgerByContext method can also be removed in the ledger_request RPC call. I expect to see similar issues in that RPC call. Check my comment here: #5036 (comment)

Nope, it should not be removed. There is a reason why we need to be able to download ledgers from other nodes, and why we need to have an ADMIN method for this action.

Bronek

@ckeshava please also update API-CHANGELOG.md for release 2.3 . You need to add section for release 2.3 and list the changes in behaviour of book_changes (basically making it like all other methods, but also will now return ledgerNotFound if the ledger is very old). I suggest asking @mDuo13 for help.

API-CHANGELOG.md

intelliot

Suggested a rewording to the changelog for consistency. I also think it's ok to merge as-is and improve the docs later on (which is easy to do).

API-CHANGELOG.md

Co-authored-by: Elliot Lee <[email protected]>

ckeshava added 2 commits August 8, 2024 19:35

[FIX] XRPLF#5034 -- book_changes rpc method accepts conventional ledg…

52cd63c

…er-strings

[FIX] XRPLF#5035 - book_changes rpc method response includes a "valid…

88ef02e

…ated" field

Bronek self-requested a review September 9, 2024 12:37

Merge branch 'develop' into bookChanges

a8a21bf

Bronek reviewed Sep 9, 2024

View reviewed changes

src/xrpld/rpc/BookChanges.h Outdated Show resolved Hide resolved

ckeshava added 2 commits September 10, 2024 14:26

specify include header directives in BookChanges header file

e20f4a6

remove non-test-framework #includes from BookChanges_test file. This …

b2603ed

…commit removes the includes for protocol, json_reader and json_writer files

godexsoft self-requested a review September 13, 2024 17:15

Bronek approved these changes Sep 13, 2024

View reviewed changes

Merge branch 'develop' into bookChanges

0651c04

godexsoft reviewed Sep 16, 2024

View reviewed changes

godexsoft self-requested a review September 16, 2024 14:37

godexsoft approved these changes Sep 16, 2024

View reviewed changes

Bronek mentioned this pull request Sep 16, 2024

book_changes takes a long time querying full-history servers #5036

Closed

Bronek added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 16, 2024

Bronek requested changes Sep 16, 2024

View reviewed changes

ckeshava and others added 2 commits September 16, 2024 14:45

update API-changelog

f69b235

Merge branch 'develop' into bookChanges

39f56ab

ckeshava commented Sep 16, 2024

View reviewed changes

API-CHANGELOG.md Outdated Show resolved Hide resolved

Bronek requested a review from intelliot September 17, 2024 08:31

Bronek approved these changes Sep 17, 2024

View reviewed changes

Bronek removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 17, 2024

intelliot approved these changes Sep 17, 2024

View reviewed changes

API-CHANGELOG.md Outdated Show resolved Hide resolved

Update API-CHANGELOG.md

75cb233

Co-authored-by: Elliot Lee <[email protected]>

Bronek added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. and removed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. labels Sep 18, 2024

Bronek added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 18, 2024

Bronek mentioned this pull request Sep 19, 2024

Proposed 2.3.0-b4 #5141

Closed

1 task

intelliot merged commit b6391fe into XRPLF:develop Sep 19, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to book_changes RPC command #5096

updates to book_changes RPC command #5096

ckeshava commented Aug 10, 2024

codecov bot commented Aug 10, 2024 •

edited

Loading

Bronek commented Sep 13, 2024

Bronek left a comment

ckeshava commented Sep 13, 2024

godexsoft left a comment

godexsoft Sep 16, 2024

Bronek Sep 16, 2024 •

edited

Loading

Bronek commented Sep 16, 2024 •

edited

Loading

Bronek left a comment

intelliot left a comment

updates to book_changes RPC command #5096

updates to book_changes RPC command #5096

Conversation

ckeshava commented Aug 10, 2024

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

Bronek commented Sep 13, 2024

Bronek left a comment

Choose a reason for hiding this comment

ckeshava commented Sep 13, 2024

godexsoft left a comment

Choose a reason for hiding this comment

godexsoft Sep 16, 2024

Choose a reason for hiding this comment

Bronek Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Bronek commented Sep 16, 2024 • edited Loading

Bronek left a comment

Choose a reason for hiding this comment

intelliot left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2024 •

edited

Loading

Bronek Sep 16, 2024 •

edited

Loading

Bronek commented Sep 16, 2024 •

edited

Loading