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

Revert "Reduce duplicate peer traffic for ledger data (#5126)" #5300

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Feb 19, 2025

High Level Overview of Change

This reverts commit dd5e655. It has introduced a regression causing slow close times and syncing issues. A fix will be attempted later.

Context of Change

#5126 introduced duplicate message checking and suppression. The absence of those messages may explain the symptoms that are being seen on test networks.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
    • (I haven't really fixed the bug, but this gets rid of it for now.)

API Impact

None

Before / After

Reverses the changes from #5126. The full set of changes does not necessarily have to be reviewed. It may be sufficient to verify that the code is what it would have been without that commit.

Future Tasks

Find and fix the actual bug. I have a feeling it's related to the message hashing.

This reverts commit dd5e655. It has
introduced a regression causing slow close times and syncing issues.
A fix will be attempted later.
@ximinez ximinez added Bug Trivial Simple change with minimal effect, or already tested. Only needs one approval. labels Feb 19, 2025
@ximinez ximinez added this to the 2.4.0 (Q1 2025) milestone Feb 19, 2025
@ximinez ximinez requested review from Bronek and vlntb February 19, 2025 20:17
@ximinez
Copy link
Collaborator Author

ximinez commented Feb 19, 2025

I can't approve my own request, but I rebased this branch, removing both dd5e655 and the new commit from this PR, and verified that the end result is identical.

$ git logdiff mywork/ximinez/revert5126 
Common ancestor
7c9d652d9b Support canonical ledger entry names (#5271)

-- mywork/ximinez/revert5126
dd5e6559dd Reduce duplicate peer traffic for ledger data (#5126)
db0fad6826 Log proposals and validations (#5291)
466849efe8 docs: Clarifies default port of hosts (#5290)
43e1d4440e fix: Switch Permissioned Domain to Supported::yes (#5287)
01fc8f2209 (upstream/release-next, upstream/develop) Set version to 2.4.0-rc1
7c8b17e739 (mywork/ximinez/revert5126) Revert "Reduce duplicate peer traffic for ledger data (#5126)"

-- HEAD
22bcd06da9 Log proposals and validations (#5291)
6f7adfc6f6 docs: Clarifies default port of hosts (#5290)
b77861ef91 fix: Switch Permissioned Domain to Supported::yes (#5287)
a4e861bca7 (HEAD -> ximinez/revert5126) Set version to 2.4.0-rc1
$ git diff 7c8b17e739 a4e861bca7

Has no output.

@ximinez 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 19, 2025
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 18.51852% with 88 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (844646d) to head (13cd843).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/PeerImp.cpp 0.0% 44 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp 15.2% 28 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedgers.cpp 52.6% 9 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedger.cpp 66.7% 2 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerMaster.cpp 0.0% 2 Missing ⚠️
src/xrpld/overlay/detail/PeerSet.cpp 0.0% 2 Missing ⚠️
src/xrpld/app/consensus/RCLConsensus.cpp 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5300     +/-   ##
=========================================
+ Coverage     77.9%   78.1%   +0.2%     
=========================================
  Files          791     790      -1     
  Lines        68006   67683    -323     
  Branches      8346    8191    -155     
=========================================
- Hits         52967   52873     -94     
+ Misses       15039   14810    -229     
Files with missing lines Coverage Δ
include/xrpl/basics/base_uint.h 96.8% <ø> (-<0.1%) ⬇️
include/xrpl/protocol/LedgerHeader.h 100.0% <ø> (ø)
src/xrpld/app/ledger/InboundLedger.h 69.2% <ø> (+4.0%) ⬆️
src/xrpld/app/ledger/detail/TimeoutCounter.cpp 87.1% <100.0%> (-1.3%) ⬇️
src/xrpld/app/ledger/detail/TimeoutCounter.h 100.0% <ø> (ø)
src/xrpld/app/misc/HashRouter.cpp 100.0% <ø> (+6.2%) ⬆️
src/xrpld/app/misc/HashRouter.h 100.0% <ø> (+4.9%) ⬆️
src/xrpld/app/misc/NetworkOPs.h 100.0% <ø> (ø)
src/xrpld/overlay/Peer.h 100.0% <ø> (ø)
src/xrpld/overlay/detail/PeerImp.h 12.8% <ø> (ø)
... and 9 more

... and 6 files with indirect coverage changes

Impacted file tree graph

@ximinez ximinez merged commit 159dfb5 into develop Feb 19, 2025
23 of 24 checks passed
@ximinez ximinez deleted the ximinez/revert5126 branch February 19, 2025 23:52
This was referenced Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. Trivial Simple change with minimal effect, or already tested. Only needs one approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants