lnwallet+rpc: add label filter to GetTransactions#10806
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces label-based filtering for chain transactions across the RPC, CLI, and internal wallet interfaces. By allowing users to filter transactions by a specific label, it improves the discoverability and management of wallet activity. The changes include updates to the gRPC definition, internal wallet controller logic, and command-line interface, accompanied by comprehensive integration tests to ensure correctness. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an optional "label" filter to the GetTransactions RPC and the listchaintxns CLI command, allowing users to filter transactions by their exact label. The changes span the API definitions, the wallet controller interface, and the btcwallet implementation, supported by new integration tests. Review feedback highlighted a placeholder PR number in the release notes that needs updating and noted significant formatting regressions in the generated gRPC code, suggesting the file be regenerated using the standard project toolchain to reduce diff noise.
github-actions
Bot
added
the
severity-critical
PR Severity: CRITICAL - lnwallet and rpcserver.go changes require expert review. <!-- pr-severity-bot --> |
|
PR Severity: CRITICAL Automated classification | 9 files | 210 lines changed (excluding auto-generated and test files) Critical (5 files):
High (2 files):
Medium (1 file): cmd/commands/commands.go Analysis: This PR modifies lnwallet/* and rpcserver.go, both CRITICAL. The wallet interface change (lnwallet/interface.go) affects channel funding, signing, and commitment transactions. The lnrpc proto addition adds new RPC surface. No severity bump: 9 files (threshold 20) and 210 lines (threshold 500). To override: add a severity-override-{critical,high,medium,low} label. |
saraogiraj94
force-pushed
the
label-filter-get-transactions
branch
from
639f5a4 to
34c70da
Compare
|
Looks good. Nice use of the in place filter! |
|
|
||
| // If a label filter is specified, only include transactions whose label | ||
| // matches exactly. | ||
| if labelFilter != "" { |
There was a problem hiding this comment.
This look solid!
However, there might be a little bug here with how pagination interacts with this filter. Since the code above this block populates txDetails from the database using the raw maxTransactions limit, the result set is already truncated before it hits this loop.
If a node has thousands of transactions, but the client queries with a default maxTransactions window (like 20 or 50) along with a specific label, the filter will only check that initial batch. If the matching transaction sits further down in the history, it'll be missed entirely. The label filter likely needs to be applied to the raw results before the pagination slices and offsets are handled.
There was a problem hiding this comment.
@TechLateef Thanks for the review! I want to clarify the flow here — the label filter is actually applied to the full result set before any pagination kicks in.
The DB call on line 1496:
txns, err := b.wallet.GetTransactions(start, stop, accountFilter, nil)
comes from the btcwallet library with this signature:
func (w *Wallet) GetTransactions(startBlock, endBlock *BlockIdentifier, accountName string, cancel <-chan struct{}) (*GetTransactionsResult, error)
The last argument is a cancel channel, not a limit — there is no maxTransactions parameter. The function returns every transaction in the block range unconditionally.
So the order of operations is:
Fetch all transactions in [startHeight, endHeight] from the DB (no limit)
Convert to txDetails slice (still the full set)
Label filter applied here (line 1528) — filters on the complete set
Pagination (indexOffset + maxTransactions) applied after — slices the already-filtered set
A client querying with maxTransactions=20 and label="pay-bob" will have all transactions in the block range filtered first, then the first 20 matching results returned. No labelled transaction can be silently missed.
There was a problem hiding this comment.
Oh, got it! That makes sense. I didn't realize GetTransactions was returning the entire block range unconditionally without a database-level limit.
Since the label filter is applied to the full dataset before the pagination slice occurs, there's no risk of missing matching records. Thanks for clarifying the order of operations!
| require.NoError(t, err) | ||
| require.Len(t, txDetails, 0) | ||
| } | ||
|
|
There was a problem hiding this comment.
The test cases cover the exact matching nicely.
One minor thing: all the test queries here pass 0 for maxTransactions, which fetches the whole history at once and disables pagination boundaries. Because of that, it doesn't catch the edge case where a client passes both a label filter and a tight max_transactions limit at the same time.
It would be great to add one more test assertion in here that uses a small maxTransactions limit to verify that pagination doesn't accidentally chop off matching records from the results.
There was a problem hiding this comment.
@TechLateef
Good catch! Added two new assertions that combine a label filter with a non-zero maxTransactions and indexOffset to cover this path:
labelA + maxTransactions=1 + offset=0 → returns exactly the first matching tx
labelA + maxTransactions=1 + offset=1 → returns exactly the second matching tx
This exercises the pagination code path on the already-filtered set, which the previous tests bypassed by passing maxTransactions=
|
@saraogiraj94, remember to re-request review from reviewers when ready |
Adds an optional `label` string field to `GetTransactionsRequest` (proto field 6) so callers can filter chain transactions by label. - `WalletController.ListTransactionDetails` gains a `labelFilter string` parameter; when non-empty, only transactions whose `Label` field exactly matches are returned (client-side filter after the full fetch). - `rpcserver.GetTransactions` threads `req.Label` through to the wallet call. - `lncli listchaintxns` gains a `--label` flag. - All mocks and the integration-test harness are updated to pass the new parameter. - New integration test `testListTransactionDetailsLabelFilter` covers: exact match by label, no-filter returns all, and unknown label returns empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All existing label filter test calls passed maxTransactions=0, which bypasses the pagination code path entirely. Add two assertions that combine a label filter with a non-zero maxTransactions and indexOffset to verify pagination operates on the already-filtered result set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
saraogiraj94
force-pushed
the
label-filter-get-transactions
branch
from
1e0f10c to
bb9436e
Compare
|
@lightninglabs-deploy the changes requested are pushed |
| @@ -1481,7 +1481,7 @@ func unminedTransactionsToDetail( | |||
| // | |||
| // This is a part of the WalletController interface. | |||
There was a problem hiding this comment.
I feel like the function documentation will require an update to reflect the new labelFilter parameter since the function signature changed.
|
With the new tests added, this is looking great on my end! Once that function doc update is in place, it's ready to go. Great work! |
4 participants
Closes #10785
Summary
labelfield (proto field 6) toGetTransactionsRequestso callers can filter chain transactions by their labelWalletController.ListTransactionDetailsgains alabelFilter stringparameter; when non-empty, only transactions whoseLabelfield exactly matches are returnedlncli listchaintxnsgains a--labelflagTesting
testListTransactionDetailsLabelFiltercovers: exact match by label A, exact match by label B, no filter returns all, unknown label returns emptyRelease Notes
Updates
docs/release-notes/release-notes-0.22.0.mdunder RPC Additions and lncli Additions.🤖 Generated with Claude Code