rpc: add gettx command to walletrpc #7654

ErikEk · 2023-05-01T07:56:42Z

Adds a new endpoint 'gettx' to the wallet sub-server.
Fixes #7323.
The btcwallet package has been updated for this endpoint.
This will only fetch transactions from the wallet db and not from a btc node in accordance with the issue description.(https://github.com/lightningnetwork/lnd/blob/04314d3254a9380d51935125869b23ee37240dfe/lnrpc/lightning.proto#L41-L44).
A test is added to lnwallet.

guggero

Thanks for the PR. This will be very useful!

I think this can be simplified quite a bit by just using a filter function. If the reason for the duplicated code is to improve performance by not reading everything from disk an then filtering in memory, I think we should instead implement the filter in btcwallet itself. Such an (optional/nilable) filter function could also be used for other stuff in the future.

cmd/lncli/commands.go

lnrpc/lightning.proto

lnrpc/walletrpc/walletkit_server.go

rpcserver.go

lnwallet/test/test_interface.go

ErikEk · 2023-06-06T03:06:11Z

@guggero Everything you asked for should be implemented now. Unfortunately we do have a dual array struct for "mined" and "unmined" transactions in the btcwallet package, so I did implement the filter in lnd instead. The lnwallet test commit is removed since this now uses a filter instead of fetching specific transactions.

guggero

Looks very good, thanks a lot for the update!
Just a bunch of nits, will run a couple of manual tests when addressed.

cmd/lncli/commands.go

lnrpc/lightning.proto

lnwallet/btcwallet/btcwallet.go

lntest/mock/walletcontroller.go

lnwallet/btcwallet/btcwallet.go

cmd/lncli/walletrpc_active.go

docs/release-notes/release-notes-0.18.0.md

lnwallet/test/test_interface.go

cmd/lncli/walletrpc_active.go

yyforyongyu

Just one nit and needs a rebase, otherwise LGTM🙏

cmd/lncli/walletrpc_active.go

yyforyongyu · 2023-11-14T06:40:21Z

itest/lnd_misc_test.go

+			&walletrpc.GetTransactionRequest{
+				TxHash: targetTx,
+			})
+		require.NotNil(ht, txResp, "target tx %v not found", targetTx)


need to use require.NotNilf to format the string properly.

itest/lnd_misc_test.go

lnwallet/test/test_interface.go

ErikEk · 2023-11-15T13:03:04Z

@yyforyongyu Added require.NotNilf and rebased. Thanks for the review.

yyforyongyu

LGTM🙏

lightninglabs-deploy · 2023-12-01T11:06:15Z

@guggero: review reminder
@ErikEk, remember to re-request review from reviewers when ready

guggero

Very nice and super close, just some last small requests.

guggero · 2023-12-06T13:37:01Z

lnrpc/walletrpc/walletkit.proto

@@ -538,6 +543,11 @@ message ListAddressesResponse {
    repeated AccountWithAddresses account_with_addresses = 1;
 }

+message GetTransactionRequest {
+    // Transaction hash.
+    string tx_hash = 1;


Since technically this is the TXID (which is the by-reversed hash of the transaction), this should either be called txid if it's a string, or be turned into a bytes field which then expects the non-reversed bytes of the transaction hash.

I'd vote for renaming to txid and leaving the string type, since that makes the REST call easier as well, since bytes are a bit harder to handle.

guggero · 2023-12-06T13:39:26Z

lnwallet/btcwallet/btcwallet.go

+
+	txDetail, err := unminedTransactionsToDetail(tx.Summary, b.netParams)
+	if err != nil {
+		return nil, err


nit: we could either add some additional error context here with fmt.Errorf(). If not, then I guess we can directly do return unminedTransactionsToDetail(...).

I will just return it then.

guggero · 2023-12-06T13:40:00Z

itest/lnd_misc_test.go

+		txResp := ainz.RPC.GetTransaction(
+			&walletrpc.GetTransactionRequest{
+				TxHash: targetTx,
+			})


nit: closing parenthesis on new line.

guggero · 2023-12-06T13:41:44Z

lnwallet/test/test_interface.go

+
+	txDetails, err := bob.GetTransactionDetails(&txHash)
+	require.NoError(t, err, "unable to receive transaction details")
+	require.Equal(t, txDetails.Value, amountSats, "expected transaction "+


There are formatting placeholders in the last argument, but Equalf() wasn't used. IMO this can just be require.Equal(t, txDetails.Value, amountSats, "tx value") since the required library will produce a nice error message already.

@guggero thanks for the review. Will finish it soon.

ErikEk · 2023-12-10T21:10:15Z

Code has been updated and rebased. @guggero: Ready for another review.

guggero

Nice, LGTM 🎉

ErikEk force-pushed the listchaintxns-add-txhash branch from 94bbc0a to 020621b Compare May 1, 2023 08:08

ErikEk mentioned this pull request May 1, 2023

[wip]: add get transaction rpc #7378

Closed

ErikEk force-pushed the listchaintxns-add-txhash branch 4 times, most recently from bed1939 to eb972b5 Compare May 4, 2023 08:52

ErikEk marked this pull request as ready for review May 4, 2023 08:53

ErikEk force-pushed the listchaintxns-add-txhash branch 7 times, most recently from c07edec to 8e3f1f1 Compare May 13, 2023 07:59

ErikEk changed the title ~~[WIP] rpc: add txhash parameter to listchaintxns~~ rpc: add txhash parameter to listchaintxns May 18, 2023

ErikEk force-pushed the listchaintxns-add-txhash branch 2 times, most recently from b6a54fe to 09b9a51 Compare May 22, 2023 06:45

guggero self-requested a review May 22, 2023 08:18

guggero requested changes May 22, 2023

View reviewed changes

Roasbeef added enhancement Improvements to existing features / behaviour rpc Related to the RPC interface labels May 23, 2023

ErikEk force-pushed the listchaintxns-add-txhash branch 4 times, most recently from 71d585a to 674e85c Compare June 6, 2023 02:58

ErikEk force-pushed the listchaintxns-add-txhash branch from 674e85c to b87b683 Compare June 6, 2023 03:11

guggero self-requested a review June 6, 2023 07:03

guggero reviewed Jun 6, 2023

View reviewed changes

ErikEk force-pushed the listchaintxns-add-txhash branch from b87b683 to a8c5382 Compare June 6, 2023 15:49

ErikEk force-pushed the listchaintxns-add-txhash branch 2 times, most recently from 213629b to 4ae1e14 Compare October 31, 2023 10:44

ErikEk requested review from guggero and yyforyongyu October 31, 2023 20:22

ErikEk force-pushed the listchaintxns-add-txhash branch 3 times, most recently from 2ffd56a to b6842d3 Compare October 31, 2023 20:33

yyforyongyu requested changes Nov 2, 2023

View reviewed changes

ErikEk mentioned this pull request Nov 3, 2023

[feature]: add get transaction rpc #7323

Closed

ErikEk force-pushed the listchaintxns-add-txhash branch 2 times, most recently from 4265a45 to b21c6e6 Compare November 6, 2023 21:23

ErikEk requested a review from yyforyongyu November 7, 2023 19:15

yyforyongyu reviewed Nov 14, 2023

View reviewed changes

ErikEk force-pushed the listchaintxns-add-txhash branch from b21c6e6 to 21465b5 Compare November 15, 2023 13:01

yyforyongyu approved these changes Nov 17, 2023

View reviewed changes

guggero reviewed Dec 6, 2023

View reviewed changes

ErikEk force-pushed the listchaintxns-add-txhash branch 2 times, most recently from cb48f3d to f9376f6 Compare December 10, 2023 21:08

ErikEk added 3 commits December 10, 2023 22:14

rpc: add gettransaction endpoint to walletrpc sub-server

f0bc6d8

itest: add gettransactiondetails

16f0510

doc: update release notes

c8a7a3d

ErikEk force-pushed the listchaintxns-add-txhash branch from f9376f6 to c8a7a3d Compare December 10, 2023 21:14

guggero approved these changes Dec 11, 2023

View reviewed changes

guggero requested review from Roasbeef and removed request for Roasbeef December 11, 2023 09:06

guggero merged commit 0ec9ac7 into lightningnetwork:master Dec 11, 2023
24 of 25 checks passed

guggero mentioned this pull request Dec 12, 2023

neutrino remove sweeptx #7800

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: add gettx command to walletrpc #7654

rpc: add gettx command to walletrpc #7654

ErikEk commented May 1, 2023 •

edited

guggero left a comment

ErikEk commented Jun 6, 2023

guggero left a comment

yyforyongyu left a comment

yyforyongyu Nov 14, 2023

ErikEk commented Nov 15, 2023

yyforyongyu left a comment

lightninglabs-deploy commented Dec 1, 2023

guggero left a comment

guggero Dec 6, 2023

guggero Dec 6, 2023

ErikEk Dec 10, 2023

guggero Dec 6, 2023

ErikEk Dec 10, 2023

guggero Dec 6, 2023

guggero Dec 6, 2023

ErikEk Dec 9, 2023

ErikEk commented Dec 10, 2023 •

edited

guggero left a comment

rpc: add gettx command to walletrpc #7654

rpc: add gettx command to walletrpc #7654

Conversation

ErikEk commented May 1, 2023 • edited

guggero left a comment

Choose a reason for hiding this comment

ErikEk commented Jun 6, 2023

guggero left a comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikEk commented Nov 15, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Dec 1, 2023

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikEk commented Dec 10, 2023 • edited

guggero left a comment

Choose a reason for hiding this comment

ErikEk commented May 1, 2023 •

edited

ErikEk commented Dec 10, 2023 •

edited