Open
Conversation
ximinez
force-pushed
the
ripple/staging-313
branch
from
f0e3bb7 to
3ba3fcf
Compare
ximinez
force-pushed
the
ripple/staging-313
branch
from
f0e3bb7 to
96af56a
Compare
Collaborator
Author
|
For the first two commits, I cherry-picked the respective commits from #6668 and #6644. However, because the files that were changed had been renamed / refactored, I essentially had to copy and paste all the changes. So far the Range-diff outputDirectly compare the commits for #6668Note that the last line is not a diff in the changes, but a difference in the starting state (which is because of formatting differences in the branches). |
ximinez
force-pushed
the
ripple/staging-313
branch
from
490fe68 to
0eb77f9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-3.1 #6703 +/- ##
=============================================
+ Coverage 79.3% 80.4% +1.1%
=============================================
Files 839 840 +1
Lines 71738 75352 +3614
Branches 8237 8198 -39
=============================================
+ Hits 56920 60591 +3671
+ Misses 14818 14761 -57
... and 459 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>
This change will only attempt to upload artifacts for CI runs performed in the XRPLF/rippled repository.
ximinez
force-pushed
the
ripple/staging-313
branch
from
628230a to
f85b917
Compare
Collaborator
Author
|
Notes before pushing to release: |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (222,814 chars). Complex issues deep in the diff may receive less attention.
Had a look at this
Four issues flagged: a critical consensus-divergence risk in LoanManage.cpp where the pre-amendment noop path no longer calls associateAsset, a misleading modulo truncation in OrderBookDB.cpp, swapped comments and duplicate assertions in the NFToken test. See inline comments for details.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V14
| return tesSUCCESS; | ||
| // Pre-amendment, associateAsset was only called on the noop (no flags) | ||
| // path. Post-amendment, we call associateAsset on all successful paths. | ||
| if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result)) |
Contributor
There was a problem hiding this comment.
Pre-amendment noop path skips associateAsset entirely — breaks ledger replay. The guard should only gate the expanded (all-paths) behavior; the noop case must still call associateAsset unconditionally to preserve pre-amendment ledger hashes and avoid consensus divergence.
| env(token::cancelOffer(buyer, {buyOffer1, sellOffer1})); | ||
| if (features[fixSecurity3_1_3]) | ||
| { | ||
| // The buy offer was deleted, so no need to cancel it |
Contributor
There was a problem hiding this comment.
Comments are swapped — the sell offer (sellOffer1) was auto-deleted; the buy offer (buyOffer1) still exists and is cancelled on line 3609:
// The sell offer was deleted, so no need to cancel it
// The buy offer still exists, so we can cancel it
| // handlers/Ledger, Unsubscribe | ||
| JSS(accounts_proposed); // in: Subscribe, Unsubscribe | ||
| JSS(action); | ||
| JSS(active); // out: OverlayImpl |
Contributor
There was a problem hiding this comment.
JSS(active) is out of alphabetical order — "active" sorts after "acquiring". Swap the two lines:
JSS(acquiring); // out: LedgerRequest
JSS(active); // out: OverlayImpl
| BEAST_EXPECT(ownerCount(env, buyer) == 1); | ||
| aliceCount++; | ||
| buyerCount++; | ||
| BEAST_EXPECT(ownerCount(env, alice) == aliceCount); |
Contributor
There was a problem hiding this comment.
Duplicate assertions — identical checks at 1239-1240 and 1242-1243 with no state change between them. Remove the first pair:
| app_.getJobQueue().addJob( | ||
| jtUPDATE_PF, | ||
| "OrderBookDB::update: " + std::to_string(ledger->seq()), | ||
| "OB" + std::to_string(ledger->seq() % 1000000000), |
Contributor
There was a problem hiding this comment.
% 1000000000 truncates valid uint32 seq values ≥ 1,000,000,000, showing a wrong value in logs. The comment's "max 9 digits" claim is also wrong (uint32 max is 10 digits, total still fits in 15 chars). Drop the modulo:
Suggested change
| "OB" + std::to_string(ledger->seq() % 1000000000), | |
| "OB" + std::to_string(ledger->seq()), |
Contributor
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (253,801 chars). Complex issues deep in the diff may receive less attention.
Went through the changes
Two issues flagged inline: a potential consensus-breaking replay difference in LoanManage.cpp (pre-amendment noop path no longer calls associateAsset), and an unnecessary modulo in OrderBookDB.cpp that introduces thread-name collisions.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V15
| return tesSUCCESS; | ||
| // Pre-amendment, associateAsset was only called on the noop (no flags) | ||
| // path. Post-amendment, we call associateAsset on all successful paths. | ||
| if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result)) |
Contributor
There was a problem hiding this comment.
Pre-amendment noop path behavior changed — old code called associateAsset unconditionally on noop; new code skips it without fixSecurity3_1_3. This is a potential consensus-breaking replay difference if any pre-amendment noop LoanManage transactions exist on-ledger. Confirm no such transactions exist, or add an else branch to preserve the old noop behavior:
| app_.getJobQueue().addJob( | ||
| jtUPDATE_PF, | ||
| "OrderBookDB::update: " + std::to_string(ledger->seq()), | ||
| "OB" + std::to_string(ledger->seq() % 1000000000), |
Contributor
There was a problem hiding this comment.
Unnecessary % 1000000000 — seq fits in 15 chars without it and the modulo causes seq collisions (e.g. 1 and 1000000001 get the same thread name).
Suggested change
| "OB" + std::to_string(ledger->seq() % 1000000000), | |
| "OB" + std::to_string(ledger->seq()), |
This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.
Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
The `ledger_entry` and `deposit_preauth` requests require an array of credentials. However, the array size is not checked before is gets processing. This fix adds checks and return errors in case array size is too big.
This change fixes an indexing typo in the `book_offers` CLI processing, and does not affect the HTTPS/WS RPC processing.
If the config disables SQL db usage, such as a validator: ``` [ledger_tx_tables] use_tx_tables = 0 ``` then the pointer to DB engine is null, but it was still resolved during startup. Although it didn't crash in Release mode, possibly due to the compiler optimizing it away, it did crash in Debug mode. This change explicitly checks for the validity of the pointer and generates a runtime error if not set.
This change: * Truncates thread names if more than 15 chars with `snprintf`. * Adds warnings for truncated thread names if `-DTRUNCATED_THREAD_NAME_LOGS=ON`. * Add a static assert for string literals to stop compiling if > 15 chars. * Shortens `Resource::Manager` to `Resource::Mngr` to fix the static assert failure. * Updates `CurrentThreadName_test` unit test specifically for Linux to verify truncation.
`PeerImp` processes `TMGetObjectByHash` queries with an unbounded per-request loop, which performs a `NodeStore` fetch and then appends retrieved data to the reply for each queried object without a local count cap or reply-byte budget. However, the `Nodestore` fetches are expensive when high in numbers, which might slow down the process overall. Hence this code change adds an upper cap on the response size.
This change introduces the `fixExpiredNFTokenOfferRemoval` amendment that allows expired offers to pass through `preclaim()` and be deleted in `doApply()`, following the same pattern used for expired credentials.
When `gateway_balances` gets called on an account that is involved in the `EscrowCreate` transaction (with MPT being escrowed), the method returns internal error. This change fixes this case by excluding the MPT type when totaling escrow amount.
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com> Co-authored-by: Mayukha Vadari <mvadari@gmail.com> Co-authored-by: Mayukha Vadari <mvadari@ripple.com> Co-authored-by: Bart <bthomee@users.noreply.github.com>
This change: * Removes a set of unnecessary brackets in the initialization of an `std::uint32_t`. * Fixes a couple of incorrect flags (same value, just wrong variables - so no amendment needed).
…#6212) This change continues the thread naming work from #5691 and #5758, which enables more useful lock contention profiling by ensuring threads/jobs have short, stable, human-readable names (rather than being truncated/failing due to OS limits). This changes diagnostic naming only (thread names and job/load-event labels), not behavior. Specific modifications are: * Shortens all thread/job names used with `beast::setCurrentThreadName`, so the effective Linux thread name stays within the 15-character limit. * Removes per-ledger sequence numbers from job/thread names to avoid long labels. This improves aggregation in lock contention profiling for short-lived job executions.
Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com>
Co-authored-by: Bart <bthomee@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Ed Hennis <ed@ripple.com>
ximinez
force-pushed
the
ripple/staging-313
branch
from
22264fa to
7ea4a5f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
To be used as the base branch for PRs that are intended for release 3.1.3.
Do not use the Github UI to merge this PR. Push the branch with all individual commits using the command line.
Context of Change
API Impact