Repository navigation
feat(tecdsa): adopt hardened tss-lib in legacy mode - #4349
Draft
piotr-roslaniec wants to merge 25 commits into
Draft
piotr-roslaniec wants to merge 25 commits into
piotr-roslaniec wants to merge 25 commits into
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
…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 -->
Point the tss-lib replace at threshold-network/tss-lib dev (f8bbaff), which makes three caller obligations mandatory: - select the proof-transcript mode before constructing each keygen and signing party: this release hardcodes ProtocolModeLegacy, which keeps the historical wire transcript and uses no session nonce; - pass the signed message's fixed byte width (32, the Bitcoin sighash) to every signing party. Add the tbtc.legacyHistoricalBobCompatibility node flag (default off). When enabled, signing re-admits round-3 Bob/BobWC proofs from signers still running the pre-hardening tss-lib, and the node logs a startup warning. The flag is scheduled for removal in the next release. tss-lib now uses btcec/v2's secp256k1 curve object, while keep-core parses wallet keys with its vendored btcec. PrivateKeyShare.PublicKey returns secp256k1 keys on keep-core's curve object so keys for the same wallet compare equal. go.mod moves to Go 1.25.7 as required by the tss-lib fork; the Docker build stages keep their base images and set GOTOOLCHAIN=auto to fetch it.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
5 tasks
Bump the tss-lib pin to 6056170 (threshold-network/tss-lib#39), whose signing end channel now carries *common.SignatureData. Receiving the protobuf message by value copied its embedded mutex; go vet ./pkg/tecdsa/... is now clean.
This branch has not been deployed
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
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.
Adopts the hardened
threshold-network/tss-lib(devat6056170, tracked formasterin threshold-network/tss-lib#37). Draft: the pin moves to a tagged tss-lib release once threshold-network/tss-lib#37 merges.Changes
replacenow points atthreshold-network/tss-lib6056170. The fork requires Go 1.25.7, sogo.modmoves from Go 1.24 to 1.25.7, along with the indirect dependency bumps it forces (golang.org/x/crypto0.32 → 0.52, among others).pkg/tecdsa/dkg/member.go,pkg/tecdsa/signing/member.go): the hardened tss-lib refuses to construct a party until a transcript mode is chosen. This release hardcodestss.ProtocolModeLegacy, which keeps the historical wire transcript and uses no session nonce, so upgraded nodes can sign together with nodes that haven't upgraded yet. Security-v2 is out of scope here.fullSigningMessageBytes = 32(the Bitcoin sighash), which every signer must agree on.tbtc.legacyHistoricalBobCompatibility(--tbtc.legacyHistoricalBobCompatibility, defaultfalse): while some signers still run the pre-hardening tss-lib, their round-3 Bob/BobWC proofs exceed the tightened bound and abort signing. Enabling the flag accepts them. When it is on, the node logs a startup warning that this widens verification and must be turned off once every signer is upgraded. The flag will be removed in the next release.pkg/tecdsa/key.go): tss-lib now uses btcec/v2's secp256k1 object, while keep-core parses wallet keys with its vendored btcec. Both describe the same curve, butecdsa.PublicKey.Equalandreflect.DeepEqualalso compare the curve object.PrivateKeyShare.PublicKey()therefore returns secp256k1 keys on keep-core's curve object, so keys for the same wallet compare equal. Without this, fivepkg/tbtctests fail: signer round trips, wallet registry loads, and coordination-window matching.*common.SignatureDataon the signing end channel;pkg/tecdsa/signingreceives it by pointer. This removes thego vetcopylocks finding thatmainalso has.GOTOOLCHAIN=autoso the Go 1.25.7 toolchain is fetched, because the official images default toGOTOOLCHAIN=local.Verification
go build ./...: pass.go vetoncmd,pkg/tbtc,pkg/tecdsa/...: pass.go test ./pkg/tecdsa/... ./pkg/tbtc/... ./pkg/tbtcpg/... ./cmd/...: pass, including the full DKG (239 s) and signing (73 s) protocol tests.docker build --check: no new warnings.Signing latency (dev tss-lib vs current pin)
Throwaway in-process benchmark: one DKG, then full signing ceremonies with 10 members and threshold 6. Two alternating runs per side, 5 signatures each, on the same 16-core host (load average 5–22, other tenants present).
2e712689)f8bbaff; threshold-network/tss-lib#39 changes only the result channel type)Signing is 1.30x slower, within the agreed 2x budget. The cost comes from constant-time Paillier and MtA arithmetic. That work grows with group size, so at the production size of 100 members the ratio could move toward the per-operation Paillier ratio, which is about 2.1x for decryption. 100 members wasn't feasible on this host. Re-measure at a larger group on dedicated hardware before release.
Rollout