Repository navigation
Conversation
1e8e5d9 to
f286bcb
Compare
f286bcb to
2fbc69f
Compare
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This PR adds a reverse flag to GetTransactions and lncli listchaintxns, so transactions can be listed oldest-first. It also switches the confirmation sort to sort.SliceStable and corrects the stale ListTransactionDetails docs. The change is small and correct for its main goal. Default ordering is unchanged, every RPCTransactionDetails caller visible in the diff is updated (ListSweeps passes false), and the new tests pin the exact output order in both modes.
Neither concern below is a defect. First, the PR's justification for the stable sort is wrong: pagination runs in the wallet layer before this sort, so the sort was never able to skip or duplicate transactions across pages. The doc comment and release note should stop saying it does. Second, the new field sits on a request message that SubscribeTransactions also uses, and nothing tells streaming clients that reverse does nothing there.
Lower-severity findings (2)
🟡 F1 (Minor) — Stable-sort docs claim a pagination fix that cannot occur · lnrpc/rpc_utils.go:126
RPCTransactionDetails only ever gets the page the wallet has already cut: rpcserver.go passes it txns, firstIdx, lastIdx from ListTransactionDetails, and the PR's own TestRPCTransactionDetailsPaginationOrdering says pagination "is applied by the wallet layer before RPCTransactionDetails is invoked". So the old sort.Slice could only reorder tied transactions inside a page; it could never make index_offset/max_transactions skip or duplicate anything. The stable sort is still worth keeping, because it makes the order inside a page deterministic. But the NOTE at rpc_utils.go:126-130 and the release note (docs/release-notes/unreleased/11365.md:8-12) present it as a pagination bug fix, which will mislead future readers. Reword both to say it gives a deterministic order within a page.
🟡 F2 (Minor) — reverse is silently ignored by SubscribeTransactions · lnrpc/lightning.proto:835
GetTransactionsRequest is also the request type for SubscribeTransactions (lightning.proto:80). The new reverse field therefore shows up on that streaming RPC too, and the regenerated swagger lists it on a second endpoint (lightning.swagger.json ~2918). Nothing in this diff reads it there, so a client that sets it on the stream gets no error and no effect. Add a sentence to the field comment saying it only applies to GetTransactions.
Reviewed at medium effort.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
2fbc69f to
ed47e28
Compare
|
Addressed both Gateway findings in ed47e28:
|
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F1 addressed: Fixed in
lnrpc/rpc_utils.go:126-130, and the release note matches atdocs/release-notes/unreleased/11365.md:8-11. The NOTE now says the stable sort gives "the returned page a deterministic order" and that pagination "is applied by the wallet layer before this sort and is unaffected by it". The release note now says "so the order within a page is deterministic", so the stable sort is no longer described as a pagination fix. - F2 addressed: Fixed in
lnrpc/lightning.proto:833-834. The field comment now says "This flag only applies to GetTransactions and is ignored by SubscribeTransactions." The regeneratedlightning.pb.go(2428-2429) carries the same sentence, as do both swaggerreverseparameters (~2706 and ~2919).
Reviewed at medium effort.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
/gateway approve |
saubyk
added
rpc
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(minor) — addressed:RPCTransactionDetailsonly ever gets the page the wallet has already cut:rpcserver.gopasses ittxns, firstIdx, lastIdxfrom `ListTra…F2(minor) — addressed:GetTransactionsRequestis also the request type forSubscribeTransactions(lightning.proto:80). The newreversefield therefore show…
Approved by @saubyk via /gateway approve. Last reviewed at ed47e28. Skill v0.6.0, model claude-opus-5-5.
|
Lint CI is failing |
ed47e28 to
6cf1f37
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (4 files)
🟡 Medium (2 files)
🟢 Low (2 files)
AnalysisThis PR adds a To override, add a |
CI is green now |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Looking good, left a few comments.
Could you check whether the sql-wallet needs to implement this in the tx table? cc @Abdulkbk If so we may need a small PR to enable it, should be small as it's a simple sql query.
| the default ordering is used, which lists unconfirmed transactions | ||
| first, followed by the most recent confirmed transactions down to the | ||
| oldest. Pagination (index_offset/max_transactions) always walks the | ||
| result set starting from the oldest transaction, regardless of this |
There was a problem hiding this comment.
RPC clients can still send start_height > end_height, and the wallet then paginates newest-first before this sort. Can we validate the range at the RPC boundary or qualify the oldest-first ordering docs?
There was a problem hiding this comment.
Good catch, the wallet does read blocks in reverse for start_height > end_height, so the page selection differs for that range even though the response order never showed it. Went with qualifying the docs rather than rejecting the range, to avoid breaking clients that rely on it today: the start_height comment now describes the reversed page selection, the reverse field comment says the same page is selected regardless of the flag (from the oldest, or from the newest when start_height > end_height), and the lnwallet docs in the first commit describe the wallet-layer ordering for both ranges rather than dropping the reverse-read statement.
| // changes the order in which the returned page is presented: by default the | ||
| // most recent transaction of the page comes first, with reverse set the | ||
| // oldest transaction of the page comes first. | ||
| func TestRPCTransactionDetailsPaginationOrdering(t *testing.T) { |
There was a problem hiding this comment.
Can we fold this into TestRPCTransactionDetailsOrdering? It repeats newDetail, the same seven transactions and txHashes, then slices the page by hand, so it does not exercise pagination. Keep the nonzero index checks in the ordering test and let TestTransactionDetailsPage cover page selection.
There was a problem hiding this comment.
Folded into TestRPCTransactionDetailsOrdering, which now asserts the nonzero first/last index pass-through in both modes and leaves page selection to TestTransactionDetailsPage.
| // transactions from the oldest block to the newest, with unconfirmed | ||
| // transactions last. Two transactions share the same block (the two | ||
| // with 3 confirmations) to exercise the stable sort tie-breaking. | ||
| txns := []*lnwallet.TransactionDetail{ |
There was a problem hiding this comment.
The old sort.Slice passes this exact fixture in both directions, so this does not catch the stability regression. Can we use a larger tie-heavy fixture that fails with the unstable sort?
There was a problem hiding this comment.
Right, sort.Slice falls back to insertion sort for <= 12 elements so the old fixture could not catch it. The fixture is now 17 transactions with four-way ties in three blocks plus four unconfirmed, and I confirmed it fails with sort.Slice and passes with sort.SliceStable.
| transactions within the response changes. This flag only applies to | ||
| GetTransactions and is ignored by SubscribeTransactions. | ||
| */ | ||
| bool reverse = 6; |
There was a problem hiding this comment.
Can we split 93ea04be0 into separate commits for the proto source and generated Go/Swagger output?
Optional: 9edce700e and 6cf1f37ec could be squashed since the release note belongs with the CLI flag. Keep the independent stable-sort fix separate.
There was a problem hiding this comment.
Done: the proto source and the regenerated Go/Swagger output are now separate commits, the CLI flag and release note are squashed, and the stable-sort fix remains its own commit.
| // by height, unconfirmed transactions will follow our oldest | ||
| // transactions, because they have lower block heights. | ||
| sort.Slice(txDetails.Transactions, func(i, j int) bool { | ||
| sort.SliceStable(txDetails.Transactions, func(i, j int) bool { |
There was a problem hiding this comment.
Nit: with SliceStable in both directions, ties keep the wallet's order in both modes, so reverse isn't the reverse of the default for same-block or unconfirmed transactions (e.g. cc, dd and ff, 11 come out the same way round in both orderings of TestRPCTransactionDetailsOrdering).
If "reverse" should be an exact mirror, one descending sort plus a reversal for the default would do it:
sort.SliceStable(txDetails.Transactions, func(i, j int) bool {
return txDetails.Transactions[i].NumConfirmations >
txDetails.Transactions[j].NumConfirmations
})
if !reverse {
slices.Reverse(txDetails.Transactions)
}That flips how ties appear in the default output. Their order was never specified before, but it's worth knowing. Otherwise, can we note in the doc comment that ties stay in wallet order in both modes? The tie-heavy fixture yy asked for would pin either choice.
There was a problem hiding this comment.
Kept ties in wallet order in both modes rather than mirroring, since mirroring would change the default output's tie order for existing clients. Documented it in the RPCTransactionDetails doc comment and the proto field comment, and the larger fixture now pins it (a1 precedes a2 in both orderings).
| @@ -0,0 +1,12 @@ | |||
| # Bug Fixes | |||
There was a problem hiding this comment.
Nit: the new RPC field and CLI flag fit better under # New Features → ## RPC Additions / ## lncli Additions from release-notes-template.md. The stable-sort note can stay under # Bug Fixes.
There was a problem hiding this comment.
Moved the RPC field under New Features / RPC Additions and the CLI flag under lncli Additions; the stable sort stays under Bug Fixes.
| // txHash returns a 64 character hex transaction hash with the given two | ||
| // character hex prefix, so transactions are easy to tell apart in test | ||
| // failures. | ||
| var txHash = func(prefix string) string { |
There was a problem hiding this comment.
Nit: can this be a plain func txHash(prefix string) string declaration rather than a package-level var? Also, newDetail sets BlockHash = h, the transaction's own hash, so cc and dd share a height but carry different block hashes. Deriving the block hash from BlockHeight would keep the fixture consistent.
There was a problem hiding this comment.
Both done: txHash is a plain func now, and the fixture derives the block hash from the block height so same-block transactions share it.
6cf1f37 to
7446d87
Compare
|
Addressed the review feedback in 7446d87, see the inline replies for details. On the sql-wallet question: no wallet-side change is needed for this PR. The reverse flag is applied in the RPC layer on the page the wallet already returned, and the WalletController.ListTransactionDetails signature is unchanged (only its docs). @Abdulkbk is there any expectation of a change in this PR for the SQL transaction store work, or is this just something to keep in mind there? |
| /* | ||
| The height from which to list transactions, inclusive. If this value is | ||
| greater than end_height, transactions will be read in reverse. | ||
| greater than end_height, the wallet reads the blocks in reverse, so |
There was a problem hiding this comment.
Can we account for the -1 sentinel here? With start_height=0,end_height=-1, the wallet treats -1 as the tip and paginates oldest-first, even though 0 > -1. An omitted RPC end height uses the same path. Qualify this in both proto comments and the wallet docs, then regenerate the Go/Swagger output.
There was a problem hiding this comment.
Done in a24898a. Confirmed in wtxmgr that any negative height is rewritten to the maximum height before the direction comparison, so start_height=0 with end_height=-1 (or an omitted end_height) reads forward, oldest-first. Both proto height comments and the lnwallet interface doc now state that -1 counts as the chain tip for that comparison, with the 0/-1 example, and the Go/Swagger output is regenerated.
| // which Go's unstable sort.Slice happens to be stable (it falls back to | ||
| // insertion sort for 12 or fewer elements), so that an unstable sort would | ||
| // reorder the ties and fail the ordering assertions. | ||
| func walletOrderedTxns(t *testing.T) []*lnwallet.TransactionDetail { |
There was a problem hiding this comment.
Can we inline this fixture into TestRPCTransactionDetailsOrdering? It has one caller and only wraps a slice literal, so the helper adds a hop without removing duplication. Keep newTxDetail and the ordering/index assertions as they are.
There was a problem hiding this comment.
Inlined the slice literal into TestRPCTransactionDetailsOrdering with its comment; newTxDetail and the ordering/index assertions are unchanged.
No change expected AFAIK. LND still reads through the deprecated |
The doc comments on WalletController.ListTransactionDetails and the btcwallet implementation only described the reverse read for a start height greater than the end height, and said nothing about the order of the returned transactions or how pagination relates to it. Document the ordering the wallet layer actually produces: confirmed transactions from the oldest to the newest block for a regular range, from the newest to the oldest block when start height is greater than end height, and unconfirmed transactions last in both cases. The special end height -1 counts as the chain tip for that comparison, so a start height of 0 with an end height of -1 is a regular, oldest-first range. Note that pagination is applied over that ordering and that any presentation ordering is left to the caller.
RPCTransactionDetails sorted transactions by number of confirmations using sort.Slice, which is not stable. Transactions sharing a number of confirmations (several in the same block, or several unconfirmed ones) could therefore change their relative order between calls. Switch to sort.SliceStable so ties keep the relative order produced by the wallet layer, giving the returned page a deterministic order. Pagination (index_offset/max_transactions) is applied by the wallet layer before this sort, so it was never affected by the instability. Fold the inline ordering rationale into the doc comment and add a test asserting the exact resulting order. The fixture is tie-heavy and larger than 12 elements, below which sort.Slice falls back to an insertion sort that happens to be stable, so the test fails with the previous unstable sort.
Add an explicit reverse flag to GetTransactionsRequest so callers can request wallet transactions oldest-first, with unconfirmed transactions last. The flag only changes the order of the transactions within the response; pagination (index_offset/max_transactions) selects the same page either way. Ties on the number of confirmations keep the order produced by the wallet in both modes, which the field comment states explicitly. SubscribeTransactions shares this request message but does not honour the flag, which the field comment also states. Also correct the start_height docs: a start_height greater than end_height makes the wallet read blocks in reverse, which changes the page that pagination selects, but the response ordering has always been governed by the RPC layer and is now governed by the reverse flag. An end_height of -1 (the default) counts as the chain tip for that comparison, so start_height=0 with end_height=-1 is a regular oldest-first range, which both height comments now state.
Regenerate the Go and Swagger output for the new reverse field and the updated start_height docs.
The GetTransactions RPC has always returned wallet transactions newest-first: RPCTransactionDetails unconditionally re-sorted the result by number of confirmations in ascending order, which destroyed the ordering produced by the wallet layer and left no way to list transactions oldest-first. Add a reverse parameter to RPCTransactionDetails that sorts by number of confirmations in descending order instead, putting the oldest confirmed transactions first and unconfirmed transactions last, and pass the new GetTransactionsRequest.reverse flag through from the rpcserver. ListSweeps keeps the default ordering. The default ordering is unchanged. Ties on the number of confirmations keep the wallet order in both modes, so the reverse ordering is not an exact mirror of the default one for them. This is documented and pinned by the ordering test, which is extended to cover the reverse case and to assert that the page indexes are passed through unmodified in both modes. Fixes lightningnetwork#7316
Expose the new GetTransactionsRequest.reverse field as a --reverse flag on lncli listchaintxns, and document both the default ordering (unconfirmed first, then newest to oldest confirmed) and the reverse ordering (oldest confirmed first, unconfirmed last) in the command description. Add the release note for the new RPC field, the new CLI flag and the stable sort.
7446d87 to
a24898a
Compare
Fixes #7316.
GetTransactions/lncli listchaintxnshas always returned transactions newest-first.RPCTransactionDetailsunconditionally re-sorts the wallet result by confirmations ascending, so the reverse ordering documented forstart_height > end_heightnever reached RPC clients, and since #9558 the CLI rejects that range anyway. There was no way to list transactions oldest-first.Changes
reverseflag toGetTransactionsRequestand--reversetolncli listchaintxns. When set, transactions are returned oldest-first with unconfirmed transactions last. The default ordering is unchanged.SubscribeTransactionsshares the request message and ignores the flag, which the field comment states.RPCTransactionDetailstosort.SliceStable. Same-block and unconfirmed transactions share a confirmation count, so the unstable sort could reorder them between calls. Pagination happens in the wallet layer before this sort, so this only makes the order within a page deterministic.ListTransactionDetailsdoc comments that still described the non-functional reversal.Pagination semantics
index_offsetalways counts from the oldest transaction, in both modes.reverseonly changes the order of the returned page, so the same page andfirst_index/last_indexcome back either way. Reversing before pagination (as sketched in the issue) would make a client paging oldest-first receive the wrong page. If reviewers prefer reverse-before-pagination, that is a small follow-up throughWalletController.ListTransactionDetails.Testing
TestRPCTransactionDetailsOrderingpins the exact order in both modes, including tie-breaking.TestRPCTransactionDetailsPaginationOrderingpins that the same page is returned in both modes.lnrpc,lnwallet,lnwallet/btcwallet,cmd/commandsand thelnwallet/testsuites (bitcoind, btcd, neutrino) pass.