Add Batch JSON-RPC support (rpc client & server) #1583

jakesylvestre · 2020-05-22T14:30:24Z

This pr aims to address #329 which has been open for a while now by adding bulk request support. This is based on from decred#841 by @davecgh

Note: This PR was previously accidentally merged in #1503 and reverted in #1549 (followed by a rebase to clean up the commit history) and as a result, there is considerable discussion at #1503

In regards to the multiple commits: once we have consensus on the implementation I'll rebase into a single commit

jakesylvestre · 2020-08-29T15:30:38Z

Rebased!

jakesylvestre · 2020-08-29T15:34:37Z

Also - adding some changes from #1503. There were like 30 commits before so I wanted to squash, but I'll rebase again when we want to merge

jakesylvestre · 2020-08-29T16:09:39Z

@everyone sorry about the fractal nature, trying to address everyone's reviews on the old pr.

@torkelrogstad curious why the custom type here would be better than a string. This allows us to use new versions without modifying the code:

Happy to implement this if there's a reason to. If we did, I'd like a validator that we can call to make sure the string conforms to the type. Something like

(rpcVersion) isValid(){
  // check if one of valid versions
}

The implementation would look like this:

The reason I didn't implement it this way is method is not a type right now

jakesylvestre · 2020-08-29T16:10:44Z

also @jcvernaleo @onyb @torkelrogstad @Roasbeef I just moved all the reviewers over from #1503 feel free to remove/add yourself (I can't add you if you're not a participant in the issue)

jakesylvestre · 2020-08-29T16:44:19Z

@torkelrogstad implemented your change in 4a97be3, I can revert if we want to. I still think if this is done method should be a custom type as well. #1596 does this with ChangeType so makes sense to be consistent

onyb

I think I have carefully reviewed most of the PR but may come back at it again later.

Can you please add some batch JSON-RPC tests to integration/rpcserver_test.go?

.gitignore

config.go

goclean.sh

onyb · 2020-09-28T18:59:26Z

btcjson/jsonrpc.go

+type RpcVersion string
+
+const (
+	// version 1 of rpc
+	RpcVersion1 RpcVersion = RpcVersion("1.0")
+	// version 2 of rpc
+	RpcVersion2 RpcVersion = RpcVersion("2.0")
+)
+
+var validRpcVersions = []RpcVersion{RpcVersion1, RpcVersion2}
+
+// check if the rpc version is a valid version
+func (r RpcVersion) IsValid() bool {
+	for _, version := range validRpcVersions {
+		if version == r {
+			return true
+		}
+	}
+	return false
+}
+
+// cast rpc version to a string
+func (r RpcVersion) String() string {
+	return string(r)
+}


I think this whole block is a bit superfluous. The most useful part of this is perhaps the IsValid() method, and currently it is used like this:

version = RpcVersion(maybeEvilJsonRpcVersion) if !version.IsValid()

First of all, using types for RpcVersion doesn't give us much type-safety. So instead of this:

btcjson.MarshalCmd(btcjson.RpcVersion1, ...)

we can also do this, and the compiler will be fine with it:

btcjson.MarshalCmd("3.0", ...)

Instead, why not have a simple function that checks the string version like this:

func IsRpcVersionValid(version string) bool { // do the check }

Also golint recommends RPCVersion as opposed to RpcVersion.

@onyb This was done to address @torkelrogstad's comment here #1503 (comment). I'm happy to change it back, but it seemed like typed constants were preferred. I added some brief comments on this here: #1583 (comment) and here: #1583 (comment)

I've added the golint reccomendation

It's not so important. Let's keep it this way - at least it improves readability.

btcjson/jsonrpc.go

rpcclient/infrastructure.go

rpcserver.go

jcvernaleo · 2020-11-03T15:03:55Z

Looking pretty good. Needs rebase, comments from @onyb addressed, and then squash.

onyb · 2020-11-21T04:29:20Z

rpcclient/chain.go

+	if err != nil {
+		return nil, err
+	}
+	var arr []IndividualBulkResult


res may have a different structure. Example:

{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}

wouldn't this be: [{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}]?

I don't remember how I encountered it, but it was during some tests. I can try to reproduce it again.

Wondering why these changes are required to begin with? Maybe you forgot to remove these changes?

rpcclient/chain.go

coveralls · 2020-12-01T16:54:20Z

Pull Request Test Coverage Report for Build 494215604

86 of 725 (11.86%) changed or added relevant lines in 8 files are covered.
20 unchanged lines in 4 files lost coverage.
Overall coverage increased (+1.1%) to 52.799%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rpcclient/rawrequest.go	0	1	0.0%
integration/rpctest/rpc_harness.go	28	31	90.32%
btcjson/jsonrpc.go	49	66	74.24%
rpcclient/infrastructure.go	0	115	0.0%
rpcserver.go	0	192	0.0%
rpcwebsocket.go	0	311	0.0%

Files with Coverage Reduction	New Missed Lines	%
rpcclient/infrastructure.go	2	0%
btcec/signature.go	3	92.48%
rpcwebsocket.go	4	0.16%
peer/peer.go	11	75.47%

Totals
Change from base Build 407350020:	1.1%
Covered Lines:	20881
Relevant Lines:	39548

💛 - Coveralls

jakesylvestre

Needs rebase.

Also just a misc comment:

It's pretty hard to tell what our actual coverage looks like with the current way the rpc harness is setup (spawning a new process). It's even harder to use a debugger - it'd be nice to figure out a nicer way to do that

jakesylvestre · 2020-12-01T19:45:21Z

rpcclient/chain.go

+	if err != nil {
+		return nil, err
+	}
+	var arr []IndividualBulkResult


wouldn't this be: [{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}]?

jakesylvestre · 2020-12-01T21:19:30Z

Alright, so I think I've addressed all the comments here other than this ongoing debate. Huge thanks to @onyb for the comprehensive review and let me know what else here needs cleanup

integration/rpcserver_test.go

integration/rpctest/rpc_harness.go

btcjson/jsonrpc.go

onyb · 2020-12-15T00:47:00Z

rpcwebsocket.go

 		}

-		// The JSON-RPC 1.0 spec defines that notifications must have their "id"


It seems a lot of code on the left-side has been removed. Can you please explain this part?

A lot of the validations been moved further down the tree since it only applied to 1.0 rpc

onyb · 2020-12-15T00:48:07Z

btcjson/jsonrpc.go

+type RpcVersion string
+
+const (
+	// version 1 of rpc
+	RpcVersion1 RpcVersion = RpcVersion("1.0")
+	// version 2 of rpc
+	RpcVersion2 RpcVersion = RpcVersion("2.0")
+)
+
+var validRpcVersions = []RpcVersion{RpcVersion1, RpcVersion2}
+
+// check if the rpc version is a valid version
+func (r RpcVersion) IsValid() bool {
+	for _, version := range validRpcVersions {
+		if version == r {
+			return true
+		}
+	}
+	return false
+}
+
+// cast rpc version to a string
+func (r RpcVersion) String() string {
+	return string(r)
+}


It's not so important. Let's keep it this way - at least it improves readability.

onyb

Adding my last set of review comments. They are mostly minor ones. Looking really good so far, and I can see the light at the end of the tunnel.

While you address the remaining comments, I'll try to take some time and do some QA.

jakesylvestre · 2020-12-15T08:46:18Z

should be all set with those changes. Let me know if you need a little more clarity here - #1583 (comment). I was a little unclear on the block you were referring too

rpcclient/infrastructure.go

integration/rpcserver_test.go

onyb · 2020-12-15T16:55:35Z

rpcclient/chain.go

+	if err != nil {
+		return nil, err
+	}
+	var arr []IndividualBulkResult


Wondering why these changes are required to begin with? Maybe you forgot to remove these changes?

rpcclient/infrastructure.go

rpcserver.go

jakesylvestre · 2021-02-04T09:53:35Z

btw - did a little load testing on this. Works great

jakesylvestre · 2021-02-09T00:56:31Z

Hey, what do we need to get this in?

onyb

I'm unable to resolve conversations on this PR, but it seems all review comments have been addressed/answered. I did some quick tests on my side, and it works great.

Congrats on making it to the end! 🚀

jcvernaleo

OK

This is awesome. Nice work.

And good job from all the reviewers too!

This was referenced May 22, 2020

Add Batch JSON-RPC support (rpc client & server) #1503

Merged

Add Batch JSON-RPC support (rpc client & server) #1582

Closed

Revert "Add Batch JSON-RPC support (rpc client & server)" #1549

Merged

jakesylvestre added enhancement low priority rpc outdated labels May 22, 2020

jakesylvestre removed the outdated label Aug 29, 2020

jakesylvestre force-pushed the rpcbulk branch from ae766bb to bd8ed49 Compare August 29, 2020 15:30

jakesylvestre requested a review from jcvernaleo August 29, 2020 15:30

jakesylvestre requested a review from Roasbeef August 29, 2020 16:11

onyb added this to the 0.22.0 milestone Sep 7, 2020

onyb suggested changes Sep 28, 2020

View reviewed changes

onyb reviewed Nov 21, 2020

View reviewed changes

rpcclient/chain.go Outdated Show resolved Hide resolved

jakesylvestre force-pushed the rpcbulk branch from e2686fa to 9ca4141 Compare December 1, 2020 17:04

jakesylvestre commented Dec 1, 2020

View reviewed changes

jakesylvestre removed the low priority label Dec 1, 2020

jakesylvestre mentioned this pull request Dec 1, 2020

[rpc] Add support for JSON-RPC 2.0 batch requests #329

Closed

jakesylvestre linked an issue Dec 1, 2020 that may be closed by this pull request

[rpc] Add support for JSON-RPC 2.0 batch requests #329

Closed

jakesylvestre closed this Dec 6, 2020

jakesylvestre force-pushed the rpcbulk branch from e1a96df to 0886f1e Compare December 6, 2020 16:40

jakesylvestre reopened this Dec 6, 2020

jakesylvestre force-pushed the rpcbulk branch from 8e5d021 to e1a96df Compare December 6, 2020 16:42

jakesylvestre assigned jakesylvestre and unassigned jakesylvestre Dec 6, 2020

ashwin111 approved these changes Dec 6, 2020

View reviewed changes

jalavosus approved these changes Dec 7, 2020

View reviewed changes

onyb reviewed Dec 15, 2020

View reviewed changes

onyb suggested changes Dec 15, 2020

View reviewed changes

jakesylvestre force-pushed the rpcbulk branch from 66ec19f to 8748484 Compare December 15, 2020 08:37

onyb reviewed Dec 15, 2020

View reviewed changes

rpcclient/infrastructure.go Show resolved Hide resolved

onyb suggested changes Dec 15, 2020

View reviewed changes

jakesylvestre force-pushed the rpcbulk branch from 8748484 to fa51543 Compare January 18, 2021 17:40

Add Batch JSON-RPC support (rpc client & server)

8c63cc3

jakesylvestre force-pushed the rpcbulk branch from fa51543 to 8c63cc3 Compare January 18, 2021 17:44

jakesylvestre mentioned this pull request Feb 4, 2021

Rpcbulk xplorfin/btcd#14

Merged

onyb approved these changes Feb 9, 2021

View reviewed changes

jcvernaleo approved these changes Feb 9, 2021

View reviewed changes

jcvernaleo merged commit 2a1aa51 into btcsuite:master Feb 9, 2021

Roasbeef mentioned this pull request Feb 17, 2021

chainntnfs: leverage batched JSON-RPC requests to speed up spend/conf rescans lightningnetwork/lnd#5041

Open

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

Add Batch JSON-RPC support (rpc client & server) #1583

Add Batch JSON-RPC support (rpc client & server) #1583

jakesylvestre commented May 22, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020

jakesylvestre commented Aug 29, 2020

jakesylvestre commented Aug 29, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020 •

edited

Loading

onyb left a comment

onyb Sep 28, 2020

onyb Sep 28, 2020

jakesylvestre Dec 1, 2020 •

edited

Loading

onyb Dec 15, 2020

jcvernaleo commented Nov 3, 2020

onyb Nov 21, 2020

jakesylvestre Dec 1, 2020

onyb Dec 15, 2020

onyb Dec 15, 2020

coveralls commented Dec 1, 2020 •

edited

Loading

jakesylvestre left a comment •

edited

Loading

jakesylvestre Dec 1, 2020

jakesylvestre commented Dec 1, 2020 •

edited

Loading

onyb Dec 15, 2020

jakesylvestre Dec 15, 2020

onyb Dec 15, 2020

onyb left a comment

jakesylvestre commented Dec 15, 2020

onyb Dec 15, 2020

jakesylvestre commented Feb 4, 2021 •

edited

Loading

jakesylvestre commented Feb 9, 2021

onyb left a comment

jcvernaleo left a comment

		}

		// The JSON-RPC 1.0 spec defines that notifications must have their "id"

Add Batch JSON-RPC support (rpc client & server) #1583

Add Batch JSON-RPC support (rpc client & server) #1583

Conversation

jakesylvestre commented May 22, 2020 • edited Loading

jakesylvestre commented Aug 29, 2020

jakesylvestre commented Aug 29, 2020

jakesylvestre commented Aug 29, 2020 • edited Loading

jakesylvestre commented Aug 29, 2020 • edited Loading

jakesylvestre commented Aug 29, 2020 • edited Loading

onyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesylvestre Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcvernaleo commented Nov 3, 2020

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

coveralls commented Dec 1, 2020 • edited Loading

Pull Request Test Coverage Report for Build 494215604

💛 - Coveralls

jakesylvestre left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesylvestre commented Dec 1, 2020 • edited Loading

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

jakesylvestre commented Dec 15, 2020

Choose a reason for hiding this comment

jakesylvestre commented Feb 4, 2021 • edited Loading

jakesylvestre commented Feb 9, 2021

onyb left a comment

Choose a reason for hiding this comment

jcvernaleo left a comment

Choose a reason for hiding this comment

jakesylvestre commented May 22, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020 •

edited

Loading

jakesylvestre commented Aug 29, 2020 •

edited

Loading

jakesylvestre Dec 1, 2020 •

edited

Loading

coveralls commented Dec 1, 2020 •

edited

Loading

jakesylvestre left a comment •

edited

Loading

jakesylvestre commented Dec 1, 2020 •

edited

Loading

jakesylvestre commented Feb 4, 2021 •

edited

Loading