Skip to content

fix: Prevents overwritting a bool value in an invariant (#6609)#6961

Open
ximinez wants to merge 10 commits intoripple/staging-313from
ximinez/invariant-bugfix-313
Open

fix: Prevents overwritting a bool value in an invariant (#6609)#6961
ximinez wants to merge 10 commits intoripple/staging-313from
ximinez/invariant-bugfix-313

Conversation

@ximinez
Copy link
Copy Markdown
Collaborator

@ximinez ximinez commented Apr 16, 2026

High Level Overview of Change

This PR backports the changes from #6609 to 3.1.x.

@github-actions
Copy link
Copy Markdown

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

@ximinez ximinez force-pushed the ximinez/invariant-bugfix-313 branch from 85985ba to 43daf4e Compare April 16, 2026 22:34
@github-actions
Copy link
Copy Markdown

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

@ximinez ximinez added this to the 3.1.3 milestone Apr 16, 2026
@ximinez ximinez changed the title fix: prevents overwritting a bool value in an invariant (#6609) fix: Prevents overwritting a bool value in an invariant (#6609) Apr 16, 2026
@ximinez ximinez marked this pull request as ready for review April 16, 2026 22:51
@ximinez
Copy link
Copy Markdown
Collaborator Author

ximinez commented Apr 16, 2026

/ai-review

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

Critical correctness bug: line 416 (bad_ = outstanding < *locked) was not removed as part of this fix and silently overwrites bad_ before the new |= logic runs, defeating the fix entirely. See inline comments.

Review by Claude Opus 4.6 · Prompt: V15

Comment thread src/xrpld/app/tx/detail/InvariantCheck.cpp Outdated
Comment thread src/xrpld/app/tx/detail/InvariantCheck.cpp
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Backports the fix from #6609 to prevent invariant state booleans from being overwritten across multiple visitEntry calls, and adds a safer isFeatureEnabled helper for contexts where transaction rules may be unset.

Changes:

  • Add amendment-gated “overwrite fix” logic to multiple invariants by accumulating (|=) rather than overwriting boolean state.
  • Extend isFeatureEnabled to support a caller-chosen default when no CurrentTransactionRules are set.
  • Update/add tests to exercise the overwrite fix both enabled and disabled, and simplify feature bitsets to rely on testable_amendments().

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/xrpld/app/tx/detail/InvariantCheck.cpp Adds overwrite-fix gating for invariant booleans (but currently contains a regression in one branch).
src/libxrpl/protocol/Rules.cpp Implements new isFeatureEnabled(feature, resultIfNoRules) behavior.
include/xrpl/protocol/Rules.h Updates isFeatureEnabled declaration and documents the new parameter (potential ABI/API considerations).
src/test/app/Invariants_test.cpp Adds helper amendments bitset + rules guard and runs overwrite tests for both enabled/disabled fix (currently has a namespace issue).
src/test/app/Vault_test.cpp Test cleanups and a type change in callbacks (currently uses an apparently invalid xrpl::Keylet type).
src/test/rpc/GatewayBalances_test.cpp Makes the feature bitset const (no behavior change).
src/test/app/Loan_test.cpp Simplifies feature set to testable_amendments().
src/test/app/LoanBroker_test.cpp Simplifies feature set to testable_amendments().
src/test/app/Batch_test.cpp Simplifies Env feature composition to use the passed-in feature bitset.
src/test/app/AMM_test.cpp Simplifies feature composition to rely on testable_amendments().

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

Comment thread src/test/app/Invariants_test.cpp Outdated
Comment thread src/test/app/Invariants_test.cpp Outdated
Comment thread src/test/app/Vault_test.cpp Outdated
Comment thread include/xrpl/protocol/Rules.h Outdated
Comment thread src/xrpld/app/tx/detail/InvariantCheck.cpp Outdated
@ximinez ximinez force-pushed the ximinez/invariant-bugfix-313 branch from 43daf4e to 7e12277 Compare April 16, 2026 22:58
@ximinez ximinez marked this pull request as draft April 16, 2026 22:59
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

Critical bug: the old bad_ = outstanding < *locked; on line 416 was never removed, so it overwrites bad_ unconditionally before the new |= guarded logic runs — the fix is a no-op. The other two invariant checks (NoXRPTrustLines, NoDeepFreezeTrustLinesWithoutFreeze) correctly dropped the old assignment; this one did not. See inline comment.

Review by Claude Opus 4.6 · Prompt: V15

Comment thread src/xrpld/app/tx/detail/InvariantCheck.cpp Outdated
@ximinez ximinez marked this pull request as ready for review April 17, 2026 16:45
- Brings over a function overload from `develop` that the new test uses
  to run with different amendments
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

ODR violation flagged inline — function body in header needs inline.


Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V15

Comment thread include/xrpl/protocol/Rules.h Outdated
@ximinez ximinez requested a review from vlntb April 17, 2026 20:48
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@github-actions
Copy link
Copy Markdown

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

ximinez added 2 commits April 17, 2026 18:01
…invariant-bugfix-313

* XRPLF/ripple/staging-313:
  fix: Add rounding to Vault invariants (6217) (6957)
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@github-actions
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@kuznetsss
Copy link
Copy Markdown
Contributor

kuznetsss commented Apr 20, 2026

It looks like changes in InvariantCheck.cpp from the original PR are missing.
Sorry, the file was at the bottom of the file list, so I missed it.

Copy link
Copy Markdown
Contributor

@kuznetsss kuznetsss left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

- Tests of the old behavior were failing because they relied on a change
  from e11f619 that did not require the output to contain "Invariant
  failed" if the transaction succeeded. Since the legacy transactions
  are expected to succeed, they were failing that test.
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@ximinez ximinez requested review from kuznetsss and vlntb April 20, 2026 20:42
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.5%. Comparing base (673af42) to head (95afd29).
⚠️ Report is 1 commits behind head on ripple/staging-313.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##           ripple/staging-313   #6961   +/-   ##
==================================================
  Coverage                79.5%   79.5%           
==================================================
  Files                     840     840           
  Lines                   71985   72000   +15     
  Branches                 8202    8201    -1     
==================================================
+ Hits                    57221   57236   +15     
  Misses                  14764   14764           
Files with missing lines Coverage Δ
include/xrpl/protocol/Rules.h 100.0% <100.0%> (ø)
src/libxrpl/protocol/Rules.cpp 98.2% <100.0%> (+0.1%) ⬆️
src/xrpld/app/tx/detail/InvariantCheck.cpp 92.9% <100.0%> (+<0.1%) ⬆️

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

@github-actions
Copy link
Copy Markdown

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

…invariant-bugfix-313

* XRPLF/ripple/staging-313:
  fix: Improve invariant check for Permissioned Domains (6134) (6958)
@github-actions
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

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

Compilation error from split string literal — see inline comment.


Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V15

messages.starts_with("Invariant failed:") ||
messages.starts_with("Transaction caused an exception"),
messages);
messages.starts_with("Transaction caused an
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.

String literal accidentally split across two lines — compilation error. Fix:

                    messages.starts_with("Transaction caused an exception"),
                    messages);

vlntb
vlntb approved these changes Apr 21, 2026
@vlntb vlntb self-requested a review April 21, 2026 11:49
Copy link
Copy Markdown
Contributor

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

The change looks correct.
Happy to approve once the erroneous line split spotter by xrplf-ai-reviewer is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants