Repository navigation
feat(clientinfo): expose chain tip staleness and leader proposal outcomes - #4347
Merged
Merged
Conversation
The tBTC branch of the peer-admission firewall asked whether a peer's staking provider held a legacy token staking delegation. Every function that could create one has since been removed from the deployed staking contract, and no code path writes the owner field it reads, so the qualifying set is frozen in both directions. Providers onboarded before the freeze are admitted forever; providers onboarded after it can never be, no matter how they are authorized. A correctly authorized operator therefore cannot exchange a single message with any peer, while every on-chain check reports it healthy. Decide the tBTC branch on eligible stake instead. That is the value the wallet registry already uses for sortition weight, it tracks the current authorization rather than a historical artifact, and it can only be raised by the owner of the authorization source. Registering an operator stays permissionless on both registries and so grants nothing on its own. The read replaces the delegation read rather than joining it, so recognition still costs the same two calls. The random beacon deliberately keeps the delegation predicate. Token staking reports no authorized stake for it, so its eligible stake is zero for every provider that has ever registered a beacon operator; giving it this predicate would recognize nobody and the watchtower would disconnect the fleet within a single round. A comment there records why. An integration test asserts the same premise against pinned mainnet state, but it needs an archive endpoint and is skipped without one, so the comment rather than the test is what guards the asymmetry against a future refactor. The two reads recognition performs move behind a narrow interface. The predicate had no seam at all, which is why none of its behaviour was under test before.
The embedded mainnet peer list is fed into DHT bootstrap as a routing seed, so every default-configured node dials each entry on start and uses it to populate its routing table. One entry points at a host that is no longer operated, for an operator identity that has since been replaced, which means the whole fleet keeps seeding its routing table from an address nobody controls any more. Remove that entry, leaving nineteen seeds. Discovery is unaffected: the list is a seed set, not a peer requirement, and any node may override it entirely via network.peers. Kept as its own commit so it can be reverted without touching anything else.
The pending-decrease integration check filtered providers down to those whose eligible stake equals the minimum authorization, then asserted that same value was positive - unreachable by construction, so the case it claimed to guard was never exercised. Assert the invariant that actually carries the guarantee instead: the predicate admits on eligible stake above zero, so a zero minimum authorization would silently reject every provider sitting on the floor. Also make the staking-provider lookup error wrappable, matching the eligible-stake error beside it.
The staking-provider lookup wraps its chain error so the cause stays reachable through errors.Is, but the test covering that path only asserted an error came back at all, so unwrapping it would have gone unnoticed. The sibling eligible-stake test already pins its own wrap; only this path was exposed. Note the firewall does not depend on the wrapping to tell a transport fault from a genuine non-recognition - it keys on its ErrNotRecognized sentinel - so these assertions are the only thing holding either wrap in place.
A requested (but not yet approved) authorization-weight decrease to zero already zeroes WalletRegistry.EligibleStake at request time, well before the ~45-day approval delay. IsRecognized read only eligible stake, so a live signing member was disconnected from the P2P network the moment a decrease was merely requested, not when it was approved. Widen tbtcAdmissionReader with PendingAuthorizationDecrease and admit on eligibleStake > 0 OR pendingDecrease > 0, keeping the peer reachable while the request is outstanding. Fix the IsRecognized doc comment, which had the zeroing behavior backwards, and the one remaining %v error verb the rest of this function already converted to %w.
BeaconChain.IsRecognized deliberately keeps the legacy stake-delegation predicate instead of the tBTC branch's eligible-stake predicate, because RandomBeacon.eligibleStake is zero for every provider and switching would disconnect the whole fleet. That invariant had zero CI-executable enforcement: BeaconChain had no injectable seam, unlike TbtcChain's tbtcAdmissionReader, and the one integration test that touches the underlying chain state is gated behind an archive RPC URL that is never set in CI. Add beaconAdmissionReader, mirroring tbtcAdmissionReader's pattern, and route IsRecognized's two chain reads through it. Wire the real chain via beaconAdmissionReaderChain in newBeaconChain; production behavior is unchanged. Add beacon_recognition_test.go asserting the predicate against a mock, including the fail-closed path on both reads. Also convert IsRecognized's error verbs from %v to %w, matching the convention this PR already applies to TbtcChain.IsRecognized, so the new fail-closed tests can assert on the error chain.
- Route every RPC call through a shared callOrSkip helper instead of only each test's first call, so a transient rate limit skips the test instead of hard-failing it partway through. - The pending-decrease floor test now actually reads PendingAuthorizationDecrease (previously it never did, so the scenario it claimed to guard was never established); the read is unconditional but only feeds an informational count, since whether a floor-sitting provider currently has an active decrease request is transient live-chain state and must not gate the structural floor assertion. - The legacy-staking-froze test now asserts the wallet-registry operator it reads eligible stake for actually maps to the beacon operator it separately queries, and fails outright rather than silently passing when the beacon doesn't recognize the operator at all. - Renamed the three tests that only exercise raw bound contract callers, never TbtcChain.IsRecognized itself, off the misleading TestTbtcChain_IsRecognized_ prefix to TestMainnetChainState_, so the name matches what they assert. - Extracted the anonymous struct duplicated across allowlistWeights' two fields into a named allowlistOperatorEntry type.
IsRecognized also admitted a provider whose eligible stake was zero but whose authorization decrease was still pending. That disjunct has no expiry: the getter returns stored decrease state, and nothing clears it until an approval that may never arrive. Eighteen of the nineteen pending-only records on mainnet are orphans whose approval reverts, so their pending amount cannot be cleared through the ordinary path at all. The admitted set is the same either way, because every pending-only provider is already carried by the beacon branch. The disjunct therefore bought no connectivity while widening the window in which a provider that governance has de-authorized keeps network admission. Remove it; eligible stake alone is the tBTC predicate. Registration short-circuiting, the fail-closed error paths and the nil-amount guard are unchanged, and the generated binding keeps its pending-decrease method because the lifecycle and floor assertions still read that state. The tests meant to hold the surrounding invariants did not hold them. Forcing beaconAdmissionReaderChain.HasStakeDelegation to return false passed every test in the repository, so the deliberate beacon asymmetry had no executable enforcement through the production adapter. Both adapters, their constructors and the startup policy assembly now run against a simulated backend with the real bindings, asserting each branch separately and the exact contract reads each one makes. The mainnet assertions claimed the change de-admits nobody. It does not: it gains one operator and loses one dormant legacy identity. They now derive both sets from the two registration histories rather than stale deploy data, and take the pinned block hash from the RPC response instead of recomputing it from a header type that predates the current fork. A rate-limited read is retried and then fails, so a throttled endpoint can no longer read the same as an assertion that ran and found nothing wrong.
Every entry in the embedded mainnet peer list pinned an operator to a literal address, so an operator changing IP silently removed itself from the fleet's routing seed until a client release shipped a corrected list and operators upgraded. Sixteen of the twenty entries carried that exposure, and two had already gone stale in exactly that way. Fourteen operators have since published a DNS record for the host their node listens on. Swap their entries to /dns4/, leaving the peer ID and port on each untouched: the identity is the peer ID, and the address is only the hint used to reach it. Resolution is symmetric in config.resolvePeers and in libp2p.Connect, so this is a data change with no code behind it. Two operators have not provided a record yet and keep their /ip4/ entry, so seventeen of nineteen seeds are now name-based. Each record was checked before being merged: the hostname resolves, the libp2p port accepts a connection, and the node's own diagnostics report the peer ID the entry claims. Two entries are worth noting because they are not plain hostname swaps. One operator's name resolves to an active-passive pair of records, which libp2p dials in turn, so failover no longer waits on a DNS edit. Another kept a non-default libp2p port through a host move, and the entry keeps that port rather than the default. Ordering is preserved so the diff reads as a per-entry swap, and the resolvePeers fixture is updated in the same commit because it mirrors the file line for line.
Two mutations of cmd/start.go left `go test ./cmd/` green. Replacing the application list with the tBTC chain alone removed the beacon branch from production admission; replacing the policy handed to initializeNetwork with firewall.Disabled admitted every peer unconditionally. Neither is caught by a test that builds a policy of its own and asserts against that, which is what the existing coverage did: it exercised a faithful copy of the wiring rather than the wiring. The network provider is now reached through a package-level variable, so a test can capture the policy the client actually hands the network layer and assert against that object — the admission table, the applications it guards with and their order, and that its allow list is empty. A separate test feeds the static-bypass assertion a policy carrying one unrelated key, so an assertion that otherwise only ever sees empty allow lists is known to be able to tell the two apart. Both mutations now fail named assertions, as do the three the earlier tests already caught: forcing the beacon adapter to report no delegation, restoring the pending-decrease disjunct, and relaxing the eligible-stake comparison. Five comments are corrected in the same pass, each having stated something the code does not do. The admission formula omitted that Validate short-circuits on the first application error, which makes beacon RPC health a hard dependency for tBTC-only identities, and that a genuine non-recognition is cached for an hour, so reauthorization is not immediately visible. The admission reader interface still described EligibleStake as admission-only after the public accessor began routing through it. The replayed read in the fault test was attributed to go-ethereum's bind layer; it comes from keep-common's error resolver, which re-invokes the contract to decode a revert reason. The beacon comment hedged about disconnection timing while FirewallCheckTick names that interval. And the fixture key helper called its keys synthetic: they are the canonical low-scalar keys, whose addresses carry live mainnet nonces, and what keeps them off a live chain is that the endpoint is local rather than the chain-ID guard, which is configured for mainnet.
The ECDSA and random beacon release jobs still publish documentation to `threshold-network/threshold`. Update both destinations and their comments to `threshold-network/docs` for the completed repository rename. The in-place rename of `threshold-network/threshold` to `threshold-network/docs` completed on 2026-09-08 (repo id 446490352 preserved; the old repository path now redirects to the new name). Both destination folders are verified present on `docs` @ `main`, so this PR is unblocked. Companion to threshold-network/solidity-contracts#148. Validation: parsed both YAML files and verified that `destinationRepo` is the only configuration value changed; `git diff --check` passes. Live documentation publishing was not executed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated documentation publishing workflows to sync release documentation to the `threshold-network/docs` repository. * Updated related workflow comments to reflect the new documentation destination. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
threshold-network/ci PR #1 is merged; 20b35345 existed only on the unmerged branch and could become unreachable. Pin all 17 references to the merge result on ci main (91f574f2), whose action input contracts and outputs are identical to the previous pin. Document that release-job environment values resolve from the pinned revision and change only via re-pinning.
Re-point all 17 action references from the ci main tip (which also carries the unrelated solidity-docs-preview-comments merge) to 86506f8d, the merge commit of threshold-network/ci PR #1. The consumed actions/config bundle is byte-identical between the two pins (verified via compare), and all four action input contracts are unchanged, so this is a purely semantic anchor fix: the pin now refers to exactly what PR #1 landed.
Repoint Go Electrum to the Threshold fork at the same immutable upstream revision (`6038cb594daa`), preserving the imported module path and implementation. Update go.sum for the new source and remove the old source checksums. Use the maintained actions from [threshold-network/ci #1](threshold-network/ci#1), pinned to the merge commit `86506f8dcdd80179a95ee149cccf4ce9a793ad8d` of PR #1, merged to the producer's main branch. This includes the vendored npm-version-bump action. Completion payloads and upstream-build queries use the Threshold module identifiers configured by the release manager. The consumed action bundle is byte-identical between this pin and the ci main tip that followed (the next producer merge touched only the docs workflow, its README, and tests), so the pin excludes no reviewed action code. Update obsolete action runtimes in the affected workflows. Environment values for release jobs (GCR registry/project, network id) resolve from the pinned threshold-network/ci revision; changing them requires re-pinning the SHA. The consumer migrations must merge before starting a new inter-repository release pipeline; `CI_GITHUB_TOKEN` must be configured where missing. npm/cloud/docs credentials stay in their existing consumer repositories. Validation: actionlint passes for every changed workflow; producer paths, input names, commit pins, dispatch input contracts, and module/query identifiers were checked against the producer. Its 51 tests and four standalone action-bundle checks pass in GitHub CI. Release/deployment/publishing paths were not invoked during validation. The shared Docker action also uses supported action runtimes, with provenance disabled to preserve the prior image format. keep-core's local docs workflow is already owned and remains local. This change is independent of the helper/local-network/common-library PRs already open. Additional validation: `go test -mod=readonly ./pkg/bitcoin/...` passes against the fork; the fork's tests and vet pass on Go 1.24.1 and 1.26.0 in CI. GitHub CI: all 1,678 Go unit tests and the Docker/runtime/binary builds pass, as do the contracts test/lint/Slither/deployment-dry-run jobs. The separate full integration suite also passes.
The last two operators pinned to a literal address in the embedded mainnet peer list have published DNS records for the hosts their nodes listen on. Move both entries to /dns4/, so an IP change on either side no longer drops that operator out of the fleet's routing seed until a client release ships a corrected list. Shoegazer moves from /ip4/167.99.58.9/tcp/3919 to /dns4/tbtc.somaweb3.llc/tcp/3919, and Colossus Digital from /ip4/138.201.251.149/tcp/3919 to /dns4/keep.colossus.digital/tcp/3919. Peer IDs and ports are unchanged. Each name resolves to the same host as the entry it replaces across the default, Google, Cloudflare and OpenDNS resolvers, the libp2p port accepts a connection, and the node's own diagnostics report the peer ID the entry claims. Both nodes report v2.6.0 (rev 4137b34). With these two swaps every seed in the mainnet list is name-based. The resolvePeers fixture in config/peers_test.go mirrors the file line for line and is updated in the same commit.
An earlier commit on this branch dropped the entry for an operator identity that has since been replaced, because its host is no longer operated. Add the replacement identity back as a /dns4/ entry, so the list again carries one seed per operator: twenty, all name-based. The replacement is the operator the admission change on this branch lets in: its staking provider holds eligible stake without a legacy delegation, which the old predicate rejects. A node only carries this list together with that change, so every node that dials the seed also admits it. The name resolves to the operator's current host across the default, Google, Cloudflare and OpenDNS resolvers, the libp2p port accepts a connection, and the node's own diagnostics report the peer ID the entry claims. On chain, the operator maps to its staking provider, which holds eligible stake, and the operator is in the sortition pool and up to date. Ordering follows the previous file, so the entry sits where the dropped one was. The resolvePeers fixture in config/peers_test.go mirrors the file line for line and is updated in the same commit.
…n case - backend.go: decode block-number param in eth_call dispatch, routing block-mismatched reads into unexpected calls instead of silently answering from current state - backend.go: clear unexpected calls in ResetCalls alongside recorded calls, so one root cause stops cascading into every later subtest - admission.go: correct StakingProvider docstring (unregistered side maps to zero address, not the shared value) - admission.go: reference tracking issue #4335 on the legacy_revoked case documenting the known, deliberately-deferred admission gap
…ssue - admission_production_test.go: clarify minimum-authorization test verifies adapter plumbing only, not contract-level floor enforcement (that's TestMainnetChainState_EligibleStakeAtMinimumAuthorization) - beacon.go: reference tracking issue #4335 in the asymmetry comment covering the legacy-delegation admission gap - ethereum_integration_test.go: remove originalHeadTbtcRecognized and pendingDecrease modeling an abandoned intermediate design proposal not present on main or in this PR's final behavior - tbtc_recognition_test.go: delete PendingOnlyRejected test and its unreachable mock field/method (tbtcAdmissionReader never exposes PendingAuthorizationDecrease, so both table rows exercised the same code path already covered elsewhere)
- run-keep-node.adoc: warn that requesting a tBTC authorization decrease to zero removes peer admission immediately (eligible stake is subtracted on request, not on approval ~45 days later), while wallet-membership signing obligations continue unchanged - registration.adoc: clarify that registering only on RandomBeacon without a pre-existing legacy delegation results in permanent peer-admission lockout on the beacon branch, with no remedy
Peer admission was a disjunction over two applications, evaluated beacon first: a peer was admitted if its staking provider carried a legacy TokenStaking delegation, or if its provider held eligible stake in the wallet registry. TIP-092 removed every function that writes the legacy delegation record, so the first branch admits a frozen set of identities that nobody can add to or remove from, and a Council decision to zero a provider's Allowlist weight never reaches the network layer while that branch answers first. Admission now consults the tBTC application alone. A peer is admitted when it is a registered tBTC operator whose provider has positive WalletRegistry eligible stake, which the Council controls through the Allowlist, and a weight decrease requested there takes effect at the next validation. The beacon handle no longer satisfies firewall.Application at all: the recognition method, its reader interface and the production adapter are deleted rather than stubbed, so putting the beacon back into the application list fails to compile. Beacon startup registration, protocol initialization and the shared RolesOf accessor are untouched; none of them was an admission authority. Measured over every operator ever registered on either registry, 262 identities lose admission with this change. None of them holds tBTC sortition pool membership or weight. Rolling back re-admits that legacy set, so a rollback is a policy decision rather than an operator convenience, and this change is only correct on top of the eligible-stake predicate it builds on. The admission fixture is tBTC-only and its traces prove that no beacon read happens on the admission path, even when beacon reads are made to fail after construction. Startup composition is asserted on the policy the client actually hands the network layer, in ordinary and bootstrap mode alike; revocation is exercised end to end through the real watchtower guard; and four mutations - disabling the policy, restoring the legacy-delegation decision, caching an RPC failure as a denial, and caching positive decisions - each fail a named assertion.
…e comments Address confirmed multi-agent-review findings for PR #4333: - TestMainnetChainState_AdmissionCensus asserted only cardinality (20 admitted, 262 retired), not identity. Add two identity-level checks: every config/_peers/mainnet seed's derived chain address must be in the current admitted set, and every retired operator must read false from WalletRegistry.IsOperatorInPool at the pinned block. - pendingDecreaseTbtcRecognized and its call sites uniformly labeled a never-deployed, superseded proposal as "historical" alongside predicates that really were live on mainnet. Reword to distinguish the two. - TestBaseChain_RolesOf_ProductionBinding's comment claimed the binding is "retained for callers outside admission" -- false since the beacon admission predicate was removed; no production caller remains. Reworded to state its actual purpose: a guard against a real misrouting bug.
Address confirmed multi-agent-review findings for PR #4333: - admissionHandoff.networkConfig / assertNetworkConfig and the bootstrap_with_configured_discovery_peers sub-tests in TestStart_AdmissionHandoff and TestInitializeNetwork_AdmissionComposition tested no distinct path: start()/initializeNetwork never branch on Bootstrap/Peers, and the stubbed connectNetwork returns before isBootstrap() is evaluated, so the bootstrap variant ran byte-identical code to its ordinary sibling. Removed the capture, the helper, and both bootstrap sub-tests, collapsing each to its single ordinary run. - TestStart_AdmissionRevocationDisconnects's doc comment implied it proves the production libp2p-to-watchtower wiring; nothing in this suite reaches that wiring (captureAdmissionPolicy stubs connectNetwork out first). Reworded to state what it actually proves: the watchtower disconnects a peer once Validate returns an error, composed here over a hand-built connection manager and guard.
…out gate Address a confirmed multi-agent-review finding for PR #4333: the release gate for the one retired identity that still holds shares in live wallets existed only in the PR description, not as a durable repo artifact. Records: the measured census split (20 admitted under the new tBTC-only predicate, 262 retired, 19 of 20 are the embedded bootstrap seeds), the wallet-continuity precondition a release must satisfy before being tagged, and an explicit placeholder for the specific excluded operator's address -- not available from the reviewed PR/issue materials, left for the PR author to fill in rather than fabricated.
…ion (#4288) Closes #4287. ## Problem The tBTC branch of the peer-admission firewall decides whether to accept a peer by asking whether that peer's staking provider holds a legacy `TokenStaking` delegation. TIP-092 removed every function capable of creating one, and no code path in the deployed implementation writes the `owner` field the check reads, so the qualifying set is frozen in both directions. Providers onboarded before the freeze are admitted permanently; providers onboarded after it can never be admitted, however they are authorized. The effect is that a correctly authorized operator cannot exchange a single message with any peer, while every on-chain check reports it healthy — `isOperatorInPool`, `isOperatorUpToDate`, allowlist weight and sortition weight all look correct. The failure is visible only in client logs and in `connected_peers_count` sitting at zero while the connect and disconnect counters climb together. This is live on mainnet, and it affects every future operator onboarding and key rotation, not one operator. ## Solution Decide the tBTC branch on `WalletRegistry.eligibleStake(stakingProvider) > 0` instead. That is the value the registry already uses for sortition weight, it tracks the current authorization source rather than a frozen historical field, and it carries the `minimumAuthorization` floor for free. It replaces the `RolesOf` read rather than adding to it, so the call count is unchanged, and the binding already exists — no regeneration. **This is a liveness fix, not a tightening.** Measured over the union of all operator addresses ever registered on either registry, the combined admitted set is unchanged at 282: one operator gains admission (the one currently locked out) and one dormant address loses it (no allowlist weight, no beacon registration, in neither pool). Nineteen of the twenty providers with non-zero eligible stake are already admitted via the beacon branch regardless of this predicate. Please don't read it as narrowing the boundary — it doesn't. `eligibleStake` can only be raised by the owner of the authorization source the registry reads, so an arbitrary address that self-registers an operator still reads zero and is still rejected. Registering an operator is permissionless on both registries and never was the boundary. The error path stays fail-closed, returning `(false, err)`. Collapsing a transient RPC failure into "not recognized" would enter the firewall's one-hour negative cache as a durable result and lock out a healthy peer for that hour. ### The beacon branch deliberately keeps `RolesOf` `beacon.go`'s admission read is refactored behind a `beaconAdmissionReader` seam (mirroring `tbtcAdmissionReader` on the tBTC side) for testability; the predicate itself (`RolesOf`/`HasStakeDelegation`) is unchanged, and the asymmetry is intentional. `TokenStaking.authorizedStake` short-circuits to zero for every application except one hard-coded constant, and the random beacon is not that application, so `RandomBeacon.eligibleStake` is zero for every staking provider that has ever registered a beacon operator. Applying this predicate there would recognize nobody. Since the watchtower re-runs the same check against every connected peer every ten minutes and disconnects on failure, a symmetric change would take the fleet apart inside a single round. An integration test asserts exactly that premise against pinned mainnet state, so the reasoning is checkable rather than merely asserted. Note that it needs an archive endpoint via `ETHEREUM_MAINNET_RPC_URL` and skips without one — CI does not currently supply that, so it will not run there until the endpoint is wired in. Until then the comment in `beacon.go` is what stops a future refactor harmonizing the asymmetry away, and the test is a manual check rather than an automated guard. ### The embedded peer list Four commits touch `config/_peers/mainnet`, all kept separate from the predicate change so each can be reverted on its own. The list is fed into DHT bootstrap as a routing seed, so every default-configured node dials each entry on start and uses it to populate its routing table. The first removes one entry that pointed at a host no longer operated, for an operator identity since replaced — the whole fleet was seeding its routing table from an address nobody controls any more. That leaves nineteen seeds. The second is an addressing change. Every remaining entry pinned its operator to a literal IP, so an operator changing address silently dropped itself out of the seed set until a client release shipped a corrected list and operators upgraded — the same failure the removed entry had already hit, and sixteen of the twenty entries carried the same exposure. Fourteen operators have since published a DNS record for the host their node listens on, and their entries move to `/dns4/` with the peer ID and port on each left untouched. The identity is the peer ID; the address is only the hint used to reach it. Resolution is symmetric in `config.resolvePeers` and in `libp2p.Connect`, so there is no code behind this. Two operators had not supplied a record yet at that point and kept their `/ip4/` entry, leaving seventeen of nineteen seeds name-based. The third moves those last two entries to `/dns4/` once their operators published records, again leaving the peer ID and port on each untouched. All nineteen seeds are now name-based. The fourth adds back, by DNS name, the replacement identity for the operator whose entry the first commit removed. That identity is the operator this change admits, and a node only carries this list together with the fix, so every node that dials it also admits it. The list is back to twenty seeds, all name-based. Every record was verified before merging: the hostname resolves, the libp2p port accepts a connection, and the node's own `/diagnostics` reports the peer ID the entry claims. Two are not plain hostname swaps. One operator's name resolves to an active-passive pair of A records, which libp2p dials in turn, so their failover no longer waits on a DNS edit. Another kept a non-default libp2p port through a host move, and the entry preserves that port rather than normalising it. Discovery is unaffected by any of these commits — the list is a seed set rather than a peer requirement, and any node can override it entirely with `network.peers`. Note that operators carrying such an override keep whatever addresses they hardcoded and will not pick up any of these changes; the release note should ask them to drop it. ## Tests Unit tests cover the predicate through a narrow two-method seam over the registry reads, added because `IsRecognized` previously had no way to be exercised without a chain behind it: an authorized provider with no legacy delegation is admitted; a pending decrease sitting exactly on the minimum authorization stays admitted; an unregistered operator and an unauthorized one are rejected; a lookup failure surfaces as an error rather than a silent non-recognition, and is not cached as one; a nil amount is treated as no stake rather than panicking. Integration tests behind the `integration` build tag pin the chain-state cases: that the beacon's eligible stake is zero for every known provider, that a provider authorized after legacy staking froze is now admitted, that a pending decrease at the floor holds, and that deprecated operators keep beacon admission and so are unaffected in production. One test asserts current behaviour rather than desired behaviour: an identity carrying a legacy delegation but zero eligible stake remains admitted through the beacon branch after this change. That gap is real and is not closed here — see below. ## What this does not do It does not close the security half of the incident behind #4287. An identity holding a pre-TIP-092 delegation retains admission through the beacon branch regardless of its current authorization, and this change does not affect that. It is tracked separately in #4335; the test above documents it so it isn't quietly assumed closed. ## Rollout The fix takes effect on the responding peers, not on the affected operator, so partial rollout gives partial connectivity — acceptable and monotonic. Upgrade the embedded bootstrap peers first: they are the affected operator's entry point to the fleet, and they only admit it once they run the fix. The lockout is observable rather than inferred. The affected operator's node is up, reachable on both its libp2p and diagnostics ports, and reports the peer ID its records claim — while sitting at zero connected peers. Its staking provider reads 40,000,000 T of eligible stake against an empty `rolesOf.owner`, which is exactly the combination the current predicate rejects and the new one admits, and its operator is in the sortition pool and up to date. The dormant identity that loses admission is the same operator's previous one: zero eligible stake, a populated `rolesOf.owner`, and not in the pool. So the net-zero claim above is, concretely, one operator's new identity replacing its own retired one. That identity link is an off-chain assertion from the reporting operator, not something the test suite verifies on-chain — the suite proves each address's individual chain state independently, not that the two addresses belong to the same operator. Expected effect on the admitted set is net zero. Watch fleet-wide `connected_peers_count` during the window: any drop toward twenty means the beacon branch has broken, and the change should be reverted. Reverting reintroduces nothing — it restores the current lockout, and the legacy-delegation admission gap described above is unaffected either way. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Updates** - tBTC operator recognition now requires positive eligible stake; pending authorization decreases alone no longer qualify. - Beacon operators are recognized based on staking-provider delegation status. - Admission checks distinguish temporary chain lookup failures from genuine recognition denials, improving recovery behavior. - Mainnet peer addresses now use DNS hostnames while preserving peer identity and ports. - **Maintenance** - Expanded validation of operator admission, chain-state behavior, and recovery scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Stacked on #4288: the base branch is that PR's head, and this change must land after it. Once #4288 merges I will retarget this PR to `main`. Reversing the order would leave the tBTC branch reading the same frozen legacy record after the beacon branch is gone, which loses an admission route without making revocation effective. ## Problem The peer-admission firewall admits a peer if either registered application recognizes it, evaluated beacon first. The beacon application recognizes a peer when its staking provider carries a legacy `TokenStaking` delegation. TIP-092 removed every function that writes that record, so the beacon branch admits a frozen set of identities that nobody can add to or remove from, and because it answers first, a Council decision to zero a provider's Allowlist weight never reaches the network layer. The Random Beacon itself has never produced a relay entry, accepted a DKG result, or registered a group on mainnet; its sortition pool is empty and locked by an expired genesis DKG. The only thing the branch does today is override the Council's current authorization decisions with a historical one. #4288 fixes the liveness half of #4287 by moving the tBTC branch to eligible stake. It leaves this half open on purpose, and its tests document the gap rather than close it. This PR closes it. ## Solution Admission now consults the tBTC application alone: a peer is admitted when it is a registered tBTC operator whose provider has positive `WalletRegistry.eligibleStake`, which the Council controls through the Allowlist. A weight decrease requested there takes effect at the next validation, and the watchtower drops an already-connected peer on its next sweep. `admissionApplications` accepts only the tBTC chain handle; `admissionPolicy` still passes an empty static allow list, so seeds and configured peers get no exception. On the beacon side the recognition method, its reader interface, the production adapter, and the admission field are deleted rather than stubbed, so `*BeaconChain` no longer satisfies `firewall.Application` and putting it back into the application list fails to compile. Beacon startup registration, `beacon.Initialize`, the Chaosnet seed source, and the shared `RolesOf` accessor are untouched; none of them was an admission authority, and the startup registration requirement is documented where the application list is built rather than removed. Measured at a pinned mainnet block over every operator ever registered on either registry, 262 identities lose admission. None of them holds tBTC sortition-pool membership or weight, and all nineteen embedded seeds are admitted under the new predicate. Exactly one of the excluded identities still holds shares in live wallets, so the release should be gated on a wallet-continuity measurement for the affected wallets — evidence of coordination and signing without that operator, including a heartbeat with at least seventy active retained members — and rolled out through the embedded seed operators first. Rolling this change back re-admits the whole legacy set, including any identity the Council has since revoked; that is a policy decision, not an operator convenience. Three known residuals are deliberately out of scope here and tracked separately: the client installs go-libp2p's default transports for outbound dials with no explicit transport restriction, the pubsub validator filters on the original author rather than the connection, and the mainnet-anchored integration tests still skip in CI because no RPC endpoint is forwarded to that job. ## Tests The admission fixture in `internal/ethtest` is now tBTC-only: the legacy-only, orphaned-pending-decrease, and beacon-only cases are rejected, an authorized provider with no beacon registration is admitted, and every trace proves that only the WalletRegistry mapping and eligibility reads happen on the admission path, even when beacon reads are made to fail after construction. `TestStart_AdmissionHandoff` and `TestInitializeNetwork_AdmissionComposition` assert the policy the client actually hands the network layer in both ordinary and bootstrap configuration with configured discovery peers, `TestStart_AdmissionRevocation` shows a legacy-owner identity going from admitted to rejected on the same production policy when its eligibility drops to zero, and `TestStart_AdmissionRevocationDisconnects` runs that revocation through the real watchtower guard. `TestBeaconChain_DoesNotImplementFirewallApplication` pins the structural guarantee. The RPC-error recovery and negative-caching coverage is retained, the dedicated beacon-recognition tests are removed with the code they tested, and the mainnet-anchored census now asserts the 20 active and 262 retired identities. Four source mutations — disabling the policy, restoring the legacy-delegation decision, folding an RPC failure into a denial, and caching positive decisions — each fail a named assertion, and re-adding the beacon to the application list fails to compile. The full client CI (format, vet, staticcheck, gosec, unit suite at 15.0% coverage, tagged integration suite, vendored btcec identity) passes locally. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Changes** - Network peer admission now relies solely on tBTC wallet-registry eligibility and positive eligible stake. - Random Beacon registration remains required at startup but no longer grants network admission. - Operators whose eligible stake is revoked are rejected and disconnected from the network. - Legacy beacon-only authorization no longer qualifies for admission. - **Documentation** - Added guidance describing the retired beacon-based admission behavior and migration considerations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…omes
Two failure modes that cost the most sweeps are invisible in today's
metrics:
- A stuck Electrum server (or a dead Ethereum subscription whose
CurrentBlock keeps returning a cached height) still passes the RPC
health check, so btc_connectivity/eth_connectivity stay 1 while every
wallet action fails. Expose the tip each health check sees and how
long since it advanced: rpc_{eth,btc}_latest_block_{number,height} and
rpc_{eth,btc}_block_stale_seconds. The Bitcoin tip is recorded before
the header check so servers returning the known-benign -32602 do not
look stale, and a lagging failover backend cannot move it backwards.
- A leader that always broadcasts a noop proposal (for example because
its ETH RPC cannot look back far enough to see pending deposits) is a
successful coordination, so no counter moves. Add
coordination_leader_proposals_total and
coordination_leader_noop_proposals_total; monitoring can join the noop
rate with on-chain pending work.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R49o3p6q8LrgPBv9hhZ8kZ
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Conflicts: - .github/workflows/contracts-ecdsa.yml: drop the random-beacon upstream query line dev removed together with the external sortition dependencies - .github/workflows/contracts-random-beacon.yml: keep dev actions versions across the board (lint uses setup-python@v4, slither and deployment jobs stay on v3/v4 as dev pinned them) - go.mod / go.sum: use the in-tree go-electrum fork dev added, drop the external go-electrum module that it replaces; keep the keep-common replace only where dev still needed it - pkg/chain/ethereum/beacon.go: take dev (post keep-common migration) and drop the beacon IsRecognized method that the branch retired - pkg/chain/ethereum/tbtc.go: apply the dev file-level split; move the branch admission seam into the split files - pkg/chain/ethereum/tbtc_sortition.go: route IsRecognized and EligibleStake through the branch admission reader - pkg/chain/ethereum/ethereum_integration_test.go: keep both import sets - pkg/clientinfo/rpc_health.go: keep dev probe-deadline handling, record tip staleness on success only - pkg/clientinfo/rpc_health_test.go: keep both new test sets
piotr-roslaniec
marked this pull request as ready for review
October 6, 2026 13:59
|
.dockerignore excluded the whole solidity/ tree from the Docker build context, so the go-build-env image carried no solidity/ecdsa/deploy-data fixtures. The admission-pinning integration tests read allowlist-weights-mainnet.json from the working tree, which is why client-integration-test failed on a red check instead of skipping. Exclude the tree and re-include only the deploy-data directory, so the two committed fixtures reach the image while the rest of solidity/ stays out of the context. Note: docker .dockerignore cannot re-include a path nested under an excluded directory (a lone negation after "solidity/**" is ignored), so the exclusion is written as solidity/** with the nested negations that re-add just the fixtures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two failure modes that cost many sweeps are invisible in today's metrics:
btc_connectivitystays 1 while every wallet action fails. Likewise,CurrentBlock()keeps returning a cached height after the Ethereum subscription dies (see Participation clock stays available when Ethereum subscription is dead #4245), so the ETH health check passes on a node that can't see the chain.What
Chain tip tracking (
pkg/clientinfo/rpc_health.go)New gauges under the existing
performancesource:rpc_eth_latest_block_numberrpc_eth_block_stale_secondsrpc_btc_latest_block_heightrpc_btc_block_stale_secondsGetLatestBlockHeightreturns a non-zero height, before the header check, so servers that answer the known-benign -32602 (nocp_heightsupport) don't look stale.Leader outcomes (
pkg/tbtc/coordination.go,pkg/clientinfo/performance.go)coordination_leader_proposals_total: incremented when this node, as leader, broadcasts a proposal.coordination_leader_noop_proposals_total: the subset that were noop.Monitoring can join the noop share with on-chain pending work to flag silent leaders.
Verification evidence
go test ./pkg/clientinfo/ ./pkg/tbtc/passes.rpc_health_test.go:CurrentBlockkeeps succeeding with the same heightTestCoordinationExecutor_Coordinatenow asserts that only the leader counts a (non-noop) proposal. I confirmed it fails without thecoordination.gochange.gofmtandgo vetare clean for both packages.Notes for reviewers
Metric names follow the existing
rpc_*andcoordination_*naming. I couldn't find a metrics reference doc in the repo to update.Agent involvement
Written by Claude Code at piotr-roslaniec's request. Draft for human review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01R49o3p6q8LrgPBv9hhZ8kZ
Generated by Claude Code