mirror of
https://github.com/luxfi/node.git
synced 2026-07-27 03:39:39 +00:00
fix(node): serialize P-chain Connected/Disconnected + plumb real peer version (RED CRITICAL #1/#2)
The uptime event-delivery plumbing (node/chain_router.go + chains/manager.go blockHandler) dropped two invariants avalanchego's handler upholds — the consensus lock and the peer version — each a mainnet-fleet crash. The uptime tracker itself (vms/platformvm/uptime_tracker.go) is RED-cleared and UNCHANGED. CRITICAL #2 — P-chain state race (concurrent map writes -> fatal): chainRouter dispatched Connected/Disconnected on the peer-lifecycle goroutine, where VM.Disconnected -> tracker.Disconnect (state.SetUptime) + state.Commit (state.write) ran concurrently with the block acceptor's state.CommitBatch (state.write) on the engine accept goroutine — no shared lock, so ordinary peer churn triggered Go "concurrent map writes" and crashed the P-chain node. Fix: one VM-owned stateLock serializes every commit of shared platform state that originates outside the engine's lock-free VM.Accept call-out — - block DECISION: block/executor Block.Accept/Reject hold &vm.stateLock (supplied to executor.NewManager) around the whole acceptor visit; - peer/lifecycle: VM.Disconnected and the onReady/onBootstrapStarted/Shutdown Start/StopTracking uptime flushes hold vm.stateLock. This is avalanchego's ctx.Lock invariant (accept serialized with engine.Connected/Disconnected), scoped to the state the platform VM owns and implemented at the VM: the Lux engine invokes VM.Accept as a lock-free call-out (no ctx.Lock over accept exists) and a chain-agnostic blockHandler cannot reach a per-VM accept lock, so routing "through the engine" is not feasible. CRITICAL #1 — C-Chain nil-version panic on state sync: blockHandler.Connected passed connector.Connected(ctx, nodeID, nil). proposervm promotes Connected to coreth, whose state-sync peer tracker compares peer versions; a nil version deref panics any C-Chain node running state sync (a fresh join OR a validator rejoining after falling behind — the launch's core invariant). Fix: plumb the REAL peer version through a versionedConnector capability — chainRouter.Connected converts the node peer version (luxfi/node/version) to the VM boundary type (luxfi/version = chain.VersionInfo) and delivers it via blockHandler.ConnectedWithVersion -> connector.Connected. Audit: geth in-process Connected is a no-op stub (nil-safe); xvm stores the pointer without deref (nil-safe); the real coreth plugin derefs -> fixed by feeding the real version. Tests (SDKROOT + CGO_ENABLED=1, -race): - vms/platformvm/uptime_state_race_test.go: state.Commit (VM.Disconnected path) concurrent with state.CommitBatch (acceptor path) under the shared lock — no race/fatal; verified meaningful (an unlocked probe trips DATA RACE). - chains/blockhandler_connected_version_test.go + node/chain_router_connected_version_test.go: the real, converted version reaches the connector non-nil (dedup covered). Uptime tracker 13/13 and the block/executor reward gate untouched and green; go build ./... and go vet clean. Co-authored-by: Hanzo Dev <dev@hanzo.ai>
This commit is contained in:
@@ -0,0 +1,107 @@
|
||||
// Copyright (C) 2019-2025, Lux Industries Inc. All rights reserved.
|
||||
// See the file LICENSE for licensing terms.
|
||||
|
||||
package chains
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/luxfi/ids"
|
||||
"github.com/luxfi/log"
|
||||
version "github.com/luxfi/version"
|
||||
"github.com/luxfi/vm/chain"
|
||||
)
|
||||
|
||||
// recordingConnector is a chain.ChainVM that records the version it is handed on
|
||||
// Connected. Every other method comes from the embedded (nil) interface and is
|
||||
// never invoked by the connect path.
|
||||
type recordingConnector struct {
|
||||
chain.ChainVM
|
||||
|
||||
mu sync.Mutex
|
||||
called bool
|
||||
gotVersion *version.Application
|
||||
}
|
||||
|
||||
func (c *recordingConnector) Connected(_ context.Context, _ ids.NodeID, nodeVersion *chain.VersionInfo) error {
|
||||
c.mu.Lock()
|
||||
defer c.mu.Unlock()
|
||||
c.called = true
|
||||
c.gotVersion = nodeVersion
|
||||
return nil
|
||||
}
|
||||
|
||||
func (c *recordingConnector) Disconnected(context.Context, ids.NodeID) error { return nil }
|
||||
|
||||
// TestBlockHandlerConnectedWithVersionDeliversRealVersion is the regression
|
||||
// guard for RED CRITICAL #1 (C-Chain nil-version panic).
|
||||
//
|
||||
// Before the fix blockHandler.Connected forwarded connector.Connected(ctx,
|
||||
// nodeID, nil) — a hardcoded nil version. proposervm promotes Connected to the
|
||||
// inner C-Chain VM (coreth), whose state-sync peer tracker compares peer
|
||||
// versions; a nil version there dereferences nil (version.Application.Compare)
|
||||
// and PANICS a state-syncing node (a fresh join, or a validator rejoining after
|
||||
// falling behind — the launch's core invariant).
|
||||
//
|
||||
// The fix plumbs the REAL peer version through ConnectedWithVersion to the
|
||||
// connector. This test drives the real blockHandler with a real version and
|
||||
// asserts the connector receives that non-nil version — never nil.
|
||||
func TestBlockHandlerConnectedWithVersionDeliversRealVersion(t *testing.T) {
|
||||
require := require.New(t)
|
||||
|
||||
rc := &recordingConnector{}
|
||||
bh := newBlockHandler(
|
||||
nil, // BlockBuilder — unused by the connect path
|
||||
rc, // connector under test
|
||||
log.Noop(), // logger
|
||||
nil, // engine
|
||||
nil, // net
|
||||
nil, // msgCreator
|
||||
ids.Empty, // chainID
|
||||
ids.Empty, // networkID
|
||||
nil, // beacons
|
||||
ids.NodeID{}, // selfNodeID
|
||||
false, // expectsStakedBeacons
|
||||
)
|
||||
|
||||
nodeID := ids.GenerateTestNodeID()
|
||||
peerVersion := &version.Application{Name: "lux", Major: 1, Minor: 36, Patch: 27}
|
||||
|
||||
require.NoError(bh.ConnectedWithVersion(context.Background(), nodeID, peerVersion))
|
||||
|
||||
rc.mu.Lock()
|
||||
defer rc.mu.Unlock()
|
||||
require.True(rc.called, "connector.Connected must be invoked")
|
||||
require.NotNil(rc.gotVersion, "connector must receive a NON-nil version (coreth dereferences it in state-sync)")
|
||||
require.Equal("lux", rc.gotVersion.Name)
|
||||
require.Equal(1, rc.gotVersion.Major)
|
||||
require.Equal(36, rc.gotVersion.Minor)
|
||||
require.Equal(27, rc.gotVersion.Patch)
|
||||
}
|
||||
|
||||
// TestBlockHandlerConnectedDedupsButStillCarriesVersion confirms the version
|
||||
// survives the once-only dedup: the first (versioned) dispatch reaches the
|
||||
// connector; a duplicate is a no-op (not a nil-version overwrite).
|
||||
func TestBlockHandlerConnectedDedupsButStillCarriesVersion(t *testing.T) {
|
||||
require := require.New(t)
|
||||
|
||||
rc := &recordingConnector{}
|
||||
bh := newBlockHandler(nil, rc, log.Noop(), nil, nil, nil, ids.Empty, ids.Empty, nil, ids.NodeID{}, false)
|
||||
|
||||
nodeID := ids.GenerateTestNodeID()
|
||||
peerVersion := &version.Application{Name: "lux", Major: 1, Minor: 36, Patch: 27}
|
||||
|
||||
require.NoError(bh.ConnectedWithVersion(context.Background(), nodeID, peerVersion))
|
||||
// A duplicate connect (e.g. dispatched again per tracked network) must be a
|
||||
// no-op — never a second call that could clobber the stored version with nil.
|
||||
require.NoError(bh.ConnectedWithVersion(context.Background(), nodeID, nil))
|
||||
|
||||
rc.mu.Lock()
|
||||
defer rc.mu.Unlock()
|
||||
require.NotNil(rc.gotVersion, "the deduped duplicate must not overwrite the real version with nil")
|
||||
require.Equal(27, rc.gotVersion.Patch)
|
||||
}
|
||||
+29
-7
@@ -69,6 +69,7 @@ import (
|
||||
"github.com/luxfi/node/upgrade"
|
||||
"github.com/luxfi/node/vms"
|
||||
validators "github.com/luxfi/validators"
|
||||
version "github.com/luxfi/version"
|
||||
"github.com/luxfi/vm/fx"
|
||||
|
||||
// "github.com/luxfi/node/vms/metervm" // Temporarily disabled - needs consensus package updates
|
||||
@@ -4065,11 +4066,33 @@ func (b *blockHandler) GetStateSummary(ctx context.Context, nodeID ids.NodeID, r
|
||||
func (b *blockHandler) StateSummary(ctx context.Context, nodeID ids.NodeID, requestID uint32, summary []byte) error {
|
||||
return nil
|
||||
}
|
||||
// Connected forwards a peer connection to this chain's VM exactly once. The
|
||||
// P-chain VM records it in its uptime tracker; other VMs use it for their own
|
||||
// peer sets. A nil connector makes this a no-op. On a forwarding error the dedup
|
||||
// entry is rolled back so a subsequent dispatch of the same connection retries.
|
||||
|
||||
// Connected satisfies handler.Handler, whose interface carries no peer version.
|
||||
// The real delivery path is ConnectedWithVersion (chainRouter uses it via the
|
||||
// versionedConnector capability); this nil-version entry exists only for the
|
||||
// interface contract and any non-router caller.
|
||||
func (b *blockHandler) Connected(ctx context.Context, nodeID ids.NodeID) error {
|
||||
return b.connect(ctx, nodeID, nil)
|
||||
}
|
||||
|
||||
// ConnectedWithVersion forwards a peer connection WITH its real application
|
||||
// version to this chain's VM. chainRouter invokes this (detecting the
|
||||
// versionedConnector capability) so the version survives to the inner VM:
|
||||
// proposervm promotes Connected to the C-Chain (coreth), whose state-sync peer
|
||||
// tracker compares peer versions — a nil version there dereferences nil and
|
||||
// panics a state-syncing node (a fresh join OR a validator rejoining after
|
||||
// falling behind). This mirrors avalanchego, which delivers msg.NodeVersion to
|
||||
// engine.Connected.
|
||||
func (b *blockHandler) ConnectedWithVersion(ctx context.Context, nodeID ids.NodeID, nodeVersion *version.Application) error {
|
||||
return b.connect(ctx, nodeID, nodeVersion)
|
||||
}
|
||||
|
||||
// connect forwards a peer connection to this chain's VM exactly once, carrying
|
||||
// nodeVersion (nil only on the interface path). The P-chain VM records it in its
|
||||
// uptime tracker; the C-Chain/X-Chain VMs store the version for their peer sets.
|
||||
// A nil connector makes this a no-op. On a forwarding error the dedup entry is
|
||||
// rolled back so a subsequent dispatch of the same connection retries.
|
||||
func (b *blockHandler) connect(ctx context.Context, nodeID ids.NodeID, nodeVersion *version.Application) error {
|
||||
if b.connector == nil {
|
||||
return nil
|
||||
}
|
||||
@@ -4081,8 +4104,7 @@ func (b *blockHandler) Connected(ctx context.Context, nodeID ids.NodeID) error {
|
||||
b.connectedNodes.Add(nodeID)
|
||||
b.connMu.Unlock()
|
||||
|
||||
// The node's p2p layer ignores the version argument; pass nil.
|
||||
if err := b.connector.Connected(ctx, nodeID, nil); err != nil {
|
||||
if err := b.connector.Connected(ctx, nodeID, nodeVersion); err != nil {
|
||||
b.connMu.Lock()
|
||||
b.connectedNodes.Remove(nodeID)
|
||||
b.connMu.Unlock()
|
||||
@@ -4106,7 +4128,7 @@ func (b *blockHandler) Disconnected(ctx context.Context, nodeID ids.NodeID) erro
|
||||
|
||||
return b.connector.Disconnected(ctx, nodeID)
|
||||
}
|
||||
func (b *blockHandler) HealthCheck(ctx context.Context) (interface{}, error) { return nil, nil }
|
||||
func (b *blockHandler) HealthCheck(ctx context.Context) (interface{}, error) { return nil, nil }
|
||||
func (b *blockHandler) Stop(ctx context.Context) {
|
||||
if b.pollerCancel != nil {
|
||||
b.pollerCancel()
|
||||
|
||||
+40
-2
@@ -14,9 +14,10 @@ import (
|
||||
"github.com/luxfi/math/set"
|
||||
"github.com/luxfi/node/message"
|
||||
"github.com/luxfi/node/proto/p2p"
|
||||
"github.com/luxfi/node/trace"
|
||||
"github.com/luxfi/node/version"
|
||||
"github.com/luxfi/timer"
|
||||
"github.com/luxfi/node/trace"
|
||||
luxversion "github.com/luxfi/version"
|
||||
)
|
||||
|
||||
// router implements Router interface for routing messages to chain handlers
|
||||
@@ -193,6 +194,32 @@ func (r *chainRouter) AddChain(ctx context.Context, chainID ids.ID, h handler.Ha
|
||||
)
|
||||
}
|
||||
|
||||
// versionedConnector is the capability a chain handler advertises when it can
|
||||
// forward a peer's REAL application version to its VM. The consensus
|
||||
// handler.Handler.Connected signature carries only the nodeID (the version was
|
||||
// dropped at that boundary); a handler that implements this receives the real
|
||||
// version instead. blockHandler implements it. The version must survive to the
|
||||
// inner VM: the C-Chain (coreth) state-sync peer tracker compares peer versions
|
||||
// and dereferences a nil version, panicking a state-syncing node.
|
||||
type versionedConnector interface {
|
||||
ConnectedWithVersion(ctx context.Context, nodeID ids.NodeID, nodeVersion *luxversion.Application) error
|
||||
}
|
||||
|
||||
// toAppVersion converts the node's peer version (github.com/luxfi/node/version)
|
||||
// to the github.com/luxfi/version.Application the VM Connected boundary
|
||||
// (chain.VersionInfo) expects. nil-safe: a nil peer version maps to nil.
|
||||
func toAppVersion(v *version.Application) *luxversion.Application {
|
||||
if v == nil {
|
||||
return nil
|
||||
}
|
||||
return &luxversion.Application{
|
||||
Name: v.Name,
|
||||
Major: v.Major,
|
||||
Minor: v.Minor,
|
||||
Patch: v.Patch,
|
||||
}
|
||||
}
|
||||
|
||||
func (r *chainRouter) Connected(nodeID ids.NodeID, nodeVersion *version.Application, netID ids.ID) {
|
||||
r.lock.Lock()
|
||||
r.connectedPeers.Add(nodeID)
|
||||
@@ -213,8 +240,19 @@ func (r *chainRouter) Connected(nodeID ids.NodeID, nodeVersion *version.Applicat
|
||||
// repeated dispatch of the same connection (once per tracked network) is
|
||||
// safe. Dispatch OUTSIDE the router lock: a handler must never re-enter the
|
||||
// router while we hold it.
|
||||
//
|
||||
// Deliver the REAL peer version through the versionedConnector capability so
|
||||
// it reaches the inner VM (proposervm → coreth state-sync). Only handlers
|
||||
// that cannot carry a version fall back to the plain nodeID-only Connected.
|
||||
appVersion := toAppVersion(nodeVersion)
|
||||
for _, h := range handlers {
|
||||
if err := h.Connected(context.Background(), nodeID); err != nil {
|
||||
var err error
|
||||
if vc, ok := h.(versionedConnector); ok {
|
||||
err = vc.ConnectedWithVersion(context.Background(), nodeID, appVersion)
|
||||
} else {
|
||||
err = h.Connected(context.Background(), nodeID)
|
||||
}
|
||||
if err != nil {
|
||||
r.log.Debug("chain handler Connected failed",
|
||||
log.Stringer("nodeID", nodeID),
|
||||
log.Err(err),
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
// Copyright (C) 2019-2025, Lux Industries Inc. All rights reserved.
|
||||
// See the file LICENSE for licensing terms.
|
||||
|
||||
package node
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/luxfi/consensus/networking/handler"
|
||||
"github.com/luxfi/constants"
|
||||
"github.com/luxfi/ids"
|
||||
"github.com/luxfi/log"
|
||||
"github.com/luxfi/math/set"
|
||||
"github.com/luxfi/node/version"
|
||||
luxversion "github.com/luxfi/version"
|
||||
)
|
||||
|
||||
// fakeVersionedHandler implements handler.Handler AND the versionedConnector
|
||||
// capability, recording which path the router used and the version delivered.
|
||||
type fakeVersionedHandler struct {
|
||||
mu sync.Mutex
|
||||
versionedHit bool
|
||||
plainHit bool
|
||||
gotAppVersion *luxversion.Application
|
||||
}
|
||||
|
||||
func (h *fakeVersionedHandler) HandleInbound(context.Context, handler.Message) error { return nil }
|
||||
func (h *fakeVersionedHandler) HandleOutbound(context.Context, handler.Message) error { return nil }
|
||||
|
||||
func (h *fakeVersionedHandler) Connected(context.Context, ids.NodeID) error {
|
||||
h.mu.Lock()
|
||||
defer h.mu.Unlock()
|
||||
h.plainHit = true
|
||||
return nil
|
||||
}
|
||||
|
||||
func (h *fakeVersionedHandler) Disconnected(context.Context, ids.NodeID) error { return nil }
|
||||
|
||||
func (h *fakeVersionedHandler) ConnectedWithVersion(_ context.Context, _ ids.NodeID, v *luxversion.Application) error {
|
||||
h.mu.Lock()
|
||||
defer h.mu.Unlock()
|
||||
h.versionedHit = true
|
||||
h.gotAppVersion = v
|
||||
return nil
|
||||
}
|
||||
|
||||
// TestChainRouterConnectedDeliversConvertedVersion is the router half of the
|
||||
// RED CRITICAL #1 fix: chainRouter.Connected must deliver the REAL peer version
|
||||
// to a version-capable handler, converting it from the node's peer version type
|
||||
// (github.com/luxfi/node/version) to the VM boundary type
|
||||
// (github.com/luxfi/version, aka chain.VersionInfo). The old code dropped the
|
||||
// version at dispatch (h.Connected(ctx, nodeID)); the fix routes through the
|
||||
// versionedConnector capability so the real, converted version survives.
|
||||
func TestChainRouterConnectedDeliversConvertedVersion(t *testing.T) {
|
||||
require := require.New(t)
|
||||
|
||||
h := &fakeVersionedHandler{}
|
||||
chainID := ids.GenerateTestID()
|
||||
|
||||
r := &chainRouter{
|
||||
log: log.Noop(),
|
||||
chains: map[ids.ID]handler.Handler{chainID: h},
|
||||
connectedPeers: set.NewSet[ids.NodeID](1),
|
||||
}
|
||||
|
||||
nodeID := ids.GenerateTestNodeID()
|
||||
peerVersion := &version.Application{Name: "lux", Major: 1, Minor: 36, Patch: 27}
|
||||
|
||||
r.Connected(nodeID, peerVersion, constants.PrimaryNetworkID)
|
||||
|
||||
h.mu.Lock()
|
||||
defer h.mu.Unlock()
|
||||
require.True(h.versionedHit, "router must use the versioned capability path for a version-capable handler")
|
||||
require.False(h.plainHit, "router must NOT fall back to the nil-version plain Connected")
|
||||
require.NotNil(h.gotAppVersion, "handler must receive a non-nil converted version")
|
||||
require.Equal("lux", h.gotAppVersion.Name)
|
||||
require.Equal(1, h.gotAppVersion.Major)
|
||||
require.Equal(36, h.gotAppVersion.Minor)
|
||||
require.Equal(27, h.gotAppVersion.Patch)
|
||||
}
|
||||
|
||||
// TestToAppVersionNilSafe documents that a nil peer version converts to nil
|
||||
// (never a panic) — the conversion is defensive at the boundary.
|
||||
func TestToAppVersionNilSafe(t *testing.T) {
|
||||
require.Nil(t, toAppVersion(nil))
|
||||
}
|
||||
@@ -87,10 +87,24 @@ func (b *Block) Verify(ctx context.Context) error {
|
||||
}
|
||||
|
||||
func (b *Block) Accept(context.Context) error {
|
||||
// Serialize the block's state commit (acceptor → state.Apply/CommitBatch →
|
||||
// state.write) with the VM's OTHER writers of the same shared state — the
|
||||
// peer Disconnect and Start/StopTracking uptime flushes, which run on
|
||||
// goroutines the consensus engine does not serialize against accept (it
|
||||
// invokes VM.Accept as a lock-free call-out). Holding the VM's stateLock for
|
||||
// the whole visit makes accept atomic w.r.t. those commits, closing the
|
||||
// concurrent-map-write in state.write(). Mirrors avalanchego's ctx.Lock,
|
||||
// which serializes block accept with engine.Connected/Disconnected.
|
||||
b.manager.stateLock.Lock()
|
||||
defer b.manager.stateLock.Unlock()
|
||||
return b.Visit(b.manager.acceptor)
|
||||
}
|
||||
|
||||
func (b *Block) Reject(context.Context) error {
|
||||
// Held under the same lock as Accept so a block DECISION (accept or reject)
|
||||
// is uniformly serialized with the VM's peer-lifecycle state commits.
|
||||
b.manager.stateLock.Lock()
|
||||
defer b.manager.stateLock.Unlock()
|
||||
return b.Visit(b.manager.rejector)
|
||||
}
|
||||
|
||||
|
||||
@@ -7,8 +7,8 @@ import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"sync"
|
||||
|
||||
"github.com/luxfi/vm/chain"
|
||||
"github.com/luxfi/ids"
|
||||
"github.com/luxfi/log"
|
||||
"github.com/luxfi/math/set"
|
||||
@@ -20,6 +20,7 @@ import (
|
||||
"github.com/luxfi/node/vms/platformvm/txs/fee"
|
||||
"github.com/luxfi/node/vms/platformvm/validators"
|
||||
"github.com/luxfi/node/vms/txs/mempool"
|
||||
"github.com/luxfi/vm/chain"
|
||||
)
|
||||
|
||||
var (
|
||||
@@ -57,6 +58,15 @@ func NewManager(
|
||||
s state.State,
|
||||
txExecutorBackend *executor.Backend,
|
||||
validatorManager validators.Manager,
|
||||
// stateLock serializes a block's accept/reject state commit with the VM's
|
||||
// OTHER writers of the same shared state — the peer-lifecycle (Disconnect)
|
||||
// and normal-ops (Start/StopTracking) uptime flushes, which run on
|
||||
// goroutines the consensus engine does not serialize. The platform VM owns
|
||||
// it and holds the SAME lock in those paths; without it a peer disconnect's
|
||||
// state.Commit races a block accept's state.CommitBatch inside state.write()
|
||||
// (concurrent Go map writes → fatal). Must be non-nil (the VM always
|
||||
// supplies &vm.stateLock).
|
||||
stateLock *sync.Mutex,
|
||||
) Manager {
|
||||
lastAccepted := s.GetLastAccepted()
|
||||
backend := &backend{
|
||||
@@ -81,6 +91,7 @@ func NewManager(
|
||||
preferred: lastAccepted,
|
||||
txExecutorBackend: txExecutorBackend,
|
||||
validatorManager: validatorManager,
|
||||
stateLock: stateLock,
|
||||
Log: log.Noop(),
|
||||
}
|
||||
}
|
||||
@@ -93,7 +104,10 @@ type manager struct {
|
||||
preferred ids.ID
|
||||
txExecutorBackend *executor.Backend
|
||||
validatorManager validators.Manager
|
||||
Log log.Logger
|
||||
// stateLock serializes block accept/reject (Block.Accept/Reject) with the
|
||||
// VM's peer-lifecycle / normal-ops state commits. See NewManager.
|
||||
stateLock *sync.Mutex
|
||||
Log log.Logger
|
||||
}
|
||||
|
||||
func (m *manager) GetBlock(blkID ids.ID) (chain.Block, error) {
|
||||
|
||||
@@ -0,0 +1,150 @@
|
||||
// Copyright (C) 2019-2025, Lux Industries Inc. All rights reserved.
|
||||
// See the file LICENSE for licensing terms.
|
||||
|
||||
package platformvm
|
||||
|
||||
import (
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/luxfi/constants"
|
||||
"github.com/luxfi/database/memdb"
|
||||
"github.com/luxfi/log"
|
||||
"github.com/luxfi/metric"
|
||||
"github.com/luxfi/runtime"
|
||||
validators "github.com/luxfi/validators"
|
||||
|
||||
"github.com/luxfi/node/upgrade"
|
||||
"github.com/luxfi/node/vms/platformvm/config"
|
||||
"github.com/luxfi/node/vms/platformvm/genesis/genesistest"
|
||||
platformvmmetrics "github.com/luxfi/node/vms/platformvm/metrics"
|
||||
"github.com/luxfi/node/vms/platformvm/reward"
|
||||
"github.com/luxfi/node/vms/platformvm/state"
|
||||
)
|
||||
|
||||
// newRaceTestState builds a REAL platform state (state.New, memdb + genesis) —
|
||||
// the same constructor the VM uses. The genesis carries a validator set, so
|
||||
// state.write (invoked by BOTH state.Commit and state.CommitBatch) iterates
|
||||
// non-empty validator/metadata maps: exactly the shared maps that mutate
|
||||
// concurrently in the fatal this test guards against.
|
||||
func newRaceTestState(t *testing.T) state.State {
|
||||
t.Helper()
|
||||
require := require.New(t)
|
||||
|
||||
reg := metric.NewRegistry()
|
||||
m, err := platformvmmetrics.New(reg)
|
||||
require.NoError(err)
|
||||
|
||||
execCfg, err := config.GetConfig(nil)
|
||||
require.NoError(err)
|
||||
|
||||
st, err := state.New(
|
||||
memdb.New(),
|
||||
genesistest.NewBytes(t, genesistest.Config{}),
|
||||
reg,
|
||||
validators.NewManager(),
|
||||
upgrade.GetConfig(constants.UnitTestID),
|
||||
execCfg,
|
||||
&runtime.Runtime{
|
||||
NetworkID: constants.UnitTestID,
|
||||
ChainID: constants.PlatformChainID,
|
||||
Log: log.Noop(),
|
||||
},
|
||||
m,
|
||||
reward.NewCalculator(reward.Config{
|
||||
MaxConsumptionRate: 120_000,
|
||||
MinConsumptionRate: 100_000,
|
||||
MintingPeriod: 365 * 24 * time.Hour,
|
||||
SupplyCap: 720 * constants.MegaLux,
|
||||
}),
|
||||
)
|
||||
require.NoError(err)
|
||||
return st
|
||||
}
|
||||
|
||||
// TestStateCommitSerializedWithAcceptNoRace is the regression guard for RED
|
||||
// CRITICAL #2 (P-chain state race).
|
||||
//
|
||||
// Before the fix the event-delivery plumbing drove VM.Disconnected on the
|
||||
// node's peer-lifecycle goroutine, where it called state.Commit() (→ state.write)
|
||||
// concurrently with the block acceptor's state.CommitBatch() (→ state.write) on
|
||||
// the consensus accept goroutine. Both mutate the shared currentValidator /
|
||||
// staker / metadata maps with no common lock → Go "concurrent map writes" FATAL
|
||||
// on ordinary peer churn.
|
||||
//
|
||||
// The fix installs ONE lock — the platform VM's stateLock — held by BOTH sides:
|
||||
// - block DECISION: block/executor Block.Accept/Reject hold &vm.stateLock
|
||||
// (supplied to executor.NewManager), around the whole acceptor visit, and
|
||||
// - peer/lifecycle commits: VM.Disconnected and the Start/StopTracking uptime
|
||||
// flushes hold vm.stateLock directly.
|
||||
//
|
||||
// This test drives the two REAL racing state operations — state.Commit()
|
||||
// (VM.Disconnected's exact call) and state.SetUptime()+state.CommitBatch()
|
||||
// (the tracker-flush + acceptor's exact calls) — from two goroutines, each
|
||||
// holding the SAME lock the fix installs. Under `-race` it must complete with
|
||||
// no data race and no fatal. Remove the shared lock and `-race` immediately
|
||||
// reports the concurrent write inside state.write() (the regression).
|
||||
func TestStateCommitSerializedWithAcceptNoRace(t *testing.T) {
|
||||
st := newRaceTestState(t)
|
||||
|
||||
// The single serializer the fix installs. In production this is
|
||||
// &vm.stateLock, held by Block.Accept (via the executor manager) and by
|
||||
// VM.Disconnected/Start/StopTracking.
|
||||
var stateLock sync.Mutex
|
||||
|
||||
// A genesis validator so SetUptime writes a real record (mirrors
|
||||
// tracker.Disconnect → state.SetUptime before state.Commit).
|
||||
nodeID := genesistest.DefaultNodeIDs[0]
|
||||
netID := constants.PrimaryNetworkID
|
||||
|
||||
const iterations = 300
|
||||
var (
|
||||
wg sync.WaitGroup
|
||||
acceptErr error
|
||||
disconnectErr error
|
||||
)
|
||||
wg.Add(2)
|
||||
|
||||
// Accept side: the acceptor's state.CommitBatch() + state.Abort() — the exact
|
||||
// calls block/executor acceptor.standardBlock makes, both routed through
|
||||
// state.write(). Block.Accept holds stateLock around this.
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
for i := 0; i < iterations; i++ {
|
||||
stateLock.Lock()
|
||||
_, err := st.CommitBatch()
|
||||
st.Abort()
|
||||
stateLock.Unlock()
|
||||
if err != nil {
|
||||
acceptErr = err
|
||||
return
|
||||
}
|
||||
}
|
||||
}()
|
||||
|
||||
// Disconnect side: tracker.Disconnect (state.SetUptime, error ignored exactly
|
||||
// as the tracker's updateUptimeLocked ignores ErrNotFound) followed by
|
||||
// state.Commit() — VM.Disconnected's exact sequence. Holds the SAME stateLock.
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
for i := 0; i < iterations; i++ {
|
||||
stateLock.Lock()
|
||||
_ = st.SetUptime(nodeID, netID, time.Duration(i)*time.Second, time.Unix(int64(i), 0))
|
||||
err := st.Commit()
|
||||
stateLock.Unlock()
|
||||
if err != nil {
|
||||
disconnectErr = err
|
||||
return
|
||||
}
|
||||
}
|
||||
}()
|
||||
|
||||
wg.Wait()
|
||||
|
||||
require := require.New(t)
|
||||
require.NoError(acceptErr)
|
||||
require.NoError(disconnectErr)
|
||||
}
|
||||
+81
-23
@@ -100,6 +100,23 @@ type VM struct {
|
||||
chainID ids.ID
|
||||
state state.State
|
||||
|
||||
// stateLock serializes every commit of the shared platform state (state.write)
|
||||
// that originates OUTSIDE the consensus engine's serialized accept path:
|
||||
// - a block DECISION (block/executor Block.Accept/Reject → state.CommitBatch);
|
||||
// the executor manager holds THIS lock (passed as &vm.stateLock),
|
||||
// - a peer Disconnect (tracker.Disconnect → state.SetUptime, then state.Commit),
|
||||
// - normal-ops Start/StopTracking uptime flushes (state.SetUptime + state.Commit).
|
||||
// The engine invokes VM.Accept as a lock-free call-out (its t.mu is released
|
||||
// before the call-out per its lock discipline), and peer connect/disconnect run
|
||||
// on the node's peer-lifecycle goroutine, so nothing else serializes these
|
||||
// writers against one another. Without this lock a disconnect's state.Commit
|
||||
// races an accept's state.CommitBatch inside state.write() → Go "concurrent map
|
||||
// writes" fatal. This is the avalanchego ctx.Lock invariant (accept serialized
|
||||
// with engine.Connected/Disconnected), scoped to the state the platform VM owns.
|
||||
// It is DISTINCT from vm.lock (which guards API/service reads): a separate lock
|
||||
// keeps the accept path off the API lock and avoids any ordering coupling.
|
||||
stateLock sync.Mutex
|
||||
|
||||
fx fx.Fx
|
||||
|
||||
// Bootstrapped remembers if this chain has finished bootstrapping or not
|
||||
@@ -302,6 +319,7 @@ func (vm *VM) Initialize(
|
||||
vm.state,
|
||||
txExecutorBackend,
|
||||
validatorManager,
|
||||
&vm.stateLock,
|
||||
)
|
||||
|
||||
txVerifier := network.NewLockedTxVerifier(&vm.lock, vm.manager)
|
||||
@@ -542,12 +560,21 @@ func (vm *VM) onBootstrapStarted() error {
|
||||
|
||||
// On a normal-ops → re-bootstrap transition, flush and stop uptime tracking
|
||||
// so connected sessions are persisted before we stop measuring. This is a
|
||||
// no-op on the first bootstrap (tracking hasn't started yet).
|
||||
if vm.tracker != nil && vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StopTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
// no-op on the first bootstrap (tracking hasn't started yet). StopTracking
|
||||
// flushes uptime into shared state (state.SetUptime → state.write), so hold
|
||||
// stateLock to serialize it with any block accept still in flight.
|
||||
if err := func() error {
|
||||
vm.stateLock.Lock()
|
||||
defer vm.stateLock.Unlock()
|
||||
if vm.tracker != nil && vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StopTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}(); err != nil {
|
||||
return err
|
||||
}
|
||||
return vm.fx.Bootstrapping()
|
||||
}
|
||||
@@ -569,18 +596,27 @@ func (vm *VM) onReady() error {
|
||||
// were already captured); StartTracking baselines every current validator's
|
||||
// uptime record and switches the tracker into live-tracking mode. Mirrors
|
||||
// avalanchego's onNormalOperationsStarted.
|
||||
if vm.tracker != nil && !vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StartTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
//
|
||||
// StartTracking (state.SetUptime) and the trailing state.Commit both write
|
||||
// shared state, so hold stateLock across them to serialize with any block
|
||||
// accept (Block.Accept holds the same lock) — the same guard as the peer
|
||||
// Disconnect path.
|
||||
if err := func() error {
|
||||
vm.stateLock.Lock()
|
||||
defer vm.stateLock.Unlock()
|
||||
if vm.tracker != nil && !vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StartTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
}
|
||||
vm.log.Info("uptime tracking started for primary network",
|
||||
log.Int("validators", len(primaryVdrIDs)))
|
||||
}
|
||||
vm.log.Info("uptime tracking started for primary network",
|
||||
log.Int("validators", len(primaryVdrIDs)))
|
||||
}
|
||||
|
||||
// Commit state BEFORE starting background goroutines to avoid race conditions
|
||||
// between state readers (forwardNotifications) and state writers (Commit)
|
||||
if err := vm.state.Commit(); err != nil {
|
||||
// Commit state BEFORE starting background goroutines to avoid race conditions
|
||||
// between state readers (forwardNotifications) and state writers (Commit)
|
||||
return vm.state.Commit()
|
||||
}(); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -618,15 +654,24 @@ func (vm *VM) Shutdown(context.Context) error {
|
||||
vm.onShutdownCtxCancel()
|
||||
|
||||
// Flush uptime for all primary-network validators before closing state, so
|
||||
// connected sessions are durably persisted across the restart.
|
||||
if vm.tracker != nil && vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StopTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := vm.state.Commit(); err != nil {
|
||||
return err
|
||||
// connected sessions are durably persisted across the restart. StopTracking
|
||||
// (state.SetUptime) + state.Commit write shared state, so hold stateLock to
|
||||
// serialize with any block accept still draining as the chain stops.
|
||||
if err := func() error {
|
||||
vm.stateLock.Lock()
|
||||
defer vm.stateLock.Unlock()
|
||||
if vm.tracker != nil && vm.tracker.StartedTracking() {
|
||||
primaryVdrIDs := vm.Validators.GetValidatorIDs(constants.PrimaryNetworkID)
|
||||
if err := vm.tracker.StopTracking(primaryVdrIDs); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := vm.state.Commit(); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}(); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
var errs []error
|
||||
@@ -790,14 +835,27 @@ func (vm *VM) Connected(ctx context.Context, nodeID ids.NodeID, nodeVersion *cha
|
||||
}
|
||||
|
||||
func (vm *VM) Disconnected(ctx context.Context, nodeID ids.NodeID) error {
|
||||
// This runs on the node's peer-lifecycle goroutine, which the consensus
|
||||
// engine does NOT serialize against block accept. tracker.Disconnect flushes
|
||||
// the peer's uptime into shared state (state.SetUptime) and state.Commit
|
||||
// persists the whole diff (state.write). Hold stateLock so both are atomic
|
||||
// w.r.t. a concurrent block accept (Block.Accept holds the same lock);
|
||||
// otherwise state.write races the acceptor's state.write → concurrent map
|
||||
// writes fatal. The p2p Network.Disconnected below touches only the p2p peer
|
||||
// set (its own lock), so it stays OUTSIDE stateLock to keep the critical
|
||||
// section to the state commit.
|
||||
vm.stateLock.Lock()
|
||||
if vm.tracker != nil {
|
||||
if err := vm.tracker.Disconnect(nodeID); err != nil {
|
||||
vm.stateLock.Unlock()
|
||||
return err
|
||||
}
|
||||
}
|
||||
if err := vm.state.Commit(); err != nil {
|
||||
vm.stateLock.Unlock()
|
||||
return err
|
||||
}
|
||||
vm.stateLock.Unlock()
|
||||
return vm.Network.Disconnected(ctx, nodeID)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user