Sponsored Content
Skip to content

fix: increment sequence when accepting new manifests - #6059

Merged
bthomee merged 2 commits into
XRPLF:developfrom
sublimator:nd-increment-sequence-when-accepting-new-manifests-2025-11-20
Feb 5, 2026
Merged

fix: increment sequence when accepting new manifests#6059
bthomee merged 2 commits into
XRPLF:developfrom
sublimator:nd-increment-sequence-when-accepting-new-manifests-2025-11-20

Conversation

@sublimator

Copy link
Copy Markdown
Contributor

The function was returning early without incrementing seq_++. OverlayImpl uses this sequence to identify/invalidate a cached TMManifests message which is exchanged with peers on connection. Depending on network size, startup sequencing, and topology, this could cause syncing issues.

@sublimator
sublimator requested a review from a team November 20, 2025 09:18
@bthomee
bthomee requested review from vlntb and vvysokikh1 November 21, 2025 16:14

// Something has changed. Keep track of it.
seq_++;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From: Xahau/xahaud#631

OverlayImpl::getManifestsMessage() caches manifest messages and only rebuilds
them when ManifestCache::sequence() changes. When accepting a new manifest,
the function returned early without incrementing seq_, causing the cache to
never invalidate. This meant peers were exchanging stale lists.

Now seq_++ is called for both new manifests and updates, ensuring the overlay
layer detects changes and sends complete validator lists to connecting peers.

I think this has been masked all these years by validator list (e.g. https://vl.ripple.com/) usage which bootstraps manifests

@vlntb vlntb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks correct to me.
One small request: can we extend Manifest_test to cover the regression where ManifestCache::sequence() wasn’t incremented when accepting a new manifest, and verify it stays unchanged on stale?
With that test in place, I’m happy to approve.

diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp
index 09054658c9..5276f188c6 100644
--- a/src/test/app/Manifest_test.cpp
+++ b/src/test/app/Manifest_test.cpp
@@ -997,11 +997,16 @@ public:
 
             // applyManifest should accept new manifests with
             // higher sequence numbers
+            auto const seq0 = cache.sequence();
             BEAST_EXPECT(
                 cache.applyManifest(clone(s_a0)) ==
                 ManifestDisposition::accepted);
+            BEAST_EXPECT(cache.sequence() > seq0);
+
+            auto const seq1 = cache.sequence();
             BEAST_EXPECT(
                 cache.applyManifest(clone(s_a0)) == ManifestDisposition::stale);
+            BEAST_EXPECT(cache.sequence() == seq1);

@sublimator

Copy link
Copy Markdown
Contributor Author

@vlntb

Hey, was quite hectic last December, and I missed this

Let me sort that out for you

@sublimator
sublimator force-pushed the nd-increment-sequence-when-accepting-new-manifests-2025-11-20 branch from 7280122 to 8eb76e0 Compare February 5, 2026 02:21
The function was returning early without incrementing seq_++.
OverlayImpl uses this sequence to identify/invalidate a cached
TMManifests message which is exchanged with peers on connection.
Depending on network size, startup sequencing, and topology, this
could cause syncing issues.
@sublimator
sublimator force-pushed the nd-increment-sequence-when-accepting-new-manifests-2025-11-20 branch from 8eb76e0 to 15d5c91 Compare February 5, 2026 03:45
@vvysokikh1

Copy link
Copy Markdown
Contributor

lgtm!

@vlntb vlntb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vlntb vlntb 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 5, 2026
@codecov

codecov Bot commented Feb 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.9%. Comparing base (e79673c) to head (bd3421c).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6059   +/-   ##
=======================================
  Coverage     79.9%   79.9%           
=======================================
  Files          840     840           
  Lines        65548   65549    +1     
  Branches      7270    7262    -8     
=======================================
+ Hits         52344   52357   +13     
+ Misses       13204   13192   -12     
Files with missing lines Coverage Δ
src/xrpld/app/misc/detail/Manifest.cpp 86.1% <100.0%> (+0.1%) ⬆️

... and 5 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bthomee
bthomee merged commit 6006c28 into XRPLF:develop Feb 5, 2026
3 checks passed
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
The `ManifestCache::applyManifest` function was returning early without incrementing `seq_`. `OverlayImpl `uses this sequence to identify/invalidate a cached `TMManifests` message, which is exchanged with peers on connection. Depending on network size, startup sequencing, and topology, this can cause syncing issues. This change therefore increments `seq_` when a new manifest is accepted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants