From b2af3bcf072012b1b13eedd98781b1a728aea7c7 Mon Sep 17 00:00:00 2001 From: Rosco Kalis Date: Sat, 26 Sep 2026 21:40:25 +0200 Subject: [PATCH 1/3] fix: address SDK findings from the 0.14 audit - Reject hex string arguments with an odd number of digits or non-hex characters, which silently encoded to different bytes, and check that `pubkey` arguments are 33 or 65 bytes - Fix the ElectrumNetworkProvider no longer connecting for new requests after a failed request, when using automatic connection management - Compare the exact fee per byte against the minimum and maximum, instead of the fee per byte rounded to two decimals - Reject `setLocktime()` values that are not an unsigned 32-bit integer - Don't throw a FailedTransactionError from `send()` when the transaction was broadcast, but could not be retrieved afterwards - Report a failing final `require(variable)` at the require's own line, when its opcodes are optimised away - MockNetworkProvider: validate transactions when `updateUtxoSet` is false, reject block height locktimes above the mock block height, and compare hex strings case-insensitively - Split the test matcher types into cashscript/vitest (Vitest 5's `Matchers`) and cashscript/jest (`jest.Matchers`), since Vitest 5 no longer reads the jest namespace Co-Authored-By: Claude Opus 5.5 --- packages/cashscript/jest/package.json | 4 +- packages/cashscript/src/Argument.ts | 22 ++++++-- packages/cashscript/src/Errors.ts | 4 +- packages/cashscript/src/TransactionBuilder.ts | 22 +++++--- packages/cashscript/src/debug-frame.ts | 15 +++++ .../src/network/ElectrumNetworkProvider.ts | 6 +- .../src/network/MockNetworkProvider.ts | 40 +++++++++++--- .../cashscript/src/test/JestExtensions.ts | 12 ++++ .../cashscript/src/test/TestExtensions.ts | 14 +---- .../cashscript/src/test/VitestExtensions.ts | 11 ++++ packages/cashscript/test/Contract.test.ts | 14 ++++- .../test/TransactionBuilder.test.ts | 53 ++++++++++++++++++ packages/cashscript/test/debugging.test.ts | 15 +++++ .../network/ElectrumNetworkProvider.test.ts | 21 ++++++- .../e2e/network/MockNetworkProvider.test.ts | 55 ++++++++++++++++++- .../fixture/debugging/debugging_contracts.ts | 10 ++++ packages/cashscript/vitest.setup.ts | 2 +- packages/cashscript/vitest/package.json | 4 +- website/docs/releases/release-notes.md | 8 +++ website/docs/sdk/other-network-providers.md | 2 +- website/docs/sdk/transaction-builder.md | 2 +- 21 files changed, 288 insertions(+), 48 deletions(-) create mode 100644 packages/cashscript/src/test/JestExtensions.ts create mode 100644 packages/cashscript/src/test/VitestExtensions.ts diff --git a/packages/cashscript/jest/package.json b/packages/cashscript/jest/package.json index 4cdc1864a..f40996b53 100644 --- a/packages/cashscript/jest/package.json +++ b/packages/cashscript/jest/package.json @@ -1,5 +1,5 @@ { "type": "module", - "types": "../dist/test/TestExtensions.d.ts", - "main": "../dist/test/TestExtensions.js" + "types": "../dist/test/JestExtensions.d.ts", + "main": "../dist/test/JestExtensions.js" } diff --git a/packages/cashscript/src/Argument.ts b/packages/cashscript/src/Argument.ts index df6570a87..d0b860bb5 100644 --- a/packages/cashscript/src/Argument.ts +++ b/packages/cashscript/src/Argument.ts @@ -1,4 +1,4 @@ -import { hexToBin } from '@bitauth/libauth'; +import { hexToBin, isHex } from '@bitauth/libauth'; import { AbiFunction, Artifact, @@ -27,14 +27,14 @@ export type EncodeFunction = (arg: FunctionArgument, typeStr: string) => Encoded * - `bool`, `int`, `string` → encoded via their CashScript primitive encoders. * - `sig` passed as a `SignatureTemplate` → returned as-is so the transaction builder can * produce the signature at build time. - * - `sig` / `datasig` / `bytes[N]` passed as a `Uint8Array` or hex string → byte-checked and + * - `sig` / `datasig` / `pubkey` / `bytes[N]` passed as a `Uint8Array` or hex string → byte-checked and * returned as bytes. * * @param argument - The runtime argument value provided by the caller. * @param typeStr - The CashScript type string from the contract ABI (e.g. `int`, `bytes20`, `sig`). * @returns The encoded argument, ready for inclusion in an unlocking script. * @throws A `TypeError` when the JS type does not match the expected CashScript type, or when a - * bounded bytes type receives a value of the wrong length. + * bounded bytes type receives a value of the wrong length. An `Error` when a hex string is not valid hex. */ export function encodeFunctionArgument(argument: FunctionArgument, typeStr: string): EncodedFunctionArgument { let type = parseType(typeStr); @@ -64,11 +64,13 @@ export function encodeFunctionArgument(argument: FunctionArgument, typeStr: stri // Convert hex string to Uint8Array if (typeof argument === 'string') { - if (argument.startsWith('0x')) { - argument = argument.slice(2); + const hex = argument.startsWith('0x') ? argument.slice(2) : argument; + + if (!isHex(hex)) { + throw Error(`Value for type ${type} should be a valid hex string with an even number of digits, found '${argument}'`); } - argument = hexToBin(argument); + argument = hexToBin(hex); } if (!(argument instanceof Uint8Array)) { @@ -83,6 +85,14 @@ export function encodeFunctionArgument(argument: FunctionArgument, typeStr: stri type = new BytesType(argument.byteLength); } + // Redefine PUBKEY as a bytes33 (compressed) or bytes65 (uncompressed) + if (type === PrimitiveType.PUBKEY) { + if (![33, 65].includes(argument.byteLength)) { + throw new TypeError(`bytes${argument.byteLength}`, type); + } + type = new BytesType(argument.byteLength); + } + // Redefine DATASIG as a bytes64 (Schnorr) or bytes70, bytes71, bytes72 (ECDSA) or bytes0 (for NULLFAIL) if (type === PrimitiveType.DATASIG) { if (![0, 64, 70, 71, 72].includes(argument.byteLength)) { diff --git a/packages/cashscript/src/Errors.ts b/packages/cashscript/src/Errors.ts index 9dc6ed82c..ad4c40e10 100644 --- a/packages/cashscript/src/Errors.ts +++ b/packages/cashscript/src/Errors.ts @@ -3,6 +3,7 @@ import { CallStackEntry, ResolvedFrame, getLocationDataForFrame, + getRequireLocationDataForFrame, resolveInlineAttribution, rootFrame, } from './debug-frame.js'; @@ -189,8 +190,9 @@ export class FailedRequireError extends FailedTransactionError { const inline = resolveInlineAttribution(artifact, resolvedFrame, requireStatement, 'requires'); const attributedFrame = inline?.frame ?? resolvedFrame; const attributedIp = inline?.entry.ip ?? failingInstructionPointer; + const requireLine = inline?.entry.line ?? requireStatement.line; - const { statement, lineNumber } = getLocationDataForFrame(attributedFrame, attributedIp); + const { statement, lineNumber } = getRequireLocationDataForFrame(attributedFrame, attributedIp, requireLine); const context = formatFrameContext(attributedFrame, artifact.contractName, lineNumber); const baseMessage = `${attributedFrame.sourceName}:${lineNumber} Require statement failed at input ${inputIndex} ${context}`; diff --git a/packages/cashscript/src/TransactionBuilder.ts b/packages/cashscript/src/TransactionBuilder.ts index daad50f24..da93ace41 100644 --- a/packages/cashscript/src/TransactionBuilder.ts +++ b/packages/cashscript/src/TransactionBuilder.ts @@ -319,8 +319,13 @@ export class TransactionBuilder { * * @param locktime - The absolute locktime to use (block height or UNIX timestamp). * @returns This builder for chaining. + * @throws If the locktime is not an unsigned 32-bit integer. */ setLocktime(locktime: number): this { + if (!Number.isInteger(locktime) || locktime < 0 || locktime > 0xffffffff) { + throw new Error(`Locktime ${locktime} should be an integer between 0 and 4294967295`); + } + this.locktime = locktime; return this; } @@ -508,9 +513,9 @@ export class TransactionBuilder { this.debug(); } + let txid: string; try { - const txid = await this.provider.sendRawTransaction(tx); - return raw ? await this.getTxDetails(txid, raw) : await this.getTxDetails(txid); + txid = await this.provider.sendRawTransaction(tx); } catch (e: any) { const reason = e.error ?? e.message; @@ -524,6 +529,9 @@ export class TransactionBuilder { throw new FailedTransactionError(reason, getBitauthUriWithFallback()); } + + // The transaction was broadcast successfully, so failing to retrieve it afterwards is not a failed transaction + return raw ? this.getTxDetails(txid, raw) : this.getTxDetails(txid); } private async getTxDetails(txid: string): Promise; @@ -543,8 +551,7 @@ export class TransactionBuilder { } } - // Should not happen - throw new Error('Could not retrieve transaction details for over 10 minutes'); + throw new Error(`Transaction ${txid} was broadcast, but its details could not be retrieved for over 10 minutes`); } /** @@ -578,19 +585,20 @@ export class TransactionBuilder { const transactionSize = this.getEncodedTransactionSize(transaction); const fee = totalInputAmount - totalOutputAmount; - const feePerByte = Number((Number(fee) / transactionSize).toFixed(2)); + const feePerByte = Number(fee) / transactionSize; if (this.options.maximumFeeSatoshis && fee > this.options.maximumFeeSatoshis) { throw new TransactionFeeTooHighError(fee, this.options.maximumFeeSatoshis); } + // The limits are checked against the exact fee per byte, which is only rounded (away from the limit) for display if (this.options.maximumFeeSatsPerByte && feePerByte > this.options.maximumFeeSatsPerByte) { - throw new TransactionFeePerByteTooHighError(feePerByte, this.options.maximumFeeSatsPerByte); + throw new TransactionFeePerByteTooHighError(Math.ceil(feePerByte * 100) / 100, this.options.maximumFeeSatsPerByte); } const STANDARD_MIN_FEE_PER_BYTE = 1.0; if (feePerByte < STANDARD_MIN_FEE_PER_BYTE) { - throw new TransactionFeePerByteTooLowError(feePerByte, STANDARD_MIN_FEE_PER_BYTE); + throw new TransactionFeePerByteTooLowError(Math.floor(feePerByte * 100) / 100, STANDARD_MIN_FEE_PER_BYTE); } } diff --git a/packages/cashscript/src/debug-frame.ts b/packages/cashscript/src/debug-frame.ts index 234131c93..c6e1e98fe 100644 --- a/packages/cashscript/src/debug-frame.ts +++ b/packages/cashscript/src/debug-frame.ts @@ -234,6 +234,21 @@ const toCallStackEntry = (frame: ResolvedFrame, instructionPointer: number): Cal }; }; +// A require statement's own opcodes can be optimised away (e.g. a final `require(ok)` where `ok` is already on top of +// the stack), in which case the failing opcode belongs to an earlier statement. We then use the require's own line. +export const getRequireLocationDataForFrame = ( + frame: ResolvedFrame, + instructionPointer: number, + requireLine: number, +): { lineNumber: number, statement: string } => { + const locationData = getLocationDataForFrame(frame, instructionPointer); + const statementEndLine = locationData.lineNumber + locationData.statement.split('\n').length - 1; + if (statementEndLine >= requireLine) return locationData; + + const statement = frame.source.split('\n')[requireLine - 1].trim().replace(/;$/, ''); + return { lineNumber: requireLine, statement }; +}; + export const getLocationDataForFrame = ( frame: ResolvedFrame, instructionPointer: number, diff --git a/packages/cashscript/src/network/ElectrumNetworkProvider.ts b/packages/cashscript/src/network/ElectrumNetworkProvider.ts index 911f2b6ff..eafe772c8 100644 --- a/packages/cashscript/src/network/ElectrumNetworkProvider.ts +++ b/packages/cashscript/src/network/ElectrumNetworkProvider.ts @@ -178,6 +178,8 @@ export default class ElectrumNetworkProvider implements NetworkProvider { try { result = await this.electrum.request(name, ...parameters); } finally { + this.concurrentRequests -= 1; + // Always disconnect the electrum client, also if the request fails // as long as no other concurrent requests are running if (this.shouldDisconnect()) { @@ -185,8 +187,6 @@ export default class ElectrumNetworkProvider implements NetworkProvider { } } - this.concurrentRequests -= 1; - if (result instanceof Error) throw result; return result; @@ -200,7 +200,7 @@ export default class ElectrumNetworkProvider implements NetworkProvider { private shouldDisconnect(): boolean { if (this.manualConnectionManagement) return false; - if (this.concurrentRequests !== 1) return false; + if (this.concurrentRequests !== 0) return false; return true; } } diff --git a/packages/cashscript/src/network/MockNetworkProvider.ts b/packages/cashscript/src/network/MockNetworkProvider.ts index 6fe888e54..c86dabb04 100644 --- a/packages/cashscript/src/network/MockNetworkProvider.ts +++ b/packages/cashscript/src/network/MockNetworkProvider.ts @@ -4,6 +4,7 @@ import { SpendableUtxo, Utxo, Network, VmTarget } from '../interfaces.js'; import NetworkProvider from './NetworkProvider.js'; import { addressToLockScript, cashScriptOutputToLibauthOutput, libauthTokenDetailsToCashScriptTokenDetails } from '../utils.js'; import { createVirtualMachine, DEFAULT_VM_TARGET } from '../libauth-template/utils.js'; +import { NetworkProviderAbsoluteTimelockError } from './errors.js'; /** * Options accepted by the `MockNetworkProvider` constructor. @@ -18,8 +19,9 @@ export interface MockNetworkProviderOptions { /** * When `true` (default), broadcasting a transaction via `sendRawTransaction` evaluates it * against the BCH VM using the *actual* locking bytecode of the spent UTXOs (like a real node - * would), rejecting invalid transactions. Requires `updateUtxoSet` to be enabled, since spent - * UTXOs are only looked up when the UTXO set is tracked. + * would), rejecting invalid transactions. Transactions with a block height locktime above the mock + * block height are rejected as non-final. Time-based locktimes and relative timelocks (sequence + * numbers) are not checked, since the mock network has no block times or UTXO confirmation heights. */ validateTransactions?: boolean; /** The BCH VM target used for local debugging and transaction validation. Defaults to the current stable VM. */ @@ -57,7 +59,7 @@ export default class MockNetworkProvider implements NetworkProvider { } async getUtxosForLockingBytecode(lockingBytecode: Uint8Array | string): Promise { - const lockingBytecodeHex = typeof lockingBytecode === 'string' ? lockingBytecode : binToHex(lockingBytecode); + const lockingBytecodeHex = typeof lockingBytecode === 'string' ? lockingBytecode.toLowerCase() : binToHex(lockingBytecode); return this.utxoSet.filter(([key]) => key === lockingBytecodeHex).map(([, utxo]) => utxo); } @@ -88,8 +90,8 @@ export default class MockNetworkProvider implements NetworkProvider { return txid; } - // If updateUtxoSet is false, we don't track spent UTXOs, so we cannot validate the transaction either - if (!this.options.updateUtxoSet) { + // Without validation or UTXO set updates, the spent UTXOs are not needed (so they don't need to exist either) + if (!this.options.validateTransactions && !this.options.updateUtxoSet) { this.transactionMap[txid] = txHex; return txid; } @@ -102,6 +104,10 @@ export default class MockNetworkProvider implements NetworkProvider { } this.transactionMap[txid] = txHex; + + // If updateUtxoSet is false, the UTXO set stays the same + if (!this.options.updateUtxoSet) return txid; + this.utxoSet = this.utxoSet.filter((entry) => !spentUtxoEntries.includes(entry)); decodedTransaction.outputs.forEach((output, vout) => { @@ -120,9 +126,9 @@ export default class MockNetworkProvider implements NetworkProvider { const remainingUtxoEntries = [...this.utxoSet]; return transaction.inputs.map((input) => { - const utxoIndex = remainingUtxoEntries.findIndex( - ([, utxo]) => utxo.txid === binToHex(input.outpointTransactionHash) && utxo.vout === input.outpointIndex, - ); + const utxoIndex = remainingUtxoEntries.findIndex(([, utxo]) => ( + utxo.txid.toLowerCase() === binToHex(input.outpointTransactionHash) && utxo.vout === input.outpointIndex + )); // TODO: we should check what error a BCHN node throws, so we can throw the same error here if (utxoIndex === -1) { @@ -135,6 +141,8 @@ export default class MockNetworkProvider implements NetworkProvider { // Evaluates the transaction against the BCH VM using the spent UTXOs (like a real node would) private validateTransaction(transaction: LibauthTransaction, spentUtxoEntries: Array<[string, SpendableUtxo]>): void { + this.validateLocktime(transaction); + const sourceOutputs = spentUtxoEntries.map(([lockingBytecode, utxo]) => cashScriptOutputToLibauthOutput({ to: hexToBin(lockingBytecode), amount: utxo.satoshis, @@ -149,6 +157,20 @@ export default class MockNetworkProvider implements NetworkProvider { } } + // A real node only accepts transactions that are final in the next block: a block height locktime must not be above + // the current block height, unless all inputs have a final sequence number (which disables the locktime) + private validateLocktime(transaction: LibauthTransaction): void { + const LOCKTIME_THRESHOLD = 500_000_000; + const SEQUENCE_FINAL = 0xffffffff; + + if (transaction.locktime >= LOCKTIME_THRESHOLD || transaction.locktime <= this.blockHeight) return; + if (transaction.inputs.every((input) => input.sequenceNumber === SEQUENCE_FINAL)) return; + + throw new NetworkProviderAbsoluteTimelockError( + `non-final: locktime ${transaction.locktime} is above the current block height ${this.blockHeight}`, + ); + } + // Note: the user can technically add the same UTXO multiple times (txid + vout), to the same or different addresses // but we don't check for this in the sendRawTransaction method. We might want to prevent duplicates from being added // in the first place. @@ -162,7 +184,7 @@ export default class MockNetworkProvider implements NetworkProvider { */ addUtxo(addressOrLockingBytecode: string, utxo: Utxo): SpendableUtxo { const lockingBytecode = isHex(addressOrLockingBytecode) ? - addressOrLockingBytecode : binToHex(addressToLockScript(addressOrLockingBytecode)); + addressOrLockingBytecode.toLowerCase() : binToHex(addressToLockScript(addressOrLockingBytecode)); const annotatedUtxo = { ...utxo, lockingBytecode }; this.utxoSet.push([lockingBytecode, annotatedUtxo]); diff --git a/packages/cashscript/src/test/JestExtensions.ts b/packages/cashscript/src/test/JestExtensions.ts new file mode 100644 index 000000000..86a095c53 --- /dev/null +++ b/packages/cashscript/src/test/JestExtensions.ts @@ -0,0 +1,12 @@ +import './TestExtensions.js'; + +declare global { + namespace jest { + // eslint-disable-next-line + interface Matchers { + toLog(value?: RegExp | string): Promise; + toFailRequireWith(value: RegExp | string): Promise; + toFailRequire(): Promise; + } + } +} diff --git a/packages/cashscript/src/test/TestExtensions.ts b/packages/cashscript/src/test/TestExtensions.ts index eb16a5858..c69aeb1ac 100644 --- a/packages/cashscript/src/test/TestExtensions.ts +++ b/packages/cashscript/src/test/TestExtensions.ts @@ -1,15 +1,7 @@ import { DebugResults } from '../debugging.js'; -declare global { - namespace jest { - // eslint-disable-next-line - interface Matchers { - toLog(value?: RegExp | string): Promise; - toFailRequireWith(value: RegExp | string): Promise; - toFailRequire(): Promise; - } - } -} +// Registers the custom matchers at runtime. Their types are declared separately for Vitest and Jest, in +// VitestExtensions.ts and JestExtensions.ts (which are exported as cashscript/vitest and cashscript/jest). interface Debuggable { debug(): DebugResults; @@ -18,7 +10,7 @@ interface Debuggable { type TestFramework = typeof vi; const testFramework: TestFramework = (globalThis as any).vi ?? (globalThis as any).jest; -// Extend Vitest with the custom matchers, this file needs to be imported in the vitest.setup.ts file or the test file +// Extend Vitest or Jest with the custom matchers expect.extend({ toLog( transaction: Debuggable, diff --git a/packages/cashscript/src/test/VitestExtensions.ts b/packages/cashscript/src/test/VitestExtensions.ts new file mode 100644 index 000000000..b25020415 --- /dev/null +++ b/packages/cashscript/src/test/VitestExtensions.ts @@ -0,0 +1,11 @@ +import './TestExtensions.js'; + +// The type parameters have to match the ones of Vitest's own Matchers interface for the declarations to merge +declare module 'vitest' { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + interface Matchers = void | Promise, T = unknown> { + toLog(value?: RegExp | string): R; + toFailRequireWith(value: RegExp | string): R; + toFailRequire(): R; + } +} diff --git a/packages/cashscript/test/Contract.test.ts b/packages/cashscript/test/Contract.test.ts index bd9c39b41..a24300d00 100644 --- a/packages/cashscript/test/Contract.test.ts +++ b/packages/cashscript/test/Contract.test.ts @@ -11,7 +11,7 @@ import { } from '../src/index.js'; import { aliceAddress, - alicePkh, alicePriv, alicePub, bobPriv, + alicePkh, alicePriv, alicePub, bobPriv, bobPub, } from './fixture/vars.js'; import { addUtxo } from './test-util.js'; import { generateLibauthSourceOutputs } from '../src/utils.js'; @@ -39,6 +39,14 @@ describe('Contract', () => { expect(() => new Contract(p2pkhArtifact, [placeholder(21)], { provider })).toThrow(); }); + it('should fail with malformed hex string constructor args', () => { + const provider = new MockNetworkProvider(); + + // An odd number of digits or non-hex characters would otherwise silently produce different bytes + expect(() => new Contract(p2pkhArtifact, [`0x${'ab'.repeat(19)}a`], { provider })).toThrow(/valid hex string/); + expect(() => new Contract(p2pkhArtifact, ['zz'.repeat(20)], { provider })).toThrow(/valid hex string/); + }); + it('should fail with artifact compiled with unsupported compiler version', async () => { const provider = new ElectrumNetworkProvider(Network.CHIPNET); const constructorArgs = [placeholder(20), placeholder(20), 1000000n]; @@ -168,11 +176,13 @@ describe('Contract', () => { expect(() => instance.unlock.spend(alicePub, new SignatureTemplate(alicePriv), 0n)).toThrow(); expect(() => bbInstance.unlock.spend(hexToBin('e803'), 1000n)).toThrow(); expect(() => bbInstance.unlock.spend(hexToBin('e803000000'), 1000n)).toThrow(); + expect(() => instance.unlock.spend(alicePkh, placeholder(65))) + .toThrow("Found type 'bytes20' where type 'pubkey' was expected"); }); it('can call spend with incorrect arguments', () => { expect(() => instance.unlock.spend(alicePub, new SignatureTemplate(bobPriv))).not.toThrow(); - expect(() => instance.unlock.spend(alicePkh, placeholder(65))).not.toThrow(); + expect(() => instance.unlock.spend(bobPub, placeholder(65))).not.toThrow(); expect(() => bbInstance.unlock.spend(hexToBin('e8031234'), 1000n)).not.toThrow(); }); diff --git a/packages/cashscript/test/TransactionBuilder.test.ts b/packages/cashscript/test/TransactionBuilder.test.ts index c07c4e5a3..3395bc4b7 100644 --- a/packages/cashscript/test/TransactionBuilder.test.ts +++ b/packages/cashscript/test/TransactionBuilder.test.ts @@ -22,6 +22,7 @@ import { TransactionBuilder } from '../src/TransactionBuilder.js'; import { addUtxo, getTxOutputs } from './test-util.js'; import { generateWcTransactionObjectFixture } from './fixture/walletconnect/fixtures.js'; import { + FailedTransactionError, InputMissingLockingBytecodeError, OutputBchChangeLockedError, OutputTokenChangeLockedError, @@ -170,6 +171,24 @@ describe('Transaction Builder', () => { expect(tx).toBeDefined(); }); + it('should fail when fee per byte is lower than 1, even when it rounds to 1', async () => { + const mockProvider = new MockNetworkProvider(); + const utxo = mockProvider.addUtxo(carolAddress, randomUtxo({ satoshis: 100_000n })); + const createTransaction = (fee: bigint): TransactionBuilder => new TransactionBuilder({ provider: mockProvider }) + .addInput(utxo, new SignatureTemplate(carolPriv).unlockP2PKH()) + .addOutput({ to: carolAddress, amount: 100_000n - fee }) + .addOpReturnOutput([`0x${'00'.repeat(100)}`]); + const buildWithFee = (fee: bigint): string => createTransaction(fee).build(); + + // The OP_RETURN output makes the transaction large enough that a fee of (size - 1) satoshis is just below + // 1 sat/byte, but rounds to 1.00, which previously passed the minimum fee check + const size = createTransaction(0n).getTransactionSize(); + expect(Number(size - 1n) / Number(size)).toBeGreaterThan(0.995); + + expect(() => buildWithFee(size - 1n)).toThrow(/Transaction fee per byte of 0\.99 is lower than the standard minimum/); + expect(() => buildWithFee(size)).not.toThrow(); + }); + // TODO: Consider improving error messages checked below to also include the input/output index it('should fail when trying to send to invalid address', async () => { @@ -342,6 +361,40 @@ describe('Transaction Builder', () => { await expect(transaction.send()).resolves.not.toThrow(); }); + it('should only accept unsigned 32-bit integer locktimes', () => { + const transaction = new TransactionBuilder({ provider }); + + expect(() => transaction.setLocktime(-1)).toThrow('Locktime -1 should be an integer between 0 and 4294967295'); + expect(() => transaction.setLocktime(2 ** 32)).toThrow('should be an integer between 0 and 4294967295'); + expect(() => transaction.setLocktime(1.5)).toThrow('should be an integer between 0 and 4294967295'); + expect(transaction.setLocktime(2 ** 32 - 1).locktime).toBe(2 ** 32 - 1); + }); + + it('should not report a failed transaction when the transaction cannot be retrieved after broadcasting', async () => { + class LookupFailingMockNetworkProvider extends MockNetworkProvider { + async getRawTransaction(): Promise { + throw new Error('not found'); + } + } + + const lookupFailingProvider = new LookupFailingMockNetworkProvider(); + const contract = new Contract(p2pkhArtifact, [carolPkh], { provider: lookupFailingProvider }); + const utxo = lookupFailingProvider.addUtxo(contract.address, randomUtxo({ satoshis: 100_000n })); + + const transaction = new TransactionBuilder({ provider: lookupFailingProvider }) + .addInput(utxo, contract.unlock.spend(carolPub, new SignatureTemplate(carolPriv))) + .addOutput({ to: carolAddress, amount: 1_000n }); + + // Retrieving the transaction is retried for 10 minutes + vi.useFakeTimers(); + const error = transaction.send().catch((e) => e); + await vi.advanceTimersByTimeAsync(10 * 60 * 1000); + vi.useRealTimers(); + + expect(await error).not.toBeInstanceOf(FailedTransactionError); + expect((await error).message).toMatch(/^Transaction [0-9a-f]{64} was broadcast, but its details could not be retrieved/); + }); + it('should preserve the Bitauth URI when broadcast fails', async () => { const failingProvider = new FailingMockNetworkProvider(); const contract = new Contract(p2pkhArtifact, [carolPkh], { provider: failingProvider }); diff --git a/packages/cashscript/test/debugging.test.ts b/packages/cashscript/test/debugging.test.ts index 36fe4f473..a999c8646 100644 --- a/packages/cashscript/test/debugging.test.ts +++ b/packages/cashscript/test/debugging.test.ts @@ -11,6 +11,7 @@ import { artifactTestRequires, artifactTestSingleFunction, artifactTestMultilineRequires, + artifactTestFinalRequireVariable, artifactTestZeroHandling, artifactTestRequireInsideLoop, artifactTestLogInsideLoop, @@ -272,6 +273,20 @@ describe('Debugging tests', () => { expect(transaction).toFailRequireWith('Failing statement: require(tx.outputs.length == 1, "should have 1 output")'); }); + // test_final_require_variable + it('should fail at the require statement when a final require only checks a variable', async () => { + // The require's own opcodes are optimised away, since the variable is already on top of the stack + const contractFinalRequireVariable = new Contract(artifactTestFinalRequireVariable, [], { provider }); + const contractFinalRequireVariableUtxo = provider.addUtxo(contractFinalRequireVariable.address, randomUtxo()); + + const transaction = new TransactionBuilder({ provider }) + .addInput(contractFinalRequireVariableUtxo, contractFinalRequireVariable.unlock.test_final_require_variable(1n)) + .addOutput({ to: contractFinalRequireVariable.address, amount: 1000n }); + + expect(transaction).toFailRequireWith('Test.cash:5 Require statement failed at input 0 in contract Test.cash at line 5.'); + expect(transaction).toFailRequireWith('Failing statement: require(isLarge)'); + }); + // test_multiple_require_statements it('it should only fail with correct error message when there are multiple require statements', async () => { const transaction = new TransactionBuilder({ provider }) diff --git a/packages/cashscript/test/e2e/network/ElectrumNetworkProvider.test.ts b/packages/cashscript/test/e2e/network/ElectrumNetworkProvider.test.ts index 69407b161..7450dc755 100644 --- a/packages/cashscript/test/e2e/network/ElectrumNetworkProvider.test.ts +++ b/packages/cashscript/test/e2e/network/ElectrumNetworkProvider.test.ts @@ -1,5 +1,5 @@ import { ElectrumNetworkProvider, Network } from '../../../src/index.js'; -import { ElectrumClient } from '@electrum-cash/network'; +import { ElectrumClient, type ElectrumClientEvents } from '@electrum-cash/network'; describe.runIf(Boolean(process.env.TESTS_USE_CHIPNET))('ElectrumNetworkProvider', () => { // TODO: Test more of the API @@ -58,3 +58,22 @@ describe.runIf(Boolean(process.env.TESTS_USE_CHIPNET))('ElectrumNetworkProvider' }); }); +describe('ElectrumNetworkProvider automatic connection management', () => { + it('should connect and disconnect again for requests after a failed request', async () => { + const electrum = { + connect: vi.fn(async () => {}), + disconnect: vi.fn(async () => true), + request: vi.fn(async () => { throw new Error('request failed'); }), + }; + const provider = new ElectrumNetworkProvider(Network.CHIPNET, { + electrum: electrum as unknown as ElectrumClient, + }); + + await expect(provider.getBlockHeight()).rejects.toThrow('request failed'); + await expect(provider.getBlockHeight()).rejects.toThrow('request failed'); + + // A failed request should not leave the provider thinking that a request is still running + expect(electrum.connect).toHaveBeenCalledTimes(2); + expect(electrum.disconnect).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/cashscript/test/e2e/network/MockNetworkProvider.test.ts b/packages/cashscript/test/e2e/network/MockNetworkProvider.test.ts index 756d56182..1783ac591 100644 --- a/packages/cashscript/test/e2e/network/MockNetworkProvider.test.ts +++ b/packages/cashscript/test/e2e/network/MockNetworkProvider.test.ts @@ -1,5 +1,5 @@ import { binToHex } from '@bitauth/libauth'; -import { Contract, MockNetworkProvider, SignatureTemplate } from '../../../src/index.js'; +import { Contract, MockNetworkProvider, NetworkProviderAbsoluteTimelockError, SignatureTemplate } from '../../../src/index.js'; import { TransactionBuilder } from '../../../src/TransactionBuilder.js'; import { addressToLockScript, randomUtxo } from '../../../src/utils.js'; import p2pkhArtifact from '../../fixture/p2pkh.artifact.js'; @@ -123,6 +123,59 @@ describe.skipIf(Boolean(process.env.TESTS_USE_CHIPNET))('MockNetworkProvider', ( await expect(nonValidatingProvider.sendRawTransaction(transaction)).resolves.toBeTruthy(); }); + + it('should also validate transactions when updateUtxoSet is set to false', async () => { + const staticProvider = new MockNetworkProvider({ updateUtxoSet: false }); + const bobLockingBytecode = binToHex(addressToLockScript(bobAddress)); + const utxo = { ...staticProvider.addUtxo(aliceAddress, randomUtxo()), lockingBytecode: bobLockingBytecode }; + + const transaction = new TransactionBuilder({ provider: staticProvider }) + .addInput(utxo, new SignatureTemplate(bobPriv).unlockP2PKH()) + .addOutput({ to: aliceAddress, amount: 5000n }) + .build(); + + await expect(staticProvider.sendRawTransaction(transaction)).rejects.toThrow(); + }); + + it('should reject transactions with a block height locktime above the current block height', async () => { + const heightProvider = new MockNetworkProvider(); + heightProvider.setBlockHeight(1000); + + const buildWithLocktime = (locktime: number, sequence?: number): string => { + const utxo = heightProvider.addUtxo(aliceAddress, randomUtxo()); + return new TransactionBuilder({ provider: heightProvider }) + .setLocktime(locktime) + .addInput(utxo, new SignatureTemplate(alicePriv).unlockP2PKH(), { sequence }) + .addOutput({ to: aliceAddress, amount: 5000n }) + .build(); + }; + + await expect(heightProvider.sendRawTransaction(buildWithLocktime(1001))) + .rejects.toThrow(NetworkProviderAbsoluteTimelockError); + await expect(heightProvider.sendRawTransaction(buildWithLocktime(1000))).resolves.toBeTruthy(); + + // Final sequence numbers disable the locktime, and time-based locktimes are not checked + await expect(heightProvider.sendRawTransaction(buildWithLocktime(1001, 0xffffffff))).resolves.toBeTruthy(); + await expect(heightProvider.sendRawTransaction(buildWithLocktime(2_000_000_000))).resolves.toBeTruthy(); + }); + + it('should compare hex strings case-insensitively', async () => { + const aliceLockingBytecode = binToHex(addressToLockScript(aliceAddress)); + const randomTxid = randomUtxo().txid; + provider.addUtxo(aliceLockingBytecode.toUpperCase(), { ...randomUtxo(), txid: randomTxid.toUpperCase() }); + + const [utxo] = await provider.getUtxosForLockingBytecode(aliceLockingBytecode); + expect(utxo).toBeDefined(); + expect(await provider.getUtxosForLockingBytecode(aliceLockingBytecode.toUpperCase())).toHaveLength(1); + + // The spent UTXO is found although its txid was added in upper case + const transaction = new TransactionBuilder({ provider }) + .addInput(utxo, new SignatureTemplate(alicePriv).unlockP2PKH()) + .addOutput({ to: aliceAddress, amount: 5000n }) + .build(); + + await expect(provider.sendRawTransaction(transaction)).resolves.toBeTruthy(); + }); }); describe('when updateUtxoSet is set to false', () => { diff --git a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts index 417f76198..f957f817b 100644 --- a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts +++ b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts @@ -308,6 +308,15 @@ contract Test() { } }`; +const CONTRACT_TEST_FINAL_REQUIRE_VARIABLE = ` +contract Test() { + function test_final_require_variable(int x) { + bool isLarge = x > 5; + require(isLarge); + } +} +`; + const CONTRACT_TEST_MULTILINE_REQUIRES = ` contract Test() { // We test this because the cleanup looks different and the final OP_VERIFY isn't removed for these kinds of functions @@ -586,6 +595,7 @@ contract Test(pubkey owner, int num, int num2, int num3, int num4, int num5) { export const artifactTestRequires = compileString(CONTRACT_TEST_REQUIRES); export const artifactTestSingleFunction = compileString(CONTRACT_TEST_REQUIRE_SINGLE_FUNCTION); export const artifactTestMultilineRequires = compileString(CONTRACT_TEST_MULTILINE_REQUIRES); +export const artifactTestFinalRequireVariable = compileString(CONTRACT_TEST_FINAL_REQUIRE_VARIABLE); export const artifactTestZeroHandling = compileString(CONTRACT_TEST_ZERO_HANDLING); export const artifactTestLogs = compileString(CONTRACT_TEST_LOGS); export const artifactTestConsecutiveLogs = compileString(CONTRACT_TEST_CONSECUTIVE_LOGS); diff --git a/packages/cashscript/vitest.setup.ts b/packages/cashscript/vitest.setup.ts index 62dd58ca6..b1f326ea3 100644 --- a/packages/cashscript/vitest.setup.ts +++ b/packages/cashscript/vitest.setup.ts @@ -1,4 +1,4 @@ import { inspect } from 'util'; -import './src/test/TestExtensions.js'; +import './src/test/VitestExtensions.js'; inspect.defaultOptions.depth = 10; diff --git a/packages/cashscript/vitest/package.json b/packages/cashscript/vitest/package.json index 4cdc1864a..0055652f2 100644 --- a/packages/cashscript/vitest/package.json +++ b/packages/cashscript/vitest/package.json @@ -1,5 +1,5 @@ { "type": "module", - "types": "../dist/test/TestExtensions.d.ts", - "main": "../dist/test/TestExtensions.js" + "types": "../dist/test/VitestExtensions.d.ts", + "main": "../dist/test/VitestExtensions.js" } diff --git a/website/docs/releases/release-notes.md b/website/docs/releases/release-notes.md index e32172a1c..a913f4639 100644 --- a/website/docs/releases/release-notes.md +++ b/website/docs/releases/release-notes.md @@ -34,6 +34,14 @@ This release contains several breaking changes, please refer to the [migration n - :hammer_and_wrench: `ElectrumNetworkProvider` now negotiates Electrum protocol 1.5.0 (was 1.4.1) and uses `blockchain.headers.get_tip` in `getBlockHeight()`, so long-lived connections are no longer subscribed to new headers. Custom electrum clients should negotiate 1.5.0 or later. - :hammer_and_wrench: Update `@electrum-cash/network` to 4.4.0. In a browser, the `ElectrumNetworkProvider`'s connection is now also closed while the page is hidden (it was already closed while offline) and opened again afterwards, restoring any subscriptions (see [Browser visibility and connectivity](/docs/sdk/electrum-network-provider#browser-visibility-and-connectivity)). - :boom: **BREAKING**: Remove `generateLockingBytecode()` from the `Unlocker` interface. +- :bug: Fix bug where hex string arguments with an odd number of digits or non-hex characters silently encoded to different bytes, they now throw an error. `pubkey` arguments are also checked to be 33 or 65 bytes. +- :bug: Fix bug where the `ElectrumNetworkProvider` stopped connecting for new requests after a request failed, when using automatic connection management. +- :bug: Fix bug where the minimum and maximum fee per byte checks rounded the fee per byte before comparing, so e.g. 0.998 sat/byte was accepted. +- :bug: Fix bug where `setLocktime()` accepted values that are not an unsigned 32-bit integer, it now throws an error. +- :bug: Fix bug where `send()` threw a `FailedTransactionError` when the transaction was broadcast successfully, but could not be retrieved afterwards. +- :bug: Fix bug where a failing final `require(variable)` statement was reported at the line of the previous statement. +- :bug: Fix missing types for the `toLog()`, `toFailRequire()` and `toFailRequireWith()` test matchers when importing `cashscript/vitest` with Vitest 5. +- :bug: The `MockNetworkProvider` now also validates transactions when `updateUtxoSet` is `false`, rejects transactions with a block height locktime above the mock block height, and compares hex strings case-insensitively. ## v0.13.3 diff --git a/website/docs/sdk/other-network-providers.md b/website/docs/sdk/other-network-providers.md index e66d1e3f6..a9d190f1b 100644 --- a/website/docs/sdk/other-network-providers.md +++ b/website/docs/sdk/other-network-providers.md @@ -43,7 +43,7 @@ interface MockNetworkProviderOptions { ``` - `updateUtxoSet` (default `true`) — update the in-memory UTXO set after a transaction is sent, consuming the spent UTXOs and adding the transaction's outputs. -- `validateTransactions` (default `true`) — evaluate sent transactions against the BCH VM using the actual locking bytecode of the spent UTXOs, rejecting transactions that a real node would reject. Requires `updateUtxoSet`. +- `validateTransactions` (default `true`) — evaluate sent transactions against the BCH VM using the actual locking bytecode of the spent UTXOs, rejecting transactions that a real node would reject. Transactions with a block height locktime above the mock block height (see `setBlockHeight()`) are rejected as non-final. Time-based locktimes and relative timelocks (sequence numbers) are not checked, since the mock network has no block times or UTXO confirmation heights. - `vmTarget` (default `BCH_2026_05`) — the BCH virtual machine version used for local debugging and transaction validation. #### Example diff --git a/website/docs/sdk/transaction-builder.md b/website/docs/sdk/transaction-builder.md index ea322a7f6..525042d34 100644 --- a/website/docs/sdk/transaction-builder.md +++ b/website/docs/sdk/transaction-builder.md @@ -207,7 +207,7 @@ Sets the locktime for the transaction to set a transaction-level absolute timelo #### Example ```ts // Set locktime one day from now -transactionBuilder.setLocktime((Date.now() / 1000) + 24 * 60 * 60); +transactionBuilder.setLocktime(Math.floor(Date.now() / 1000) + 24 * 60 * 60); ``` ### getTransactionSize() From 8aaf17d4bcb9a8f3a4d9778f3249ede6a137357b Mon Sep 17 00:00:00 2001 From: Rosco Kalis Date: Mon, 28 Sep 2026 11:43:31 +0200 Subject: [PATCH 2/3] docs: update release notes --- website/docs/releases/release-notes.md | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/website/docs/releases/release-notes.md b/website/docs/releases/release-notes.md index a913f4639..6b72858d6 100644 --- a/website/docs/releases/release-notes.md +++ b/website/docs/releases/release-notes.md @@ -33,15 +33,14 @@ This release contains several breaking changes, please refer to the [migration n - :hammer_and_wrench: **BREAKING**: The `TransactionBuilder` now requires UTXOs to include their `lockingBytecode`, and validates it against the provided unlocker. - :hammer_and_wrench: `ElectrumNetworkProvider` now negotiates Electrum protocol 1.5.0 (was 1.4.1) and uses `blockchain.headers.get_tip` in `getBlockHeight()`, so long-lived connections are no longer subscribed to new headers. Custom electrum clients should negotiate 1.5.0 or later. - :hammer_and_wrench: Update `@electrum-cash/network` to 4.4.0. In a browser, the `ElectrumNetworkProvider`'s connection is now also closed while the page is hidden (it was already closed while offline) and opened again afterwards, restoring any subscriptions (see [Browser visibility and connectivity](/docs/sdk/electrum-network-provider#browser-visibility-and-connectivity)). +- :hammer_and_wrench: Make `cashscript/vitest` types compatible with Vitest 5. - :boom: **BREAKING**: Remove `generateLockingBytecode()` from the `Unlocker` interface. -- :bug: Fix bug where hex string arguments with an odd number of digits or non-hex characters silently encoded to different bytes, they now throw an error. `pubkey` arguments are also checked to be 33 or 65 bytes. -- :bug: Fix bug where the `ElectrumNetworkProvider` stopped connecting for new requests after a request failed, when using automatic connection management. -- :bug: Fix bug where the minimum and maximum fee per byte checks rounded the fee per byte before comparing, so e.g. 0.998 sat/byte was accepted. -- :bug: Fix bug where `setLocktime()` accepted values that are not an unsigned 32-bit integer, it now throws an error. -- :bug: Fix bug where `send()` threw a `FailedTransactionError` when the transaction was broadcast successfully, but could not be retrieved afterwards. -- :bug: Fix bug where a failing final `require(variable)` statement was reported at the line of the previous statement. -- :bug: Fix missing types for the `toLog()`, `toFailRequire()` and `toFailRequireWith()` test matchers when importing `cashscript/vitest` with Vitest 5. -- :bug: The `MockNetworkProvider` now also validates transactions when `updateUtxoSet` is `false`, rejects transactions with a block height locktime above the mock block height, and compares hex strings case-insensitively. +- :bug: Fix bug where invalid hex strings were silently encoded to different bytes. +- :bug: Fix bug where `pubkey` arguments were not checked to be 33 or 65 bytes. +- :bug: Fix bug in `ElectrumNetworkProvider` automatic connection management. +- :bug: Fix minimum fee check rounding bug. +- :bug: Fix bug where `setLocktime()` accepted invalid values. +- :bug: Fix edge case bug where a failing final `require(variable)` statement was reported incorrectly in debugging. ## v0.13.3 From 5e8b55acdca954dbf9f5f1187e4bb75bf1924d0e Mon Sep 17 00:00:00 2001 From: Rosco Kalis Date: Mon, 28 Sep 2026 11:44:15 +0200 Subject: [PATCH 3/3] docs: update --- website/docs/sdk/other-network-providers.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/sdk/other-network-providers.md b/website/docs/sdk/other-network-providers.md index a9d190f1b..c389cfadf 100644 --- a/website/docs/sdk/other-network-providers.md +++ b/website/docs/sdk/other-network-providers.md @@ -43,7 +43,7 @@ interface MockNetworkProviderOptions { ``` - `updateUtxoSet` (default `true`) — update the in-memory UTXO set after a transaction is sent, consuming the spent UTXOs and adding the transaction's outputs. -- `validateTransactions` (default `true`) — evaluate sent transactions against the BCH VM using the actual locking bytecode of the spent UTXOs, rejecting transactions that a real node would reject. Transactions with a block height locktime above the mock block height (see `setBlockHeight()`) are rejected as non-final. Time-based locktimes and relative timelocks (sequence numbers) are not checked, since the mock network has no block times or UTXO confirmation heights. +- `validateTransactions` (default `true`) — evaluate sent transactions against the BCH VM using the actual locking bytecode of the spent UTXOs, rejecting transactions that a real node would reject. - `vmTarget` (default `BCH_2026_05`) — the BCH virtual machine version used for local debugging and transaction validation. #### Example