Reduce duplicate peer traffic for ledger data #5126

ximinez · 2024-09-11T13:10:20Z

High Level Overview of Change

Several changes to help reduce message traffic and improve logging and visibility.

Suppress duplicate TMGetLedger and TMLedgerData messages, reducing the overhead of processing those messages.
Reduce the number of those messages sent to peers.
Improve logging related to those messages, as well as a few other areas.
Introduces a new protocol version to gate a new feature on the TMLedgerData message, which allows multiple identical requests to be replied to with one message.

These changes are organized into several commits which are organized logically separating each functional operation. They can be merged as-is, or squashed.

Context of Change

Analysis of the issue that led to #5115 identified heavy TMGetLedger request and TMLedgerData response traffic between nodes leading up to the syncing incidents. It was later determined that those messages were more a symptom of the problem, and not the root cause. However, leading up to identification of the root cause, these changes were being implemented to cut down on those messages, detect duplicates, etc. That reduction in unnecessary traffic is still valuable, so it's being included here.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance (increase or change in throughput and/or latency)

API Impact

None.

Test Plan

Verify that nodes are at least as reliable at staying in sync as they are now.
Verify that the indicated messages are sent less frequently by upgraded nodes.
Verify that peers that both have this change send fewer ledger messages to each other.

Future Tasks

I am still working on a follow-up to #4764 that makes use of these changes and other improvements to reduce the number of requests initiated by a given node in the first place.

* Also log as warning when the state lowers

codecov · 2024-09-11T14:13:59Z

Codecov Report

Attention: Patch coverage is 24.34988% with 320 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (7c9d652) to head (5438161).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/overlay/detail/PeerImp.cpp	0.0%	210 Missing ⚠️
src/xrpld/overlay/detail/ProtocolMessage.h	0.0%	32 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp	21.4%	22 Missing ⚠️
src/xrpld/overlay/detail/PeerSet.cpp	20.8%	19 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedgers.cpp	67.9%	17 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedger.cpp	54.5%	5 Missing ⚠️
src/xrpld/app/ledger/InboundLedger.h	60.0%	4 Missing ⚠️
src/xrpld/app/misc/HashRouter.cpp	60.0%	4 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerMaster.cpp	0.0%	3 Missing ⚠️
src/xrpld/app/misc/HashRouter.h	77.8%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5126     +/-   ##
=========================================
- Coverage     78.2%   77.9%   -0.2%     
=========================================
  Files          790     791      +1     
  Lines        67653   67975    +322     
  Branches      8161    8321    +160     
=========================================
+ Hits         52879   52968     +89     
- Misses       14774   15007    +233

Files with missing lines	Coverage Δ
include/xrpl/basics/base_uint.h	`96.9% <100.0%> (+<0.1%)`	⬆️
include/xrpl/protocol/LedgerHeader.h	`100.0% <ø> (ø)`
src/xrpld/app/ledger/detail/TimeoutCounter.cpp	`88.4% <100.0%> (+1.3%)`	⬆️
src/xrpld/app/ledger/detail/TimeoutCounter.h	`100.0% <ø> (ø)`
src/xrpld/app/misc/NetworkOPs.h	`100.0% <ø> (ø)`
src/xrpld/overlay/Peer.h	`100.0% <ø> (ø)`
src/xrpld/overlay/detail/PeerImp.h	`12.8% <ø> (ø)`
src/xrpld/overlay/detail/ProtocolVersion.cpp	`86.4% <ø> (ø)`
include/xrpl/basics/CanProcess.h	`95.5% <95.5%> (ø)`
src/xrpld/app/consensus/RCLConsensus.cpp	`65.4% <0.0%> (ø)`
... and 10 more

... and 6 files with indirect coverage changes

* Allow a retry after 30s in case of peer or network congestion. * Addresses RIPD-1870 * (Changes levelization. That is not desirable, and will need to be fixed.)

* Allow a retry after 15s in case of peer or network congestion. * Collate duplicate TMGetLedger requests: * The requestCookie is ignored when computing the hash, thus increasing the chances of detecting duplicate messages. * With duplicate messages, keep track of the different requestCookies (or lack of cookie). When work is finally done for a given request, send the response to all the peers that are waiting on the request, sending a separate message for each requestCookie. * Addresses RIPD-1871

* Addresses RIPD-1869 --------- Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Co-authored-by: Ed Hennis <ed@ripple.com>

* When work is done for a given TMGetLedger request, send the response to all the peers that are waiting on the request, sending one message per peer, including all the cookies and a "directResponse" flag indicating the data is intended for the sender, too.

Bronek · 2024-09-13T11:22:46Z

include/xrpl/basics/CanProcess.h

+    insert()
+    {
+        std::unique_lock<Mutex> lock_(mtx_);
+        bool exists = collection_.contains(item_);


This would avoid extra lookup

auto [_, inserted] = collection_.insert(item_); return inserted;

Good catch. Fixed.

* Avoid an unnecessary lookup in CanProcess

* upstream/develop: Set version to 2.3.0-b4 feat(SQLite): allow configurable database pragma values (5135) refactor: re-order PRAGMA statements (5140) fix(book_changes): add "validated" field and reduce RPC latency (5096) chore: fix typos in comments (5094) Set version to 2.2.3 Update SQLite3 max_page_count to match current defaults (5114)

vlntb

Only minor comments. Happy to approve once addressed.

include/xrpl/basics/base_uint.h

vlntb · 2024-10-10T14:22:16Z

src/xrpld/overlay/detail/PeerImp.cpp

+                        return ledger;
+                    }
+
+                    JLOG(p_journal_.trace())


[nit]: Should this be a warn instead of trace? Not having a peer to relay the request may indicate some configuration or environment issues.

Not necessarily, though. It could mean that the node has already sent the request to all it's peers. Also not that the original message is trace.

But there's something odd here. It looks like this code block was somehow duplicated! I must have messed up resolving a conflict when I rebased from master to develop. I've removed the duplicate.

vlntb · 2024-10-10T14:23:53Z

src/xrpld/overlay/detail/PeerImp.cpp

@@ -2936,7 +3078,9 @@ getPeerWithLedger(
 void
 PeerImp::sendLedgerBase(
    std::shared_ptr<Ledger const> const& ledger,
-    protocol::TMLedgerData& ledgerData)
+    protocol::TMLedgerData& ledgerData,
+    std::map<std::shared_ptr<Peer>, std::set<std::optional<uint64_t>>> const&


std::map<std::shared_ptr<Peer>, std::set<std::optional<uint64_t>>> is mentioned in four places. It would be more readable to define an alias:
using PeerCookieMap = std::map<std::shared_ptr<Peer>, std::set<std::optional<uint64_t>>>;

Good suggestion! Fixed.

* Add unit tests for to_short_string(base_uint * Remove duplicated code * Use type aliases for cookie maps

* That's what I get for rushing to push

* upstream/develop: Set version to 2.4.0-b3 Set version to 2.3.1 Update conan in the "nix" CI jobs Add RPC "simulate" to execute a dry run of a transaction (5069) Set version to 2.3.1-rc1 Reduce the peer charges for well-behaved peers:

* upstream/develop: Add deep freeze feature (XLS-77d) (5187)

* upstream/develop: Amendment `fixFrozenLPTokenTransfer` (5227) Improve git commit hash lookup (5225)

* upstream/develop: Updates Conan dependencies (5256)

* upstream/develop: fix: Do not allow creating Permissioned Domains if credentials are not enabled (5275) fix: issues in `simulate` RPC (5265)

bthomee · 2025-02-10T17:52:31Z

@Bronek Any outstanding issues for @ximinez to address?

* upstream/develop: fix: Amendment to add transaction flag checking functionality for Credentials (5250) fix: Omit superfluous setCurrentThreadName call in GRPCServer.cpp (5280)

Bronek · 2025-02-11T17:45:54Z

src/xrpld/app/ledger/InboundLedger.h

+        case CONSENSUS:
+            return "CONSENSUS";
+        default:
+            assert(false);


please replace with UNREACHABLE and a nice & short description why (see CONTRIBUTING.md , section " Contracts and instrumentation" for naming guideline)

please replace with UNREACHABLE and a nice & short description why (see CONTRIBUTING.md , section " Contracts and instrumentation" for naming guideline)

Done.

Bronek · 2025-02-11T17:47:14Z

src/xrpld/app/ledger/detail/InboundLedger.cpp

+        {
+            // Done. Something else (probably consensus) built the ledger
+            // locally while waiting for data (or possibly before requesting)
+            assert(isDone());


please replace with XRPL_ASSERT and a short description why; see CONTRIBUTING.md for contracts and instrumentation naming guidelines

please replace with XRPL_ASSERT and a short description why; see CONTRIBUTING.md for contracts and instrumentation naming guidelines

Done

Bronek · 2025-02-11T17:56:31Z

src/xrpld/app/misc/HashRouter.cpp

+
+    for (auto const peerID : s.peekPeerSet())
+    {
+        if (!callback(peerID))


How fast are these callbacks - we are running them over a loop inside critical section. Might need to think of an alternative solution, to avoid hogging the mutex_. Not a major issue, because as indicated in first sentence - I do not know if these callbacks are fast or slow (or we don't know ?)

The callbacks are very fast, but now I don't remember why I wanted to do this under lock.
PeerImp.cpp line 3528

std::set<HashRouter::PeerShortID> peers; app_.getHashRouter().forEachPeer( mHash, [&](HashRouter::PeerShortID const& peerID) { // callback is called under lock, so finish as fast as // possible. peers.insert(peerID); return true; });

Maybe I wanted avoid making a copy of the std::set? But this is effectively making a copy of the set anyway, so that's dumb. I'd guess I originally wrote it with the loop contents inside the callback, but decided that was too slow, and didn't think before just copying.

Bronek · 2025-02-11T18:10:28Z

src/xrpld/overlay/detail/PeerImp.cpp

There are 10 assert added here, can you please turn them to XRPL_ASSERT, following naming guideline in CONTRIBUTING.md ?

Bronek

Only minor remarks related to use of assert, once fixed feel free to merge

Note to self: add unit tests for PeerImp.cpp in a future PR

Bronek

Actually I realized I added a comment re. use of a loop and callback inside critical section. A sufficient explanation why this is safe might be sufficient to address this comment, but for now this is not approved.

Bronek · 2025-02-11T18:26:24Z

@Bronek Any outstanding issues for @ximinez to address?

Yes, just added some. The loop and callback inside critical section can be probably resolved with an explanation e.g. a comment in code. The assert ones are trivial to fix.

* upstream/develop: chore: Rename missing-commits job, and combine nix job files (5268) docs: Add a summary of the git commit message rules (5283)

* upstream/develop: fix: Replace charge() by fee_.update() in OnMessage functions (5269) docs: ensure build_type and CMAKE_BUILD_TYPE match (5274) chore: Fix small typos in protocol files (5279)

ximinez · 2025-02-13T23:26:23Z

Commit message:

Reduce duplicate peer traffic for ledger data (#5126)

- Drop duplicate outgoing TMGetLedger messages per peer
  - Allow a retry after 30s in case of peer or network congestion.
  - Addresses RIPD-1870
  - (Changes levelization. That is not desirable, and will need to be fixed.)
- Drop duplicate incoming TMGetLedger messages per peer
  - Allow a retry after 15s in case of peer or network congestion.
  - The requestCookie is ignored when computing the hash, thus increasing
    the chances of detecting duplicate messages.
  - With duplicate messages, keep track of the different requestCookies
    (or lack of cookie). When work is finally done for a given request,
    send the response to all the peers that are waiting on the request,
    sending one message per peer, including all the cookies and
    a "directResponse" flag indicating the data is intended for the
    sender, too.
  - Addresses RIPD-1871
- Drop duplicate incoming TMLedgerData messages
  - Addresses RIPD-1869
- Improve logging related to ledger acquisition
- Class "CanProcess" to keep track of processing of distinct items

---------

Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com>

- Convert a bunch of old asserts to XRPL_ASSERT - Change HashRouter::forEachPeer to getPeers, which just returns the set

Bronek

Thank you @ximinez , this is just to confirm that you have covered all the feedback I had for this PR, and I particularly like how you changed forEachPeer into getPeers and also paid attention to contract names. Nice !

ximinez added 2 commits September 11, 2024 12:34

Log the caller / reason for server state changes:

f0cf1fd

* Also log as warning when the state lowers

Class "CanProcess" to keep track of processing of distinct items

756cad9

ximinez added the Perf Attn Needed Attention needed from RippleX Performance Team label Sep 11, 2024

ximinez force-pushed the pr/getledger branch from c357abb to 9e5487c Compare September 11, 2024 13:55

ximinez force-pushed the pr/getledger branch 3 times, most recently from 09c4156 to c69b443 Compare September 11, 2024 14:48

ximinez and others added 5 commits September 11, 2024 11:09

Drop duplicate outgoing TMGetLedger messages per peer:

8a17f16

* Allow a retry after 30s in case of peer or network congestion. * Addresses RIPD-1870 * (Changes levelization. That is not desirable, and will need to be fixed.)

Drop duplicate incoming TMLedgerData messages:

d5ec2d3

* Addresses RIPD-1869 --------- Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Co-authored-by: Ed Hennis <ed@ripple.com>

Improve logging related to ledger acquisition

e490e57

ximinez force-pushed the pr/getledger branch from c69b443 to e490e57 Compare September 11, 2024 15:10

ximinez changed the title ~~Reduce ledger protocol message traffic~~ Reduce peer message traffic for ledger data Sep 11, 2024

ximinez requested a review from mtrippled September 11, 2024 20:37

ximinez assigned mtrippled Sep 11, 2024

ximinez added this to the 2.3.0 (August 2024) milestone Sep 11, 2024

ximinez marked this pull request as ready for review September 11, 2024 20:45

ximinez mentioned this pull request Sep 12, 2024

Fix deadlock in InboundLedgers and NetworkOPs #5124

Merged

1 task

Bronek reviewed Sep 13, 2024

View reviewed changes

ximinez added 2 commits September 24, 2024 20:14

Review feedback from @Bronek:

30eee9b

* Avoid an unnecessary lookup in CanProcess

vlntb assigned vlntb and unassigned vlntb Oct 1, 2024

vlntb self-requested a review October 1, 2024 10:57

vlntb reviewed Oct 10, 2024

View reviewed changes

ximinez added 2 commits October 11, 2024 19:10

[FOLD] Review feedback from @vlntb:

b8b7b31

* Add unit tests for to_short_string(base_uint * Remove duplicated code * Use type aliases for cookie maps

[FOLD] Fix typo in unit test:

43b6e3e

* That's what I get for rushing to push

vlntb approved these changes Oct 15, 2024

View reviewed changes

ximinez added 5 commits January 30, 2025 13:38

Merge remote-tracking branch 'upstream/develop' into pr/getledger

79f9bb8

* upstream/develop: Add deep freeze feature (XLS-77d) (5187)

Merge remote-tracking branch 'upstream/develop' into pr/getledger

fd94b55

* upstream/develop: Amendment `fixFrozenLPTokenTransfer` (5227) Improve git commit hash lookup (5225)

Merge remote-tracking branch 'upstream/develop' into pr/getledger

7e52abd

* upstream/develop: Updates Conan dependencies (5256)

Merge remote-tracking branch 'upstream/develop' into pr/getledger

829e4f1

* upstream/develop: fix: Do not allow creating Permissioned Domains if credentials are not enabled (5275) fix: issues in `simulate` RPC (5265)

Merge remote-tracking branch 'upstream/develop' into pr/getledger

ca07176

* upstream/develop: fix: Amendment to add transaction flag checking functionality for Credentials (5250) fix: Omit superfluous setCurrentThreadName call in GRPCServer.cpp (5280)

Bronek reviewed Feb 11, 2025

View reviewed changes

Bronek approved these changes Feb 11, 2025

View reviewed changes

Bronek self-requested a review February 11, 2025 18:19

Bronek reviewed Feb 11, 2025

View reviewed changes

ximinez added 3 commits February 12, 2025 11:32

Merge remote-tracking branch 'upstream/develop' into pr/getledger

39fe006

* upstream/develop: chore: Rename missing-commits job, and combine nix job files (5268) docs: Add a summary of the git commit message rules (5283)

Merge remote-tracking branch 'upstream/develop' into pr/getledger

43dcbe3

* upstream/develop: fix: Replace charge() by fee_.update() in OnMessage functions (5269) docs: ensure build_type and CMAKE_BUILD_TYPE match (5274) chore: Fix small typos in protocol files (5279)

Fix formatting

11d6fec

Merge branch 'develop' into pr/getledger

3a6ba60

ximinez requested a review from Bronek February 14, 2025 00:08

ximinez added 3 commits February 13, 2025 19:09

Review feedback from @Bronek

6a6e895

- Convert a bunch of old asserts to XRPL_ASSERT - Change HashRouter::forEachPeer to getPeers, which just returns the set

Merge branch 'develop' into pr/getledger

db098fa

Merge branch 'develop' into pr/getledger

5438161

ximinez 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 14, 2025

ximinez changed the title ~~Reduce peer message traffic for ledger data~~ Reduce duplicate peer traffic for ledger data Feb 14, 2025

ximinez merged commit dd5e655 into XRPLF:develop Feb 14, 2025
21 of 22 checks passed

ximinez deleted the pr/getledger branch February 14, 2025 23:51

Bronek reviewed Feb 15, 2025

View reviewed changes

legleux mentioned this pull request Feb 18, 2025

Proposed 2.4.0-rc1 #5295

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce duplicate peer traffic for ledger data #5126

Reduce duplicate peer traffic for ledger data #5126

ximinez commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

Bronek Sep 13, 2024 •

edited

Loading

ximinez Sep 25, 2024

vlntb left a comment

vlntb Oct 10, 2024

ximinez Oct 11, 2024

vlntb Oct 10, 2024

ximinez Oct 11, 2024

bthomee commented Feb 10, 2025

Bronek Feb 11, 2025

ximinez Feb 13, 2025

Bronek Feb 11, 2025

ximinez Feb 13, 2025

Bronek Feb 11, 2025

ximinez Feb 14, 2025

Bronek Feb 11, 2025

Bronek left a comment

Bronek left a comment

Bronek commented Feb 11, 2025

ximinez commented Feb 13, 2025

Bronek left a comment

Reduce duplicate peer traffic for ledger data #5126

Reduce duplicate peer traffic for ledger data #5126

Conversation

ximinez commented Sep 11, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Future Tasks

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Bronek Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlntb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthomee commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bronek left a comment

Choose a reason for hiding this comment

Bronek left a comment

Choose a reason for hiding this comment

Bronek commented Feb 11, 2025

ximinez commented Feb 13, 2025

Bronek left a comment

Choose a reason for hiding this comment

ximinez commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

Bronek Sep 13, 2024 •

edited

Loading