rpcclient: Add getblockfilter JSON-RPC client command #1579

lindlof · 2020-05-16T10:14:46Z

Adds getblockfilter call to JSON-RPC client.

onyb · 2020-05-23T17:38:24Z

btcjson/chainsvrcmds.go

+// NewGetBlockFilterCmd returns a new instance which can be used to issue a
+// getblockfilter JSON-RPC command.


We generally add the following comment whenever there are optional arguments.

Suggested change

// NewGetBlockFilterCmd returns a new instance which can be used to issue a

// getblockfilter JSON-RPC command.

// NewGetBlockFilterCmd returns a new instance which can be used to issue a

// getblockfilter JSON-RPC command.

//

// The parameters which are pointers indicate they are optional. Passing nil

// for optional parameters will use the default value.

Right on. Added these.

onyb · 2020-05-23T17:43:45Z

btcjson/chainsvrresults.go

+	Filter string `json:"filter"`
+	Header string `json:"header"`


Nit: Whenever possible, adding extra comments always helps, since it shows up beautifully on pkg.go.dev.

Suggested change

Filter string `json:"filter"`

Header string `json:"header"`

Filter string `json:"filter"` // the hex-encoded filter data

Header string `json:"header"` // the hex-encoded filter header

Good shout makes the result easier to consume. Didn't consider pkg.go.dev

Also added comments to GetBlockFilterCmd fields though they're not as visible through rpcclient.

onyb

Looks good. 👍 Just added some minor remarks about comments.

lindlof

I appended the changes to the original commit. Now I realised perhaps it would be easier for reviewers if I would have introduced fixes in a new commit and in the end you'd squash merge.

torkelrogstad · 2020-05-25T12:50:33Z

btcjson/chainsvrcmds.go

@@ -163,6 +163,24 @@ func NewGetBlockCountCmd() *GetBlockCountCmd {
 	return &GetBlockCountCmd{}
 }

+// GetBlockFilterCmd defines the getblockfilter JSON-RPC command.
+type GetBlockFilterCmd struct {
+	BlockHash  string  // The hash of the block


I'd suggest using the chainhash.Hash type here.

These btcjson.*Cmd structs are generally using basic types assigned through NewCmd constructors. btcjson.assignField function makes a conversion from command-line input to the basic type that NewCmd takes.

While it's possible to implement JSON Unmarshaler to use a more specific type as you suggest I think the idea is to keep these types as stupid as possible.

In the JSON-RPC request the type will be string that the server has to unmarshal. Just taking the string from command-line and converting it to a JSON-RPC request without any semantic validation offloads the validation to server. Doing validation client-side would result in double validation which would be more fragile and more complex to consume.

As I see it GetBlockFilterCmd defines the JSON-RPC command which shouldn't be aware of non-JSON types.

rpcclient.GetBlockFilter now takes chainhash.Hash. rpcclient is where to take the Golang call and transform it to JSON.

torkelrogstad · 2020-05-25T12:51:21Z

btcjson/chainsvrcmds.go

+// GetBlockFilterCmd defines the getblockfilter JSON-RPC command.
+type GetBlockFilterCmd struct {
+	BlockHash  string  // The hash of the block
+	FilterType *string // The type name of the filter, default=basic


Looking at the Bitcoin Core codebase, "basic" is the only known filter type there. Are there any other known types out in the wild? Might make sense to create a custom type for this instead of passing in a string.

Haven't seen others than types basic. Not sure how standard the filter names are but first see
my previous response about doing validation only server-side.

I can see that for instance in btcjson.AddNodeCmd there is a custom type AddNodeSubCmd being used but in that case the options are in the spec.

In case of getblockfilter the spec only mentions basic which is the default anyway.

BIP 157 describes that initial filter types are defined in BIP 158 which defines only the Basic filter.

Bitcoin Core CLI seems to allow describing any getblockfilter in a CLI command and will send the request server which will reject any unknown filter type name. I'd have the similar behavior in btcctl and just handle this as a string.

In rpcclient the second parameter of rpcclient.GetBlockFilter could have a more specific type, for instance we can call it FilterTypeName. For the server-side implementation of getblockfilter to resolve the filter for the name we can map the basic FilterTypeName to wire.GCSFilterRegular.

I'll look at how to best implement FilterTypeName. It seems a good idea for rpcclient.

Good points about validation, as well as that in this specific scenario it might not be possible to construct a good type.

Just so you understand my reasoning: custom types are (at least for me) very beneficial when coding with btcd packages as a library, because they limit the scope of possibilities and place you on a narrower path of "valid actions". Does this make sense?

Agreed definitely should have types for the rpcclient API. Sorry about the fuss - with the way you suggested the CLI mechanism works just as well when defining FilterTypeName as a type of string (facepalm)

Please see the new commit 9d7bd87

rpcclient/chain.go

torkelrogstad

LGTM 🥇

torkelrogstad · 2020-05-27T10:07:09Z

btcjson/helpers.go

+func NewFilterTypeName(v FilterTypeName) *FilterTypeName {
+	p := new(FilterTypeName)
+	*p = v
+	return p
+}


Was about to comment that it might make sense to make the const FilterTypeBasic a var so it would be possible to do &FilterTypeBasic, but seems like you already anticipated this and added a nice solution:-)

Wasn't super happy about adding helper for this but it allows using const instead of var and is pretty consistent with how rpcclient API is used

Add type for second getblockfilter param

lindlof · 2020-05-27T22:04:46Z

@onyb @torkelrogstad Squashed ready to merge

Rjected

The rpcclient implementation looks good, but since our existing getcfilter does the exact same thing, I think we can just alias getblockfilter to the existing code for GetCFilterCmd, and skip rewriting filter types.

lindlof · 2020-07-15T20:38:37Z

The rpcclient implementation looks good, but since our existing getcfilter does the exact same thing, I think we can just alias getblockfilter to the existing code for GetCFilterCmd, and skip rewriting filter types.

@Rjected GetCFilterCmd takes byte filtertype and GetBlockFilterCmd takes string filtertype. Not sure if there is any sensible way to reuse GetCFilterCmd. The response type is different too. Do miss something?

Rjected

@lindlof sorry for the late response. Yeah I get what you're saying now, unless we want to deviate from core we need these new types. If we ever decide to support this on the server side, we could potentially reuse implementation but yeah I don't see how the client implementation could really benefit from the type reuse.

jcvernaleo

OK

lindlof changed the title ~~Add getblockfilter JSON-RPC client command~~ rpcclient: Add getblockfilter JSON-RPC client command May 16, 2020

onyb reviewed May 23, 2020

View reviewed changes

onyb requested changes May 23, 2020

View reviewed changes

lindlof force-pushed the getblockfilter branch from 5e82cef to 2f69de6 Compare May 24, 2020 18:02

lindlof commented May 24, 2020

View reviewed changes

onyb approved these changes May 25, 2020

View reviewed changes

torkelrogstad reviewed May 25, 2020

View reviewed changes

lindlof commented May 25, 2020

View reviewed changes

rpcclient/chain.go Outdated Show resolved Hide resolved

lindlof force-pushed the getblockfilter branch from 6626f7a to 9d7bd87 Compare May 26, 2020 23:34

torkelrogstad approved these changes May 27, 2020

View reviewed changes

Add getblockfilter JSON-RPC client command

257af47

Add type for second getblockfilter param

lindlof force-pushed the getblockfilter branch from 9d7bd87 to 257af47 Compare May 27, 2020 22:00

Rjected suggested changes Jul 6, 2020

View reviewed changes

Rjected added enhancement rpc labels Jul 17, 2020

Rjected approved these changes Aug 12, 2020

View reviewed changes

jcvernaleo approved these changes Aug 31, 2020

View reviewed changes

jcvernaleo merged commit b68c50e into btcsuite:master Aug 31, 2020

onyb mentioned this pull request Apr 13, 2021

Release planning for 0.22.0-beta #1706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpcclient: Add getblockfilter JSON-RPC client command #1579

rpcclient: Add getblockfilter JSON-RPC client command #1579

lindlof commented May 16, 2020 •

edited

Loading

onyb May 23, 2020

lindlof May 24, 2020

onyb May 23, 2020

lindlof May 24, 2020

onyb left a comment

lindlof left a comment

torkelrogstad May 25, 2020

lindlof May 25, 2020 •

edited

Loading

lindlof May 25, 2020

torkelrogstad May 25, 2020

lindlof May 25, 2020

lindlof May 25, 2020

lindlof May 26, 2020

torkelrogstad May 26, 2020

lindlof May 26, 2020 •

edited

Loading

torkelrogstad left a comment

torkelrogstad May 27, 2020

lindlof May 27, 2020

lindlof commented May 27, 2020

Rjected left a comment

lindlof commented Jul 15, 2020

Rjected left a comment

jcvernaleo left a comment

		// NewGetBlockFilterCmd returns a new instance which can be used to issue a
		// getblockfilter JSON-RPC command.

rpcclient: Add getblockfilter JSON-RPC client command #1579

rpcclient: Add getblockfilter JSON-RPC client command #1579

Conversation

lindlof commented May 16, 2020 • edited Loading

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

onyb left a comment

Choose a reason for hiding this comment

lindlof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindlof May 25, 2020 • edited Loading

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

lindlof May 26, 2020 • edited Loading

Choose a reason for hiding this comment

torkelrogstad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindlof commented May 27, 2020

Rjected left a comment

Choose a reason for hiding this comment

lindlof commented Jul 15, 2020

Rjected left a comment

Choose a reason for hiding this comment

jcvernaleo left a comment

Choose a reason for hiding this comment

lindlof commented May 16, 2020 •

edited

Loading

lindlof May 25, 2020 •

edited

Loading

lindlof May 26, 2020 •

edited

Loading