fix: address multi-agent review findings from testnet4 deployment PR (#3932)#3949
Merged
lionakhnazarov merged 13 commits intofeat/testnet4-deployment-supportfrom May 5, 2026
Merged
Conversation
Validate that activeThreshold/groupQuorum is not less than groupThreshold/honestThreshold when reading group parameters from EcdsaDkgValidator. Malformed on-chain data could otherwise produce contradictory protocol parameters that pass the individual range checks while being internally inconsistent.
The Sepolia/Developer branch already logs the parameters it chooses. Add the same log in the default (mainnet) branch so operators can verify the group sizing on startup regardless of network.
Replace the inline anonymous interface assertion with a named package-level interface. This makes the contract explicit, enables jump-to-definition, and follows standard Go interface naming conventions.
Cleanup of cancelled watchers happens on the next block broadcast, not immediately on context cancellation. Document this so callers understand why channels may remain open after cancellation in no-block scenarios. Also add defer ticker.Stop() for hygiene (count runs as a long-lived goroutine, but the deferred stop is correct and silences the linter).
…Alive 1. Replace string-based -32603 detection with a typed feeOracleFailure sentinel. getFeeBtcPerKbOnce classifies oracle failures at the source; isElectrumFeeOracleFailure uses errors.As for type-safe detection. 2. Stop the previous ticker before reassigning in keepAlive. Each successful ping resets the interval by creating a new ticker; without Stop() the old ticker's goroutine and timer resource leaked until GC.
Add network bitcoin.Network parameter to Connect so the connection knows which network it is operating on. EstimateSatPerVByteFee now returns an error instead of the 2 sat/vByte static fallback when all fee oracle queries fail on mainnet. The fallback remains active on testnet4 and other non-mainnet networks where quiet mempools are expected. All call sites updated to pass the bitcoin network from config.
…ined Using context.Background() blocked indefinitely if a retarget transaction was never mined (e.g. network congestion, dropped mempool). Replace with a 30-minute deadline so the maintainer goroutine can detect a stuck transaction and return an error instead of blocking shutdown.
Before calling the hand-maintained ABI getters on EcdsaDkgValidator, verify that the target address actually contains contract code. Without this check, an EOA address or a wrong deployment address causes a cryptic ABI decode failure instead of an actionable error message.
When IdleOnPreflightFailure is true, the maintainer silently idles on min-difficulty epochs with no signal for operators monitoring health. Track consecutive idle ticks and emit Errorf after 20 ticks (~20 minutes with default IdleBackOffTime=60s) so the LightRelay falling behind the Bitcoin chain surfaces in monitoring. Reset the counter when an epoch is successfully proven.
…pdate needed On non-mainnet networks, EcdsaDkgValidator is redeployed whenever bytecode changes (e.g. groupSize 100→3). Emit a log warning that includes the old address so operators know to update the WalletRegistry to point at the new validator before the system is used.
…ript The ABI check (ifaceHasFunction) is the authoritative guard for whether approveApplication exists on TokenStaking. The subsequent try/catch for 'No method named approveApplication' is unreachable given the guard and could silently swallow genuine revert errors. Remove it to make error handling deterministic.
- gofmt: electrum_test.go, tbtc.go, bitcoin_difficulty_test.go - eslint: replace bare template literal with double-quoted string in 02_deploy_dkg_validator.ts (line 29, no substitution)
piotr-roslaniec
changed the base branch from
main
to
feat/testnet4-deployment-support
…and DKG validator parameters Extract recordIdleTick/resetIdleTicks from proveEpochs loop for deterministic counter testing. Narrow internal ecdsa_dkg_validator_chain functions to bind.ContractCaller (2 methods) to enable a lightweight mock. Extract feeOracleFallback from EstimateSatPerVByteFee for direct testability. Tests added: - btcdiff: idle counter increments to threshold, resets, and re-escalates - ecdsa_dkg_validator_chain: zero address, no-code preflight, quorum<threshold invariant, and happy-path parameter extraction - electrum: isElectrumFeeOracleFailure unwrapping, mainnet refusal, and non-mainnet static fallback
lionakhnazarov
approved these changes
May 5, 2026
Collaborator
lionakhnazarov
left a comment
lionakhnazarov
left a comment
There was a problem hiding this comment.
Did not find correctness regressions in the reviewed changes. No blocking issues found in the follow-up delta.
Looks good to me
lionakhnazarov
merged commit 53009af
into
feat/testnet4-deployment-support
22 of 23 checks passed
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
2 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.
Summary
Follow-up to #3932 (
feat/testnet4-deployment-support) addressing all valid findings from the multi-agent review. Each fix is a separate commit for bisectability.fix(ethereum): Replacecontext.Background()with 30-min timeout inwaitDeployBackendTransactionMinedto prevent indefinite blocking on stuck transactionsdocs(local_v1): Document lazy watcher cleanup and adddefer ticker.Stop()inlocalBlockCounter.count()fix(electrum): Gate static fee fallback on non-mainnet; addnetwork bitcoin.Networkparam toConnect(compiler-enforced, not config struct drift)fix(electrum): Replace string-based-32603detection with typedfeeOracleFailuresentinel (errors.As); fix ticker leak inkeepAlivefix(ecdsa): AddCodeAtpreflight before ABI calls inecdsaWalletGroupParametersFromValidatorto fail fast on wrong/undeployed addressrefactor(tbtc): Extract namedgroupParametersProviderinterface (was anonymous inline)fix(btcdiff): Escalate idle log fromInfoftoErrorfafter 20 consecutive idle ticks; reset counter on proven epochfix(ecdsa): AddgroupQuorum >= honestThresholdinvariant check after existing range checksfix(deploy): Log warning whenEcdsaDkgValidatoris redeployed on non-mainnet (WalletRegistry must be updated to point to new address)fix(deploy): Remove unreachable try/catch in07_approve_wallet_registry.ts; theifaceHasFunctionABI guard is the authoritative checkTest plan
go test ./pkg/...cd solidity/ecdsa && npx hardhat compileConnectsignature (network param added)🤖 Generated with Claude Code