💡 Tips: fix: Fix regressions in `server_definitions` by mvadari · Pull Request #7008 · XRPLF/rippled · GitHub

mvadari · 2026-04-23T20:55:19Z

High Level Overview of Change

This PR fixes regressions in the output of server_definitions, namely:

The FIELDS array was not sorted by field value
sfGeneric was listed twice in the FIELDS array

Context of Change

These regressions were missed in #5590, caught by @pdp2121.

API Impact

Only fixes, no API format change

mvadari · 2026-04-23T20:55:50Z

This constructor is only used by sfGeneric and sfInvalid

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

Copilot

Pull request overview

This PR aims to restore expected server_definitions output behavior by making the FIELDS section deterministic and removing a duplicate sfGeneric entry that was introduced in #5590.

Changes:

Remove the manual Generic insertion and prevent duplicates between manually-added fields and registered SFields.
Make FIELDS ordering deterministic by iterating over a sorted view of SField::getKnownCodeToField().
Update server_definitions unit tests to reflect the new ordering and add a duplicate-name check.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/xrpld/rpc/handlers/server_info/ServerDefinitions.cpp	Switches `FIELDS` generation to a deterministic, sorted iteration and filters out manually-added field names to avoid duplicates.
src/test/rpc/ServerDefinitions_test.cpp	Updates assertions for the new `FIELDS` ordering and adds a no-duplicates check.
src/libxrpl/protocol/SField.cpp	Adjusts default signing-field behavior for the `(fieldCode, name)` `SField` constructor (affecting `sfGeneric` / `sfInvalid`).

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

Copilot · 2026-04-23T21:00:42Z

+                auto const field = result[jss::result][jss::FIELDS][6u];
+                BEAST_EXPECT(field[0u].asString() == "LedgerEntryType");
+                BEAST_EXPECT(field[1][jss::isSerialized].asBool() == true);
+                BEAST_EXPECT(field[1][jss::isSigningField].asBool() == true);
+                BEAST_EXPECT(field[1][jss::isVLEncoded].asBool() == false);


The assertion that FIELDS[6] is LedgerEntryType is brittle: adding any new SField with a smaller sort key will shift indices even if ordering is still correct. Consider locating the field by name (and then asserting its properties) and/or asserting the ordering property directly instead of relying on a hard-coded index.

That's done on purpose. Everything below it is hard-coded.

codecov · 2026-04-23T21:19:34Z

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #7008     +/-   ##
=========================================
- Coverage     82.1%   82.1%   -0.0%     
=========================================
  Files         1010    1010             
  Lines        76033   76024      -9     
  Branches      7379    7373      -6     
=========================================
- Hits         62426   62392     -34     
- Misses       13607   13632     +25

Files with missing lines	Coverage Δ
...pld/rpc/handlers/server_info/ServerDefinitions.cpp	`98.0% <100.0%> (-0.1%)`	⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

godexsoft

Looks alright to me. Please address copilot's concerns 👍

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

ximinez

I am not super-familiar with ServerDefinitions, but the changes look right, aside from the couple of comments below.

ximinez · 2026-04-30T18:20:18Z

    , fieldMeta(sMD_Never)
    , fieldNum(++num)
-    , signingField(IsSigning::yes)
+    , signingField(IsSigning::no)


This has been yes for 11 years (and true before that). I'm not entirely clear on why it needs to change. Could you explain what's going on?

sfGeneric also used to be labeled as IsSigning::no in the definitions.json output, so one of those is wrong then.

This constructor is only used for these two SFields, to clarify:

SField const sfInvalid(access, -1, ""); SField const sfGeneric(access, 0, "Generic");

sfGeneric also used to be labeled as IsSigning::no in the definitions.json output, so one of those is wrong then.

This constructor is only used for these two SFields, to clarify:

That makes sense.

However, it looks like the only reason sfGeneric was listed as non-signing is because it had been hard-coded. https://github.com/XRPLF/rippled/pull/7008/changes#diff-c9864c2527cd17eeac088af7a96edca5f1347208dc6b77a4317e3427cc2dca36L158-L160. This PR removes that hard-coding.

My concern is that even though these fields are only used as placeholders, and never intentionally used in transaction data, they may be ignored if they ever found their way into transaction data. E.g. through a bug, or perhaps a malicious injection into serialized data / protocol messages.

Please feel free to disagree, but I think the way forward here is to

revert the change SField.cpp back to IsSigning::Yes,

and then in ServerDefinitions.cpp either

Put the hard-coded "Generic" back, and skip over it when found in the loop (if (field->fieldName.empty() || field->fieldName == "Generic") continue;,

OR put an override in the assignment. innerObj[jss::isSigningField] = field->fieldName == "Generic" ? false : field->shouldInclude(false);

We can skip step 2 if you're okay with labeling sfGeneric as isSigningField: true since it seems that that's more accurate.

We can skip step 2 if you're okay with labeling sfGeneric as isSigningField: true since it seems that that's more accurate.

Yeah, I'm very happy with that! I didn't realize that was an acceptable outcome. 😀

I just assumed what was there before was the desired answer, based on this conversation that's not the case :)

xrplf-ai-reviewer

Had a look at this

The static_assert introduced for STI_VECTOR256 uses 15U instead of 19U — this will fail to compile. See inline.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V15

Copilot

Pull request overview

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

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

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

github-actions · 2026-05-03T11:09:42Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

godexsoft · 2026-05-04T14:23:52Z

We recently merged a refactor to develop that enables clang-tidy's readability-identifier-naming. Your branch now has heavy conflicts that are largely mechanical. Below is a workflow that aligns your branch's naming with develop before merging, which should minimize the merge conflicts.

One-time setup

If you don't already have clang-tidy working in your env, on macOS:

brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.

Workflow on your branch (before merging develop)

1. Grab the new .clang-tidy from develop without pulling anything else. Sync your fork on GitHub first, then:

git remote -v   # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy

2. Reconfigure conan/cmake so compile_commands.json is fresh.

3. Apply renames for the files modified in your PR:

git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
  | grep -E '\.(cpp|h|hpp|ipp)$' \
  | xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called

4. Build + test, then commit as a single dedicated commit:

cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"

5. Now merge develop:

git merge upstream/develop

Extra

Run clang-tidy once more after the merge to catch any stragglers introduced from develop's side:

run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called

github-actions · 2026-05-04T17:00:44Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/xrpld/rpc/handlers/server_info/ServerDefinitions.cpp:246

After removing the hard-coded "Generic" entry, the remaining "Generic" row will now be generated from kSF_GENERIC (which has fieldType == STI_UNKNOWN). With the current predicate (type < 10000 && name != "hash" && name != "index"), that makes Generic.isSerialized evaluate to true, which differs from the previous explicit isSerialized=false semantics for Generic and is likely misleading for an STI_UNKNOWN placeholder field. Consider excluding STI_UNKNOWN (and/or field->fieldCodeMem <= 0 / !field->isUseful()) from the isSerialized predicate, or reintroduce a dedicated exception entry for Generic and skip it in the loop to preserve the intended output contract.

        // whether the field is included in serialization
        innerObj[jss::isSerialized] =
            (type < 10000 && field->fieldName != "hash" &&
             field->fieldName !=
                 "index");  // hash, index, TRANSACTION, LEDGER_ENTRY, VALIDATION, METADATA

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

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V15

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

fix: Regressions in server_definitions

6afe1ba

Copilot AI review requested due to automatic review settings April 23, 2026 20:55

mvadari commented Apr 23, 2026

View reviewed changes

Copilot started reviewing on behalf of mvadari April 23, 2026 20:55 View session

clean up

a6d9060

mvadari requested review from godexsoft and ximinez April 23, 2026 20:57

xrplf-ai-reviewer Bot approved these changes Apr 23, 2026

View reviewed changes

Copilot AI reviewed Apr 23, 2026

View reviewed changes

godexsoft approved these changes Apr 27, 2026

View reviewed changes

mvadari added 2 commits April 27, 2026 09:46

fix type

0039ab8

Merge branch 'develop' into mvadari/fix-defs

3a7d3fb

xrplf-ai-reviewer Bot approved these changes Apr 27, 2026

View reviewed changes

ximinez requested changes Apr 30, 2026

View reviewed changes

mvadari added 2 commits April 30, 2026 14:51

Merge branch 'develop' into mvadari/fix-defs

bf15d04

add static asserts

e0ad496

Copilot AI review requested due to automatic review settings April 30, 2026 18:56

Copilot started reviewing on behalf of mvadari April 30, 2026 18:56 View session

xrplf-ai-reviewer Bot reviewed Apr 30, 2026

View reviewed changes

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread src/xrpld/rpc/handlers/server_info/ServerDefinitions.cpp Outdated

oops

8a8f471

xrplf-ai-reviewer Bot approved these changes Apr 30, 2026

View reviewed changes

github-actions Bot added the PR: has conflicts label May 3, 2026

Merge branch 'develop' into mvadari/fix-defs

cc6376d

Copilot AI review requested due to automatic review settings May 4, 2026 17:00

github-actions Bot removed the PR: has conflicts label May 4, 2026

Copilot started reviewing on behalf of mvadari May 4, 2026 17:01 View session

xrplf-ai-reviewer Bot approved these changes May 4, 2026

View reviewed changes

Copilot AI reviewed May 4, 2026

View reviewed changes

mvadari added 2 commits May 4, 2026 14:43

fix clang-tidy issues

33cd9c2

respond to comments

062d70c

xrplf-ai-reviewer Bot approved these changes May 4, 2026

View reviewed changes

ximinez approved these changes May 4, 2026

View reviewed changes

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 May 5, 2026

Merge branch 'develop' into mvadari/fix-defs

e7ae19c

Copilot AI review requested due to automatic review settings May 5, 2026 16:42

xrplf-ai-reviewer Bot approved these changes May 5, 2026

View reviewed changes

Copilot AI reviewed May 5, 2026

bthomee changed the title ~~fix: Regressions in server_definitions~~ fix: Fix regressions in server_definitions May 5, 2026

bthomee added this pull request to the merge queue May 5, 2026

Merged via the queue into develop with commit 5e1c35f May 5, 2026
1 check passed

bthomee deleted the mvadari/fix-defs branch May 5, 2026 18:02

mvadari added this to the 3.2.0 milestone May 20, 2026

Patel-Raj11 mentioned this pull request May 25, 2026

Update definitions.json from server_definitions RPC output XRPLF/xrpl4j#794

Merged

beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026

fix: Fix regressions in server_definitions (XRPLF#7008)

dd62c31

Uh oh!

Conversation

mvadari commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 3, 2026

Uh oh!

godexsoft commented May 4, 2026

One-time setup

Workflow on your branch (before merging develop)

Extra

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mvadari commented Apr 23, 2026 •

edited

Loading

codecov Bot commented Apr 23, 2026 •

edited

Loading