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

jakesylvestre · 2019-12-04T07:07:52Z

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

…terfaces

allow non local tls disable for compatibility with private network in…

Rpcbulk

This reverts commit c25e1e8.

jakesylvestre · 2019-12-04T08:06:07Z

I think we need some sort of bulk handler we can tack other methods on too here.

torkelrogstad · 2019-12-04T16:05:15Z

@jakesyl Do you want review on this now, or should I hold off until it's less of a WIP?

jakesylvestre · 2019-12-04T17:17:51Z

@torkelrogstad, let's hold off for now. I'll have this done by EoD. Feel free to check it out in the meantime. I think I know how I want to solve the bulk querying in post mode now.

jakesylvestre · 2019-12-04T22:18:52Z

Actually @torkelrogstad, I'd love to get your input on the best way to queue these bulk requests. Here's how it's done in python:

I'm playing with how to do it in btcd

jakesylvestre · 2019-12-04T22:22:22Z

I'm thinking of adding a batch method onto client that returns a modified client. Here's the aforementioned implementation

jakesylvestre · 2019-12-05T07:38:10Z

@torkelrogstad I'm sure we need some work here, but this is what I've got so far. Please review and let me know what you think

torkelrogstad · 2019-12-05T09:24:16Z

I've started looking it over, great stuff! Review is still pending, I'll wait until I've finished the entire PR before publishing. Hopefully I'll find some time later today.

jakesylvestre · 2019-12-17T22:50:28Z

Hey, any idea if any changes are required here?

torkelrogstad

Sorry for delaying this so much! But here's some review for you.

Tested this against bitcoind 0.19 on my machine, worked great. 🎉

torkelrogstad · 2019-12-05T09:00:14Z

btcjson/jsonrpc.go

+	request.ID = data["id"]
+	methodValue, hasMethod := data["method"]
+	if hasMethod {
+		request.Method = methodValue.(string)


The cast here and on line 93 seems a bit dangerous to me, would blow up if the fields are accessed later and it turns out we have the wrong type.
What's your thoughts on this?

metod, ok := methodValue.(string) if ok { request.Method = method }

Interesting point. I don't see an alternative though

@torkelrogstad I've gone ahead and addressed this on #1583

torkelrogstad · 2019-12-05T09:05:39Z

btcjson/jsonrpc.go

+	paramsValue, hasParams := data["params"]
+	if !hasParams {
+		// set the request param to an empty array if it is ommited in the request
+		request.Params = []json.RawMessage{}
+	}
+	if hasParams {
+		// assert the request params is an array of data
+		params, paramsOk := paramsValue.([]interface{})
+		if paramsOk {
+			rawParams := make([]json.RawMessage, 0, len(params))
+			for _, param := range params {
+				marshalledParam, err := json.Marshal(param)
+				if err != nil {
+					return err
+				}
+				rawMessage := json.RawMessage(marshalledParam)
+				rawParams = append(rawParams, rawMessage)
+			}
+
+			request.Params = rawParams
+		}
+	}


Can avoid some nesting this way:

paramsValue, hasParams := data["params"] if !hasParams { // set the request param to an empty array if it is ommited in the request request.Params = []json.RawMessage{} // assert the request params is an array of data } else if params, paramsOk := paramsValue.([]interface{}); paramsOk { rawParams := make([]json.RawMessage, 0, len(params)) for _, param := range params { marshalledParam, err := json.Marshal(param) if err != nil { return err } rawMessage := json.RawMessage(marshalledParam) rawParams = append(rawParams, rawMessage) } request.Params = rawParams } else { // error here, saying we got the wrong type of data? }

torkelrogstad · 2019-12-05T09:07:53Z

btcjson/jsonrpc.go

+// request.
+func NewRequest(rpcVersion string, id interface{}, method string, params []interface{}) (*Request, error) {
+	// default to JSON-RPC 1.0 if RPC type is not specified
+	if rpcVersion != "2.0" && rpcVersion != "1.0" {


This strikes me as a bit dangerous. Two potential solutions, could (should, perhaps) do both IMO:

Introduce a type for it

type rpcVersion string var ( rpcVersion1 rpcVersion = rpcVersion("1.0") rpcVersion2 rpcVersion = rpcVersion("2.0") )

I think it's better to error out here, instead of silently changing behavior.

torkelrogstad · 2019-12-05T09:08:59Z

btcjson/jsonrpc.go

 // Typically callers will instead want to create the fully marshalled JSON-RPC
 // response to send over the wire with the MarshalResponse function.
-func NewResponse(id interface{}, marshalledResult []byte, rpcErr *RPCError) (*Response, error) {
+func NewResponse(rpcVersion string, id interface{}, marshalledResult []byte, rpcErr *RPCError) (*Response, error) {
+	if rpcVersion != "2.0" && rpcVersion != "1.0" {


Same comment about RPC versions from above apply here

torkelrogstad · 2019-12-05T09:09:16Z

btcjson/jsonrpc.go

+// a JSON-RPC response byte slice that is suitable for transmission to a
+// JSON-RPC client.
+func MarshalResponse(rpcVersion string, id interface{}, result interface{}, rpcErr *RPCError) ([]byte, error) {
+	if rpcVersion != "2.0" && rpcVersion != "1.0" {


RPC version comment from above

torkelrogstad · 2019-12-20T12:27:16Z

rpcserver.go

+			if len(batchedRequests) == 0 {
+				jsonErr := &btcjson.RPCError{
+					Code:    btcjson.ErrRPCInvalidRequest.Code,
+					Message: fmt.Sprint("Invalid request: empty batch"),


can use string literal directly, no need for fmt.Sprint

torkelrogstad · 2019-12-20T12:28:11Z

rpcwebsocket.go

+			if req.Method == "" || req.Params == nil {
+				jsonErr := &btcjson.RPCError{
+					Code:    btcjson.ErrRPCInvalidRequest.Code,
+					Message: fmt.Sprintf("Invalid request: malformed"),


can use string literal directly

torkelrogstad · 2019-12-20T12:29:04Z

rpcwebsocket.go

+
+					jsonErr := &btcjson.RPCError{
+						Code:    btcjson.ErrRPCInvalidRequest.Code,
+						Message: fmt.Sprint("Invalid request: empty batch"),


can use string literal directly

torkelrogstad · 2019-12-20T12:29:22Z

rpcwebsocket.go

+						if req.Method == "" || req.Params == nil {
+							jsonErr := &btcjson.RPCError{
+								Code:    btcjson.ErrRPCInvalidRequest.Code,
+								Message: fmt.Sprintf("Invalid request: malformed"),


can use string literal directly

torkelrogstad · 2019-12-20T12:31:20Z

rpcclient/chain.go

+// the best block in the longest block chain.
+func (r FutureGetBulkResult) Receive() (BulkResult, error) {
+	m := make(BulkResult)
+	res, err := receiveFuture(r)


res, err := receiveFuture(r) if err != nil { return nil, err }

jakesylvestre · 2020-02-09T04:04:27Z

Thanks @pavel-main, looks like that fixed it!

See #1537

$GOPATH caching has led to flaky tests as per btcsuite#1503 and btcsuite#1536. The speedup is marginal and while the false negatives are a headache, false positives are potentially dangerous.

jakesylvestre · 2020-03-04T06:01:16Z

Any update here? Looks like prereq #1537 has been fixed

jakesylvestre · 2020-03-04T12:51:47Z

@jcvernaleo

low priority

$GOPATH caching has led to flaky tests as per btcsuite#1503 and btcsuite#1536. The speedup is marginal and while the false negatives are a headache, false positives are potentially dangerous.

jakesylvestre · 2020-03-09T21:39:00Z

Going to need to squash

jcvernaleo

Generally looking good but along with my question (which I think leads to a lot of unnecessary changes and the questions by @torkelrogstad which need to be resolved, this badly needs to be rebased and squashed. I can do another review after that (and after we resolve those outstanding things) so the set of changes is clearer. I definitely want this to make it in though.

jcvernaleo · 2020-03-10T15:18:29Z

btcjson/btcdextcmds_test.go

@@ -193,7 +193,7 @@ func TestBtcdExtCmds(t *testing.T) {
 	for i, test := range tests {
 		// Marshal the command as created by the new static command
 		// creation function.
-		marshalled, err := btcjson.MarshalCmd(testID, test.staticCmd())
+		marshalled, err := btcjson.MarshalCmd("1.0", testID, test.staticCmd())


Why are you changing from a variable to a hardcoded number here (and all the places below?

fair point, I'll have to refactor. I've held off on squashing till these revisions are complete, but can do this earlier fi it helps your review

Appreciate the feedback

Generally I agree about waiting to squash until revisions are done but this needs the rebase before reviewing it for real so you might as well squash at the same time (I hate making people do extra steps if I don't have to).

jakesylvestre · 2020-03-10T19:20:23Z

@jcvernaleo I merged this by accident, went ahead and reverted instantly.

jakesylvestre · 2020-03-10T19:20:29Z

can I force push to undo?

jakesylvestre · 2020-03-10T19:25:01Z

@jcvernaleo merged the wrong branch here - I thought this was on my local. I undid immediately after (#1549), but can't push the history here because the branch is protected :(, please advise

jakesylvestre · 2020-03-10T19:43:06Z

@davecgh tagging you here as well

jakesylvestre · 2020-03-11T11:31:22Z

btw:

onyb · 2020-05-01T12:59:55Z

@jakesyl Are you still actively working on this?

jakesylvestre · 2020-05-01T20:03:50Z

Hi @onyb yes - I've been very busy the past month but have a few PRs queued here including this one that I need to work on. I'm still active in the btcd irc

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

Recreated in #1583

jakesylvestre added 6 commits October 31, 2019 11:32

allow non local tls disable for compatibility with private network in…

4ace7c8

…terfaces

Merge pull request #1 from xplorfin/concurrent-write

706aff9

allow non local tls disable for compatibility with private network in…

new flag

22338ac

add bulk request changes

c5ecee4

server http

2b6b35c

bulk first pass

e4c7407

jakesylvestre closed this Dec 4, 2019

tls unwrangle

c25e1e8

jakesylvestre changed the title ~~Rpcbulk~~ [WIP] Bulk RPC Support Dec 4, 2019

jakesylvestre reopened this Dec 4, 2019

jakesylvestre added 2 commits December 4, 2019 02:14

Merge pull request #4 from xplorfin/rpcbulk

9d2e844

Rpcbulk

Revert "tls unwrangle"

e740498

This reverts commit c25e1e8.

jakesylvestre added 3 commits December 4, 2019 17:29

add bulk mode

a59d23d

batch chan

fe2192d

add batch method

a5d1ff5

jakesylvestre changed the title ~~[WIP] Bulk RPC Support~~ Bulk RPC Support Dec 5, 2019

jakesylvestre changed the title ~~Bulk RPC Support~~ Add Batch JSON-RPC support Dec 5, 2019

jakesylvestre changed the title ~~Add Batch JSON-RPC support~~ Add Batch JSON-RPC support (rpc client & server) Dec 5, 2019

(rpcclient) add batch requests

6278ae4

jakesylvestre force-pushed the rpcbulk branch from 6cf1fa2 to 6278ae4 Compare December 5, 2019 09:46

(rpcclient) dereference fix

bd91ca2

torkelrogstad reviewed Dec 20, 2019

View reviewed changes

pavel-main mentioned this pull request Feb 10, 2020

Initial Signet support #1536

Closed

jakesylvestre mentioned this pull request Feb 10, 2020

Remove $GOPATH Caching #1537

Merged

jcvernaleo added enhancement low priority rpc labels Mar 4, 2020

jakesylvestre requested a review from jcvernaleo March 9, 2020 18:44

jakesylvestre mentioned this pull request Mar 9, 2020

How to modify chaincfg. #1518

Open

jcvernaleo requested changes Mar 10, 2020

View reviewed changes

jakesylvestre merged commit 5781b98 into btcsuite:master Mar 10, 2020

jakesylvestre deleted the rpcbulk branch March 10, 2020 19:18

jakesylvestre mentioned this pull request Mar 10, 2020

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

Merged

jakesylvestre restored the rpcbulk branch May 22, 2020 14:27

This was referenced May 22, 2020

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

Closed

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

Merged

This was referenced Aug 29, 2020

Remove $GOPATH Caching xplorfin/btcd#9

Merged

Remove $GOPATH Caching xplorfin/btcd#10

Merged

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

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

Conversation

jakesylvestre commented Dec 4, 2019 • edited Loading

jakesylvestre commented Dec 4, 2019

torkelrogstad commented Dec 4, 2019

jakesylvestre commented Dec 4, 2019

jakesylvestre commented Dec 4, 2019

jakesylvestre commented Dec 4, 2019

jakesylvestre commented Dec 5, 2019

torkelrogstad commented Dec 5, 2019

jakesylvestre commented Dec 17, 2019

torkelrogstad left a comment • 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

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

jakesylvestre commented Feb 9, 2020 • edited Loading

jakesylvestre commented Mar 4, 2020

jakesylvestre commented Mar 4, 2020

jakesylvestre commented Mar 9, 2020

jcvernaleo 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

jakesylvestre commented Mar 10, 2020

jakesylvestre commented Mar 10, 2020

jakesylvestre commented Mar 10, 2020

jakesylvestre commented Mar 10, 2020

jakesylvestre commented Mar 11, 2020

onyb commented May 1, 2020

jakesylvestre commented May 1, 2020

jakesylvestre commented May 22, 2020

jakesylvestre commented Dec 4, 2019 •

edited

Loading

torkelrogstad left a comment •

edited

Loading

jakesylvestre commented Feb 9, 2020 •

edited

Loading