Remove bootstrap nodes and replace with operator peers#3909
Remove bootstrap nodes and replace with operator peers#3909lrsaturnino wants to merge 25 commits intomainfrom
Conversation
Replace all Boar bootstrap node entries with curated DNS-backed operator peers to complete the bootstrap infrastructure decommission. This is the final phase — Staked nodes were removed in v2.5.1. Peer list changes: - Mainnet: 2 Boar entries replaced with 5 operator peers at keep-nodes.io - Testnet: 1 Boar entry replaced with 2 operator peers at test.keep-nodes.io - All entries use /dns4/ format for IP change resilience Security — AllowList decoupling: - Pass firewall.EmptyAllowList instead of extracting embedded peer keys - All peers (including embedded operators) now pass IsRecognized() staking checks with no firewall bypass - Remove dead ExtractPeersPublicKeys function and its tests Deprecations and renames: - Deprecate --network.bootstrap flag with runtime warning - Rename connected_bootstrap_count metric to connected_wellknown_peers_count Documentation: - Add operator migration guide covering Boar address removal, --network.peers override behavior, and monitoring updates
lrsaturnino
changed the title
Operator migration guidance will be distributed separately from the code release.
Replace placeholder mainnet entries with the beta staker node (143.198.18.229:3919) as the sole embedded peer for initial testing. Testnet placeholders remain until operator coordination is complete.
gotestsum's default `./...` package pattern only applies when no args are passed after `--`. With `-- -timeout 15m`, it forwards args directly to `go test`, which defaults to `.` (root package only). The root package has no test files, so CI has been silently running 0 tests.
Replace placeholder hostnames with actual values from config/_peers/testnet.
Remove TestConnectedWellknownPeersCountMetricName and TestMetricConstants which only assert that string constants equal themselves. The compiler already ensures rename safety. Keep the callable integration test which validates the function exists and executes without panicking.
…d var Change EmptyAllowList from an exported mutable package-level var to an exported function returning the package-level singleton. This prevents external code from accidentally mutating the shared empty allowlist.
Add a note that connected_wellknown_peers_count was previously named connected_bootstrap_count, so operators can update Prometheus queries and Grafana dashboards accordingly.
Specify concrete removal version so the deprecated flag does not linger indefinitely.
Add a TODO comment noting that at least one additional mainnet peer across a different operator/ASN should be added before production rollout to avoid a single point of failure for initial peer discovery.
Go 1.24 vet rejects non-constant format strings in fmt.Errorf. This pre-existing issue was hidden because CI was not running tests.
## Summary - CI has been silently running **0 tests** because `gotestsum -- -timeout 15m` (without `./...`) only tests the root package, which has no test files - Fix: add explicit `./...` so all subpackages are tested - Also fix `peers_test.go` placeholder hostnames to match actual `config/_peers/testnet` values — this test failure was hidden by the above bug ## Root cause `gotestsum`'s default `./...` package pattern only applies when **no args** are passed after `--`. With `-- -timeout 15m`, gotestsum forwards args directly to `go test`, which defaults to `.` (current directory only). CI log confirms: `DONE 0 tests in 8.107s`. ## Test plan - [ ] CI should now run all Go tests (expect 100+ tests instead of 0) - [ ] `TestResolvePeers/sepolia_network` should pass with corrected hostnames 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gotestsum's default `./...` package pattern only applies when no args are passed after `--`. With `-- -timeout 15m`, it forwards args directly to `go test`, which defaults to `.` (root package only). The root package has no test files, so CI has been silently running 0 tests.
Replace placeholder hostnames with actual values from config/_peers/testnet.
Remove TestConnectedWellknownPeersCountMetricName and TestMetricConstants which only assert that string constants equal themselves. The compiler already ensures rename safety. Keep the callable integration test which validates the function exists and executes without panicking.
…d var Change EmptyAllowList from an exported mutable package-level var to an exported function returning the package-level singleton. This prevents external code from accidentally mutating the shared empty allowlist.
Add a note that connected_wellknown_peers_count was previously named connected_bootstrap_count, so operators can update Prometheus queries and Grafana dashboards accordingly.
Specify concrete removal version so the deprecated flag does not linger indefinitely.
Add a TODO comment noting that at least one additional mainnet peer across a different operator/ASN should be added before production rollout to avoid a single point of failure for initial peer discovery.
Go 1.24 vet rejects non-constant format strings in fmt.Errorf. This pre-existing issue was hidden because CI was not running tests.
## Summary - CI has been silently running **0 tests** because `gotestsum -- -timeout 15m` (without `./...`) only tests the root package, which has no test files - Fix: add explicit `./...` so all subpackages are tested - Also fix `peers_test.go` placeholder hostnames to match actual `config/_peers/testnet` values — this test failure was hidden by the above bug ## Root cause `gotestsum`'s default `./...` package pattern only applies when **no args** are passed after `--`. With `-- -timeout 15m`, gotestsum forwards args directly to `go test`, which defaults to `.` (current directory only). CI log confirms: `DONE 0 tests in 8.107s`. ## Test plan - [ ] CI should now run all Go tests (expect 100+ tests instead of 0) - [ ] `TestResolvePeers/sepolia_network` should pass with corrected hostnames 🤖 Generated with [Claude Code](https://claude.com/claude-code)
piotr-roslaniec
force-pushed
the
feature/decouple-firewall-allowlist
branch
from
16ace1b to
5a6bb93
Compare
…com/threshold-network/keep-core into feature/decouple-firewall-allowlist
The count() loop could call close(watcher.channel) on consecutive ticks before the WatchBlocks cleanup goroutine removed the cancelled watcher from the list, causing a "close of closed channel" panic. Use sync.Once to guarantee the channel is closed exactly once.
|
I strongly advise against this change. Bootstrap nodes in the tBTC client are not just an address book. They work in a relay mode, improving the dissemination of network messages. Given the specificity of the tECDSA algorithm used by tBTC nodes, the delivery of messages on time is essential, and messages are dropped by nodes when CPU load is high executing tECDSA steps. This is how libp2p floodsub works, and this is how we designed message delivery buffers in the client. Another symptom that is clearly visible in metrics is the number of connections being dropped when executing CPU-intensive tECDSA steps. Bootstrap nodes work on a high-availability infrastructure, do not participate in tECDSA, and provide a stable entrance point to the network. If several operator nodes lose their in-memory address books for any reason (could be, for example, a high CPU load leading to a restart or panic in the algorithm), they will find HA bootstrap immediately and recover the peer list in a matter of seconds. Eliminating bootstrap nodes should be preceded by changing the network layer design, probably moving to gossip mode, and performing load tests on a larger network resembling mainnet conditions. We made an attempt at this architecture change in the past, and it was not a trivial endeavour. |
|
I agree with @pdyraga here. The current network architecture has been battle-tested over a long period of time. It went through months of iterative testing on testnet and then further validation in production. This setup is not accidental, it reflects a lot of learnings around reliability and behavior under load. Bootstrap nodes seem to play a role beyond just peer discovery, especially when it comes to message propagation and overall network stability during heavy tECDSA workloads. Removing them without a proven alternative introduces a real risk. I strongly recommend against changing such a critical part of the system without thorough validation. At minimum, this should be backed by extensive testing and observation in controlled environments and then gradual rollout with production monitoring. It’s also worth noting that the testnet is no longer maintained and has effectively been shut down, which makes it even harder to properly validate changes like this before exposing them to mainnet conditions. In short, I’m in favor of the security improvements, but removing bootstrap infrastructure at this stage feels premature without stronger evidence and testing. |
5 participants
Problem
The network relies on centrally-managed bootstrap nodes for initial peer discovery. Their embedded public keys bypass firewall
IsRecognized()staking checks via the AllowList, meaning an unstaked or slashed embedded peer retains permanent network access. Operators hardcoding bootstrap addresses in--network.peerswill lose connectivity when bootstrap infrastructure is decommissioned.Solution
/dns4/or/ip4/format)firewall.EmptyAllowListso all peers passIsRecognized()staking checksExtractPeersPublicKeysfunction--network.bootstrapflag with runtime warningconnected_bootstrap_countmetric toconnected_wellknown_peers_countTests
TestValidate_EmptyAllowList_RecognizedPeerAccepted— recognized peer passes via IsRecognized pathTestValidate_EmptyAllowList_UnrecognizedPeerRejected— unrecognized peer rejected with no AllowList bypassTestValidate_EmptyAllowList_PreviouslyAllowlistedPeerMustPassIsRecognized— previously allowlisted peer no longer bypasses checksTestResolvePeers— updated expectations for new operator peer entries (mainnet and testnet)TestNetworkBootstrapFlagDescription_ContainsDeprecationNotice— flag description includes deprecation textTestIsBootstrap— returns correct boolean valueTestConnectedWellknownPeersCountMetricName— metric constant has correct valueTestObserveConnectedWellknownPeersCount_Callable— renamed function exists and executes without panic