luxfi/node
1
0
Fork 0
mirror of https://github.com/luxfi/node.git synced 2026-07-27 03:39:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(platformvm): gate P-chain uptime state.Commit on an actual write (v1.36.28)

Browse Source 
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 <dev@hanzo.ai>
This commit is contained in:
zeekay
2026-07-24 04:20:00 -07:00
co-authored by Hanzo Dev
parent e70f687c10
commit 52de39485d
4 changed files with 84 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
version/constants.go
+1 -1
View File
@@ -77,7 +77,7 @@ var (
const (
defaultMajor = 1
defaultMinor = 36
defaultPatch = 26
defaultPatch = 28
)
func init() {
vms/platformvm/uptime_tracker.go
+21 -13
View File
@@ -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
vms/platformvm/uptime_tracker_test.go
+47 -8
View File
@@ -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:
vms/platformvm/vm.go
+15 -5
View File
@@ -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)