netsync: require block-serving services on regtest/simnet sync peers
What changed, and why it matters
This commit fixes a bug in btcd's test-network synchronization logic. A previous change accidentally allowed light clients (such as Neutrino wallets) to be chosen as sync peers on regtest/simnet. Because light clients advertise a recent block height but cannot actually serve blocks, a node could pick one as its sync peer, stall, disconnect it, then pick another light client and stall again—potentially livelocking synchronization. The fix restores the requirement that sync peers must signal they can serve full blocks, while still allowing any network address on these test networks.
Review and merge the patch; verify that regtest/simnet nodes still sync correctly from Docker/non-localhost peers that advertise the required service flags, and that neutrino or other light clients are no longer selected as sync candidates.
Security signals we found
Denial-of-service via sync livelock on regtest/simnet
Light client eligible as sync peer due to missing service-flag check
Peer service flags not validated before sync election on test networks
Evidence from the diff
In netsync/manager.go, isSyncCandidate() previously short-circuited to true for RegressionNet/SimNet, bypassing all service-flag checks. This patch moves the regtest/simnet exception to occur only after verifying the peer advertises SFNodeNetwork or SFNodeNetworkLimited and, for limited peers, that its last block is within the pruned-node threshold. The segwit deployment check now runs only for non-test networks. Tests are updated to verify service-flag behavior rather than address behavior.
Changed components
netsync/manager.go:isSyncCandidate()netsync/manager_test.go:TestIsSyncCandidateRegtestInspect captured patch +55 / −44
diff --git a/netsync/manager.go b/netsync/manager.go
index 9addc5c..e6243e4 100644
--- a/netsync/manager.go
+++ b/netsync/manager.go
@@ -360,38 +360,13 @@ func (sm *SyncManager) startSync() {
// isSyncCandidate returns whether or not the peer is a candidate to consider
// syncing from.
func (sm *SyncManager) isSyncCandidate(peer *peerpkg.Peer) bool {
- // Typically a peer is not a candidate for sync if it's not a full node,
- // however regression test is special in that the regression tool is
- // not a full node and still needs to be considered a sync candidate.
- switch sm.chainParams.Name {
- case chaincfg.RegressionNetParams.Name, chaincfg.SimNetParams.Name:
- // In regtest/simnet mode, any peer is a valid sync candidate
- // regardless of its address or service flags. This allows
- // syncing from peers on non-localhost networks such as Docker
- // bridge networks.
- return true
- }
-
- // If the segwit soft-fork package has activated, then the peer must
- // also be upgraded.
- segwitActive, err := sm.chain.IsDeploymentActive(
- chaincfg.DeploymentSegwit,
- )
- if err != nil {
- log.Errorf("Unable to query for segwit soft-fork state: %v",
- err)
- }
-
- if segwitActive && !peer.IsWitnessEnabled() {
- return false
- }
-
var (
nodeServices = peer.Services()
fullNode = nodeServices.HasFlag(wire.SFNodeNetwork)
prunedNode = nodeServices.HasFlag(wire.SFNodeNetworkLimited)
)
+ // We check the node's ability to serve blocks first.
switch {
case fullNode:
// Node is a sync candidate if it has all the blocks.
@@ -418,6 +393,29 @@ func (sm *SyncManager) isSyncCandidate(peer *peerpkg.Peer) bool {
return false
}
+ // We can skip the deployment requirement for local test networks.
+ switch sm.chainParams.Name {
+ case chaincfg.RegressionNetParams.Name, chaincfg.SimNetParams.Name:
+ // Being able to serve blocks in the range we need is the only
+ // requirement for regtest and simnet. Any light clients such as
+ // Neutrino would fail above already.
+ return true
+ }
+
+ // If the segwit soft-fork package has activated, then the peer must
+ // also be upgraded.
+ segwitActive, err := sm.chain.IsDeploymentActive(
+ chaincfg.DeploymentSegwit,
+ )
+ if err != nil {
+ log.Errorf("Unable to query for segwit soft-fork state: %v",
+ err)
+ }
+
+ if segwitActive && !peer.IsWitnessEnabled() {
+ return false
+ }
+
// Candidate if all checks passed.
return true
}
diff --git a/netsync/manager_test.go b/netsync/manager_test.go
index 7e662fc..4d304fc 100644
--- a/netsync/manager_test.go
+++ b/netsync/manager_test.go
@@ -1220,9 +1220,8 @@ func TestStartSyncChainCurrent(t *testing.T) {
"ibdMode should not be activated when chain is already current")
}
-// TestIsSyncCandidateRegtest verifies that isSyncCandidate accepts any peer
-// on regtest regardless of address, including non-localhost Docker bridge
-// addresses.
+// TestIsSyncCandidateRegtest verifies that isSyncCandidate accepts peers
+// on regtest and simnet based on their service flags.
func TestIsSyncCandidateRegtest(t *testing.T) {
t.Parallel()
@@ -1231,29 +1230,41 @@ func TestIsSyncCandidateRegtest(t *testing.T) {
defer tearDown()
tests := []struct {
- name string
- addr string
- want bool
+ name string
+ flags wire.ServiceFlag
+ lastBlock int32
+ want bool
}{
{
- name: "localhost",
- addr: "127.0.0.1:18444",
- want: true,
+ name: "just node network",
+ flags: wire.SFNodeNetwork,
+ want: true,
+ },
+ {
+ name: "just limited network",
+ flags: wire.SFNodeNetworkLimited,
+ want: true,
+ },
+ {
+ name: "limited network with block ahead",
+ flags: wire.SFNodeNetworkLimited,
+ lastBlock: wire.NodeNetworkLimitedBlockThreshold + 1,
+ want: false,
},
{
- name: "docker bridge ip",
- addr: "172.18.0.2:18444",
- want: true,
+ name: "node network and limited node network",
+ flags: wire.SFNodeNetwork | wire.SFNodeNetworkLimited,
+ want: true,
},
{
- name: "remote ip",
- addr: "93.184.216.34:18444",
- want: true,
+ name: "no flags",
+ flags: 0,
+ want: false,
},
{
- name: "ipv6 loopback",
- addr: "[::1]:18444",
- want: true,
+ name: "different flag",
+ flags: wire.SFNodeBloom,
+ want: false,
},
}
@@ -1261,7 +1272,9 @@ func TestIsSyncCandidateRegtest(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
p := peer.NewInboundPeer(&peer.Config{
ChainParams: sm.chainParams,
+ Services: tc.flags,
})
+ p.UpdateLastBlockHeight(tc.lastBlock)
got := sm.isSyncCandidate(p)
require.Equal(t, tc.want, got)
Why this scored 35/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.