Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Reduce duplicate peer traffic for ledger data #5301

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Feb 19, 2025

Recreates the changes from #5126, and the related bug fix once I find it.

  • 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

High Level Overview of Change

Context of Change

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)

@ximinez ximinez changed the title DRAFT: Reduce duplicate peer traffic for ledger data (#5126) DRAFT: Reduce duplicate peer traffic for ledger data Feb 19, 2025
Copy link

codecov bot commented Feb 20, 2025

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 (844646d) to head (a78f44f).
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

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5301     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          791     791             
  Lines        68006   68006             
  Branches      8346    8350      +4     
=========================================
- Hits         52967   52960      -7     
- Misses       15039   15046      +7     
Files with missing lines Coverage Δ
include/xrpl/basics/base_uint.h 96.9% <100.0%> (ø)
include/xrpl/protocol/LedgerHeader.h 100.0% <ø> (ø)
src/xrpld/app/ledger/detail/TimeoutCounter.cpp 88.4% <100.0%> (ø)
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 4 files with indirect coverage changes

Impacted file tree graph

- 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 <[email protected]>
@ximinez ximinez force-pushed the ximinez/fix-getledger branch from a78f44f to 69c2c6c Compare February 20, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant