From 61c215ec1a592bc236a99ccb1f723637645b6273 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Sat, 30 May 2026 16:47:18 +0900 Subject: [PATCH 1/5] ci: run rpctest integration tests `make build`, `make unit-cover`, and `make unit-race` all use plain `go test` without the rpctest build tag, so anything under //go:build rpctest -- the entire integration/ package outside of rpctest/, and parts of rpctest/ itself -- is not exercised by CI. Bugs that only surface under -tags=rpctest can land on master without detection. Add a test-rpctest job that runs `make unit` (which sets -tags=rpctest) so rpctest-tagged tests are part of every push and PR. --- .github/workflows/main.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 3fb2f37a..f6394a9f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -145,3 +145,18 @@ jobs: - name: Test run: make unit-race + + test-rpctest: + name: Unit rpctest + runs-on: ubuntu-latest + steps: + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + + - name: Check out source + uses: actions/checkout@v4 + + - name: Test + run: make unit From 221178501c6cc46b8bc3dbc93e135b9cfbab02ca Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Sat, 30 May 2026 13:51:00 +0900 Subject: [PATCH 2/5] integration: fix p2a_test build under -tags=rpctest p2a_test.go calls btcutil.NewAddressPayToAnchor, but the NewAddressPayToAnchor constructor lives in the address/v2 module's address package. Import that package and call it through there so the integration package builds when -tags=rpctest is set. --- integration/p2a_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/integration/p2a_test.go b/integration/p2a_test.go index 059e56b7..e986db23 100644 --- a/integration/p2a_test.go +++ b/integration/p2a_test.go @@ -6,6 +6,7 @@ package integration import ( "testing" + "github.com/btcsuite/btcd/address/v2" "github.com/btcsuite/btcd/btcutil/v2" "github.com/btcsuite/btcd/chaincfg/v2" "github.com/btcsuite/btcd/integration/rpctest" @@ -46,7 +47,7 @@ func TestPayToAnchorSimple(t *testing.T) { // Create a P2A output using the helper to get a P2A address. This // ensures we're using the same P2A script generation logic. - p2aAddr, err := btcutil.NewAddressPayToAnchor(&chaincfg.SimNetParams) + p2aAddr, err := address.NewAddressPayToAnchor(&chaincfg.SimNetParams) if err != nil { t.Fatalf("unable to create P2A address: %v", err) } From 79752a8880bf03e330bc1f2def93a29fc645cf54 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Sat, 30 May 2026 13:53:12 +0900 Subject: [PATCH 3/5] btcjson: accept null in StringOrArray.UnmarshalJSON StringOrArray.MarshalJSON emits JSON null for a nil slice (see existing test "nil slice marshals as null" in TestStringOrArrayMarshalJSON), but UnmarshalJSON did not have a matching case for null and fell to the default branch, returning "invalid string_or_array value: ". A round trip of a nil slice therefore failed. This bit the rpcclient against btcd's own getblockchaininfo, whose Warnings field is a StringOrArray that the server leaves as a nil slice when there are no warnings. Every rpctest integration test that touches GetBlockChainInfo (TestBIP0009, TestBIP0068AndBIP0112Activation, TestBIP0113Activation, TestPrune) failed to decode the response. Handle the nil case explicitly so null decodes back to a nil slice, and add regression cases for "warnings: null" and an omitted warnings field to TestGetBlockChainInfoWarnings. --- btcjson/chainsvrresults.go | 3 +++ btcjson/chainsvrresults_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/btcjson/chainsvrresults.go b/btcjson/chainsvrresults.go index 3ee2b8b6..5cd1e907 100644 --- a/btcjson/chainsvrresults.go +++ b/btcjson/chainsvrresults.go @@ -385,6 +385,9 @@ func (h *StringOrArray) UnmarshalJSON(data []byte) error { } switch v := unmarshalled.(type) { + case nil: + *h = nil + case string: *h = []string{v} diff --git a/btcjson/chainsvrresults_test.go b/btcjson/chainsvrresults_test.go index fb681a9c..f37adb4b 100644 --- a/btcjson/chainsvrresults_test.go +++ b/btcjson/chainsvrresults_test.go @@ -350,6 +350,16 @@ func TestGetBlockChainInfoWarnings(t *testing.T) { result: `{"warnings": []}`, expected: btcjson.StringOrArray{}, }, + { + name: "blockchain info with null warnings", + result: `{"warnings": null}`, + expected: nil, + }, + { + name: "blockchain info with warnings field omitted", + result: `{}`, + expected: nil, + }, } for _, test := range tests { From bfb36e52e72dacf576b0a0f6f0c509c7231dd438 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Sat, 30 May 2026 13:56:11 +0900 Subject: [PATCH 4/5] netsync: process inv announcements when no syncPeer is set handleInvMsg early-returned for any inv from a non-syncPeer whenever sm.current() was false, with the comment that it prevents fetching a mass of orphans. That guard assumes a syncPeer is already fetching blocks; when syncPeer is nil, the assumption breaks down and the early return becomes a deadlock. The deadlock is reachable whenever two nodes connect at equal heights: startSync exits without picking a syncPeer (no peer is "higher"), and nothing later promotes the freshly-mined blocks the peer announces via inv. The pre-verack disconnect and sync-race regression tests in integration/sync_race_test.go fail consistently because of this. Only skip the inv when we actually have a syncPeer. When syncPeer is nil, fall through and let the normal request path queue the block -- the inv is the only signal that there are blocks to fetch. --- netsync/manager.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/netsync/manager.go b/netsync/manager.go index cf9c898b..9addc5ca 100644 --- a/netsync/manager.go +++ b/netsync/manager.go @@ -1124,9 +1124,10 @@ func (sm *SyncManager) handleInvMsg(imsg *invMsg) { peer.UpdateLastAnnouncedBlock(&invVects[lastBlock].Hash) } - // Ignore invs from peers that aren't the sync if we are not current. - // Helps prevent fetching a mass of orphans. - if peer != sm.syncPeer && !sm.current() { + // Ignore invs from peers that aren't the sync peer if we are not + // current. Helps prevent fetching a mass of orphans. When syncPeer + // is nil, accept invs from any peer. + if sm.syncPeer != nil && peer != sm.syncPeer && !sm.current() { return } From f8ce7a7da824e9e5664fda5d5b93ce23f0c550ab Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Sat, 30 May 2026 13:57:18 +0900 Subject: [PATCH 5/5] rpctest: scope shared state to the current process Two pieces of rpctest's global state silently aliased across concurrent test processes (which is what `go test ./...` does by default, so any `make unit` that exercises -tags=rpctest hit this): - btcdExecutablePath compiled to a fixed path /tmp/btcd/rpctest/btcd. Two `go build` invocations would race on the same file, occasionally yielding a truncated or stale binary and downstream "tls: certificate signed by unknown authority" failures when the harness tried to talk to the resulting node. - lastPort started at the same defaultNodePort in every process. The bind-test in NextAvailablePort closes the listener before returning, so two processes climbing from the same base would frequently hand out the same port and one harness would die with "connection refused" when btcd failed to bind. Suffix the executable with a random uint32 and seed lastPort with a random offset into a 50k-port window so each process climbs through its own range. --- integration/rpctest/btcd.go | 9 +++++++-- integration/rpctest/rpc_harness.go | 11 ++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/integration/rpctest/btcd.go b/integration/rpctest/btcd.go index 29642c84..22717a5c 100644 --- a/integration/rpctest/btcd.go +++ b/integration/rpctest/btcd.go @@ -6,6 +6,7 @@ package rpctest import ( "fmt" + "math/rand/v2" "os/exec" "path/filepath" "runtime" @@ -43,8 +44,12 @@ func btcdExecutablePath() (string, error) { return "", err } - // Build btcd and output an executable in a static temp path. - outputPath := filepath.Join(testDir, "btcd") + // Build btcd to a random path so concurrent `go test` processes + // (e.g. when test packages run in parallel under `make unit`) do + // not race on the same output file. Each test process pays a + // one-time compile cost; within a process the compileMtx-guarded + // cache keeps it to one build. + outputPath := filepath.Join(testDir, fmt.Sprintf("btcd-%d", rand.Uint32())) if runtime.GOOS == "windows" { outputPath += ".exe" } diff --git a/integration/rpctest/rpc_harness.go b/integration/rpctest/rpc_harness.go index 1d3d42da..9c9cb852 100644 --- a/integration/rpctest/rpc_harness.go +++ b/integration/rpctest/rpc_harness.go @@ -6,6 +6,7 @@ package rpctest import ( "fmt" + "math/rand/v2" "net" "os" "path/filepath" @@ -76,7 +77,15 @@ var ( // lastPort is the last port determined to be free for use by a new // node. It should be used atomically. - lastPort uint32 = defaultNodePort + // + // Seed with a random offset so concurrent `go test` processes + // (e.g. when integration/ and integration/rpctest/ run in parallel + // under `make unit`) do not race on the same port range. The + // bind-test in NextAvailablePort closes the listener before + // returning, leaving a window where another process could grab the + // same port; staggering each process's starting point avoids the + // collision. The 50k-port window leaves headroom below 65535. + lastPort uint32 = defaultNodePort + rand.Uint32N(50000) ) // HarnessTestCase represents a test-case which utilizes an instance of the