From 52de39485d01c8566a70ad31cdda4721f288c72e Mon Sep 17 00:00:00 2001 From: zeekay Date: Fri, 24 Jul 2026 04:20:00 -0700 Subject: [PATCH] fix(platformvm): gate P-chain uptime state.Commit on an actual write (v1.36.28) Disconnect/updateUptimeLocked now return (mutated bool, err); VM.Disconnected commits only when the flush wrote uptime state. Before StartTracking (bootstrap churn) and for non-validator peers the flush is a no-op, so the prior unconditional Commit was an empty full-write+fsync on every such disconnect. Skipping it is correct: every writer under stateLock commits its own diff, so no orphaned write depends on the disconnect path. RED round-2 verdict: SHIP-READY (H1 phantom verified, mutated-gate correct, H2 pre-existing/not-worsened, block-height orthogonal). Isolated from the uncommitted staking-KMS WIP, which is quarantined on feat/staking-kms-native pending its own Blue->Red. Co-authored-by: Hanzo Dev --- version/constants.go | 2 +- vms/platformvm/uptime_tracker.go | 34 ++++++++++------- vms/platformvm/uptime_tracker_test.go | 55 +++++++++++++++++++++++---- vms/platformvm/vm.go | 20 +++++++--- 4 files changed, 84 insertions(+), 27 deletions(-) diff --git a/version/constants.go b/version/constants.go index 034714768..80036a13b 100644 --- a/version/constants.go +++ b/version/constants.go @@ -77,7 +77,7 @@ var ( const ( defaultMajor = 1 defaultMinor = 36 - defaultPatch = 26 + defaultPatch = 28 ) func init() { diff --git a/vms/platformvm/uptime_tracker.go b/vms/platformvm/uptime_tracker.go index f1d5305d5..e3d13a2ad 100644 --- a/vms/platformvm/uptime_tracker.go +++ b/vms/platformvm/uptime_tracker.go @@ -106,14 +106,17 @@ func (t *uptimeTracker) IsConnected(nodeID ids.NodeID) bool { } // Disconnect records that [nodeID] disconnected, flushing its accrued session -// into persistent state. Flushing is a no-op before StartTracking (the session -// is not yet being measured), matching avalanchego. -func (t *uptimeTracker) Disconnect(nodeID ids.NodeID) error { +// into persistent state. It returns whether that flush actually wrote uptime +// state (a SetUptime the caller must Commit). Flushing is a no-op — mutated +// false — before StartTracking (the session is not yet being measured, matching +// avalanchego) and for a peer with no uptime record (a non-validator), so the +// caller can skip an empty state.Commit. +func (t *uptimeTracker) Disconnect(nodeID ids.NodeID) (mutated bool, err error) { t.mu.Lock() defer t.mu.Unlock() defer delete(t.connections, nodeID) if !t.startedTracking { - return nil + return false, nil } return t.updateUptimeLocked(nodeID) } @@ -131,7 +134,7 @@ func (t *uptimeTracker) StartTracking(nodeIDs []ids.NodeID) error { return errAlreadyStartedTracking } for _, nodeID := range nodeIDs { - if err := t.updateUptimeLocked(nodeID); err != nil { + if _, err := t.updateUptimeLocked(nodeID); err != nil { return err } } @@ -149,7 +152,7 @@ func (t *uptimeTracker) StopTracking(nodeIDs []ids.NodeID) error { return errNotStartedTracking } for _, nodeID := range nodeIDs { - if err := t.updateUptimeLocked(nodeID); err != nil { + if _, err := t.updateUptimeLocked(nodeID); err != nil { return err } } @@ -204,18 +207,23 @@ func (t *uptimeTracker) calculateUptimeLocked(nodeID ids.NodeID) (time.Duration, return upDuration + now.Sub(connectedAt), now, nil } -// updateUptimeLocked persists the current up-duration for [nodeID]. A node -// without a state record (i.e. not a current validator) is silently skipped, so -// tracking non-validator peers is harmless. The caller must hold t.mu (write). -func (t *uptimeTracker) updateUptimeLocked(nodeID ids.NodeID) error { +// updateUptimeLocked persists the current up-duration for [nodeID], returning +// whether it actually wrote (mutated). A node without a state record (i.e. not a +// current validator) is silently skipped — mutated false — so tracking +// non-validator peers is harmless and never dirties the state. The caller must +// hold t.mu (write). +func (t *uptimeTracker) updateUptimeLocked(nodeID ids.NodeID) (mutated bool, err error) { upDuration, lastUpdated, err := t.calculateUptimeLocked(nodeID) if errors.Is(err, database.ErrNotFound) { - return nil + return false, nil } if err != nil { - return err + return false, err } - return t.state.SetUptime(nodeID, t.netID, upDuration, lastUpdated) + if err := t.state.SetUptime(nodeID, t.netID, upDuration, lastUpdated); err != nil { + return false, err + } + return true, nil } // CalculateUptime returns (upDuration, totalDuration) for [nodeID], where diff --git a/vms/platformvm/uptime_tracker_test.go b/vms/platformvm/uptime_tracker_test.go index fc5d94807..a7b982ae9 100644 --- a/vms/platformvm/uptime_tracker_test.go +++ b/vms/platformvm/uptime_tracker_test.go @@ -185,7 +185,9 @@ func TestUptimeTrackerConnectDisconnectFlush(t *testing.T) { tracker.Connect(nodeID) now = now.Add(30 * time.Minute) - require.NoError(tracker.Disconnect(nodeID)) + mutated, err := tracker.Disconnect(nodeID) + require.NoError(err) + require.True(mutated) // a tracked validator's session flush writes uptime require.Equal(30*time.Minute, state.uptimes[nodeID]) require.False(tracker.IsConnected(nodeID)) @@ -197,6 +199,36 @@ func TestUptimeTrackerConnectDisconnectFlush(t *testing.T) { require.InDelta(0.5, pct, 0.001) } +// TestUptimeTrackerDisconnectBeforeTrackingIsNoWrite is the regression test for +// the empty-commit elimination (RED round-2 LOW). A peer disconnect during +// bootstrap — before StartTracking — must report mutated=false so VM.Disconnected +// skips an empty state.Commit (a full-write+fsync). Under bootstrap churn that +// commit ran on every disconnect for no reason. +func TestUptimeTrackerDisconnectBeforeTrackingIsNoWrite(t *testing.T) { + require := require.New(t) + + state := newFakeUptimeState() + netID := ids.GenerateTestID() + nodeID := ids.GenerateTestNodeID() + + now := time.Now().Truncate(time.Second) + clk := func() time.Time { return now } + + state.addValidator(nodeID, netID, now) + + tracker := newUptimeTracker(state, netID, clk) + // Deliberately NO StartTracking — we are still bootstrapping. + + tracker.Connect(nodeID) + now = now.Add(30 * time.Minute) + + mutated, err := tracker.Disconnect(nodeID) + require.NoError(err) + require.False(mutated) // no write → VM.Disconnected must skip Commit + require.False(tracker.IsConnected(nodeID)) + require.Equal(time.Duration(0), state.uptimes[nodeID]) // untouched +} + // TestUptimeTrackerDisconnectedValidatorGetsZero verifies that, once tracking, a // validator that never connects earns 0% uptime (it is offline from this node's // perspective) — the property that lets the reward gate withhold rewards. @@ -303,13 +335,16 @@ func TestUptimeTrackerUnknownValidator(t *testing.T) { // StartTracking must not fail on a node that has no state record. require.NoError(tracker.StartTracking([]ids.NodeID{unknownNode})) - // Connecting then disconnecting an unknown node is a no-op, not an error. + // Connecting then disconnecting an unknown node is a no-op, not an error, and + // must NOT report a state mutation (no uptime record to write). tracker.Connect(unknownNode) now = now.Add(time.Minute) - require.NoError(tracker.Disconnect(unknownNode)) + mutated, err := tracker.Disconnect(unknownNode) + require.NoError(err) + require.False(mutated) // A percent query for an unknown validator surfaces the error. - _, err := tracker.CalculateUptimePercent(unknownNode, netID) + _, err = tracker.CalculateUptimePercent(unknownNode, netID) require.Error(err) } @@ -358,7 +393,9 @@ func TestUptimeTrackerDoubleConnect(t *testing.T) { now = now.Add(5 * time.Minute) tracker.Connect(nodeID) // duplicate — must NOT reset the 5-minute-old session now = now.Add(5 * time.Minute) - require.NoError(tracker.Disconnect(nodeID)) + mutated, err := tracker.Disconnect(nodeID) + require.NoError(err) + require.True(mutated) // Ten minutes total, not five. require.Equal(10*time.Minute, state.uptimes[nodeID]) @@ -383,7 +420,8 @@ func TestUptimeTrackerRapidConnectDisconnect(t *testing.T) { // 100 cycles with no clock advance — zero accrual. for i := 0; i < 100; i++ { tracker.Connect(nodeID) - require.NoError(tracker.Disconnect(nodeID)) + _, err := tracker.Disconnect(nodeID) + require.NoError(err) } require.Equal(time.Duration(0), state.uptimes[nodeID]) @@ -391,7 +429,8 @@ func TestUptimeTrackerRapidConnectDisconnect(t *testing.T) { for i := 0; i < 50; i++ { tracker.Connect(nodeID) now = now.Add(time.Second) - require.NoError(tracker.Disconnect(nodeID)) + _, err := tracker.Disconnect(nodeID) + require.NoError(err) } require.Equal(50*time.Second, state.uptimes[nodeID]) } @@ -427,7 +466,7 @@ func TestUptimeTrackerConcurrent(t *testing.T) { case 0: tracker.Connect(nodeID) case 1: - _ = tracker.Disconnect(nodeID) + _, _ = tracker.Disconnect(nodeID) case 2: _, _ = tracker.CalculateUptimePercent(nodeID, netID) case 3: diff --git a/vms/platformvm/vm.go b/vms/platformvm/vm.go index e5038e3c7..902c56d35 100644 --- a/vms/platformvm/vm.go +++ b/vms/platformvm/vm.go @@ -844,16 +844,26 @@ func (vm *VM) Disconnected(ctx context.Context, nodeID ids.NodeID) error { // 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. + // + // Commit ONLY when the flush actually wrote uptime state. Before StartTracking + // (bootstrap churn) and for non-validator peers, Disconnect is a no-op, and an + // unconditional Commit would be an empty full-write+fsync on every such + // disconnect. A no-write disconnect leaves the state clean, so skipping Commit + // is correct — every writer under stateLock (block accept, Start/StopTracking) + // commits its own diff, so there is never an orphaned write relying on us. vm.stateLock.Lock() if vm.tracker != nil { - if err := vm.tracker.Disconnect(nodeID); err != nil { + mutated, err := vm.tracker.Disconnect(nodeID) + if err != nil { vm.stateLock.Unlock() return err } - } - if err := vm.state.Commit(); err != nil { - vm.stateLock.Unlock() - return err + if mutated { + if err := vm.state.Commit(); err != nil { + vm.stateLock.Unlock() + return err + } + } } vm.stateLock.Unlock() return vm.Network.Disconnected(ctx, nodeID)