rpc: Add caching support (#9650)

* Set cache control in the HTTP-RPC response header * Add a simply cache policy to the RPC routes * add a condition to check the RPC request has default height settings * fix cherry pick error * update pending log * use options struct intead of single parameter * refacor FuncOptions to functional options * add functional options in WebSocket RPC function * revert doc * replace deprecated function call * revise functional options * remove unuse comment * fix revised error * adjust cache-control settings * Update rpc/jsonrpc/server/http_json_handler.go Co-authored-by: Thane Thomson <[email protected]> * linter: Fix false positive Signed-off-by: Thane Thomson <[email protected]> * rpc: Separate cacheable and non-cacheable HTTP response writers Allows us to roll this change out in a non-API-breaking way, since this is an additive change. Signed-off-by: Thane Thomson <[email protected]> * rpc: Ensure consistent caching strategy Ensure a consistent caching strategy across both JSONRPC- and URI-based requests. This requires a bit of a refactor of the previous caching logic, which is complicated a little by the complex reflection-based approach taken in the Tendermint RPC. Signed-off-by: Thane Thomson <[email protected]> * rpc: Add more tests for caching Signed-off-by: Thane Thomson <[email protected]> * Update CHANGELOG_PENDING Signed-off-by: Thane Thomson <[email protected]> * light: Sync routes config with RPC core Signed-off-by: Thane Thomson <[email protected]> * rpc: Update OpenAPI docs Signed-off-by: Thane Thomson <[email protected]> Signed-off-by: Thane Thomson <[email protected]> Co-authored-by: jayt106 <[email protected]> Co-authored-by: jay tseng <[email protected]> Co-authored-by: JayT106 <[email protected]>
celestiaorg · Nov 3, 2022 · 816c6ba · 816c6ba
1 parent 9ec9085
commit 816c6ba
Show file tree

Hide file tree

Showing 13 changed files with 357 additions and 68 deletions.
diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
@@ -28,6 +28,7 @@
 - [pubsub] \#7319 Performance improvements for the event query API (@creachadair)
 - [p2p/pex] \#6509 Improve addrBook.hash performance (@cuonglm)
 - [crypto/merkle] \#6443 & \#6513 Improve HashAlternatives performance (@cuonglm, @marbar3778)
+- [rpc] \#9650 Enable caching of RPC responses (@JayT106)
 
 ### BUG FIXES
 
@@ -97,4 +98,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 
 - [consensus] \#9229 fix round number of `enterPropose` when handling `RoundStepNewRound` timeout. (@fatcat22)
 - [docker] \#9073 enable cross platform build using docker buildx
-- [blocksync] \#9518 handle the case when the sending queue is full: retry block request after a timeout
+- [blocksync] \#9518 handle the case when the sending queue is full: retry block request after a timeout
diff --git a/cmd/tendermint/commands/debug/util.go b/cmd/tendermint/commands/debug/util.go
@@ -67,7 +67,7 @@ func copyConfig(home, dir string) error {
 func dumpProfile(dir, addr, profile string, debug int) error {
 	endpoint := fmt.Sprintf("%s/debug/pprof/%s?debug=%d", addr, profile, debug)
 
-	//nolint:all
+	//nolint:gosec,nolintlint
 	resp, err := http.Get(endpoint)
 	if err != nil {
 		return fmt.Errorf("failed to query for %s profile: %w", profile, err)

diff --git a/light/proxy/routes.go b/light/proxy/routes.go
@@ -21,22 +21,22 @@ func RPCRoutes(c *lrpc.Client) map[string]*rpcserver.RPCFunc {
 		"health":               rpcserver.NewRPCFunc(makeHealthFunc(c), ""),
 		"status":               rpcserver.NewRPCFunc(makeStatusFunc(c), ""),
 		"net_info":             rpcserver.NewRPCFunc(makeNetInfoFunc(c), ""),
-		"blockchain":           rpcserver.NewRPCFunc(makeBlockchainInfoFunc(c), "minHeight,maxHeight"),
-		"genesis":              rpcserver.NewRPCFunc(makeGenesisFunc(c), ""),
-		"genesis_chunked":      rpcserver.NewRPCFunc(makeGenesisChunkedFunc(c), ""),
-		"block":                rpcserver.NewRPCFunc(makeBlockFunc(c), "height"),
-		"header":               rpcserver.NewRPCFunc(makeHeaderFunc(c), "height"),
-		"header_by_hash":       rpcserver.NewRPCFunc(makeHeaderByHashFunc(c), "hash"),
-		"block_by_hash":        rpcserver.NewRPCFunc(makeBlockByHashFunc(c), "hash"),
-		"block_results":        rpcserver.NewRPCFunc(makeBlockResultsFunc(c), "height"),
-		"commit":               rpcserver.NewRPCFunc(makeCommitFunc(c), "height"),
-		"tx":                   rpcserver.NewRPCFunc(makeTxFunc(c), "hash,prove"),
+		"blockchain":           rpcserver.NewRPCFunc(makeBlockchainInfoFunc(c), "minHeight,maxHeight", rpcserver.Cacheable()),
+		"genesis":              rpcserver.NewRPCFunc(makeGenesisFunc(c), "", rpcserver.Cacheable()),
+		"genesis_chunked":      rpcserver.NewRPCFunc(makeGenesisChunkedFunc(c), "", rpcserver.Cacheable()),
+		"block":                rpcserver.NewRPCFunc(makeBlockFunc(c), "height", rpcserver.Cacheable("height")),
+		"header":               rpcserver.NewRPCFunc(makeHeaderFunc(c), "height", rpcserver.Cacheable("height")),
+		"header_by_hash":       rpcserver.NewRPCFunc(makeHeaderByHashFunc(c), "hash", rpcserver.Cacheable()),
+		"block_by_hash":        rpcserver.NewRPCFunc(makeBlockByHashFunc(c), "hash", rpcserver.Cacheable()),
+		"block_results":        rpcserver.NewRPCFunc(makeBlockResultsFunc(c), "height", rpcserver.Cacheable("height")),
+		"commit":               rpcserver.NewRPCFunc(makeCommitFunc(c), "height", rpcserver.Cacheable("height")),
+		"tx":                   rpcserver.NewRPCFunc(makeTxFunc(c), "hash,prove", rpcserver.Cacheable()),
 		"tx_search":            rpcserver.NewRPCFunc(makeTxSearchFunc(c), "query,prove,page,per_page,order_by"),
 		"block_search":         rpcserver.NewRPCFunc(makeBlockSearchFunc(c), "query,page,per_page,order_by"),
-		"validators":           rpcserver.NewRPCFunc(makeValidatorsFunc(c), "height,page,per_page"),
+		"validators":           rpcserver.NewRPCFunc(makeValidatorsFunc(c), "height,page,per_page", rpcserver.Cacheable("height")),
 		"dump_consensus_state": rpcserver.NewRPCFunc(makeDumpConsensusStateFunc(c), ""),
 		"consensus_state":      rpcserver.NewRPCFunc(makeConsensusStateFunc(c), ""),
-		"consensus_params":     rpcserver.NewRPCFunc(makeConsensusParamsFunc(c), "height"),
+		"consensus_params":     rpcserver.NewRPCFunc(makeConsensusParamsFunc(c), "height", rpcserver.Cacheable("height")),
 		"unconfirmed_txs":      rpcserver.NewRPCFunc(makeUnconfirmedTxsFunc(c), "limit"),
 		"num_unconfirmed_txs":  rpcserver.NewRPCFunc(makeNumUnconfirmedTxsFunc(c), ""),
 
@@ -47,7 +47,7 @@ func RPCRoutes(c *lrpc.Client) map[string]*rpcserver.RPCFunc {
 
 		// abci API
 		"abci_query": rpcserver.NewRPCFunc(makeABCIQueryFunc(c), "path,data,height,prove"),
-		"abci_info":  rpcserver.NewRPCFunc(makeABCIInfoFunc(c), ""),
+		"abci_info":  rpcserver.NewRPCFunc(makeABCIInfoFunc(c), "", rpcserver.Cacheable()),
 
 		// evidence API
 		"broadcast_evidence": rpcserver.NewRPCFunc(makeBroadcastEvidenceFunc(c), "evidence"),

diff --git a/rpc/core/routes.go b/rpc/core/routes.go
@@ -17,23 +17,23 @@ var Routes = map[string]*rpc.RPCFunc{
 	"health":               rpc.NewRPCFunc(Health, ""),
 	"status":               rpc.NewRPCFunc(Status, ""),
 	"net_info":             rpc.NewRPCFunc(NetInfo, ""),
-	"blockchain":           rpc.NewRPCFunc(BlockchainInfo, "minHeight,maxHeight"),
-	"genesis":              rpc.NewRPCFunc(Genesis, ""),
-	"genesis_chunked":      rpc.NewRPCFunc(GenesisChunked, "chunk"),
-	"block":                rpc.NewRPCFunc(Block, "height"),
-	"block_by_hash":        rpc.NewRPCFunc(BlockByHash, "hash"),
-	"block_results":        rpc.NewRPCFunc(BlockResults, "height"),
-	"commit":               rpc.NewRPCFunc(Commit, "height"),
-	"header":               rpc.NewRPCFunc(Header, "height"),
-	"header_by_hash":       rpc.NewRPCFunc(HeaderByHash, "hash"),
-	"check_tx":             rpc.NewRPCFunc(CheckTx, "tx"),
-	"tx":                   rpc.NewRPCFunc(Tx, "hash,prove"),
+	"blockchain":           rpc.NewRPCFunc(BlockchainInfo, "minHeight,maxHeight", rpc.Cacheable()),
+	"genesis":              rpc.NewRPCFunc(Genesis, "", rpc.Cacheable()),
+	"genesis_chunked":      rpc.NewRPCFunc(GenesisChunked, "chunk", rpc.Cacheable()),
+	"block":                rpc.NewRPCFunc(Block, "height", rpc.Cacheable("height")),
+	"block_by_hash":        rpc.NewRPCFunc(BlockByHash, "hash", rpc.Cacheable()),
+	"block_results":        rpc.NewRPCFunc(BlockResults, "height", rpc.Cacheable("height")),
+	"commit":               rpc.NewRPCFunc(Commit, "height", rpc.Cacheable("height")),
+	"header":               rpc.NewRPCFunc(Header, "height", rpc.Cacheable("height")),
+	"header_by_hash":       rpc.NewRPCFunc(HeaderByHash, "hash", rpc.Cacheable()),
+	"check_tx":             rpc.NewRPCFunc(CheckTx, "tx", rpc.Cacheable()),
+	"tx":                   rpc.NewRPCFunc(Tx, "hash,prove", rpc.Cacheable()),
 	"tx_search":            rpc.NewRPCFunc(TxSearch, "query,prove,page,per_page,order_by"),
 	"block_search":         rpc.NewRPCFunc(BlockSearch, "query,page,per_page,order_by"),
-	"validators":           rpc.NewRPCFunc(Validators, "height,page,per_page"),
+	"validators":           rpc.NewRPCFunc(Validators, "height,page,per_page", rpc.Cacheable("height")),
 	"dump_consensus_state": rpc.NewRPCFunc(DumpConsensusState, ""),
 	"consensus_state":      rpc.NewRPCFunc(ConsensusState, ""),
-	"consensus_params":     rpc.NewRPCFunc(ConsensusParams, "height"),
+	"consensus_params":     rpc.NewRPCFunc(ConsensusParams, "height", rpc.Cacheable("height")),
 	"unconfirmed_txs":      rpc.NewRPCFunc(UnconfirmedTxs, "limit"),
 	"num_unconfirmed_txs":  rpc.NewRPCFunc(NumUnconfirmedTxs, ""),
 
@@ -44,7 +44,7 @@ var Routes = map[string]*rpc.RPCFunc{
 
 	// abci API
 	"abci_query": rpc.NewRPCFunc(ABCIQuery, "path,data,height,prove"),
-	"abci_info":  rpc.NewRPCFunc(ABCIInfo, ""),
+	"abci_info":  rpc.NewRPCFunc(ABCIInfo, "", rpc.Cacheable()),
 
 	// evidence API
 	"broadcast_evidence": rpc.NewRPCFunc(BroadcastEvidence, "evidence"),

diff --git a/rpc/jsonrpc/jsonrpc_test.go b/rpc/jsonrpc/jsonrpc_test.go
@@ -7,8 +7,10 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 	"os"
 	"os/exec"
+	"strings"
 	"testing"
 	"time"
 
@@ -37,9 +39,7 @@ const (
 	testVal = "acbd"
 )
 
-var (
-	ctx = context.Background()
-)
+var ctx = context.Background()
 
 type ResultEcho struct {
 	Value string `json:"value"`
@@ -57,13 +57,18 @@ type ResultEchoDataBytes struct {
 	Value tmbytes.HexBytes `json:"value"`
 }
 
+type ResultEchoWithDefault struct {
+	Value int `json:"value"`
+}
+
 // Define some routes
 var Routes = map[string]*server.RPCFunc{
 	"echo":            server.NewRPCFunc(EchoResult, "arg"),
 	"echo_ws":         server.NewWSRPCFunc(EchoWSResult, "arg"),
 	"echo_bytes":      server.NewRPCFunc(EchoBytesResult, "arg"),
 	"echo_data_bytes": server.NewRPCFunc(EchoDataBytesResult, "arg"),
 	"echo_int":        server.NewRPCFunc(EchoIntResult, "arg"),
+	"echo_default":    server.NewRPCFunc(EchoWithDefault, "arg", server.Cacheable("arg")),
 }
 
 func EchoResult(ctx *types.Context, v string) (*ResultEcho, error) {
@@ -86,6 +91,14 @@ func EchoDataBytesResult(ctx *types.Context, v tmbytes.HexBytes) (*ResultEchoDat
 	return &ResultEchoDataBytes{v}, nil
 }
 
+func EchoWithDefault(ctx *types.Context, v *int) (*ResultEchoWithDefault, error) {
+	val := -1
+	if v != nil {
+		val = *v
+	}
+	return &ResultEchoWithDefault{val}, nil
+}
+
 func TestMain(m *testing.M) {
 	setup()
 	code := m.Run()
@@ -199,26 +212,47 @@ func echoDataBytesViaHTTP(cl client.Caller, bytes tmbytes.HexBytes) (tmbytes.Hex
 	return result.Value, nil
 }
 
+func echoWithDefaultViaHTTP(cl client.Caller, v *int) (int, error) {
+	params := map[string]interface{}{}
+	if v != nil {
+		params["arg"] = *v
+	}
+	result := new(ResultEchoWithDefault)
+	if _, err := cl.Call(ctx, "echo_default", params, result); err != nil {
+		return 0, err
+	}
+	return result.Value, nil
+}
+
 func testWithHTTPClient(t *testing.T, cl client.HTTPClient) {
 	val := testVal
 	got, err := echoViaHTTP(cl, val)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, got, val)
 
 	val2 := randBytes(t)
 	got2, err := echoBytesViaHTTP(cl, val2)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, got2, val2)
 
 	val3 := tmbytes.HexBytes(randBytes(t))
 	got3, err := echoDataBytesViaHTTP(cl, val3)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, got3, val3)
 
 	val4 := tmrand.Intn(10000)
 	got4, err := echoIntViaHTTP(cl, val4)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, got4, val4)
+
+	got5, err := echoWithDefaultViaHTTP(cl, nil)
+	require.NoError(t, err)
+	assert.Equal(t, got5, -1)
+
+	val6 := tmrand.Intn(10000)
+	got6, err := echoWithDefaultViaHTTP(cl, &val6)
+	require.NoError(t, err)
+	assert.Equal(t, got6, val6)
 }
 
 func echoViaWS(cl *client.WSClient, val string) (string, error) {
@@ -233,7 +267,6 @@ func echoViaWS(cl *client.WSClient, val string) (string, error) {
 	msg := <-cl.ResponsesCh
 	if msg.Error != nil {
 		return "", err
-
 	}
 	result := new(ResultEcho)
 	err = json.Unmarshal(msg.Result, result)
@@ -255,7 +288,6 @@ func echoBytesViaWS(cl *client.WSClient, bytes []byte) ([]byte, error) {
 	msg := <-cl.ResponsesCh
 	if msg.Error != nil {
 		return []byte{}, msg.Error
-
 	}
 	result := new(ResultEchoBytes)
 	err = json.Unmarshal(msg.Result, result)
@@ -399,6 +431,74 @@ func TestWSClientPingPong(t *testing.T) {
 	time.Sleep(6 * time.Second)
 }
 
+func TestJSONRPCCaching(t *testing.T) {
+	httpAddr := strings.Replace(tcpAddr, "tcp://", "http://", 1)
+	cl, err := client.DefaultHTTPClient(httpAddr)
+	require.NoError(t, err)
+
+	// Not supplying the arg should result in not caching
+	params := make(map[string]interface{})
+	req, err := types.MapToRequest(types.JSONRPCIntID(1000), "echo_default", params)
+	require.NoError(t, err)
+
+	res1, err := rawJSONRPCRequest(t, cl, httpAddr, req)
+	defer func() { _ = res1.Body.Close() }()
+	require.NoError(t, err)
+	assert.Equal(t, "", res1.Header.Get("Cache-control"))
+
+	// Supplying the arg should result in caching
+	params["arg"] = tmrand.Intn(10000)
+	req, err = types.MapToRequest(types.JSONRPCIntID(1001), "echo_default", params)
+	require.NoError(t, err)
+
+	res2, err := rawJSONRPCRequest(t, cl, httpAddr, req)
+	defer func() { _ = res2.Body.Close() }()
+	require.NoError(t, err)
+	assert.Equal(t, "public, max-age=86400", res2.Header.Get("Cache-control"))
+}
+
+func rawJSONRPCRequest(t *testing.T, cl *http.Client, url string, req interface{}) (*http.Response, error) {
+	reqBytes, err := json.Marshal(req)
+	require.NoError(t, err)
+
+	reqBuf := bytes.NewBuffer(reqBytes)
+	httpReq, err := http.NewRequest(http.MethodPost, url, reqBuf)
+	require.NoError(t, err)
+
+	httpReq.Header.Set("Content-type", "application/json")
+
+	return cl.Do(httpReq)
+}
+
+func TestURICaching(t *testing.T) {
+	httpAddr := strings.Replace(tcpAddr, "tcp://", "http://", 1)
+	cl, err := client.DefaultHTTPClient(httpAddr)
+	require.NoError(t, err)
+
+	// Not supplying the arg should result in not caching
+	args := url.Values{}
+	res1, err := rawURIRequest(t, cl, httpAddr+"/echo_default", args)
+	defer func() { _ = res1.Body.Close() }()
+	require.NoError(t, err)
+	assert.Equal(t, "", res1.Header.Get("Cache-control"))
+
+	// Supplying the arg should result in caching
+	args.Set("arg", fmt.Sprintf("%d", tmrand.Intn(10000)))
+	res2, err := rawURIRequest(t, cl, httpAddr+"/echo_default", args)
+	defer func() { _ = res2.Body.Close() }()
+	require.NoError(t, err)
+	assert.Equal(t, "public, max-age=86400", res2.Header.Get("Cache-control"))
+}
+
+func rawURIRequest(t *testing.T, cl *http.Client, url string, args url.Values) (*http.Response, error) {
+	req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(args.Encode()))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	return cl.Do(req)
+}
+
 func randBytes(t *testing.T) []byte {
 	n := tmrand.Intn(10) + 2
 	buf := make([]byte, n)

diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go
@@ -55,6 +55,11 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 			requests = []types.RPCRequest{request}
 		}
 
+		// Set the default response cache to true unless
+		// 1. Any RPC request error.
+		// 2. Any RPC request doesn't allow to be cached.
+		// 3. Any RPC request has the height argument and the value is 0 (the default).
+		cache := true
 		for _, request := range requests {
 			request := request
 
@@ -72,11 +77,13 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 					responses,
 					types.RPCInvalidRequestError(request.ID, fmt.Errorf("path %s is invalid", r.URL.Path)),
 				)
+				cache = false
 				continue
 			}
 			rpcFunc, ok := funcMap[request.Method]
-			if !ok || rpcFunc.ws {
+			if !ok || (rpcFunc.ws) {
 				responses = append(responses, types.RPCMethodNotFoundError(request.ID))
+				cache = false
 				continue
 			}
 			ctx := &types.Context{JSONReq: &request, HTTPReq: r}
@@ -88,11 +95,16 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 						responses,
 						types.RPCInvalidParamsError(request.ID, fmt.Errorf("error converting json params to arguments: %w", err)),
 					)
+					cache = false
 					continue
 				}
 				args = append(args, fnArgs...)
 			}
 
+			if cache && !rpcFunc.cacheableWithArgs(args) {
+				cache = false
+			}
+
 			returns := rpcFunc.f.Call(args)
 			result, err := unreflectResult(returns)
 			if err != nil {
@@ -103,7 +115,13 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 		}
 
 		if len(responses) > 0 {
-			if wErr := WriteRPCResponseHTTP(w, responses...); wErr != nil {
+			var wErr error
+			if cache {
+				wErr = WriteCacheableRPCResponseHTTP(w, responses...)
+			} else {
+				wErr = WriteRPCResponseHTTP(w, responses...)
+			}
+			if wErr != nil {
 				logger.Error("failed to write responses", "res", responses, "err", wErr)
 			}
 		}
@@ -128,7 +146,6 @@ func mapParamsToArgs(
 	params map[string]json.RawMessage,
 	argsOffset int,
 ) ([]reflect.Value, error) {
-
 	values := make([]reflect.Value, len(rpcFunc.argNames))
 	for i, argName := range rpcFunc.argNames {
 		argType := rpcFunc.args[i+argsOffset]
@@ -153,7 +170,6 @@ func arrayParamsToArgs(
 	params []json.RawMessage,
 	argsOffset int,
 ) ([]reflect.Value, error) {
-
 	if len(rpcFunc.argNames) != len(params) {
 		return nil, fmt.Errorf("expected %v parameters (%v), got %v (%v)",
 			len(rpcFunc.argNames), rpcFunc.argNames, len(params), params)