Skip to content

fix(service-disco): apply advertExpiry cap after retry-time subtraction, not before#2839

Merged
SionoiS merged 5 commits into
fix--wait-time-capfrom
fix--wait-time-cap-retry-order
Jul 16, 2026
Merged

fix(service-disco): apply advertExpiry cap after retry-time subtraction, not before#2839
SionoiS merged 5 commits into
fix--wait-time-capfrom
fix--wait-time-cap-retry-order

Conversation

@tinniaru3005

Copy link
Copy Markdown
Contributor

Summary

waitingTime() capped the wait at advertExpiry before registration()
subtracted the elapsed retry time (updateWaitAfterRetry()). That's the
wrong order: the cap collapses to the elapsed-time subtraction on every
retry instead of the other way around.

Fix:

  • waitingTime() no longer caps by advertExpiry — it returns the raw,
    un-accumulated wait, as it must for registration() to subtract elapsed
    retry time correctly.
  • registration() applies min(max(advertExpiry, ZeroDuration), tWait)
    after updateWaitAfterRetry(), right before the ticket is built. The
    max(..., ZeroDuration) also guards against a misconfigured negative
    advertExpiry producing a negative tWaitFor.

Affected Areas

  • Gossipsub

  • Transports

  • Peer Management / Discovery

  • Protocol Logic

  • Build / Tooling

  • Other

Compatibility & Downstream Validation

Reference PRs / branches / commits demonstrating successful integration:

  • Nimbus:

  • Waku:

  • Codex:

Impact on Library Users

Risk Assessment

References

Additional Notes

@tinniaru3005
tinniaru3005 requested a review from a team as a code owner July 16, 2026 08:42
@tinniaru3005
tinniaru3005 requested review from richard-ramos and vladopajic and removed request for a team July 16, 2026 08:42
@tinniaru3005 tinniaru3005 changed the title wait time cap order fix fix(service-disco): apply advertExpiry cap after retry-time subtraction, not before Jul 16, 2026
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@tinniaru3005
tinniaru3005 requested a review from SionoiS July 16, 2026 08:46
@github-project-automation github-project-automation Bot moved this from new to In Progress in nim-libp2p Jul 16, 2026
@codecov-commenter

codecov-commenter commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (fix--wait-time-cap@de85173). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##             fix--wait-time-cap    #2839   +/-   ##
=====================================================
  Coverage                      ?   81.23%           
=====================================================
  Files                         ?      170           
  Lines                         ?    30741           
  Branches                      ?       12           
=====================================================
  Hits                          ?    24971           
  Misses                        ?     5770           
  Partials                      ?        0           
Files with missing lines Coverage Δ
libp2p/protocols/service_discovery/registrar.nim 67.25% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SionoiS SionoiS 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

Just need a clean-up. Thank you.

Comment thread libp2p/protocols/service_discovery/registrar.nim Outdated
Comment thread tests/libp2p/service_discovery/test_registrar.nim Outdated
Comment thread tests/libp2p/service_discovery/test_registrar.nim Outdated
Comment thread tests/libp2p/service_discovery/test_registrar.nim Outdated
@SionoiS
SionoiS merged commit 10e973b into fix--wait-time-cap Jul 16, 2026
41 checks passed
@SionoiS
SionoiS deleted the fix--wait-time-cap-retry-order branch July 16, 2026 14:00
@github-project-automation github-project-automation Bot moved this from In Progress to done in nim-libp2p Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: done

Development

Successfully merging this pull request may close these issues.

6 participants