Add `gettxspendingprevout` for `btcd` and fix version check #2125

yyforyongyu · 2024-02-23T14:36:48Z

This PR adds the RPC gettxspendingprevout for btcd so we can do mempool lookups. This RPC is also ported to the rpcclient to replace the raw request made on the btcwallet side.

In addition, we now start to track btcd versions in rpcclient to make sure RPC updates are handled properly.

coveralls · 2024-02-23T14:39:39Z

Pull Request Test Coverage Report for Build 8077551462

Details

-76 of 169 (55.03%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.03%) to 56.753%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rpcclient/infrastructure.go	0	9	0.0%
rpcclient/backend_version.go	43	65	66.15%
rpcclient/rawtransactions.go	0	45	0.0%

Files with Coverage Reduction	New Missed Lines	%
rpcclient/infrastructure.go	1	39.21%

Totals
Change from base Build 8057525961:	0.03%
Covered Lines:	29284
Relevant Lines:	51599

💛 - Coveralls

Roasbeef

LGTM 🌂

rpcserver.go

rpcserver_test.go

Roasbeef · 2024-02-27T00:23:35Z

rpcclient/infrastructure.go

@@ -1648,6 +1658,9 @@ func parseBitcoindVersion(version string) BackendVersion {
 	case version < bitcoind22Str:
 		return BitcoindPre22

+	case version < bitcoind24Str:
+		return BitcoindPre24


IIUC, we need to start to check this in btcwallet? FWIW, didn't see it in the latest PR of yours in that repo.

I think we'll need to check this in lnd at the callsite. If we check it in btcwallet, then it's another error to be returned to the caller. On the other hand, I think the rpcclient package should be put in btcwallet instead to keep btcd "pure" - came up with a draft PR for this when I was dealing with updating errors returned from TestMempoolAccept, will put more details there.

I think the rpcclient package should be put in btcwallet instead to keep btcd "pure" - came up with a draft PR for this when I was dealing with updating errors returned from TestMempoolAccept, will put more details there.

Don't think I agree with this, rpcclient as is does generic JSON-RPC for conformant Bitcoin nodes. We even use the client to test directly against (in itests) the server-side implementation of the various RPC calls.

I don't see why the check can't just be in the wallet though. We already have that check for bitcoind when we wan to use the newer set of RPC calls.

integration/chain_test.go

rpcclient/chain.go

This commit adds the RPC method `gettxspendingprevout` for btcd.

We need this for `gettxspendingprevout`.

This commit adds a new interface, `BackendVersion`, to support checking versions for multiple both btcd and bitcoind. Once neutrino version is also pinned here, it should also be checked.

Roasbeef

LGTM 🎒

saubyk · 2024-02-29T15:22:34Z

cc: @Crypt-iQ for review

saubyk · 2024-02-29T15:22:41Z

cc: @guggero

Chinwendu20 · 2024-03-01T06:14:06Z

Nice pr @yyforyongyu I like the progression.

question/suggestion: Do you think we should separate the implementations of the Backend version interface into separate files? like bitcoin.go and btcd.go in the same folder. In the future if we want to track more backends we just create a new file and that way it is easier to follow?

guggero

Very nice, LGTM 🎉

guggero · 2024-03-01T14:35:30Z

rpcserver.go

@@ -3906,6 +3907,49 @@ func handleTestMempoolAccept(s *rpcServer, cmd interface{},
 	return results, nil
 }

+// handleGetTxSpendingPrevOut implements the gettxspendingprevout command.
+func handleGetTxSpendingPrevOut(s *rpcServer, cmd interface{},
+	closeChan <-chan struct{}) (interface{}, error) {


nit: since closeChan is unused, should we just use _ as the argument name?

guggero · 2024-03-01T14:35:59Z

rpcserver_test.go

+
+	// Call the method handler and assert the error is returned.
+	closeChan := make(chan struct{})
+	results, err := handleGetTxSpendingPrevOut(s, cmd, closeChan)


nit: could just pass in nil since closeChan is not used anyway.

guggero · 2024-03-01T14:40:52Z

rpcclient/backend_version.go

-type BackendVersion uint8
+// BackendVersion defines an interface to handle the version of the backend
+// used by the client.
+type BackendVersion interface {


guggero · 2024-03-01T14:41:58Z

rpcclient/infrastructure.go

-		c.backendVersion = &version
-		return *c.backendVersion, nil
+		version := parseBtcdVersion(info.Version)
+		c.backendVersion = version


nit: why not c.backendVersion = parseBtcdVersion(info.Version) directly?

Roasbeef · 2024-03-01T21:04:15Z

Do you think we should separate the implementations of the Backend version interface into separate files? like bitcoin.go and btcd.go in the same folder. In the future if we want to track more backends we just create a new file and that way it is easier to follow?

@Chinwendu20, I think if we did that, we'd want to add a version prefix/suffix to the file names, so it's clear that this is code for parsing the versions, rather than a bitcoind specific implementation of something.

yyforyongyu mentioned this pull request Feb 23, 2024

[3/5]sweep: make pending inputs stateful lightningnetwork/lnd#8423

Merged

Roasbeef approved these changes Feb 27, 2024

View reviewed changes

yyforyongyu added 7 commits February 27, 2024 22:58

btcjson: add new command GetTxSpendingPrevOutCmd

dd31767

rpcserver+mempool: implement gettxspendingprevout for btcd

72bbdd5

This commit adds the RPC method `gettxspendingprevout` for btcd.

rpcclient: track bitcoind version 24.0.0

5f71df1

We need this for `gettxspendingprevout`.

rpcclient+integration: add new method GetTxSpendingPrevOut

a033b0d

rpcclient: handle backend versioning in one file

0d52a89

rpcclient: start tracking btcd version

9e6070a

multi: turn BackendVersion into an interface

4cf49bd

This commit adds a new interface, `BackendVersion`, to support checking versions for multiple both btcd and bitcoind. Once neutrino version is also pinned here, it should also be checked.

yyforyongyu force-pushed the add-gettxspendingprevout branch from 2cfa802 to 4cf49bd Compare February 27, 2024 15:05

rpcclient: replace ErrBitcoindVersion with ErrBackendVersion

34721f0

Roasbeef approved these changes Feb 27, 2024

View reviewed changes

yyforyongyu mentioned this pull request Feb 28, 2024

lnwallet: skip testmempoolaccept when the backend version is too low lightningnetwork/lnd#8505

Merged

btcd: increase version number to 0.24.1

0d4ed16

guggero requested review from guggero and Crypt-iQ February 29, 2024 15:26

guggero approved these changes Mar 1, 2024

View reviewed changes

Roasbeef merged commit 1a2b599 into btcsuite:master Mar 1, 2024
3 checks passed

yyforyongyu deleted the add-gettxspendingprevout branch March 2, 2024 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `gettxspendingprevout` for `btcd` and fix version check #2125

Add `gettxspendingprevout` for `btcd` and fix version check #2125

yyforyongyu commented Feb 23, 2024

coveralls commented Feb 23, 2024 •

edited

Loading

Roasbeef left a comment

Roasbeef Feb 27, 2024

yyforyongyu Feb 27, 2024

Roasbeef Feb 27, 2024

Roasbeef left a comment

saubyk commented Feb 29, 2024

saubyk commented Feb 29, 2024

Chinwendu20 commented Mar 1, 2024 •

edited

Loading

guggero left a comment

guggero Mar 1, 2024

guggero Mar 1, 2024

guggero Mar 1, 2024

guggero Mar 1, 2024

Roasbeef commented Mar 1, 2024

Add gettxspendingprevout for btcd and fix version check #2125

Add gettxspendingprevout for btcd and fix version check #2125

Conversation

yyforyongyu commented Feb 23, 2024

coveralls commented Feb 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8077551462

Details

💛 - Coveralls

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Feb 27, 2024

Choose a reason for hiding this comment

yyforyongyu Feb 27, 2024

Choose a reason for hiding this comment

Roasbeef Feb 27, 2024

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

saubyk commented Feb 29, 2024

saubyk commented Feb 29, 2024

Chinwendu20 commented Mar 1, 2024 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

guggero Mar 1, 2024

Choose a reason for hiding this comment

guggero Mar 1, 2024

Choose a reason for hiding this comment

guggero Mar 1, 2024

Choose a reason for hiding this comment

guggero Mar 1, 2024

Choose a reason for hiding this comment

Roasbeef commented Mar 1, 2024

Add `gettxspendingprevout` for `btcd` and fix version check #2125

Add `gettxspendingprevout` for `btcd` and fix version check #2125

coveralls commented Feb 23, 2024 •

edited

Loading

Chinwendu20 commented Mar 1, 2024 •

edited

Loading