diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f46e4be1..5987ee1c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.36.29] + +### Fixed +- **`/v1/health` reported a chain the node denies exists.** The `bootstrapped` check publishes `chains.Nets.Bootstrapping()` verbatim as its message, but `Nets.chains` is keyed by **net** ID and the aggregate appended the map **key**, not the chain. Every primary-network chain that failed to converge therefore surfaced as the single ID `11111111111111111111111111111111LpoYY` — `constants.PrimaryNetworkID` (`ids.Empty`) — so an operator resolving it got "there is no chain with alias/ID", and N stuck chains collapsed into one indistinguishable entry (measured on devnet/testnet: `"message":["11111111111111111111111111111111LpoYY"],"contiguousFailures":3213`). The net owns the bootstrapping set, so the net now names its own chains: new `nets.Net.Bootstrapping() []ids.ID` (`nets/net.go`), and `chains/chains.go` aggregates those instead of the keys. `TestNetsBootstrappingReportsChainsNotNets` asserts `ids.Empty` never appears and that two stuck chains are individually named; `TestNetsBootstrapping` no longer asserts the bug (it demanded the net ID). +- Ships the GET `/v1/health` encoder fix from `dada5a31` (`{"healthy":…,"error":"health reply encode failed"}` on every node — jsonv2 has no representation for `apihealth.Result.Duration`), which was on `main` but had never been tagged. + ## [1.36.13] ### Fixed diff --git a/chains/chains.go b/chains/chains.go index 4e45afe07..3917abd08 100644 --- a/chains/chains.go +++ b/chains/chains.go @@ -72,15 +72,22 @@ func (s *Nets) IsChainBootstrapped(chainID ids.ID) bool { // Bootstrapping returns the chainIDs of any chains that are still // bootstrapping. +// +// s.chains is keyed by NET id, not chain id. Reporting the key named the net +// instead of the chain: every primary-network chain that failed to converge +// surfaced in /v1/health as the single ID +// "11111111111111111111111111111111LpoYY" — constants.PrimaryNetworkID, i.e. +// ids.Empty — a "chain" the chain manager has never heard of, so the operator +// chasing it got "there is no chain with alias/ID". Worse, N stuck chains +// collapsed into one indistinguishable entry. Ask each net which of ITS chains +// are still bootstrapping; the net owns that set. func (s *Nets) Bootstrapping() []ids.ID { s.lock.RLock() defer s.lock.RUnlock() chainsBootstrapping := make([]ids.ID, 0, len(s.chains)) - for chainID, chain := range s.chains { - if !chain.IsBootstrapped() { - chainsBootstrapping = append(chainsBootstrapping, chainID) - } + for _, chain := range s.chains { + chainsBootstrapping = append(chainsBootstrapping, chain.Bootstrapping()...) } return chainsBootstrapping diff --git a/chains/chains_test.go b/chains/chains_test.go index 58e743bab..93dbfc108 100644 --- a/chains/chains_test.go +++ b/chains/chains_test.go @@ -158,12 +158,54 @@ func TestNetsBootstrapping(t *testing.T) { chain, ok := chains.GetOrCreate(netID) require.True(ok) - // Start bootstrapping + // Start bootstrapping. What comes back is the CHAIN that is syncing, never + // the net that holds it — this assertion used to demand netID, which is + // how the phantom "11111111111111111111111111111111LpoYY" survived review. chain.AddChain(chainID) bootstrapping := chains.Bootstrapping() - require.Contains(bootstrapping, netID) + require.Equal([]ids.ID{chainID}, bootstrapping) + require.NotContains(bootstrapping, netID) // Finish bootstrapping chain.Bootstrapped(chainID) require.Empty(chains.Bootstrapping()) } + +// The "bootstrapped" health check publishes Nets.Bootstrapping() verbatim as +// its message. s.chains is keyed by NET id, so returning the key reported +// constants.PrimaryNetworkID (ids.Empty, cb58 +// "11111111111111111111111111111111LpoYY") as though it were an unbootstrapped +// CHAIN. Operators saw a chain ID the chain manager denies exists, and every +// stuck chain on the net collapsed into that one phantom entry. +func TestNetsBootstrappingReportsChainsNotNets(t *testing.T) { + require := require.New(t) + + chains, err := NewNets(ids.EmptyNodeID, map[ids.ID]nets.Config{ + constants.PrimaryNetworkID: {}, + }) + require.NoError(err) + + primary, _ := chains.GetOrCreate(constants.PrimaryNetworkID) + cChainID := ids.GenerateTestID() + dChainID := ids.GenerateTestID() + primary.AddChain(cChainID) + primary.AddChain(dChainID) + + bootstrapping := chains.Bootstrapping() + + // The phantom: never the net's own ID. + require.NotContains(bootstrapping, constants.PrimaryNetworkID) + require.NotContains( + bootstrapping, + ids.Empty, + "health check reported the primary NET id as an unbootstrapped chain", + ) + // Both stuck chains must be individually nameable. + require.ElementsMatch([]ids.ID{cChainID, dChainID}, bootstrapping) + + primary.Bootstrapped(cChainID) + require.Equal([]ids.ID{dChainID}, chains.Bootstrapping()) + + primary.Bootstrapped(dChainID) + require.Empty(chains.Bootstrapping()) +} diff --git a/nets/net.go b/nets/net.go index 1c03323ea..3f4f05ecf 100644 --- a/nets/net.go +++ b/nets/net.go @@ -34,6 +34,11 @@ type Net interface { // IsBootstrapped returns true if the chains in this chain are done bootstrapping IsBootstrapped() bool + // Bootstrapping returns the IDs of the chains in this net that have NOT yet + // finished initial sync. The net owns the bootstrapping set, so it is the + // net — not its caller — that can name those chains. + Bootstrapping() []ids.ID + // IsChainBootstrapped reports whether a SPECIFIC chain in this net has finished // initial sync — i.e. Bootstrapped(chainID) was called for it (the chain reached // the network frontier and its VM went to normal operation). This is the per-chain @@ -75,6 +80,13 @@ func (s *chain) IsBootstrapped() bool { return s.bootstrapping.Len() == 0 } +func (s *chain) Bootstrapping() []ids.ID { + s.lock.RLock() + defer s.lock.RUnlock() + + return s.bootstrapping.List() +} + // IsChainBootstrapped assumes MONOTONIC per-process bootstrapped state: a chain // only ever moves bootstrapping→bootstrapped (Bootstrapped is forward-only and // AddChain refuses to re-add a chain already in either set), so this signal — and diff --git a/nets/net_test.go b/nets/net_test.go index 656dfe8fa..97a3b2698 100644 --- a/nets/net_test.go +++ b/nets/net_test.go @@ -68,3 +68,26 @@ func TestIsAllowed(t *testing.T) { require.False(s.IsAllowed(ids.GenerateTestNodeID(), false), "Non-validator should not be allowed with validator only rules and allowed nodes") require.True(s.IsAllowed(allowedNodeID, true), "Non-validator allowed node should be allowed with validator only rules and allowed nodes") } + +// Bootstrapping must name the CHAINS still syncing. The net-level aggregate +// IsBootstrapped() only says "something here is unconverged"; only the chain +// IDs say what. +func TestNetBootstrappingNamesTheChains(t *testing.T) { + require := require.New(t) + + chainID0 := ids.GenerateTestID() + chainID1 := ids.GenerateTestID() + + s := New(ids.GenerateTestNodeID(), Config{}) + require.Empty(s.Bootstrapping()) + + s.AddChain(chainID0) + s.AddChain(chainID1) + require.ElementsMatch([]ids.ID{chainID0, chainID1}, s.Bootstrapping()) + + s.Bootstrapped(chainID0) + require.Equal([]ids.ID{chainID1}, s.Bootstrapping()) + + s.Bootstrapped(chainID1) + require.Empty(s.Bootstrapping()) +} diff --git a/version/constants.go b/version/constants.go index 80036a13b..a2113fa83 100644 --- a/version/constants.go +++ b/version/constants.go @@ -77,7 +77,7 @@ var ( const ( defaultMajor = 1 defaultMinor = 36 - defaultPatch = 28 + defaultPatch = 29 ) func init() {