From 925006483747d39fa6d6038a8270a979bccfdadd Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 11 Nov 2020 14:16:03 +0100 Subject: [PATCH 1/4] integration: allow setting custom btcd exe path To allow using a custom btcd executable, we allow specifying a path to a file. If the path is empty, the harness will fall back to compiling one from scratch. --- integration/bip0009_test.go | 4 ++-- integration/csv_fork_test.go | 4 ++-- integration/rpcserver_test.go | 4 +++- integration/rpctest/node.go | 16 ++++++++++++---- integration/rpctest/rpc_harness.go | 9 ++++++--- integration/rpctest/rpc_harness_test.go | 12 ++++++------ 6 files changed, 31 insertions(+), 18 deletions(-) diff --git a/integration/bip0009_test.go b/integration/bip0009_test.go index df3721b1..b7206559 100644 --- a/integration/bip0009_test.go +++ b/integration/bip0009_test.go @@ -129,7 +129,7 @@ func assertSoftForkStatus(r *rpctest.Harness, t *testing.T, forkKey string, stat // specific soft fork deployment to test. func testBIP0009(t *testing.T, forkKey string, deploymentID uint32) { // Initialize the primary mining node with only the genesis block. - r, err := rpctest.New(&chaincfg.RegressionNetParams, nil, nil) + r, err := rpctest.New(&chaincfg.RegressionNetParams, nil, nil, "") if err != nil { t.Fatalf("unable to create primary harness: %v", err) } @@ -320,7 +320,7 @@ func TestBIP0009Mining(t *testing.T) { t.Parallel() // Initialize the primary mining node with only the genesis block. - r, err := rpctest.New(&chaincfg.SimNetParams, nil, nil) + r, err := rpctest.New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatalf("unable to create primary harness: %v", err) } diff --git a/integration/csv_fork_test.go b/integration/csv_fork_test.go index 345217c8..b623487e 100644 --- a/integration/csv_fork_test.go +++ b/integration/csv_fork_test.go @@ -109,7 +109,7 @@ func TestBIP0113Activation(t *testing.T) { t.Parallel() btcdCfg := []string{"--rejectnonstd"} - r, err := rpctest.New(&chaincfg.SimNetParams, nil, btcdCfg) + r, err := rpctest.New(&chaincfg.SimNetParams, nil, btcdCfg, "") if err != nil { t.Fatal("unable to create primary harness: ", err) } @@ -405,7 +405,7 @@ func TestBIP0068AndBIP0112Activation(t *testing.T) { // relative lock times. btcdCfg := []string{"--rejectnonstd"} - r, err := rpctest.New(&chaincfg.SimNetParams, nil, btcdCfg) + r, err := rpctest.New(&chaincfg.SimNetParams, nil, btcdCfg, "") if err != nil { t.Fatal("unable to create primary harness: ", err) } diff --git a/integration/rpcserver_test.go b/integration/rpcserver_test.go index df526442..e5528453 100644 --- a/integration/rpcserver_test.go +++ b/integration/rpcserver_test.go @@ -109,7 +109,9 @@ func TestMain(m *testing.M) { // ensure that non-standard transactions aren't accepted into the // mempool or relayed. btcdCfg := []string{"--rejectnonstd"} - primaryHarness, err = rpctest.New(&chaincfg.SimNetParams, nil, btcdCfg) + primaryHarness, err = rpctest.New( + &chaincfg.SimNetParams, nil, btcdCfg, "", + ) if err != nil { fmt.Println("unable to create primary harness: ", err) os.Exit(1) diff --git a/integration/rpctest/node.go b/integration/rpctest/node.go index 6aec2b11..73dc15fc 100644 --- a/integration/rpctest/node.go +++ b/integration/rpctest/node.go @@ -41,10 +41,18 @@ type nodeConfig struct { } // newConfig returns a newConfig with all default values. -func newConfig(prefix, certFile, keyFile string, extra []string) (*nodeConfig, error) { - btcdPath, err := btcdExecutablePath() - if err != nil { - btcdPath = "btcd" +func newConfig(prefix, certFile, keyFile string, extra []string, + customExePath string) (*nodeConfig, error) { + + var btcdPath string + if customExePath != "" { + btcdPath = customExePath + } else { + var err error + btcdPath, err = btcdExecutablePath() + if err != nil { + btcdPath = "btcd" + } } a := &nodeConfig{ diff --git a/integration/rpctest/rpc_harness.go b/integration/rpctest/rpc_harness.go index 1c2612e4..ee7b62fd 100644 --- a/integration/rpctest/rpc_harness.go +++ b/integration/rpctest/rpc_harness.go @@ -94,11 +94,12 @@ type Harness struct { // New creates and initializes new instance of the rpc test harness. // Optionally, websocket handlers and a specified configuration may be passed. // In the case that a nil config is passed, a default configuration will be -// used. +// used. If a custom btcd executable is specified, it will be used to start the +// harness node. Otherwise a new binary is built on demand. // // NOTE: This function is safe for concurrent access. func New(activeNet *chaincfg.Params, handlers *rpcclient.NotificationHandlers, - extraArgs []string) (*Harness, error) { + extraArgs []string, customExePath string) (*Harness, error) { harnessStateMtx.Lock() defer harnessStateMtx.Unlock() @@ -144,7 +145,9 @@ func New(activeNet *chaincfg.Params, handlers *rpcclient.NotificationHandlers, miningAddr := fmt.Sprintf("--miningaddr=%s", wallet.coinbaseAddr) extraArgs = append(extraArgs, miningAddr) - config, err := newConfig("rpctest", certFile, keyFile, extraArgs) + config, err := newConfig( + "rpctest", certFile, keyFile, extraArgs, customExePath, + ) if err != nil { return nil, err } diff --git a/integration/rpctest/rpc_harness_test.go b/integration/rpctest/rpc_harness_test.go index 717f8f45..25faf969 100644 --- a/integration/rpctest/rpc_harness_test.go +++ b/integration/rpctest/rpc_harness_test.go @@ -105,7 +105,7 @@ func assertConnectedTo(t *testing.T, nodeA *Harness, nodeB *Harness) { func testConnectNode(r *Harness, t *testing.T) { // Create a fresh test harness. - harness, err := New(&chaincfg.SimNetParams, nil, nil) + harness, err := New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatal(err) } @@ -153,7 +153,7 @@ func testActiveHarnesses(r *Harness, t *testing.T) { numInitialHarnesses := len(ActiveHarnesses()) // Create a single test harness. - harness1, err := New(&chaincfg.SimNetParams, nil, nil) + harness1, err := New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatal(err) } @@ -181,7 +181,7 @@ func testJoinMempools(r *Harness, t *testing.T) { // Create a local test harness with only the genesis block. The nodes // will be synced below so the same transaction can be sent to both // nodes without it being an orphan. - harness, err := New(&chaincfg.SimNetParams, nil, nil) + harness, err := New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatal(err) } @@ -281,7 +281,7 @@ func testJoinMempools(r *Harness, t *testing.T) { func testJoinBlocks(r *Harness, t *testing.T) { // Create a second harness with only the genesis block so it is behind // the main harness. - harness, err := New(&chaincfg.SimNetParams, nil, nil) + harness, err := New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatal(err) } @@ -469,7 +469,7 @@ func testGenerateAndSubmitBlockWithCustomCoinbaseOutputs(r *Harness, func testMemWalletReorg(r *Harness, t *testing.T) { // Create a fresh harness, we'll be using the main harness to force a // re-org on this local harness. - harness, err := New(&chaincfg.SimNetParams, nil, nil) + harness, err := New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { t.Fatal(err) } @@ -566,7 +566,7 @@ const ( func TestMain(m *testing.M) { var err error - mainHarness, err = New(&chaincfg.SimNetParams, nil, nil) + mainHarness, err = New(&chaincfg.SimNetParams, nil, nil, "") if err != nil { fmt.Println("unable to create main harness: ", err) os.Exit(1) From 93cc7f36cffbd66e3ad181dbc19aea5e05bc6b8b Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 11 Nov 2020 14:24:14 +0100 Subject: [PATCH 2/4] integration: allow overwriting address generator --- integration/rpctest/rpc_harness.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/integration/rpctest/rpc_harness.go b/integration/rpctest/rpc_harness.go index ee7b62fd..e8cd7e89 100644 --- a/integration/rpctest/rpc_harness.go +++ b/integration/rpctest/rpc_harness.go @@ -58,6 +58,13 @@ var ( // Used to protest concurrent access to above declared variables. harnessStateMtx sync.RWMutex + + // ListenAddressGenerator is a function that is used to generate two + // listen addresses (host:port), one for the P2P listener and one for + // the RPC listener. This is exported to allow overwriting of the + // default behavior which isn't very concurrency safe (just selecting + // a random port can produce collisions and therefore flakes). + ListenAddressGenerator = generateListeningAddresses ) // HarnessTestCase represents a test-case which utilizes an instance of the @@ -153,7 +160,7 @@ func New(activeNet *chaincfg.Params, handlers *rpcclient.NotificationHandlers, } // Generate p2p+rpc listening addresses. - config.listen, config.rpcListen = generateListeningAddresses() + config.listen, config.rpcListen = ListenAddressGenerator() // Create the testing node bounded to the simnet. node, err := newNode(config, nodeTestData) From 65d2b7a18cf4fc89680f67ea2e541abe0c17489b Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 11 Nov 2020 14:29:17 +0100 Subject: [PATCH 3/4] integration: allow specifying connection behavior --- integration/rpctest/rpc_harness.go | 42 +++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/integration/rpctest/rpc_harness.go b/integration/rpctest/rpc_harness.go index e8cd7e89..aaf9e906 100644 --- a/integration/rpctest/rpc_harness.go +++ b/integration/rpctest/rpc_harness.go @@ -34,6 +34,14 @@ const ( // BlockVersion is the default block version used when generating // blocks. BlockVersion = 4 + + // DefaultMaxConnectionRetries is the default number of times we re-try + // to connect to the node after starting it. + DefaultMaxConnectionRetries = 20 + + // DefaultConnectionRetryTimeout is the default duration we wait between + // two connection attempts. + DefaultConnectionRetryTimeout = 50 * time.Millisecond ) var ( @@ -58,7 +66,7 @@ var ( // Used to protest concurrent access to above declared variables. harnessStateMtx sync.RWMutex - + // ListenAddressGenerator is a function that is used to generate two // listen addresses (host:port), one for the P2P listener and one for // the RPC listener. This is exported to allow overwriting of the @@ -85,15 +93,22 @@ type Harness struct { // to. ActiveNet *chaincfg.Params + // MaxConnRetries is the maximum number of times we re-try to connect to + // the node after starting it. + MaxConnRetries int + + // ConnectionRetryTimeout is the duration we wait between two connection + // attempts. + ConnectionRetryTimeout time.Duration + Node *rpcclient.Client node *node handlers *rpcclient.NotificationHandlers wallet *memWallet - testNodeDir string - maxConnRetries int - nodeNum int + testNodeDir string + nodeNum int sync.Mutex } @@ -200,13 +215,14 @@ func New(activeNet *chaincfg.Params, handlers *rpcclient.NotificationHandlers, } h := &Harness{ - handlers: handlers, - node: node, - maxConnRetries: 20, - testNodeDir: nodeTestData, - ActiveNet: activeNet, - nodeNum: nodeNum, - wallet: wallet, + handlers: handlers, + node: node, + MaxConnRetries: DefaultMaxConnectionRetries, + ConnectionRetryTimeout: DefaultConnectionRetryTimeout, + testNodeDir: nodeTestData, + ActiveNet: activeNet, + nodeNum: nodeNum, + wallet: wallet, } // Track this newly created test instance within the package level @@ -322,9 +338,9 @@ func (h *Harness) connectRPCClient() error { var err error rpcConf := h.node.config.rpcConnConfig() - for i := 0; i < h.maxConnRetries; i++ { + for i := 0; i < h.MaxConnRetries; i++ { if client, err = rpcclient.New(&rpcConf, h.handlers); err != nil { - time.Sleep(time.Duration(i) * 50 * time.Millisecond) + time.Sleep(time.Duration(i) * h.ConnectionRetryTimeout) continue } break From 9e8bb3eddba2fb9ce953f3658b386ae39f115d8e Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 12 Nov 2020 10:08:54 +0100 Subject: [PATCH 4/4] btcjson+rpcserverhelp: restore bitcoind compatibility The PR #1594 introduced a change that made the order of parameters relevant, if one of them is nil. This makes it harder to be backward compatible with the same JSON message if an existing parameter in bitcoind was re-purposed to have a different meaning. --- btcjson/chainsvrcmds.go | 68 ++++++++++++++++++++++++++++++++---- btcjson/chainsvrcmds_test.go | 34 ++++++++++++++---- rpcserverhelp.go | 10 +++--- 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/btcjson/chainsvrcmds.go b/btcjson/chainsvrcmds.go index 0e683af0..aa1d4415 100644 --- a/btcjson/chainsvrcmds.go +++ b/btcjson/chainsvrcmds.go @@ -11,6 +11,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "reflect" "github.com/btcsuite/btcd/wire" ) @@ -819,11 +820,60 @@ func NewSearchRawTransactionsCmd(address string, verbose, skip, count *int, vinE } } +// AllowHighFeesOrMaxFeeRate defines a type that can either be the legacy +// allowhighfees boolean field or the new maxfeerate int field. +type AllowHighFeesOrMaxFeeRate struct { + Value interface{} +} + +// String returns the string representation of this struct, used for printing +// the marshaled default value in the help text. +func (a AllowHighFeesOrMaxFeeRate) String() string { + b, _ := a.MarshalJSON() + return string(b) +} + +// MarshalJSON implements the json.Marshaler interface +func (a AllowHighFeesOrMaxFeeRate) MarshalJSON() ([]byte, error) { + // The default value is false which only works with the legacy versions. + if a.Value == nil || + (reflect.ValueOf(a.Value).Kind() == reflect.Ptr && + reflect.ValueOf(a.Value).IsNil()) { + + return json.Marshal(false) + } + + return json.Marshal(a.Value) +} + +// UnmarshalJSON implements the json.Unmarshaler interface +func (a *AllowHighFeesOrMaxFeeRate) UnmarshalJSON(data []byte) error { + if len(data) == 0 { + return nil + } + + var unmarshalled interface{} + if err := json.Unmarshal(data, &unmarshalled); err != nil { + return err + } + + switch v := unmarshalled.(type) { + case bool: + a.Value = Bool(v) + case float64: + a.Value = Int32(int32(v)) + default: + return fmt.Errorf("invalid allowhighfees or maxfeerate value: "+ + "%v", unmarshalled) + } + + return nil +} + // SendRawTransactionCmd defines the sendrawtransaction JSON-RPC command. type SendRawTransactionCmd struct { - HexTx string - AllowHighFees *bool `jsonrpcdefault:"false"` - MaxFeeRate *int32 + HexTx string + FeeSetting *AllowHighFeesOrMaxFeeRate `jsonrpcdefault:"false"` } // NewSendRawTransactionCmd returns a new instance which can be used to issue a @@ -833,8 +883,10 @@ type SendRawTransactionCmd struct { // for optional parameters will use the default value. func NewSendRawTransactionCmd(hexTx string, allowHighFees *bool) *SendRawTransactionCmd { return &SendRawTransactionCmd{ - HexTx: hexTx, - AllowHighFees: allowHighFees, + HexTx: hexTx, + FeeSetting: &AllowHighFeesOrMaxFeeRate{ + Value: allowHighFees, + }, } } @@ -844,8 +896,10 @@ func NewSendRawTransactionCmd(hexTx string, allowHighFees *bool) *SendRawTransac // A 0 maxFeeRate indicates that a maximum fee rate won't be enforced. func NewBitcoindSendRawTransactionCmd(hexTx string, maxFeeRate int32) *SendRawTransactionCmd { return &SendRawTransactionCmd{ - HexTx: hexTx, - MaxFeeRate: &maxFeeRate, + HexTx: hexTx, + FeeSetting: &AllowHighFeesOrMaxFeeRate{ + Value: &maxFeeRate, + }, } } diff --git a/btcjson/chainsvrcmds_test.go b/btcjson/chainsvrcmds_test.go index d44a2ece..263d6d7e 100644 --- a/btcjson/chainsvrcmds_test.go +++ b/btcjson/chainsvrcmds_test.go @@ -1224,29 +1224,49 @@ func TestChainSvrCmds(t *testing.T) { { name: "sendrawtransaction", newCmd: func() (interface{}, error) { - return btcjson.NewCmd("sendrawtransaction", "1122") + return btcjson.NewCmd("sendrawtransaction", "1122", &btcjson.AllowHighFeesOrMaxFeeRate{}) }, staticCmd: func() interface{} { return btcjson.NewSendRawTransactionCmd("1122", nil) }, - marshalled: `{"jsonrpc":"1.0","method":"sendrawtransaction","params":["1122"],"id":1}`, + marshalled: `{"jsonrpc":"1.0","method":"sendrawtransaction","params":["1122",false],"id":1}`, unmarshalled: &btcjson.SendRawTransactionCmd{ - HexTx: "1122", - AllowHighFees: btcjson.Bool(false), + HexTx: "1122", + FeeSetting: &btcjson.AllowHighFeesOrMaxFeeRate{ + Value: btcjson.Bool(false), + }, }, }, { name: "sendrawtransaction optional", newCmd: func() (interface{}, error) { - return btcjson.NewCmd("sendrawtransaction", "1122", false) + return btcjson.NewCmd("sendrawtransaction", "1122", &btcjson.AllowHighFeesOrMaxFeeRate{Value: btcjson.Bool(false)}) }, staticCmd: func() interface{} { return btcjson.NewSendRawTransactionCmd("1122", btcjson.Bool(false)) }, marshalled: `{"jsonrpc":"1.0","method":"sendrawtransaction","params":["1122",false],"id":1}`, unmarshalled: &btcjson.SendRawTransactionCmd{ - HexTx: "1122", - AllowHighFees: btcjson.Bool(false), + HexTx: "1122", + FeeSetting: &btcjson.AllowHighFeesOrMaxFeeRate{ + Value: btcjson.Bool(false), + }, + }, + }, + { + name: "sendrawtransaction optional, bitcoind >= 0.19.0", + newCmd: func() (interface{}, error) { + return btcjson.NewCmd("sendrawtransaction", "1122", &btcjson.AllowHighFeesOrMaxFeeRate{Value: btcjson.Int32(1234)}) + }, + staticCmd: func() interface{} { + return btcjson.NewBitcoindSendRawTransactionCmd("1122", 1234) + }, + marshalled: `{"jsonrpc":"1.0","method":"sendrawtransaction","params":["1122",1234],"id":1}`, + unmarshalled: &btcjson.SendRawTransactionCmd{ + HexTx: "1122", + FeeSetting: &btcjson.AllowHighFeesOrMaxFeeRate{ + Value: btcjson.Int32(1234), + }, }, }, { diff --git a/rpcserverhelp.go b/rpcserverhelp.go index 75a63be0..c1cb8cd0 100644 --- a/rpcserverhelp.go +++ b/rpcserverhelp.go @@ -561,11 +561,11 @@ var helpDescsEnUS = map[string]string{ "searchrawtransactions--result0": "Hex-encoded serialized transaction", // SendRawTransactionCmd help. - "sendrawtransaction--synopsis": "Submits the serialized, hex-encoded transaction to the local peer and relays it to the network.", - "sendrawtransaction-hextx": "Serialized, hex-encoded signed transaction", - "sendrawtransaction-allowhighfees": "Whether or not to allow insanely high fees (btcd does not yet implement this parameter, so it has no effect)", - "sendrawtransaction-maxfeerate": "Used by bitcoind on or after v0.19.0", - "sendrawtransaction--result0": "The hash of the transaction", + "sendrawtransaction--synopsis": "Submits the serialized, hex-encoded transaction to the local peer and relays it to the network.", + "sendrawtransaction-hextx": "Serialized, hex-encoded signed transaction", + "sendrawtransaction-feesetting": "Whether or not to allow insanely high fees in bitcoind < v0.19.0 or the max fee rate for bitcoind v0.19.0 and later (btcd does not yet implement this parameter, so it has no effect)", + "sendrawtransaction--result0": "The hash of the transaction", + "allowhighfeesormaxfeerate-value": "Either the boolean value for the allowhighfees parameter in bitcoind < v0.19.0 or the numerical value for the maxfeerate field in bitcoind v0.19.0 and later", // SetGenerateCmd help. "setgenerate--synopsis": "Set the server to generate coins (mine) or not.",