Remove bootstrap nodes and replace with operator peers#3909
Remove bootstrap nodes and replace with operator peers#3909lrsaturnino wants to merge 31 commits into
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. |
- Updated Electrum URL from WebSocket to TCP. - Replaced existing peer addresses with new IP-based entries for testnet.
Replace the single mainnet peer (a documented single point of failure for initial peer discovery) with the full set of staying-operator nodes. Peer IDs were verified against live node diagnostics and each address confirmed reachable on its libp2p port; operators advertising stable DNS names use /dns4/ so entries survive host/IP changes, and non-default ports are preserved. Resolves the in-file TODO to add peers across different operators/ASNs before production rollout.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR deprecates bootstrap-based peer networking in favor of well-known peers, refactors the firewall allow-list API, updates metrics naming, removes obsolete bootstrap extraction logic, and refreshes peer configuration. Network initialization no longer derives firewall policy from bootstrap peer keys. ChangesBootstrap deprecation and well-known peers migration
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/flags.go (1)
205-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
network.peershelp text to match the well-known peers migration.The deprecation messaging here is clear, but the
network.peersdescription still says “bootstrap nodes,” which is now misleading for operators.Suggested text update
cmd.Flags().StringSliceVar( &cfg.LibP2P.Peers, "network.peers", []string{}, - "Addresses of the network bootstrap nodes.", + "Addresses of well-known network peers.", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/flags.go` around lines 205 - 219, The help text for the "network.peers" flag (registered via cmd.Flags().StringSliceVar and stored in cfg.LibP2P.Peers) still refers to "bootstrap nodes"; update that description to reference the well-known peers migration instead (e.g., "Addresses of peers to connect to after the well-known peers migration" or similar) so operators are not misled by the old "bootstrap nodes" wording. Ensure you only change the flag description string passed to cmd.Flags().StringSliceVar for "network.peers".
🧹 Nitpick comments (1)
pkg/chain/local_v1/blockcounter.go (1)
18-22: ⚡ Quick winConsider adding documentation for
closeOncefield.The
closeOncefield prevents a subtle race where multiplecount()iterations might attempt to close the same watcher's channel after its context is cancelled but before the removal goroutine removes it from the slice. A brief comment would help future maintainers understand this synchronization.📝 Suggested documentation
type watcher struct { ctx context.Context channel chan uint64 + // closeOnce ensures channel is closed exactly once even if multiple + // count() iterations detect ctx is done before removal goroutine runs. closeOnce sync.Once }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/chain/local_v1/blockcounter.go` around lines 18 - 22, Add a short comment to the watcher struct explaining the purpose of the closeOnce field: it is used to ensure the watcher's channel is closed exactly once to avoid a race where concurrent count() iterations (or the context-cancel path) may attempt to close the same channel before the removal goroutine removes the watcher from the slice. Mention the relevant symbols watcher, closeOnce, count(), and the removal goroutine so future maintainers understand the synchronization intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/_electrum_urls/testnet`:
- Line 1: The testnet Electrum endpoint file currently contains a single
hard-coded entry which creates a single point-of-failure; update the testnet
list (config/_electrum_urls/testnet) to include multiple Electrum endpoints
(prefer DNS-backed hostnames where available) so
resolveElectrum/readElectrumUrls in config/electrum.go can failover; add several
geographically and provider-diverse entries (mix of tcp://host:port and
tls://host:port or DNS seeds) to provide redundancy and ensure
auto-configuration succeeds if one host is down.
In `@config/_peers/testnet`:
- Around line 1-3: The three testnet peer multiaddresses
(/ip4/143.198.69.177/tcp/3919/ipfs/16Uiu2HAmNqFJzYacvQJhTm116LmpNhTXbEMinuCXibKxTEj2W8iu,
/ip4/143.198.69.177/tcp/3920/ipfs/16Uiu2HAmNqFJzYacvQJhTm116LmpNhTXbEMinuCXibKxTEj2W8iu,
/ip4/143.198.69.177/tcp/3921/ipfs/16Uiu2HAm3yEkd3vXaCnSkxU5ViP2pDvV7fLN6V3fwPZrMYbH6PV3,
etc.) all use the same IP creating a SPOF; replace duplicate /ip4/143.198.69.177
entries with peers on distinct IPs or convert to DNS-based multiaddresses (using
/dnsaddr/host.example.com/tcp/<port>/ipfs/<peerID>) so you can update endpoints
without client changes, and ensure each peer entry references a different
host/IP and correct peer ID before committing.
In `@config/peers_test.go`:
- Around line 20-22: The test's expectedPeers slice only includes one mainnet
endpoint but the source list in config/_peers/mainnet contains 20 entries;
update the test (the expectedPeers value in peers_test.go / the test that loads
peers) to include all 20 mainnet multiaddrs from config/_peers/mainnet so the
assertion matches the actual list, or alternatively change the test to
explicitly assert the expected length (20) and/or perform a documented
spot-check rather than requiring exact equality; modify the expectedPeers
variable or the test assertions accordingly (look for expectedPeers and the test
that compares loaded peers).
- Around line 26-27: The sepolia test expectations in TestResolvePeers (the
sepolia_network case in config/peers_test.go) expect DNS-based multiaddrs that
the resolver does not produce; update the test so the expected peers for the
sepolia_network case match the resolver output (replace the two
"/dns4/keep-operator-*.test.keep-nodes.io/tcp/3920/ipfs/..." entries with the
actual resolved "/ip4/143.198.69.177/tcp/{3919,3920,3921}/ipfs/..." multiaddrs
and correct peer IDs/ports), or alternatively change the resolver/config that
builds ResolvePeers to emit the DNS multiaddrs—locate the sepolia case in the
TestResolvePeers table/expected slice and make the expected slice match the real
resolved addresses.
---
Outside diff comments:
In `@cmd/flags.go`:
- Around line 205-219: The help text for the "network.peers" flag (registered
via cmd.Flags().StringSliceVar and stored in cfg.LibP2P.Peers) still refers to
"bootstrap nodes"; update that description to reference the well-known peers
migration instead (e.g., "Addresses of peers to connect to after the well-known
peers migration" or similar) so operators are not misled by the old "bootstrap
nodes" wording. Ensure you only change the flag description string passed to
cmd.Flags().StringSliceVar for "network.peers".
---
Nitpick comments:
In `@pkg/chain/local_v1/blockcounter.go`:
- Around line 18-22: Add a short comment to the watcher struct explaining the
purpose of the closeOnce field: it is used to ensure the watcher's channel is
closed exactly once to avoid a race where concurrent count() iterations (or the
context-cancel path) may attempt to close the same channel before the removal
goroutine removes the watcher from the slice. Mention the relevant symbols
watcher, closeOnce, count(), and the removal goroutine so future maintainers
understand the synchronization intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1be4a938-bded-4b31-8380-7382f768a852
📒 Files selected for processing (18)
.github/workflows/client.ymlcmd/flags.gocmd/start.gocmd/start_test.goconfig/_electrum_urls/testnetconfig/_peers/mainnetconfig/_peers/testnetconfig/peers_test.gogo.modpkg/chain/local_v1/blockcounter.gopkg/clientinfo/metrics.gopkg/clientinfo/metrics_test.gopkg/firewall/firewall.gopkg/firewall/firewall_test.gopkg/net/libp2p/libp2p.gopkg/net/libp2p/libp2p_test.gopkg/tbtc/node_test.gopkg/tbtcpg/internal/test/marshaling.go
💤 Files with no reviewable changes (2)
- pkg/net/libp2p/libp2p.go
- pkg/net/libp2p/libp2p_test.go
| @@ -1 +1 @@ | |||
| wss://electrum.testnet.boar.network:443/QxbJgaSLUHqrgAa9BW7bDpnGPxrlhnCa | |||
| tcp://134.199.227.217:50001 | |||
There was a problem hiding this comment.
Avoid shipping only one hard-coded testnet Electrum endpoint.
With a one-entry list here, default Electrum resolution has no failover path (see config/electrum.go, readElectrumUrls / resolveElectrum, Lines 12-75). If this host is unavailable, auto-configuration fails for all fresh clients. Please provide multiple endpoints (preferably DNS-backed where possible) to remove this SPOF.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/_electrum_urls/testnet` at line 1, The testnet Electrum endpoint file
currently contains a single hard-coded entry which creates a single
point-of-failure; update the testnet list (config/_electrum_urls/testnet) to
include multiple Electrum endpoints (prefer DNS-backed hostnames where
available) so resolveElectrum/readElectrumUrls in config/electrum.go can
failover; add several geographically and provider-diverse entries (mix of
tcp://host:port and tls://host:port or DNS seeds) to provide redundancy and
ensure auto-configuration succeeds if one host is down.
| /ip4/143.198.69.177/tcp/3919/ipfs/16Uiu2HAmNqFJzYacvQJhTm116LmpNhTXbEMinuCXibKxTEj2W8iu | ||
| /ip4/143.198.69.177/tcp/3920/ipfs/16Uiu2HAm3yEkd3vXaCnSkxU5ViP2pDvV7fLN6V3fwPZrMYbH6PV3 | ||
| /ip4/143.198.69.177/tcp/3921/ipfs/16Uiu2HAm8WXhNRasZvRpt5SJxuY4B7jkkKT8PYYqw4pWbMZyYJ2s No newline at end of file |
There was a problem hiding this comment.
All testnet peers share the same IP address, creating a single point of failure.
All three peer multiaddresses point to 143.198.69.177. If this IP becomes unreachable (network partition, host failure, DDoS), the entire testnet peer discovery fails. This contradicts the high-availability relay concerns raised by reviewers (pdyraga, nkuba) and increases the SPOF risk documented in the PR commits.
Consider distributing peers across multiple IP addresses or reverting to DNS-based multiaddresses that can be updated to point to different IPs without client code changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/_peers/testnet` around lines 1 - 3, The three testnet peer
multiaddresses
(/ip4/143.198.69.177/tcp/3919/ipfs/16Uiu2HAmNqFJzYacvQJhTm116LmpNhTXbEMinuCXibKxTEj2W8iu,
/ip4/143.198.69.177/tcp/3920/ipfs/16Uiu2HAmNqFJzYacvQJhTm116LmpNhTXbEMinuCXibKxTEj2W8iu,
/ip4/143.198.69.177/tcp/3921/ipfs/16Uiu2HAm3yEkd3vXaCnSkxU5ViP2pDvV7fLN6V3fwPZrMYbH6PV3,
etc.) all use the same IP creating a SPOF; replace duplicate /ip4/143.198.69.177
entries with peers on distinct IPs or convert to DNS-based multiaddresses (using
/dnsaddr/host.example.com/tcp/<port>/ipfs/<peerID>) so you can update endpoints
without client changes, and ensure each peer entry references a different
host/IP and correct peer ID before committing.
| expectedPeers: []string{ | ||
| "/dns4/bst-a01.tbtc.boar.network/tcp/5001/ipfs/16Uiu2HAmAmCrLuUmnBgpavU8y8JBUN6jWAQ93JwydZy3ABRyY6wU", | ||
| "/dns4/bst-b01.tbtc.boar.network/tcp/5001/ipfs/16Uiu2HAm4w5HdJQxBnadGRepaiGfWVvtMzhdAGZVcrf9i71mv69V", | ||
| "/ip4/143.198.18.229/tcp/3919/ipfs/16Uiu2HAmDP4Z6LCogRMictJ6deGs4DRo99A5JTz5u3CLMg7URxC6", | ||
| }}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare actual mainnet peer count with test expectations
echo "=== Actual mainnet peers count ==="
cat config/_peers/mainnet | grep -v '^#' | grep -v '^$' | wc -l
echo -e "\n=== Test expects this many mainnet peers ==="
rg -A 3 '"mainnet network"' config/peers_test.go | rg '/ip4/' | wc -l
echo -e "\n=== All mainnet peers ==="
cat config/_peers/mainnetRepository: threshold-network/keep-core
Length of output: 1982
Align mainnet peer test expectations with the full mainnet list
config/_peers/mainnet contains 20 peer endpoints, but config/peers_test.go currently expects only 1 mainnet peer (/ip4/143.198.18.229/tcp/3919/ipfs/...). If the PR intent is to replace mainnet peers with 20 endpoints, update the test to cover all 20 (or adjust the PR/logic to explicitly document that this test is only a spot-check).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/peers_test.go` around lines 20 - 22, The test's expectedPeers slice
only includes one mainnet endpoint but the source list in config/_peers/mainnet
contains 20 entries; update the test (the expectedPeers value in peers_test.go /
the test that loads peers) to include all 20 mainnet multiaddrs from
config/_peers/mainnet so the assertion matches the actual list, or alternatively
change the test to explicitly assert the expected length (20) and/or perform a
documented spot-check rather than requiring exact equality; modify the
expectedPeers variable or the test assertions accordingly (look for
expectedPeers and the test that compares loaded peers).
The testnet peer list and its test referenced stale identities: the embedded peers carried re-keyed peer IDs, and the test expected DNS names (keep-operator-N.test.keep-nodes.io) that no longer resolve. Refresh to the live testnet4 operator cohort on 143.198.69.177 (ports 3919-3921), peer IDs verified against each node's diagnostics endpoint, and align peers_test.go to match. Host and ports are unchanged; only the peer IDs are corrected.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/peers_test.go (1)
26-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude the full sepolia peer set in expectedPeers.
Given the PR objective says testnet peers were refreshed to ports 3919–3921, the
sepolia networkexpectations on Line 26–Line 27 validate only two peers. This weakens the regression check for peer resolution completeness.Suggested update
"sepolia network": { network: network.Testnet, expectedPeers: []string{ "/ip4/143.198.69.177/tcp/3919/ipfs/16Uiu2HAkvjus5MH3y2tJBC6Bt1Ff9tiSowxGCw8J4FzLonnfDeG2", "/ip4/143.198.69.177/tcp/3920/ipfs/16Uiu2HAmSBn6CgZ4r7HnC4RVMMFMe5vfkLvykUUfS3MnKiHLSuPD", + "/ip4/143.198.69.177/tcp/3921/ipfs/<peer-id-for-3921>", }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/peers_test.go` around lines 26 - 27, The sepolia expectedPeers slice in peers_test.go only contains two peer entries (ports 3919 and 3920) but the testnet peers were refreshed to include ports 3919–3921; update the expectedPeers value used in the Test (the expectedPeers variable/fixture in peers_test.go) to include the full sepolia peer set (add the missing peer for port 3921 with the corresponding /ipfs/ multiaddr) so the assertion validates all three peers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@config/peers_test.go`:
- Around line 26-27: The sepolia expectedPeers slice in peers_test.go only
contains two peer entries (ports 3919 and 3920) but the testnet peers were
refreshed to include ports 3919–3921; update the expectedPeers value used in the
Test (the expectedPeers variable/fixture in peers_test.go) to include the full
sepolia peer set (add the missing peer for port 3921 with the corresponding
/ipfs/ multiaddr) so the assertion validates all three peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fb4666b-3874-4b6e-84c3-8e12676aad1d
📒 Files selected for processing (2)
config/_peers/testnetconfig/peers_test.go
✅ Files skipped from review due to trivial changes (1)
- config/_peers/testnet
…oint The testnet Electrum URL was switched to the self-hosted testnet4 ElectrumX endpoint (tcp://134.199.227.217:50001), but electrum_test.go still expected the legacy Boar endpoint. Update the testnet expectation to match the embedded URL file; the endpoint was verified live (ElectrumX 1.19.0, protocol 1.4) and is actively used by the testnet4 operator cohort. Mainnet electrum is unchanged.
Resolve conflicts from concurrent firewall and CI changes on main:
- pkg/firewall/firewall_test.go: combine main's removal of the positive
result cache (positive recognition is now rechecked on each Validate
call) with this branch's EmptyAllowList()-as-function refactor. Drop
the positiveResultCache field from all test policies, adopt the renamed
TestValidate_PeerRecognized_Rechecked assertion, and keep the new
EmptyAllowList decoupling tests, all calling EmptyAllowList().
- pkg/tbtcpg/internal/test/marshaling.go: take main's errors.New fix for
the non-constant format string, superseding this branch's fmt.Errorf("%s").
- .github/workflows/client.yml: take main's Go test step, which already
runs ./... and adds coverage reporting, superseding this branch's
standalone ./... addition.
The testnet Electrum URL had been pointed at a self-hosted server running Bitcoin testnet4, while keep-core's testnet stack and the embedded config-validation integration tests target testnet3. A testnet4 server cannot serve the testnet3 transactions, blocks, and Merkle proofs the integration tests expect, so the electrum integration suite failed with 'not found' and 'out of range' errors. Restore the testnet3 Electrum endpoint, which is live, on testnet3, and at the chain tip the tests expect, and align the resolveElectrum expectation.
|
Thanks @pdyraga, @nkuba — I take this seriously and agree it shouldn't be a hard cutover. Let me be precise about scope, share what's been validated so far, and lay out how I'd roll it out. Scope. This retires the dedicated The tradeoff — it's the one you're flagging: the bootstrap hosts are HA and don't run tECDSA, so they never restart under signing load and give a stable re-entry point; the operator peers that now carry discovery do run tECDSA. We're trading a dedicated non-tECDSA anchor for redundancy across the ~20 operator peers. That's a real change, which is why I'd stage and monitor it rather than cut over. Validation completed so far (controlled environment — Bitcoin testnet4 + Sepolia L1, 3-operator cohort, client
These were validated on a Sepolia dev deployment (not the canonical Threshold Sepolia contracts) plus Bitcoin testnet4 — reproducible below:
Scope: 3-operator, group-size-3 cohort — it validates functional correctness and behavior under CPU stress at small scale; it does not reproduce mainnet TSS message volume (~100-of-N groups). A few items remain open (a re-staking rerun, refreshing the stale Sepolia verification pointer, and restoring SPV diagnostics coverage). That gap is exactly why the mainnet step is staged and monitored rather than released directly. Rollout:
|
|
Thanks @nkuba, and agreed on the security improvements. On testnet — you're right the public testnet is effectively gone; the validation above ran on a privately-operated testnet4 cohort against Sepolia. We won't overstate it (group size 3, three operator nodes run by one party, so it doesn't reproduce mainnet TSS volume) — which is exactly why we're leaning on the staged mainnet rollout with production monitoring as the real validation vehicle rather than presenting testnet as proof of mainnet safety. |
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 panicSummary by CodeRabbit
Deprecations
--network.bootstrapCLI flag is now deprecated and scheduled for removal in v3.0.0.Network Configuration
Metrics