feat(testnet4)!: Bitcoin testnet4 (BIP-94) support — bridge SPV, breaking SDK switch, deploy patches - #949
Merged
lionakhnazarov merged 84 commits intoJun 19, 2026
Conversation
…rget logic Added a new constant to handle minimum difficulty blocks in Bitcoin testnet scenarios. Updated the retarget logic to accommodate this new target, ensuring that the relay can correctly validate post-retarget conditions. Enhanced test coverage to verify behavior with the new difficulty target in various scenarios.
- Updated to load environment variables from a parent directory and set timeouts and gas prices based on environment variables. - Introduced new deployment scripts for Sepolia, including for initializing LightRelay with Bitcoin testnet genesis block. - Added deployment patches to handle existing deployments gracefully, including checks for already deployed contracts and avoiding redundant operations. - Implemented a script to apply patches to vendored deploy scripts, ensuring compatibility with existing deployment processes. - Enhanced the script for approving DKG results with additional command-line options and error handling.
…upport - Loaded environment variables from a parent directory to improve configuration management. - Updated deployment settings to include timeouts and gas prices based on environment variables. - Added support for Bitcoin Testnet4 in the Electrum client and updated network handling. - Introduced new error types and functions in the WalletRegistry contract for better governance and error handling. - Updated deployment artifacts for Sepolia with new contract addresses and transaction hashes.
lionakhnazarov
requested review from
Shadowfiend,
lrsaturnino,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
April 7, 2026 07:11
lrsaturnino
reviewed
Apr 13, 2026
lrsaturnino
left a comment
lrsaturnino
left a comment
Member
There was a problem hiding this comment.
yarn lint in typescript/ fails with 15 new errors across files introduced by this PR — all absent on main.
Affected files:
scripts/approve-dkg-result.ts— 3 prettier + 1 new-cap (line 170)scripts/e2e-deposit.ts— 2 prettierscripts/request-redemption.ts— 2 prettiersrc/lib/bitcoin/client-with-network-override.ts— 4 prettiersrc/lib/electrum/client.ts— 1 prettier + 2 valid-jsdoc (line 280, missing@returnsand@param electrumongetNetworkFromElectrum)
12 of the 15 are auto-fixable with yarn lint --fix. The remaining 3 need manual attention: the new-cap violation in approve-dkg-result.ts:170 and the two JSDoc gaps in client.ts:280.
- Added new-cap rule exceptions for specific constructors in ESLint configuration to accommodate BigNumber usage. - Introduced a new .yarnrc.yml file to set the node linker to node-modules for Yarn. - Updated package.json to remove duplicate dependency entry for @mysten/sui. - Refactored various scripts for improved readability and consistency in logging.
- Updated package.json and yarn.lock to change the version of @solana/web3.js from ^1.98.0 to ~1.97.0 for better compatibility with existing code.
…tions - Updated package.json to include a resolutions field for secp256k1 version 4.0.3. - Modified yarn.lock to reflect the updated version of secp256k1 and its dependencies, including elliptic.
…kage.json - Introduced an overrides field in package.json to ensure secp256k1 version 4.0.3 is used consistently across the project.
…olutions - Updated package.json to specify secp256k1 version 4.0.4 in the resolutions field. - Modified yarn.lock to reflect the updated version of secp256k1 and its dependencies.
…lows - Updated Node.js version from 14.x to 18.15.0 in multiple workflow jobs. - Upgraded setup-node action from v3 to v4 for improved compatibility. - Added Corepack enablement step to utilize Yarn from the lockfile. - Adjusted package.json to reflect the new Node.js engine requirement and added Yarn version specification.
…pport - Introduced .pnp.cjs and .pnp.loader.mjs files to enable Yarn Plug'n'Play (PnP) functionality. - Updated package.json to remove the overrides for secp256k1 and adjusted resolutions accordingly. - Modified yarn.lock to reflect the new dependencies and their versions, including elliptic and node-addon-api.
…dling - Updated package.json to specify Yarn version 4.12.0. - Modified .pnp.cjs to include a new pnpZipBackend configuration for improved zip handling. - Refactored the read method in FileHandle class to support both Buffer and ArrayBufferView types. - Introduced LibZipImpl class for enhanced zip file management and error handling.
- Added .yarn/install-state.gz to .gitignore to exclude Yarn 4 local state files. - Removed obsolete .pnp.cjs and .pnp.loader.mjs files as they are no longer needed for the project.
- Introduced function to handle ether balance checks for contracts in fraud tests. - Added function to streamline assertions in redemption tests, ensuring consistent comparison of proposal tuples. - Updated multiple test cases to utilize these new utility functions for improved readability and maintainability. - Refactored gas reimbursement checks in BTC depositor tests for clarity and consistency.
…nd yarn.lock - Added @solana/web3.js version 1.97.0 to the resolutions and overrides fields in package.json. - Updated yarn.lock to reflect the new version of @solana/web3.js and its dependencies.
- Reorganized environment variable loading in hardhat.config.ts for consistency. - Enhanced formatting in deployment scripts for better readability, including consistent indentation and line breaks. - Updated log messages for clarity in various deployment patches. - Removed unnecessary strict mode declarations from deployment scripts.
…thod data - Expanded gasReporterOutput.json to include detailed method information for various contracts. - Added gas data and call counts for methods in BTCUtils, EcdsaDkgValidator, and WalletRegistry. - Enhanced the structure of the output file for better readability and analysis of gas usage.
- Updated the gas limit in MaintainerProxy tests to allow for a small headroom, ensuring more accurate gas usage measurements across different environments. - This change enhances the reliability of gas consumption assertions in the test suite.
- Introduced new Bitcoin address configurations for testnet4, including P2PKH, P2WPKH, P2SH, and P2WSH types. - Added corresponding redeemer output scripts and scriptPubKeys for each address type. - Enhanced the btcAddressFromPublicKey mapping with testnet4 public keys and addresses for improved testing coverage.
- Added missing newline characters at the end of Bridge.json, TBTC.json, TBTCVault.json, and WalletRegistry.json to comply with JSON formatting standards. - This change ensures better compatibility with various tools and prevents potential issues during file parsing.
… details - Expanded gasReporterOutput.json to include a comprehensive list of methods and their respective gas data for multiple contracts. - Enhanced the output structure for improved readability and analysis of gas usage metrics across the project.
- Add BREAKING CHANGE notice to initializeSepolia() JSDoc: testnet3 to testnet4 migration is silent at compile/runtime due to shared address prefixes (tb1/m/2) -- callers must update their Bitcoin tooling - Narrow Electrum server.features error swallow to JSON-RPC -32601 only; previously caught any error containing "not found" - Wrap Promise.all in getTransactionHistory with try/catch to surface per-transaction fetch errors with context - Reorder testnet4 Electrum URLs: SSL endpoints first, TCP last - Remove --private-key CLI flag from approve-dkg-result.ts; private keys in argv are visible in ps aux and shell history; use PRIVATE_KEY env var - Make loadWalletRegistryAbi async and use fs.promises.readFile; wrap JSON.parse in try/catch for better error messages - Fix NaN propagation in hardhat.config.ts gasPrice when GAS_PRICE_GWEI is absent or malformed; default to 5 gwei - Add version pins to all ten deploy-patches header comments; replace fs.writeFileSync with fs.promises.writeFile and add try/catch for JSON.parse in 07_deploy_token_staking.js - Add .ubsignore to suppress vendor coding-style warnings in deploy-patches - Add break; semicolons to switch cases (UBS requires semicolons on break)
- Updated the Node.js setup action from v3 to v4 and changed the node version from 18 to 20.19.0 across multiple jobs in the cross-chain-arbitrum.yml workflow for improved compatibility and performance.
…d Yarn Berry - Upgraded actions in the deployment-artifacts.yml workflow to use Node.js setup v4 and Yarn version 4.12.0. - Changed the Node.js version to 20.19.0 for improved compatibility. - Updated the yarn install command to use the --immutable flag for Yarn Berry. - Added corepack prepare command to ensure the correct Yarn version is activated before installation.
- Updated the deployment function name from a generic `async function` to `async function runDeployment` in multiple deployment scripts for improved clarity and consistency. - Added missing newline at the end of the `.gitignore` file to ensure proper formatting.
When the Electrum server does not implement server.features (e.g. mempool-electrs), the network is detected by hashing the block 0 header. computeHash256 returns the raw double-SHA256, but fromGenesisHash compares against the byte-reversed display form, so every network resolved to Unknown and deposit flows failed with "network not supported".
|
Warning Review limit reached
More reviews will be available in 2 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR updates CI and package-manager setup, adds Sepolia/testnet4 contract and deployment support, and introduces new TypeScript Bitcoin/Electrum behavior, scripts, and documentation. ChangesCI and workspace setup
Solidity contracts, deploy scripts, and Sepolia artifacts
TypeScript Bitcoin and Electrum updates
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
typescript/api-reference/classes/TBTCCore.md (1)
223-254:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
initializeSepoliathrows text to say Sepolia, not mainnet.This section now correctly documents Bitcoin testnet4, but Lines 249-250 still say the method throws unless the signer is on Ethereum mainnet. That contradicts
typescript/src/services/tbtc-core.ts:77-102, whereinitializeSepoliabinds toChains.Ethereum.Sepolia, and it will send users down the wrong path when troubleshooting chain mismatches.🤖 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 `@typescript/api-reference/classes/TBTCCore.md` around lines 223 - 254, Update the Throws documentation for initializeSepolia to reference Sepolia instead of Ethereum mainnet: change the sentence that currently reads "Throws an error if the signer's Ethereum network is other than Ethereum mainnet" to state that it throws if the signer is not on Chains.Ethereum.Sepolia (or Sepolia), matching the implementation in initializeSepolia which binds to Chains.Ethereum.Sepolia; ensure the wording clearly says the signer must be on Sepolia.typescript/src/lib/electrum/client.ts (1)
198-235:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRetry each connection attempt with a fresh
Electruminstance.
p-timeoutonly rejects the wrapper promise; it does not cancelelectrum.connect(). Because the retrier reuses the sameelectrumobject, a timed-out or half-open attempt can keep running underneath the next retry, leaking sockets or racing the client into a bad state. Build/close a new client inside each retry attempt and return the one that actually connected.Proposed direction
- const electrum: Electrum = new Electrum( - credentials.host, - credentials.port, - credentials.protocol, - this.options, - credentials.path - ) - - try { - await this.withBackoffRetrier()(async () => { - return pTimeout( - (async () => { - try { - await electrum.connect("tbtc-v2", "1.4.2") - await electrum.server_ping() - return - } catch (error) { - throw new Error( - `Electrum server connection failure: [${error}]` - ) - } - })(), - this.connectionTimeout, - `timed out on electrum connect after ${this.connectionTimeout} ms` - ) - }) - return electrum - } catch (err) { - try { - electrum.close() - } catch { - // Ignore close errors (e.g. Node TLS destroySSL race) - } - throw err - } + return this.withBackoffRetrier<Electrum>()(async () => { + const electrum: Electrum = new Electrum( + credentials.host, + credentials.port, + credentials.protocol, + this.options, + credentials.path + ) + + try { + await pTimeout( + (async () => { + await electrum.connect("tbtc-v2", "1.4.2") + await electrum.server_ping() + })(), + this.connectionTimeout, + `timed out on electrum connect after ${this.connectionTimeout} ms` + ) + + return electrum + } catch (err) { + try { + electrum.close() + } catch {} + throw err + } + })🤖 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 `@typescript/src/lib/electrum/client.ts` around lines 198 - 235, The current code constructs a single Electrum instance before retries which can leak sockets because pTimeout doesn't cancel electrum.connect; instead, move construction of the Electrum client into the retrier callback (create a new Electrum inside the async function passed to this.withBackoffRetrier()), call pTimeout around the connect+server_ping calls, and ensure any failed or timed-out attempt closes its own electrum (call electrum.close() in the attempt's catch/finally) so only the successfully connected Electrum instance is returned by the outer function; update references to Electrum, withBackoffRetrier(), pTimeout, connectionTimeout, electrum.connect and electrum.close accordingly.solidity/package.json (1)
65-65:⚠️ Potential issue | 🟠 MajorMake
postinstallfail when the Hardhat undici timeout patch can’t be applied
solidity/package.jsonkeeps"hardhat": "^2.10.0"butpostinstallrunssolidity/scripts/patch-hardhat-undici-connect-timeout.sh, which patchesnode_modules/hardhat/internal/core/providers/http.jsonly if it finds the exact stringthis._dispatcher = client ?? new Pool(url.origin);. When that string isn’t present, the script logs an error and stillprocess.exit(0), allowing installs/deploys to proceed with the original (unpatched) timeout behavior.Pin
hardhatto the tested version and/or change the script toexit non-zerowhen the pattern isn’t found so the fix can’t silently skip.🤖 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 `@solidity/package.json` at line 65, The postinstall currently can silently skip the Hardhat undici timeout patch; update solidity/package.json to pin "hardhat" to the tested version (e.g., the exact version used in CI) and modify the solidity/scripts/patch-hardhat-undici-connect-timeout.sh script to exit non-zero when the target pattern ("this._dispatcher = client ?? new Pool(url.origin);") is not found so the install fails loudly; ensure the postinstall step references the same script and that the script prints a clear error and returns a non-zero exit code when the patch cannot be applied.
🧹 Nitpick comments (2)
.github/workflows/cross-chain-arbitrum.yml (1)
54-55: ⚡ Quick winActivate specific Yarn version across all cross-chain workflows.
The cross-chain workflows (
cross-chain-arbitrum.yml,cross-chain-bob.yml,cross-chain-optimism.yml,cross-chain-polygon.yml, andcross-chain-sei.yml) runcorepack enablebut do not explicitly activate Yarn 4.12.0 withcorepack prepare yarn@4.12.0 --activate. Without this, Corepack uses its default Yarn version rather than the version specified in workspacepackage.jsonfiles. This is inconsistent withcontracts.ymlanddeployment-artifacts.yml, which do activate the specific version. The shared root cause is incomplete Corepack setup; the fix is to addcorepack prepare yarn@4.12.0 --activatein all affected jobs across all five workflows.🤖 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 @.github/workflows/cross-chain-arbitrum.yml around lines 54 - 55, Add an explicit activation of Yarn 4.12.0 after enabling Corepack in each cross-chain workflow: replace the single "corepack enable" step in cross-chain-arbitrum.yml, cross-chain-bob.yml, cross-chain-optimism.yml, cross-chain-polygon.yml and cross-chain-sei.yml by adding a second command "corepack prepare yarn@4.12.0 --activate" (i.e., run corepack enable then corepack prepare yarn@4.12.0 --activate) in every job that currently only runs corepack enable so Corepack uses the pinned Yarn version..github/workflows/contracts.yml (1)
88-88: ⚡ Quick winConsider using
--immutablefor consistency.Several jobs in this workflow use
yarn install --immutable(deployment-dry-run, deployment-testnet, etherscan-verification, dapp-development-deployment-testnet) to enforce lockfile integrity, butcontracts-build-and-test,contracts-format, andcontracts-slitheruse plainyarn install. This inconsistency means some CI jobs allow lockfile modifications while others don't.If lockfile integrity is important, consider using
--immutablein all jobs.Also applies to: 406-406, 464-464
🤖 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 @.github/workflows/contracts.yml at line 88, Update the inconsistent Yarn installs in the CI by replacing plain "yarn install" with "yarn install --immutable" in the jobs contracts-build-and-test, contracts-format, and contracts-slither so they enforce lockfile integrity consistently with deployment-dry-run, deployment-testnet, etherscan-verification, and dapp-development-deployment-testnet; ensure the run steps that currently use "yarn install" are changed to "yarn install --immutable" (and verify the workspace has an up-to-date lockfile to avoid CI failures).
🤖 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 @.github/workflows/contracts.yml:
- Around line 430-433: The contracts-slither job currently enables Corepack
twice by running the same step ("Enable Corepack (Yarn from lockfile)") and
invoking `corepack enable` / `corepack prepare yarn@4.12.0 --activate` a second
time; remove the redundant invocation so Corepack is enabled only once (keep the
first occurrence) or consolidate into a single step, ensuring the duplicate
`corepack enable` and `corepack prepare yarn@4.12.0 --activate` commands are
deleted from the extra step.
In @.github/workflows/format.yml:
- Around line 16-21: Update the actions/setup-node@v3 step to use Node 18.x
instead of 14 by changing the node-version value (e.g., from "14" to "18.x") in
the format.yml workflow; keep the existing "Enable Corepack" step (corepack
enable) intact. Also apply the same node-version change in the yearn.yml
workflow so both workflows match the other CI files using Node 18.x and provide
stable Corepack support.
In `@solidity/contracts/bridge/BitcoinTx.sol`:
- Around line 244-279: The loop in determineRequestedDifficulty iterates and
calls extractTargetAt on bitcoinHeaders before the header blob structure is
validated, which can cause low-level reverts when the input has trailing bytes;
fix by performing structural validation up-front (e.g.,
require(bitcoinHeaders.length % 80 == 0) or call validateHeaderChain(...) before
the for-loop) so you never call extractTargetAt on a partial header, and apply
the same guard to the analogous loop/logic around the other header-scan block
referencing extractTargetAt.
In `@solidity/deploy-patches/04_deploy_random_beacon.js`:
- Around line 21-31: The script currently deploys BLS, BeaconAuthorization,
BeaconDkg, BeaconInactivity and RandomBeacon before verifying pool ownership;
change it to read BeaconSortitionPool.owner() at the very start (using
deployments.read("BeaconSortitionPool", {}, "owner") or similar) and abort
immediately unless the owner equals the expected deployer or the known prior
RandomBeacon address (randomBeaconFromPriorPhase.address). Move this ownership
check to before any deploy calls for BLS, BeaconAuthorization, BeaconDkg,
BeaconInactivity, and RandomBeacon (also apply the same early-abort check in the
similar block around lines 76-94) so no partial on-chain state or artifacts are
created when ownership prevents transfer.
In `@solidity/deploy-patches/05_transfer_t.js`:
- Around line 43-50: The branch that checks deployerBn.lt(needed) should not
silently log and continue because that indicates an unfunded shortfall (vmBn <
targetBn but deployer lacks funds); update the code handling for
deployerBn.lt(needed) in 05_transfer_t.js to fail loudly (throw an Error or exit
non‑zero) instead of continuing, and include contextual details
(from1e18(deployerBn), from1e18(needed), from1e18(vmBn), from1e18(targetBn)) in
the error message so the deployment stops and reports the precise shortfall;
locate the block using variables needed, deployerBn, vmBn, targetBn and replace
the log+continue with the throwing/exit logic.
In `@solidity/deploy-patches/07_approve_wallet_registry.js`:
- Around line 12-35: The script assumes TokenStaking has applicationInfo and
approveApplication, but some Threshold TokenStaking ABIs lack them; wrap the ABI
probe and calls in the same tolerant pattern used in
05_approve_random_beacon_in_token_staking.js: attempt to read
TokenStaking.applicationInfo (using read("TokenStaking", {}, "applicationInfo",
WalletRegistry.address)) inside a try/catch and if the call throws with a
missing-method/revert then log that this TokenStaking variant doesn’t support
application approvals and return/skip; only when the read succeeds parse status
and, if not already approved, call execute("TokenStaking", { from: deployer,...
}, "approveApplication", WalletRegistry.address) also guarded with try/catch to
handle absent approveApplication gracefully. Ensure you reference
TokenStaking.applicationInfo and approveApplication and mirror the exact
error-tolerant flow from the RandomBeacon patch.
In `@solidity/deploy-patches/07_deploy_token_staking.js`:
- Around line 30-63: Mainnet branch deploys a proxy but never registers it with
hardhat-deploy, so downstream calls to
deployments.get/read/execute("TokenStaking") will fail; update the mainnet path
after you obtain tokenStaking (tokenStakingAddress) and parsedAbi to call
deployments.save("TokenStaking", { address: tokenStakingAddress, abi: parsedAbi
}) (and include any relevant metadata/receipt if available) so hardhat-deploy
has a recorded deployment for TokenStaking; place this registration right after
writing TokenStaking.json and ensure you reference the tokenStaking
variable/address and parsedAbi when saving.
In `@solidity/deployments/sepolia/NuCypherToken.json`:
- Around line 509-710: The artifact NuCypherToken.json contains mismatched
metadata/storageLayout pointing to contracts/test/TestToken.sol:TestToken;
regenerate the artifact from the actual NuCypherToken compilation and deployment
so the metadata.compilationTarget, metadata.sources and storageLayout.contract
entries reflect the real NuCypherToken contract (remove or replace any
references to TestToken), by recompiling the NuCypherToken sources with the same
compiler settings used for deployment and re-emitting the JSON (ensure the
metadata JSON and storageLayout arrays reference the NuCypherToken contract name
and source files rather than contracts/test/TestToken.sol:TestToken).
In `@solidity/hardhat.config.ts`:
- Around line 138-140: Validate process.env.CHAIN_HTTP_TIMEOUT_MS before using
parseInt for the timeout in the Hardhat config: ensure the string consists only
of digits (a positive integer) and parse it safely, otherwise fall back to the
default 120000 or throw; update the timeout assignment that currently uses
parseInt(process.env.CHAIN_HTTP_TIMEOUT_MS, 10) so it rejects values like
"120_000" or "60s" and only accepts full numeric millisecond values, referencing
the timeout property and the CHAIN_HTTP_TIMEOUT_MS env var in your fix.
In `@solidity/test/relay/LightRelay.test.ts`:
- Around line 512-589: The tests mutate nbits (via setHeaderCompactBits /
concatenateHexStrings) which also changes the block hash so the failures prove
PoW failure rather than the post-retarget target check; fix by using header
fixtures whose mutated hash is precomputed to satisfy the chosen compact bits
(or by calling/asserting the target-validation helper directly) so the
retarget(retargetHeaders) path deterministically exercises the post-retarget
target validation in relay.retarget; locate tests using setHeaderCompactBits,
extractTargetFromHeaderHex, compactBitsToTarget and either replace the mutated
last-header with a fixture header whose hash is known to meet the altBits/DIFF1
target or add a helper assertion that invokes the same target-check function
used by retarget to validate the expected outcome deterministically.
In `@typescript/package.json`:
- Line 42: The postinstall script in package.json invokes "patch-package" but
patch-package is only listed under devDependencies and the package's "files"
array omits the patches/ directory, which breaks installs for consumers; to fix,
either remove the "postinstall": "patch-package" entry from package.json
(preferred for published libs) or move "patch-package" from devDependencies to
dependencies and add "patches/" to the package.json "files" whitelist so the
tarball contains patches/electrum-client-js+0.1.1.patch; update the package.json
scripts and dependency sections accordingly and ensure the patches/ path and the
exact patch filename (patches/electrum-client-js+0.1.1.patch) are included.
In `@typescript/scripts/request-redemption.ts`:
- Around line 13-19: The CHAIN_ID handling is misleading because the code always
loads Sepolia contracts (via loadEthereumCoreContracts(...,
Chains.Ethereum.Sepolia) and TBTC.initializeSepolia) regardless of CHAIN_ID;
update the initialization to either (a) derive the contract bundle from CHAIN_ID
(map CHAIN_ID to the correct Chains.Ethereum enum and call the matching
TBTC.initialize* and loadEthereumCoreContracts with that enum) or (b) add an
explicit guard that checks process.env.CHAIN_ID and throws/exit unless it equals
"11155111" so you fail-fast; locate and change the calls to
loadEthereumCoreContracts and TBTC.initializeSepolia and the CHAIN_ID usage to
implement one of these fixes.
In `@typescript/src/lib/electrum/urls/testnet4.json`:
- Around line 3-5: The default Testnet4 Electrum endpoints include a plaintext
TCP fallback ("tcp://134.199.227.217:50001") which makes insecure transport part
of ElectrumClient.fromDefaultConfig for BitcoinNetwork.Testnet4; remove the
plaintext "tcp://..." entry from the testnet4 endpoint list so the default
config only contains TLS (ssl://) endpoints, and leave plaintext TCP reachable
only via an explicit override mechanism (do not change
ElectrumClient.fromDefaultConfig logic—just delete the TCP URL from the testnet4
JSON list).
---
Outside diff comments:
In `@solidity/package.json`:
- Line 65: The postinstall currently can silently skip the Hardhat undici
timeout patch; update solidity/package.json to pin "hardhat" to the tested
version (e.g., the exact version used in CI) and modify the
solidity/scripts/patch-hardhat-undici-connect-timeout.sh script to exit non-zero
when the target pattern ("this._dispatcher = client ?? new Pool(url.origin);")
is not found so the install fails loudly; ensure the postinstall step references
the same script and that the script prints a clear error and returns a non-zero
exit code when the patch cannot be applied.
In `@typescript/api-reference/classes/TBTCCore.md`:
- Around line 223-254: Update the Throws documentation for initializeSepolia to
reference Sepolia instead of Ethereum mainnet: change the sentence that
currently reads "Throws an error if the signer's Ethereum network is other than
Ethereum mainnet" to state that it throws if the signer is not on
Chains.Ethereum.Sepolia (or Sepolia), matching the implementation in
initializeSepolia which binds to Chains.Ethereum.Sepolia; ensure the wording
clearly says the signer must be on Sepolia.
In `@typescript/src/lib/electrum/client.ts`:
- Around line 198-235: The current code constructs a single Electrum instance
before retries which can leak sockets because pTimeout doesn't cancel
electrum.connect; instead, move construction of the Electrum client into the
retrier callback (create a new Electrum inside the async function passed to
this.withBackoffRetrier()), call pTimeout around the connect+server_ping calls,
and ensure any failed or timed-out attempt closes its own electrum (call
electrum.close() in the attempt's catch/finally) so only the successfully
connected Electrum instance is returned by the outer function; update references
to Electrum, withBackoffRetrier(), pTimeout, connectionTimeout, electrum.connect
and electrum.close accordingly.
---
Nitpick comments:
In @.github/workflows/contracts.yml:
- Line 88: Update the inconsistent Yarn installs in the CI by replacing plain
"yarn install" with "yarn install --immutable" in the jobs
contracts-build-and-test, contracts-format, and contracts-slither so they
enforce lockfile integrity consistently with deployment-dry-run,
deployment-testnet, etherscan-verification, and
dapp-development-deployment-testnet; ensure the run steps that currently use
"yarn install" are changed to "yarn install --immutable" (and verify the
workspace has an up-to-date lockfile to avoid CI failures).
In @.github/workflows/cross-chain-arbitrum.yml:
- Around line 54-55: Add an explicit activation of Yarn 4.12.0 after enabling
Corepack in each cross-chain workflow: replace the single "corepack enable" step
in cross-chain-arbitrum.yml, cross-chain-bob.yml, cross-chain-optimism.yml,
cross-chain-polygon.yml and cross-chain-sei.yml by adding a second command
"corepack prepare yarn@4.12.0 --activate" (i.e., run corepack enable then
corepack prepare yarn@4.12.0 --activate) in every job that currently only runs
corepack enable so Corepack uses the pinned Yarn version.
🪄 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: 5b4c1157-f36a-4b5c-832d-c81ffce7a95a
⛔ Files ignored due to path filters (2)
solidity/yarn.lockis excluded by!**/yarn.lock,!**/*.locktypescript/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (138)
.github/workflows/contracts-docs.yml.github/workflows/contracts.yml.github/workflows/cross-chain-arbitrum.yml.github/workflows/cross-chain-bob.yml.github/workflows/cross-chain-optimism.yml.github/workflows/cross-chain-polygon.yml.github/workflows/cross-chain-sei.yml.github/workflows/deployment-artifacts.yml.github/workflows/format.yml.github/workflows/monitoring.yml.github/workflows/npm-contracts.yml.github/workflows/npm-typescript.yml.github/workflows/system-tests.yml.github/workflows/typescript.yml.github/workflows/yearn.yml.ubsignoresolidity/.eslintrcsolidity/.gitignoresolidity/.yarnrc.ymlsolidity/contracts/bridge/BitcoinTx.solsolidity/contracts/relay/LightRelay.solsolidity/contracts/test/SepoliaLightRelay.solsolidity/contracts/test/TestBitcoinTx.solsolidity/contracts/test/TestSepoliaLightRelay.solsolidity/deploy-patches/03_deploy_wallet_registry.jssolidity/deploy-patches/04_deploy_random_beacon.jssolidity/deploy-patches/05_approve_random_beacon_in_token_staking.jssolidity/deploy-patches/05_transfer_t.jssolidity/deploy-patches/07_approve_wallet_registry.jssolidity/deploy-patches/07_deploy_token_staking.jssolidity/deploy-patches/08_transfer_governance.jssolidity/deploy-patches/09_deploy_wallet_registry_governance.jssolidity/deploy-patches/10_transfer_governance.jssolidity/deploy-patches/13_authorize_in_random_beacon.jssolidity/deploy-patches/32_configure_tokenholder_timelock.jssolidity/deploy/06_deploy_bridge.tssolidity/deploy/08_deploy_donation_vault.tssolidity/deploy/09_deploy_bridge_governance.tssolidity/deploy/10_deploy_maintainer_proxy.tssolidity/deploy/11_bank_update_bridge.tssolidity/deploy/12_authorize_tbtc_vault_in_bridge.tssolidity/deploy/13_authorize_maintainer_proxy_in_bridge.tssolidity/deploy/19_authorize_spv_maintainer_in_bridge.tssolidity/deploy/20_transfer_bank_ownership.tssolidity/deploy/21_transfer_bridge_governance.tssolidity/deploy/22_transfer_bridge_governance_ownership.tssolidity/deploy/91_genesis_relay_sepolia.tssolidity/deployments/sepolia/.chainIdsolidity/deployments/sepolia/Allowlist.jsonsolidity/deployments/sepolia/BLS.jsonsolidity/deployments/sepolia/Bank.jsonsolidity/deployments/sepolia/BeaconAuthorization.jsonsolidity/deployments/sepolia/BeaconDkg.jsonsolidity/deployments/sepolia/BeaconDkgValidator.jsonsolidity/deployments/sepolia/BeaconInactivity.jsonsolidity/deployments/sepolia/BeaconSortitionPool.jsonsolidity/deployments/sepolia/Bridge.jsonsolidity/deployments/sepolia/BridgeGovernance.jsonsolidity/deployments/sepolia/BridgeGovernanceParameters.jsonsolidity/deployments/sepolia/BridgeTestnet4BitcoinTxSpvImplementation.jsonsolidity/deployments/sepolia/Deposit.jsonsolidity/deployments/sepolia/DepositSweep.jsonsolidity/deployments/sepolia/DonationVault.jsonsolidity/deployments/sepolia/EcdsaDkgValidator.jsonsolidity/deployments/sepolia/EcdsaInactivity.jsonsolidity/deployments/sepolia/EcdsaSortitionPool.jsonsolidity/deployments/sepolia/Fraud.jsonsolidity/deployments/sepolia/LightRelay.jsonsolidity/deployments/sepolia/LightRelayMaintainerProxy.jsonsolidity/deployments/sepolia/MaintainerProxy.jsonsolidity/deployments/sepolia/MovingFunds.jsonsolidity/deployments/sepolia/NativeBTCDepositor.jsonsolidity/deployments/sepolia/NuCypherToken.jsonsolidity/deployments/sepolia/RandomBeacon.jsonsolidity/deployments/sepolia/RandomBeaconChaosnet.jsonsolidity/deployments/sepolia/RandomBeaconGovernance.jsonsolidity/deployments/sepolia/Redemption.jsonsolidity/deployments/sepolia/RedemptionWatchtower.jsonsolidity/deployments/sepolia/ReimbursementPool.jsonsolidity/deployments/sepolia/T.jsonsolidity/deployments/sepolia/TBTC.jsonsolidity/deployments/sepolia/TBTCToken.jsonsolidity/deployments/sepolia/TBTCVault.jsonsolidity/deployments/sepolia/Timelock.jsonsolidity/deployments/sepolia/TokenStaking.jsonsolidity/deployments/sepolia/TokenholderGovernor.jsonsolidity/deployments/sepolia/TokenholderTimelock.jsonsolidity/deployments/sepolia/VendingMachine.jsonsolidity/deployments/sepolia/VendingMachineNuCypher.jsonsolidity/deployments/sepolia/VendingMachineV2.jsonsolidity/deployments/sepolia/VendingMachineV3.jsonsolidity/deployments/sepolia/WalletProposalValidator.jsonsolidity/deployments/sepolia/WalletRegistry.jsonsolidity/deployments/sepolia/WalletRegistryGovernance.jsonsolidity/deployments/sepolia/Wallets.jsonsolidity/hardhat.config.tssolidity/package.jsonsolidity/scripts/apply-solidity-contracts-export-deploy-patch.shsolidity/scripts/patch-hardhat-undici-connect-timeout.shsolidity/test/bridge/BitcoinTx.test.tssolidity/test/bridge/Bridge.Frauds.test.tssolidity/test/bridge/RedemptionWatchtower.test.tssolidity/test/cross-chain/wormhole/BTCDepositorWormhole.test.tssolidity/test/cross-chain/wormhole/L1BTCDepositorWormhole.test.tssolidity/test/depositor/NativeBTCDepositor.test.tssolidity/test/maintainer/MaintainerProxy.test.tssolidity/test/relay/LightRelay.test.tssolidity/test/relay/SepoliaLightRelay.test.tstypescript/.eslintrc.jstypescript/.yarnrctypescript/.yarnrc.ymltypescript/api-reference/README.mdtypescript/api-reference/classes/BitcoinClientWithNetworkOverride.mdtypescript/api-reference/classes/ElectrumClient.mdtypescript/api-reference/classes/TBTC.mdtypescript/api-reference/classes/TBTCCore.mdtypescript/api-reference/enums/BitcoinNetwork-1.mdtypescript/api-reference/interfaces/BitcoinClient.mdtypescript/api-reference/interfaces/ElectrumCredentials.mdtypescript/api-reference/modules/BitcoinNetwork.mdtypescript/package.jsontypescript/patches/electrum-client-js+0.1.1.patchtypescript/scripts/approve-dkg-result.tstypescript/scripts/e2e-deposit.tstypescript/scripts/request-redemption.tstypescript/src/lib/bitcoin/client-with-network-override.tstypescript/src/lib/bitcoin/index.tstypescript/src/lib/bitcoin/network.tstypescript/src/lib/electrum/client.tstypescript/src/lib/electrum/urls/testnet4.jsontypescript/src/lib/ethereum/artifacts/sepolia/Bridge.jsontypescript/src/lib/ethereum/artifacts/sepolia/TBTC.jsontypescript/src/lib/ethereum/artifacts/sepolia/TBTCVault.jsontypescript/src/lib/ethereum/artifacts/sepolia/WalletRegistry.jsontypescript/src/services/tbtc-core.tstypescript/src/services/tbtc.tstypescript/test/data/bitcoin.tstypescript/test/lib/bitcoin.test.ts
Comment on lines
+21
to
+31
| const BeaconSortitionPool = await deployments.get("BeaconSortitionPool") | ||
| const randomBeaconFromPriorPhase = await deployments.getOrNull("RandomBeacon") | ||
| if (randomBeaconFromPriorPhase) { | ||
| const poolOwner = await deployments.read("BeaconSortitionPool", {}, "owner") | ||
| if (helpers.address.equal(poolOwner, randomBeaconFromPriorPhase.address)) { | ||
| log( | ||
| "BeaconSortitionPool already owned by RandomBeacon from keep-core Phase D — skipping redeploy and transferOwnership" | ||
| ) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate BeaconSortitionPool ownership before deploying a new beacon.
If the pool was already transferred to an older RandomBeacon but the local deployments/RandomBeacon record is missing or stale, this script still deploys BLS, BeaconAuthorization, BeaconDkg, BeaconInactivity, and a fresh RandomBeacon before failing on the final ownership check. That leaves partial on-chain state and artifacts from a run that can never complete. Read BeaconSortitionPool.owner() up front and abort before any deploys unless it is still deployer or matches the reused beacon address.
Also applies to: 76-94
🤖 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 `@solidity/deploy-patches/04_deploy_random_beacon.js` around lines 21 - 31, The
script currently deploys BLS, BeaconAuthorization, BeaconDkg, BeaconInactivity
and RandomBeacon before verifying pool ownership; change it to read
BeaconSortitionPool.owner() at the very start (using
deployments.read("BeaconSortitionPool", {}, "owner") or similar) and abort
immediately unless the owner equals the expected deployer or the known prior
RandomBeacon address (randomBeaconFromPriorPhase.address). Move this ownership
check to before any deploy calls for BLS, BeaconAuthorization, BeaconDkg,
BeaconInactivity, and RandomBeacon (also apply the same early-abort check in the
similar block around lines 76-94) so no partial on-chain state or artifacts are
created when ownership prevents transfer.
Comment on lines
+512
to
+589
| context("with MIN_DIFFICULTY_TARGET in post-retarget window", () => { | ||
| let retargetHeaders: string | ||
|
|
||
| before(async () => { | ||
| await createSnapshot() | ||
|
|
||
| const baseRetargetHeaders = headerHex.slice(5, 13) // 8 headers total | ||
| const originalLastHeader = baseRetargetHeaders[7] | ||
|
|
||
| // Use DIFF1 `nbits` but keep the nonce from the fixture header. | ||
| // This will almost certainly fail the PoW check, which proves that | ||
| // the MIN_DIFFICULTY_TARGET exception does not bypass PoW validation. | ||
| const diff1LastHeader = setHeaderCompactBits( | ||
| originalLastHeader, | ||
| MIN_DIFFICULTY_BITS | ||
| ) | ||
|
|
||
| const modifiedHeaders = [ | ||
| ...baseRetargetHeaders.slice(0, 7), | ||
| diff1LastHeader, | ||
| ] | ||
| retargetHeaders = concatenateHexStrings(modifiedHeaders) | ||
| }) | ||
|
|
||
| after(async () => { | ||
| await restoreSnapshot() | ||
| }) | ||
|
|
||
| it("should revert with Invalid work", async () => { | ||
| await expect( | ||
| relay.connect(thirdParty).retarget(retargetHeaders) | ||
| ).to.be.revertedWith("Invalid work") | ||
| }) | ||
| }) | ||
|
|
||
| context("with an unexpected post-retarget target", () => { | ||
| let retargetHeaders: string | ||
|
|
||
| before(async () => { | ||
| await createSnapshot() | ||
|
|
||
| const baseRetargetHeaders = headerHex.slice(5, 13) // 8 headers total | ||
| const originalLastHeader = baseRetargetHeaders[7] | ||
|
|
||
| const minedTarget = extractTargetFromHeaderHex(baseRetargetHeaders[4]) | ||
|
|
||
| // Pick a very-easy compact bits value. We only mutate `nbits` | ||
| // (keep nonce/timestamp/merkle root), so PoW should pass, and | ||
| // the retarget logic should fail only at the post-retarget target | ||
| // comparison. | ||
| const altBits = "0x2100ffff" | ||
| const altTarget = compactBitsToTarget(altBits) | ||
|
|
||
| // Ensure the test meaning: it's neither the mined target nor DIFF1. | ||
| expect(altTarget).to.not.equal(minedTarget) | ||
| expect(altTarget).to.not.equal(MIN_DIFFICULTY_TARGET) | ||
|
|
||
| const altLastHeader = setHeaderCompactBits( | ||
| originalLastHeader, | ||
| altBits | ||
| ) | ||
| const modifiedHeaders = [ | ||
| ...baseRetargetHeaders.slice(0, 7), | ||
| altLastHeader, | ||
| ] | ||
| retargetHeaders = concatenateHexStrings(modifiedHeaders) | ||
| }) | ||
|
|
||
| after(async () => { | ||
| await restoreSnapshot() | ||
| }) | ||
|
|
||
| it("should revert with Unexpected target change after retarget", async () => { | ||
| await expect( | ||
| relay.connect(thirdParty).retarget(retargetHeaders) | ||
| ).to.be.revertedWith("Unexpected target change after retarget") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These new retarget negatives do not deterministically exercise the target-validation path.
Changing nbits also changes the block hash, so the DIFF1 case is effectively proving “mutated header fails PoW” rather than “DIFF1 is rejected after retarget,” and the altBits case is still flaky because it only reaches Unexpected target change after retarget when the new hash happens to satisfy the easier target. That leaves the new security rule under-tested and can intermittently fail CI. Please use a header fixture whose mutated hash is already known to satisfy the chosen target, or expose/assert the target-validation helper directly instead of relying on incidental PoW outcomes.
🤖 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 `@solidity/test/relay/LightRelay.test.ts` around lines 512 - 589, The tests
mutate nbits (via setHeaderCompactBits / concatenateHexStrings) which also
changes the block hash so the failures prove PoW failure rather than the
post-retarget target check; fix by using header fixtures whose mutated hash is
precomputed to satisfy the chosen compact bits (or by calling/asserting the
target-validation helper directly) so the retarget(retargetHeaders) path
deterministically exercises the post-retarget target validation in
relay.retarget; locate tests using setHeaderCompactBits,
extractTargetFromHeaderHex, compactBitsToTarget and either replace the mutated
last-header with a fixture header whose hash is known to meet the altBits/DIFF1
target or add a helper assertion that invokes the same target-check function
used by retarget to validate the expected outcome deterministically.
Comment on lines
+13
to
+19
| * CHAIN_ID=11155111 Sepolia default; set if your RPC uses another chain | ||
| * ELECTRUM_URL=tcp://... (same as e2e-deposit) | ||
| * BITCOIN_NETWORK=testnet4 (Electrum network override) | ||
| * REDEMPTION_MAX=1 redeem full TBTC balance (token 1e18 precision; amount | ||
| * is rounded down to whole satoshi) | ||
| * STRICT_REDEMPTION_AMOUNT=1 fail if REDEMPTION_AMOUNT_SAT exceeds TBTC balance | ||
| * (default: cap to your on-chain balance and continue) |
There was a problem hiding this comment.
CHAIN_ID changes the provider network, but the contract bundle is still fixed to Sepolia.
The script advertises CHAIN_ID as a way to use another chain, yet both initialization branches still load Sepolia contracts (loadEthereumCoreContracts(..., Chains.Ethereum.Sepolia) / TBTC.initializeSepolia). Against any non-Sepolia RPC this will read from and send to the wrong addresses. Either resolve the contract set from CHAIN_ID, or fail fast unless it is 11155111.
Minimal fail-fast fix
const chainId = parseInt(process.env.CHAIN_ID || "11155111", 10)
+ if (chainId !== 11155111) {
+ throw new Error(
+ "request-redemption currently supports Sepolia only; set CHAIN_ID=11155111 or add chain-specific contract resolution."
+ )
+ }
const provider = new ethers.providers.StaticJsonRpcProvider(rpcUrl, chainId)Also applies to: 102-146
🤖 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 `@typescript/scripts/request-redemption.ts` around lines 13 - 19, The CHAIN_ID
handling is misleading because the code always loads Sepolia contracts (via
loadEthereumCoreContracts(..., Chains.Ethereum.Sepolia) and
TBTC.initializeSepolia) regardless of CHAIN_ID; update the initialization to
either (a) derive the contract bundle from CHAIN_ID (map CHAIN_ID to the correct
Chains.Ethereum enum and call the matching TBTC.initialize* and
loadEthereumCoreContracts with that enum) or (b) add an explicit guard that
checks process.env.CHAIN_ID and throws/exit unless it equals "11155111" so you
fail-fast; locate and change the calls to loadEthereumCoreContracts and
TBTC.initializeSepolia and the CHAIN_ID usage to implement one of these fixes.
The testnet4 default config shipped a tcp:// endpoint consumed directly by ElectrumClient.fromDefaultConfig, putting unauthenticated transport on the default path for every testnet4/Sepolia caller. An on-path attacker could tamper with UTXO/SPV responses without the caller opting into insecure transport. Remove it so defaults stay on encrypted transports only, matching the mainnet and testnet configs. Add a regression test asserting every default Electrum config uses an encrypted transport (ssl/wss).
The postinstall step runs patch-package, which applies the bundled electrum-client-js patch that reorders the connection handshake (server.version before server.banner) required by Fulcrum/electrs servers, including the testnet4 default endpoints. As shipped, patch-package was in devDependencies (not installed for consumers) and patches/ was omitted from the files whitelist, so published consumers received an unpatched client and could not connect to the testnet4 default servers. Move patch-package to dependencies and add patches/ to files so the patch is present and applicable on consumer installs.
The deployerBn.lt(needed) branch logged and continued, masking a genuinely underfunded deploy as a successful idempotent replay. The true rerun case is already handled by the vmBn.gte(targetBn) branch, so reaching this point means the vending machine is below target and the deployer cannot cover the gap. Throw with the precise shortfall so the deploy stops instead of letting later phases run with less T than expected.
The script assumed TokenStaking.applicationInfo and approveApplication always exist and that the read always succeeds, so it threw unhandled on Threshold TokenStaking variants that lack those methods. Mirror the tolerant pattern already used in 05_approve_random_beacon_in_token_staking.js: probe the ABI for approveApplication, tolerate applicationInfo read failures, and handle absent or already-approved approveApplication, keeping this phase replay-safe.
The mainnet branch deploys TokenStaking via hardhat.upgrades.deployProxy and
writes a standalone TokenStaking.json, but deployProxy creates no hardhat-deploy
record. Downstream deploy-patches call deployments.get/read/execute("TokenStaking"),
which had nothing to bind to on mainnet. Call deployments.save to register the
proxy address and ABI, matching the non-mainnet path that registers via
deployments.deploy.
The script fell back to a hardcoded Bitcoin address when BTC_RECOVERY_ADDRESS was unset, silently routing deposit timeout refunds to an address the operator does not control. Fail fast when the variable is missing so refunds always go to an operator-controlled address.
parseInt accepted partial strings, so values like "120_000" or "60s" became 120 / 60 and silently reduced the RPC timeout to milliseconds, re-triggering the deploy timeouts this setting is meant to prevent. Parse the full value and fall back to the 120s default unless it is a positive integer, matching the gasPrice validation idiom in the same file.
determineRequestedDifficulty walked the header blob (calling extractTargetAt at each 80-byte offset) before any length validation. A non-multiple-of-80 input reverted with a low-level out-of-bounds panic on the trailing partial header instead of the explicit length error. Add a require(length % 80 == 0) guard so malformed proofs revert with a clear message. Every path already reverted, so there is no behavioral change beyond the error surfaced. Add a regression test covering a non-multiple-of-80 headers input.
The contracts-slither job enabled Corepack twice; remove the redundant bare `corepack enable` step. format.yml and yearn.yml ran on Node 14 (EOL, Corepack only experimental), which is unstable for Corepack-managed Yarn. Upgrade them to Node 20.19.0 and setup-node@v4 to match the other workflows.
initializeSepolia now connects to Bitcoin testnet4 instead of testnet3. Because both networks share address prefixes, the switch is silent at compile and run time, so it is a breaking change for existing callers. Bump the package to 4.0.0-dev so semver signals the break, and correct the JSDoc that referenced '(v2)' to '(v4)'.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/format.yml:
- Line 16: Replace all mutable tag references to actions/setup-node with their
immutable commit SHA equivalents to prevent supply-chain drift in CI. In
`.github/workflows/format.yml` line 16, replace `actions/setup-node@v4` with the
full SHA-pinned reference (e.g., `actions/setup-node@<commit-sha>`). In
`.github/workflows/yearn.yml` line 43, replace the `actions/setup-node@v4`
reference with its corresponding full commit SHA. In
`.github/workflows/yearn.yml` line 74, replace the `actions/setup-node@v4`
reference with its corresponding full commit SHA. Ensure all three locations use
the same commit SHA that corresponds to the v4 release of the action to maintain
consistency.
🪄 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: 0783a539-b176-4afe-a873-b4a834429415
📒 Files selected for processing (15)
.github/workflows/contracts.yml.github/workflows/format.yml.github/workflows/yearn.ymlsolidity/contracts/bridge/BitcoinTx.solsolidity/deploy-patches/05_transfer_t.jssolidity/deploy-patches/07_approve_wallet_registry.jssolidity/deploy-patches/07_deploy_token_staking.jssolidity/hardhat.config.tssolidity/test/bridge/BitcoinTx.test.tstypescript/package.jsontypescript/scripts/e2e-deposit.tstypescript/src/lib/electrum/urls/testnet4.jsontypescript/src/services/tbtc-core.tstypescript/src/services/tbtc.tstypescript/test/lib/electrum.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/contracts.yml
✅ Files skipped from review due to trivial changes (1)
- typescript/src/lib/electrum/urls/testnet4.json
🚧 Files skipped from review as they are similar to previous changes (8)
- solidity/deploy-patches/05_transfer_t.js
- solidity/deploy-patches/07_approve_wallet_registry.js
- typescript/package.json
- solidity/deploy-patches/07_deploy_token_staking.js
- solidity/hardhat.config.ts
- solidity/contracts/bridge/BitcoinTx.sol
- typescript/scripts/e2e-deposit.ts
- solidity/test/bridge/BitcoinTx.test.ts
| - uses: actions/checkout@v3 | ||
|
|
||
| - uses: actions/setup-node@v3 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
Pin GitHub Actions by commit SHA instead of mutable tags.
Using mutable tags (for example, @v4) on changed workflow steps leaves CI exposed to supply-chain drift. Please pin each action to an immutable commit SHA.
.github/workflows/format.yml#L16-L16: Replaceactions/setup-node@v4with a full SHA-pinned reference..github/workflows/yearn.yml#L43-L43: Replaceactions/setup-node@v4with a full SHA-pinned reference..github/workflows/yearn.yml#L74-L74: Replaceactions/setup-node@v4with a full SHA-pinned reference.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/format.yml#L16-L16(this comment).github/workflows/yearn.yml#L43-L43.github/workflows/yearn.yml#L74-L74
🤖 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 @.github/workflows/format.yml at line 16, Replace all mutable tag references
to actions/setup-node with their immutable commit SHA equivalents to prevent
supply-chain drift in CI. In `.github/workflows/format.yml` line 16, replace
`actions/setup-node@v4` with the full SHA-pinned reference (e.g.,
`actions/setup-node@<commit-sha>`). In `.github/workflows/yearn.yml` line 43,
replace the `actions/setup-node@v4` reference with its corresponding full commit
SHA. In `.github/workflows/yearn.yml` line 74, replace the
`actions/setup-node@v4` reference with its corresponding full commit SHA. Ensure
all three locations use the same commit SHA that corresponds to the v4 release
of the action to maintain consistency.
Source: Linters/SAST tools
Resync the generated api-reference with the updated initializeSepolia JSDoc (BREAKING CHANGE v4).
piotr-roslaniec
changed the title
Apply prettier formatting to the two files changed in this round that it flagged: collapse the testnet4 URL array to one line and wrap the CHAIN_HTTP_TIMEOUT_MS ternary that now exceeds the print width.
piotr-roslaniec
approved these changes
Jun 15, 2026
lionakhnazarov
added a commit
that referenced
this pull request
Jun 15, 2026
…tection) (#980) Follow-up fixes for #949, surfaced by a validated review pass against the current branch HEAD. Targets the PR branch so it folds into #949. ## 🔴 Headline: testnet4 network detection is broken by default Commit `d3b4cfda` (browser-compatible hash util) replaced the Node-crypto path with `computeHash256` but **dropped the byte reversal** that `06d06526` had added. `computeHash256` returns natural (little-endian) double-SHA256; the genesis-hash constants are display (big-endian) order. So the block-0 header fallback produced `43f08bda…` and never matched, returning `Unknown`. This is the *operative* path in the default config: **mempool.space is Electrs and does not implement `server.features`**, and it is the first default testnet4 endpoint — so the fallback runs every time and testnet4 detection has been broken since `d3b4cfda`. Restored the `.reverse()` and added unit tests covering both the `server.features` path and the block-0 fallback. Verified the test fails without the fix (`expected 'unknown' to equal 'testnet4'`) and passes with it. ## Other fixes - **`electrum`** — removed the plaintext `tcp://` endpoint from the default `testnet4.json`. `fromDefaultConfig` consumes it directly, so it made unauthenticated transport part of the default path.⚠️ This reverses the earlier decision to keep it reordered-last; flagging for the author — it's an availability-vs-security call. - **`bridge`** — `determineRequestedDifficulty` scans headers with `extractTargetAt` before `validateHeaderChain`; a non-80-aligned blob would revert with an opaque low-level error. Added a `length % 80 == 0` guard returning the explicit message, plus a regression test. `validateProof` gas unchanged. - **`deploy`** — `parseInt(CHAIN_HTTP_TIMEOUT_MS)` truncated `120_000` to `120` ms. Now parsed with `Number(...)` + positive-integer validation, falling back to the 120 s default. ## Verification - `typescript`: `electrum.test.ts` 16 passing; eslint + prettier clean. - `solidity`: `BitcoinTx.test.ts` 12 passing (incl. new length-guard test); contracts compile. The full Bridge suite could not run in the review environment (deployment fixture fails at `NuCypherToken` resolution, unrelated to these changes) — CI covers it. ## Deliberately not included - `server_features()` retry wrapper — wrapping it in `withBackoffRetrier` would retry the deterministic method-not-found on every Electrs connection (a latency regression). The fallback already handles it. - Semver bump for the Testnet→Testnet4 switch, mainnet `deployments.save("TokenStaking")`, and a full mixed-DIFF1 `validateProof` test — left for the author (release decision / needs mainnet-deploy-usage confirmation / larger test).
Merged
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
3 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
This PR delivers the full
feat/testnet4-deployment-supportdelta, including protocol/security fixes, deployment and artifact updates, TypeScript SDK/runtime improvements, and CI/workflow hardening needed to keep the branch mergeable.1) Bridge/Relay security and correctness (Testnet4 handling)
2) Electrum + TypeScript SDK robustness
BitcoinClientWithNetworkOverrideand updated SDK-facing docs/API references accordingly.3) Deployment pipeline and on-chain artifacts
4) Solidity deploy script lint/format compliance
func-names) by naming async deploy functions in affected scripts..gitignore) to satisfycontracts-format.5) CI/workflow hardening (multi-workflow)
20.19.0andactions/setup-node@v4where dependency engines required newer runtimes.--immutablevs Classic flags) to align with lockfile/tooling expectations per workflow.contracts.ymlcross-chain-arbitrum.ymldeployment-artifacts.ymlWhy
The branch had multiple independent breakpoints:
This PR brings those streams together so the feature branch is functionally correct, security-reviewed, and CI-stable.
Test plan
Notable areas touched
Review fixes (post-review hardening)
Findings from a multi-agent review of this PR, each addressed as its own commit:
tcp://endpoint from the testnet4 default Electrum config; added a regression test asserting every default config uses encrypted transport (ssl/wss).patch-packagemoved todependenciesandpatches/added tofiles[], so consumers receive and apply theelectrum-client-jshandshake patch required by the testnet4 default servers (otherwise published consumers cannot connect to testnet4).05_transfer_t.jsthrows on an unfunded vending-machine shortfall instead of masking it as a successful replay.07_approve_wallet_registry.jsguards the TokenStaking API (ABI probe + tolerant handling) for Threshold variants, mirroring the random-beacon approval patch.07_deploy_token_staking.jsregisters the mainnet proxy with hardhat-deploy (deployments.save) so downstream patches can resolve it.e2e-deposit.tsfails fast whenBTC_RECOVERY_ADDRESSis unset instead of routing refunds to a hardcoded address.determineRequestedDifficultyvalidates header-chain length before the scan (clean revert instead of a low-level panic); regression test added.hardhat.config.tsvalidatesCHAIN_HTTP_TIMEOUT_MSas a positive integer.format.yml/yearn.ymloff Node 14 to 20.19.0.4.0.0-dev, corrected theinitializeSepoliaJSDoc, and regenerated the API reference.Still open (tracked, not in this round): regenerate
solidity/deployments/sepolia/NuCypherToken.jsonfrom a real deployment — its embedded metadata currently referencescontracts/test/TestToken.sol:TestToken.Summary by CodeRabbit
New Features
Bug Fixes
Chores