Sponsored Content
Skip to content

fix: Minor RPC fixes - #6730

Merged
bthomee merged 4 commits into
developfrom
mvadari/rpc-fixes
Apr 6, 2026
Merged

fix: Minor RPC fixes#6730
bthomee merged 4 commits into
developfrom
mvadari/rpc-fixes

Conversation

@mvadari

@mvadari mvadari commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

  • ping: The ip field is no longer returned as an empty string for proxied connections without a forwarded-for header. It is now omitted, consistent with the behavior for identified connections.
    • There was also a missing break statement in the proxied connection case block.
  • gRPC GetLedgerDiff: Fixed error message that incorrectly said "base ledger not validated" when the desired ledger was not validated.

Context of Change

AI review/triage

BUG-U002: PROXY case unconditionally emits empty ip field unlike IDENTIFIED case

Severity: LOW

In doPing(), the IDENTIFIED case (lines 22-26) conditionally includes the ip field only when context.headers.forwardedFor is non-empty. The PROXY case (lines 28-30) unconditionally writes context.headers.forwardedFor to ret[jss::ip], even when the value is empty. This creates an inconsistency where PROXY connections may return "ip": "" in the response, while IDENTIFIED connections would omit the field entirely. A downstream client parsing the response could misinterpret an empty string as a valid IP address or treat the field's presence as meaningful.

Root cause: Lines 28-30 in Ping.cpp set ret[jss::ip] without checking !context.headers.forwardedFor.empty() first, unlike the analogous check at line 25 for the IDENTIFIED case.

BUG-U001: Missing break in Ping switch causes PROXY case to fall through to default

Severity: LOW

In doPing(), the switch statement on context.role handles Role::PROXY at line 28-30 but is missing a break statement. The case falls through directly to default:;. Currently the default case is an empty statement, so there is no behavioral change. However, this is a classic C++ switch-fallthrough bug pattern that violates defensive coding practices. If a future maintainer adds any logic to the default case (e.g., error logging, default field values), the PROXY case would silently execute that code as well, leading to unexpected behavior.

Root cause: The case Role::PROXY: block at line 28-30 in Ping.cpp does not end with a break statement. The case Role::IDENTIFIED: block above it correctly has a break at line 27, making the omission in the PROXY case clearly unintentional rather than a deliberate fallthrough pattern.

BUG-U004: Wrong error message for desired ledger in doLedgerDiffGrpc

Severity: LOW

When the desired ledger's dynamic_pointer_cast fails in doLedgerDiffGrpc, the error message incorrectly states 'base ledger not validated' instead of 'desired ledger not validated'. This misleads API consumers about which ledger parameter caused the error.

Root cause: LedgerDiff.cpp line 39: the error message string was copy-pasted from the base ledger check (line 31) without updating the text.

API Impact

Minor bugfixes, no API format change.

Copilot AI review requested due to automatic review settings April 6, 2026 21:37
@mvadari mvadari added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Apr 6, 2026
@mvadari
mvadari requested a review from bthomee April 6, 2026 21:37

Copilot AI 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.

Pull request overview

This PR applies small correctness fixes to RPC responses and gRPC error reporting, improving consistency and clarity for API consumers.

Changes:

  • ping: Omit the ip field for proxied connections when no forwarded-for header is present, and add the missing break in the Role::PROXY switch case.
  • gRPC GetLedgerDiff: Correct the NOT_FOUND error message when the desired ledger is not validated.
  • Add/adjust tests and document the behavior in the API changelog.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/xrpld/rpc/handlers/utility/Ping.cpp Fixes proxied ping response consistency and prevents switch fallthrough.
src/xrpld/rpc/handlers/ledger/LedgerDiff.cpp Fixes misleading gRPC error message for unvalidated desired ledger.
src/test/rpc/Roles_test.cpp Adds regression coverage ensuring proxied ping omits ip when absent.
API-CHANGELOG.md Documents the ping and gRPC LedgerDiff bugfixes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread API-CHANGELOG.md Outdated
@mvadari mvadari 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 Apr 6, 2026
@bthomee
bthomee enabled auto-merge April 6, 2026 22:15
@bthomee
bthomee added this pull request to the merge queue Apr 6, 2026
@codecov

codecov Bot commented Apr 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (077e03f) to head (9ebccf9).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/rpc/handlers/ledger/LedgerDiff.cpp 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6730     +/-   ##
=========================================
- Coverage     81.5%   81.4%   -0.0%     
=========================================
  Files         1006    1006             
  Lines        74461   74463      +2     
  Branches      7582    7557     -25     
=========================================
- Hits         60651   60640     -11     
- Misses       13810   13823     +13     
Files with missing lines Coverage Δ
src/xrpld/rpc/handlers/utility/Ping.cpp 100.0% <100.0%> (ø)
src/xrpld/rpc/handlers/ledger/LedgerDiff.cpp 0.0% <0.0%> (ø)

... and 4 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.

Merged via the queue into develop with commit 00761db Apr 6, 2026
2 of 3 checks passed
@bthomee
bthomee deleted the mvadari/rpc-fixes branch April 6, 2026 22:33
mvadari added a commit that referenced this pull request Apr 7, 2026
@mvadari mvadari linked an issue Apr 10, 2026 that may be closed by this pull request
marek-foss-neti pushed a commit to marek-foss-neti/rippled that referenced this pull request May 5, 2026
@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
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. 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.

Missing break in Ping handler PROXY case causes fallthrough

3 participants