From 76677664df1daf97fcdaa7b8fc26294baf959504 Mon Sep 17 00:00:00 2001 From: Mathieu Geukens Date: Tue, 29 Sep 2026 00:06:02 +0200 Subject: [PATCH 1/3] fix: debugging edge cases from the 0.14 audit - Each input is now debugged with the artifact of its own contract, looked up by its exact unlocking script ID. The artifact was found by contract name prefix, so with contracts `Vault` and `VaultSidecar` in one transaction the sidecar input could be debugged with Vault's artifact: logs printed for code that did not run, and a failing require crashed the debugger instead of being reported. - A failing final `require(f(x))` where `f` is defined with OP_DEFINE was located using steps of the function body as well, giving an ip that does not point into the contract's own bytecode. The failure was reported against an unrelated statement without its require message. Only the steps of the failing frame are used now. - `console.log` statements were matched against the step that raised the error and against libauth's repeated final state, so a log after a failing instruction was printed (twice), and a log after a division by zero made debug() throw a plain Error. Steps with an error and the repeated final state are skipped now. - formatBitAuthScript read past the end of the script when nothing follows the parameter type checks (e.g. `require(b)` for a `bool b`), so debug(), send() and getBitauthUri() threw a TypeError for a valid spend. The anchor is now clamped to the last opcode. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/cashscript/src/debugging.ts | 24 ++++++-- .../src/libauth-template/LibauthTemplate.ts | 13 ++-- packages/cashscript/test/debugging.test.ts | 59 +++++++++++++++++++ .../fixture/debugging/debugging_contracts.ts | 50 ++++++++++++++++ .../multi_contract_debugging_contracts.ts | 21 +++++++ .../test/multi-contract-debugging.test.ts | 34 ++++++++++- packages/utils/src/bitauth-script.ts | 3 +- 7 files changed, 193 insertions(+), 11 deletions(-) diff --git a/packages/cashscript/src/debugging.ts b/packages/cashscript/src/debugging.ts index 31a90b392..e498da649 100644 --- a/packages/cashscript/src/debugging.ts +++ b/packages/cashscript/src/debugging.ts @@ -10,8 +10,13 @@ import { VmTarget } from './interfaces.js'; export type DebugResult = AuthenticationProgramStateCommon[]; export type DebugResults = Record; -// debugs the template, optionally logging the execution data -export const debugTemplate = (template: WalletTemplate, artifacts: Artifact[]): DebugResults => { +// debugs the template, optionally logging the execution data. The artifacts are keyed by the ID of the unlocking script +// that spends the contract, so every input is debugged with the artifact of its own contract +export const debugTemplate = ( + template: WalletTemplate, artifactsByUnlockingScriptId: Record, +): DebugResults => { + const artifacts = Object.values(artifactsByUnlockingScriptId); + // If a contract has the same name, but a different bytecode, then it is considered a name collision const hasArtifactNameCollision = artifacts.some( (artifact) => ( @@ -29,7 +34,7 @@ export const debugTemplate = (template: WalletTemplate, artifacts: Artifact[]): for (const unlockingScriptId of unlockingScriptIds) { const scenarioIds = (template.scripts[unlockingScriptId] as WalletTemplateScriptUnlocking).passes ?? []; - const matchingArtifact = artifacts.find((artifact) => unlockingScriptId.startsWith(artifact.contractName)); + const matchingArtifact = artifactsByUnlockingScriptId[unlockingScriptId]; for (const scenarioId of scenarioIds) { results[`${unlockingScriptId}.${scenarioId}`] = debugSingleScenario(template, matchingArtifact, unlockingScriptId, scenarioId); @@ -72,6 +77,13 @@ const debugSingleScenario = ( // - multiple log statements may exist for the same ip, so we need to handle all of them. const executedLogs = executedDebugSteps .flatMap((debugStep, index) => { + // A step with an error did not complete its instruction, so the log entries after it were not reached. libauth + // also repeats the final state at the end of the trace, which should only be matched once. + const previousDebugStep = executedDebugSteps[index - 1]; + const isRepeatedFinalState = index === executedDebugSteps.length - 1 + && debugStep.ip === previousDebugStep?.ip && debugStep.instructions === previousDebugStep?.instructions; + if (debugStep.error || isRepeatedFinalState) return []; + const frame = resolveFrame(artifact, debugStep); const logEntries = frame.logs.filter((log) => log.ip === debugStep.ip); if (logEntries.length === 0) return []; @@ -151,7 +163,11 @@ const debugSingleScenario = ( // Check if the evaluation failed matches any of the possible failure cases if (failedFinalVerify(evaluationResult)) { - const finalExecutedVerifyIp = getFinalExecutedVerifyIp(executedDebugSteps); + // Only the steps of the frame that failed are used, since ips of other frames (e.g. a function body that was + // invoked by the final require statement) do not point into this frame + const finalExecutedVerifyIp = getFinalExecutedVerifyIp( + executedDebugSteps.filter((step) => step.instructions === lastExecutedDebugStep.instructions), + ); // The final executed verify instruction points to the "implicit" VERIFY that is added at the end of the script. // This instruction does not exist in the sourcemap, so we need to decrement the instruction pointer to get the diff --git a/packages/cashscript/src/libauth-template/LibauthTemplate.ts b/packages/cashscript/src/libauth-template/LibauthTemplate.ts index f10e2dff8..a2b542a6b 100644 --- a/packages/cashscript/src/libauth-template/LibauthTemplate.ts +++ b/packages/cashscript/src/libauth-template/LibauthTemplate.ts @@ -66,12 +66,15 @@ export const getLibauthTemplate = ( }; export const debugLibauthTemplate = (template: WalletTemplate, transaction: TransactionBuilder): DebugResults => { - const allArtifacts = transaction.inputs - .map(input => isContractUnlocker(input.unlocker) ? input.unlocker.contract : undefined) - .filter((contract): contract is Contract => Boolean(contract)) - .map(contract => contract.artifact); + // Artifacts are matched to inputs by the exact unlocking script ID (P2PKH inputs do not have an artifact) + const artifactEntries = transaction.inputs.flatMap((input, inputIndex): Array<[string, Artifact]> => { + if (!isContractUnlocker(input.unlocker)) return []; - return debugTemplate(template, allArtifacts); + const { contract, abiFunction } = input.unlocker; + return [[getUnlockScriptName(contract, abiFunction, inputIndex), contract.artifact]]; + }); + + return debugTemplate(template, Object.fromEntries(artifactEntries)); }; export const getBitauthUri = (template: WalletTemplate): string => { diff --git a/packages/cashscript/test/debugging.test.ts b/packages/cashscript/test/debugging.test.ts index 887233fb5..5faae82fb 100644 --- a/packages/cashscript/test/debugging.test.ts +++ b/packages/cashscript/test/debugging.test.ts @@ -13,6 +13,9 @@ import { artifactTestSingleFunction, artifactTestMultilineRequires, artifactTestFinalRequireVariable, + artifactTestFinalRequireDefinedFunction, + artifactTestLogAfterFailure, + artifactTestOnlyParameterCheck, artifactTestZeroHandling, artifactTestRequireInsideLoop, artifactTestLogInsideLoop, @@ -227,6 +230,33 @@ describe('Debugging tests', () => { expect(transaction).toLog(new RegExp('^\\[Input #0] Test.cash:29 for i: 2 sum: 3$')); }); + it('should not log console.log statements after a failing require statement', async () => { + const contract = new Contract(artifactTestLogAfterFailure, [], { provider }); + const utxo = provider.addUtxo(contract.address, randomUtxo()); + + const transaction = new TransactionBuilder({ provider }) + .addInput(utxo, contract.unlock.test_log_after_failed_require(1n)) + .addOutput({ to: contract.address, amount: 10000n }); + + expect(transaction).not.toLog(); + expect(transaction).toFailRequireWith('Failing statement: require(x > 5, "x big")'); + }); + + it('should not log console.log statements after a division by zero', async () => { + const contract = new Contract(artifactTestLogAfterFailure, [], { provider }); + const utxo = provider.addUtxo(contract.address, randomUtxo()); + + const transaction = new TransactionBuilder({ provider }) + .addInput(utxo, contract.unlock.test_log_after_division_by_zero(0n)) + .addOutput({ to: contract.address, amount: 10000n }); + + expect(transaction).not.toLog(); + // The division by zero is reported as the failure (a FailedTransactionEvaluationError) + expect(() => transaction.debug()).toThrow(expect.objectContaining({ + libauthErrorMessage: expect.stringContaining(AuthenticationErrorCommon.divisionByZero), + })); + }); + it.todo('should log intermediate results that get optimised out inside a loop'); }); @@ -288,6 +318,19 @@ describe('Debugging tests', () => { expect(transaction).toFailRequireWith('Failing statement: require(isLarge)'); }); + it('should fail at the require statement when a final require checks the result of a defined function', async () => { + // With inlining disabled, isPositive is defined with OP_DEFINE, so it is evaluated in its own frame + const contract = new Contract(artifactTestFinalRequireDefinedFunction, [], { provider }); + const contractUtxo = provider.addUtxo(contract.address, randomUtxo()); + + const transaction = new TransactionBuilder({ provider }) + .addInput(contractUtxo, contract.unlock.spend(-1n)) + .addOutput({ to: contract.address, amount: 1000n }); + + expect(transaction).toFailRequireWith('Test.cash:14 Require statement failed at input 0 in contract Test.cash at line 14 with the following message: x must be positive.'); + expect(transaction).toFailRequireWith('Failing statement: require(isPositive(x), "x must be positive")'); + }); + // 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 }) @@ -693,6 +736,22 @@ describe('Debugging tests', () => { }); }); + // The function body compiles to nothing but the parameter type check, so the BitAuth script (which is also used + // for debugging) has no opcode after the parameter prologue + it('should debug and send a function that only consists of a parameter type check', async () => { + const provider = new MockNetworkProvider(); + const contract = new Contract(artifactTestOnlyParameterCheck, [], { provider }); + const contractUtxo = provider.addUtxo(contract.address, randomUtxo()); + + const transaction = new TransactionBuilder({ provider }) + .addInput(contractUtxo, contract.unlock.spend(true)) + .addOutput({ to: contract.address, amount: 1000n }); + + expect(() => transaction.getLibauthTemplate()).not.toThrow(); + expect(transaction).not.toFailRequire(); + await expect(transaction.send()).resolves.toBeDefined(); + }); + describe('TestExtensions', () => { const provider = new MockNetworkProvider(); const contractTestRequires = new Contract(artifactTestRequires, [], { provider }); diff --git a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts index 644d721e1..ef17ca4a9 100644 --- a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts +++ b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts @@ -317,6 +317,50 @@ contract Test() { } `; +// With inlining disabled, isPositive is defined with OP_DEFINE, so the final require evaluates its result +const CONTRACT_TEST_FINAL_REQUIRE_DEFINED_FUNCTION = ` +function isPositive(int value) returns (bool) { + return value > 0; +} + +contract Test() { + function other(int y) { + require(y == 5, "y should be 5"); + require(y != 6, "y should not be 6"); + } + + function spend(int x) { + require(x < 100, "x must be small"); + require(isPositive(x), "x must be positive"); + } +} +`; + +const CONTRACT_TEST_LOG_AFTER_FAILURE = ` +contract Test() { + function test_log_after_failed_require(int x) { + require(x > 5, "x big"); + console.log("after", x); + require(x < 100, "x small"); + } + + function test_log_after_division_by_zero(int x) { + int y = 10 / x; + console.log("after", y); + require(y > 1, "y big"); + } +} +`; + +// Nothing follows the parameter type check, so the function body compiles to no opcodes of its own +const CONTRACT_TEST_ONLY_PARAMETER_CHECK = ` +contract Test() { + function spend(bool b) { + require(b); + } +} +`; + 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 @@ -611,6 +655,12 @@ 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 artifactTestFinalRequireDefinedFunction = compileString( + CONTRACT_TEST_FINAL_REQUIRE_DEFINED_FUNCTION, + { disableInlining: true }, +); +export const artifactTestLogAfterFailure = compileString(CONTRACT_TEST_LOG_AFTER_FAILURE); +export const artifactTestOnlyParameterCheck = compileString(CONTRACT_TEST_ONLY_PARAMETER_CHECK); 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/test/fixture/debugging/multi_contract_debugging_contracts.ts b/packages/cashscript/test/fixture/debugging/multi_contract_debugging_contracts.ts index 412a7b423..80eaa1ce2 100644 --- a/packages/cashscript/test/fixture/debugging/multi_contract_debugging_contracts.ts +++ b/packages/cashscript/test/fixture/debugging/multi_contract_debugging_contracts.ts @@ -41,7 +41,28 @@ contract FunctionNameCollision(int a) { } `; +// Two contracts where the name of one is a prefix of the name of the other +const PREFIX_NAME = ` +contract Vault(int a) { + function spend(int b) { + console.log("vault b is", b); + require(a == b, "b should be a"); + } +} +`; + +const PREFIXED_NAME = ` +contract VaultSidecar() { + function spend(int c, int d) { + require(c + d == 10, "c + d should be 10"); + require(c > d, "c should be larger than d"); + } +} +`; + export const ARTIFACT_SAME_NAME_DIFFERENT_PATH = compileString(SAME_NAME_DIFFERENT_PATH); export const ARTIFACT_NAME_COLLISION = compileString(NAME_COLLISION); export const ARTIFACT_CONTRACT_NAME_COLLISION = compileString(CONTRACT_NAME_COLLISION); export const ARTIFACT_FUNCTION_NAME_COLLISION = compileString(FUNCTION_NAME_COLLISION); +export const ARTIFACT_PREFIX_NAME = compileString(PREFIX_NAME); +export const ARTIFACT_PREFIXED_NAME = compileString(PREFIXED_NAME); diff --git a/packages/cashscript/test/multi-contract-debugging.test.ts b/packages/cashscript/test/multi-contract-debugging.test.ts index 38a29de88..920349880 100644 --- a/packages/cashscript/test/multi-contract-debugging.test.ts +++ b/packages/cashscript/test/multi-contract-debugging.test.ts @@ -16,7 +16,7 @@ import { } from './fixture/vars.js'; import p2pkhArtifact from './fixture/p2pkh.artifact.js'; import bigintArtifact from './fixture/bigint.artifact.js'; -import { ARTIFACT_FUNCTION_NAME_COLLISION, ARTIFACT_NAME_COLLISION, ARTIFACT_CONTRACT_NAME_COLLISION, ARTIFACT_SAME_NAME_DIFFERENT_PATH } from './fixture/debugging/multi_contract_debugging_contracts.js'; +import { ARTIFACT_FUNCTION_NAME_COLLISION, ARTIFACT_NAME_COLLISION, ARTIFACT_CONTRACT_NAME_COLLISION, ARTIFACT_SAME_NAME_DIFFERENT_PATH, ARTIFACT_PREFIX_NAME, ARTIFACT_PREFIXED_NAME } from './fixture/debugging/multi_contract_debugging_contracts.js'; import { addressToLockScript } from '../src/utils.js'; import SiblingIntrospectionArtifact from './fixture/SiblingIntrospection.artifact.js'; @@ -339,6 +339,38 @@ describe('Multi-Contract-Debugging tests', () => { }); }); + // Each input is debugged with the artifact of its own contract, also if one contract name is a prefix of the other + describe('Contract names that are a prefix of each other', () => { + const vault = new Contract(ARTIFACT_PREFIX_NAME, [1n], { provider }); + const sidecar = new Contract(ARTIFACT_PREFIXED_NAME, [], { provider }); + + const createTransaction = (vaultFirst: boolean, c: bigint, d: bigint): TransactionBuilder => { + const vaultInput = { ...provider.addUtxo(vault.address, randomUtxo()), unlocker: vault.unlock.spend(1n) }; + const sidecarInput = { ...provider.addUtxo(sidecar.address, randomUtxo()), unlocker: sidecar.unlock.spend(c, d) }; + + return new TransactionBuilder({ provider }) + .addInputs(vaultFirst ? [vaultInput, sidecarInput] : [sidecarInput, vaultInput]) + .addOutput({ to: vault.address, amount: 10000n }); + }; + + it.each([true, false])('should only log the console.log statements of the executed contract (vault first: %s)', async (vaultFirst) => { + const transaction = createTransaction(vaultFirst, 6n, 4n); + const vaultInputIndex = vaultFirst ? 0 : 1; + const sidecarInputIndex = vaultFirst ? 1 : 0; + + expect(transaction).toLog(`[Input #${vaultInputIndex}] Vault.cash:4 vault b is 1`); + expect(transaction).not.toLog(`Input #${sidecarInputIndex}`); + await expect(transaction.send()).resolves.toBeDefined(); + }); + + it.each([true, false])('should fail with the require statement of the failing contract (vault first: %s)', (vaultFirst) => { + const transaction = createTransaction(vaultFirst, 4n, 6n); + + expect(transaction).toFailRequireWith('VaultSidecar.cash:5 Require statement failed at input'); + expect(transaction).toFailRequireWith('Failing statement: require(c > d, "c should be larger than d")'); + }); + }); + describe('Non-require error messages', () => { it('should fail with the correct error message when there are name collisions on the contractName', () => { const nameCollision = new Contract(ARTIFACT_NAME_COLLISION, [0n], { provider }); diff --git a/packages/utils/src/bitauth-script.ts b/packages/utils/src/bitauth-script.ts index 03dd67d32..21376a953 100644 --- a/packages/utils/src/bitauth-script.ts +++ b/packages/utils/src/bitauth-script.ts @@ -303,7 +303,8 @@ function deriveAnchor( // The prologue is one contiguous opcode block at the function's start const lastPrologueOpcode = Math.max(...prologueTags.map((t) => t.endIndex)); - const firstBodyOpcode = lastPrologueOpcode + 1; + // If nothing follows the prologue (e.g. `require(b)` for a bool parameter), it is anchored to its own last opcode + const firstBodyOpcode = Math.min(lastPrologueOpcode + 1, locationData.length - 1); const firstBodyLine = getDisplayLine(locationData[firstBodyOpcode]); // `insertAfterLine` of `firstBodyLine - 1` lands all the prologue annotations directly above the From eb6875c6acec7891417de60295d82a03463418af Mon Sep 17 00:00:00 2001 From: Mathieu Geukens Date: Tue, 29 Sep 2026 00:06:15 +0200 Subject: [PATCH 2/3] fix: transaction builder and network provider findings from the 0.14 audit - send() wrapped every broadcast error in a FailedTransactionError without a cause, so the documented NetworkProvider*Error classes could not be caught. A NetworkProviderError is now thrown as it is, the ElectrumNetworkProvider falls back to a NetworkProviderError (not a plain Error) for unrecognised rejections, and the MockNetworkProvider throws a NetworkProviderMissingInputsError for a missing or spent UTXO. - addBchChangeOutputIfNeeded() calculated the fee from ECDSA signatures made before the change output existed. Signing again can make an ECDSA signature a byte longer, so about a quarter of transactions with one ECDSA input and a fee rate of 1 ended up below 1 sat/byte and failed to build. ECDSA signatures are now sized at their maximum length (73 bytes) for the change calculation. The docs mention that placeholder inputs assume Schnorr. - addOpReturnOutput() silently encoded invalid hex ('0xabc' became ab0c, '0xzz' became 00), like function arguments did before #454. It now throws. - Hex strings were compared case-sensitively when matching UTXOs to unlockers, token categories for token change and implicit burn checks, change locks and gatherFungibleTokenUtxos(). An upper case category passed to addTokenChangeOutputIfNeeded() added no change output, so with allowImplicitFungibleTokenBurn the tokens were burned. They are now compared case-insensitively. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/cashscript/src/TransactionBuilder.ts | 73 +++++++++++-- .../src/network/ElectrumNetworkProvider.ts | 5 +- .../src/network/MockNetworkProvider.ts | 5 +- packages/cashscript/src/transaction-utils.ts | 2 +- packages/cashscript/src/utils.ts | 17 ++- .../test/TransactionBuilder.test.ts | 101 +++++++++++++++++- website/docs/sdk/transaction-builder.md | 6 +- 7 files changed, 186 insertions(+), 23 deletions(-) diff --git a/packages/cashscript/src/TransactionBuilder.ts b/packages/cashscript/src/TransactionBuilder.ts index da93ace41..9fbe3527e 100644 --- a/packages/cashscript/src/TransactionBuilder.ts +++ b/packages/cashscript/src/TransactionBuilder.ts @@ -1,5 +1,6 @@ import { binToHex, + decodeAuthenticationInstructions, decodeTransaction, decodeTransactionUnsafe, encodeTransaction, @@ -19,12 +20,15 @@ import { StandardUnlockableUtxo, VmResourceUsage, isContractUnlocker, + isP2PKHUnlocker, isPlaceholderUnlocker, + SignatureAlgorithm, BchChangeOutputOptions, TokenChangeOutputOptions, } from './interfaces.js'; import { PLACEHOLDER_P2PKH_UNLOCKING_SIZE } from './constants.js'; import { NetworkProvider } from './network/index.js'; +import { NetworkProviderError } from './network/errors.js'; import { calculateDust, cashScriptOutputToLibauthOutput, @@ -47,6 +51,7 @@ import { TransactionTooLargeError, } from './Errors.js'; import { DebugResults } from './debugging.js'; +import SignatureTemplate from './SignatureTemplate.js'; import { debugLibauthTemplate, getLibauthTemplate, getBitauthUri } from './libauth-template/LibauthTemplate.js'; import { getWcContractInfo, WcSourceOutput, WcTransactionOptions } from './walletconnect-utils.js'; import semver from 'semver'; @@ -188,6 +193,7 @@ export class TransactionBuilder { * * @param chunks - The data chunks to include after the `OP_RETURN` opcode. * @returns This builder for chaining. + * @throws If a `0x`-prefixed chunk is not a valid hex string. */ addOpReturnOutput(chunks: string[]): this { this.addOutput(createOpReturnOutput(chunks)); @@ -211,7 +217,13 @@ export class TransactionBuilder { const totalBchOutputAmount = this.outputs.reduce((total, output) => total + output.amount, 0n); const tentativeSurplus = totalBchInputAmount - totalBchOutputAmount; - const tentativeTransactionSize = this.getTransactionSize(); + + // ECDSA signatures vary in length, and are generated again once the change output is added, so the fee is + // calculated as if every ECDSA signature has its maximum length + const tentativeTransaction = this.buildLibauthTransaction(true); + const tentativeTransactionSize = BigInt( + this.getEncodedTransactionSize(tentativeTransaction) + this.getEcdsaSignatureSizeMargin(tentativeTransaction), + ); const tentativeFee = BigInt(Math.ceil(changeOutputOptions.feeRate * Number(tentativeTransactionSize))); const tentativeChangeAmount = tentativeSurplus - tentativeFee; @@ -255,14 +267,16 @@ export class TransactionBuilder { * or BCH change output was already added. */ addTokenChangeOutputIfNeeded(changeOutputOptions: TokenChangeOutputOptions): this { - const { category, to } = changeOutputOptions; + // Token categories are hex strings, which are compared case-insensitively + const category = changeOutputOptions.category.toLowerCase(); + const { to } = changeOutputOptions; const inputAmount = this.inputs - .filter((input) => input.token?.category === category) + .filter((input) => input.token?.category.toLowerCase() === category) .reduce((total, input) => total + input.token!.amount, 0n); const outputAmount = this.outputs - .filter((output) => output.token?.category === category) + .filter((output) => output.token?.category.toLowerCase() === category) .reduce((total, output) => total + output.token!.amount, 0n); const changeAmount = inputAmount - outputAmount; @@ -300,6 +314,24 @@ export class TransactionBuilder { return encodeTransaction(transaction).byteLength + placeholderInputCount * PLACEHOLDER_P2PKH_UNLOCKING_SIZE; } + // The number of bytes that the transaction's ECDSA signatures are shorter than their maximum length + private getEcdsaSignatureSizeMargin(transaction: LibauthTransaction): number { + // 72-byte DER signature + sighash byte + const MAX_ECDSA_SIGNATURE_SIZE = 73; + + return this.inputs.reduce((margin, input, inputIndex) => { + const signatureIndices = getEcdsaSignaturePushIndices(input.unlocker); + if (signatureIndices.length === 0) return margin; + + const instructions = decodeAuthenticationInstructions(transaction.inputs[inputIndex].unlockingBytecode); + return signatureIndices.reduce((total, index) => { + const instruction = instructions[index]; + const signatureSize = instruction && 'data' in instruction ? instruction.data.length : MAX_ECDSA_SIGNATURE_SIZE; + return total + MAX_ECDSA_SIGNATURE_SIZE - signatureSize; + }, margin); + }, 0); + } + /** * Calculate the transaction fee in satoshis and the fee per byte. * @@ -489,7 +521,8 @@ export class TransactionBuilder { * require statements or VM errors surface with descriptive messages. * * @returns The decoded transaction details (including txid and raw hex). - * @throws A `FailedTransactionError` if the network rejects the transaction, or any of the + * @throws A `NetworkProviderError` (or one of its subclasses) if the network provider rejects the transaction, + * a `FailedTransactionError` if broadcasting fails otherwise, or any of the * build / local evaluation errors (e.g. fee cap, implicit burn, failing require statement). */ async send(): Promise; @@ -500,7 +533,8 @@ export class TransactionBuilder { * * @param raw - Pass `true` to receive the raw transaction hex instead of decoded details. * @returns The raw transaction hex as retrieved from the network after broadcast. - * @throws A `FailedTransactionError` if the network rejects the transaction, or any of the + * @throws A `NetworkProviderError` (or one of its subclasses) if the network provider rejects the transaction, + * a `FailedTransactionError` if broadcasting fails otherwise, or any of the * build / local evaluation errors (e.g. fee cap, implicit burn, failing require statement). */ async send(raw: true): Promise; @@ -517,6 +551,9 @@ export class TransactionBuilder { try { txid = await this.provider.sendRawTransaction(tx); } catch (e: any) { + // Network provider errors describe why the network rejected the transaction, so they are thrown as they are + if (e instanceof NetworkProviderError) throw e; + const reason = e.error ?? e.message; const getBitauthUriWithFallback = (): string => { @@ -608,14 +645,17 @@ export class TransactionBuilder { const tokenInputAmounts: Record = {}; const tokenOutputAmounts: Record = {}; + // Token categories are hex strings, which are compared case-insensitively for (const input of this.inputs) { if (input.token?.amount) { - tokenInputAmounts[input.token.category] = (tokenInputAmounts[input.token.category] || 0n) + input.token.amount; + const category = input.token.category.toLowerCase(); + tokenInputAmounts[category] = (tokenInputAmounts[category] || 0n) + input.token.amount; } } for (const output of this.outputs) { if (output.token?.amount) { - tokenOutputAmounts[output.token.category] = (tokenOutputAmounts[output.token.category] || 0n) + output.token.amount; + const category = output.token.category.toLowerCase(); + tokenOutputAmounts[category] = (tokenOutputAmounts[category] || 0n) + output.token.amount; } } @@ -653,3 +693,20 @@ export class TransactionBuilder { } } } + +// The positions of the pushes in the unlocking bytecode that hold an ECDSA signature generated by a SignatureTemplate +function getEcdsaSignaturePushIndices(unlocker: Unlocker): number[] { + const isEcdsaTemplate = (param: unknown): boolean => ( + param instanceof SignatureTemplate && param.signatureAlgorithm === SignatureAlgorithm.ECDSA + ); + + // P2PKH unlocking bytecode is + if (isP2PKHUnlocker(unlocker)) return isEcdsaTemplate(unlocker.template) ? [0] : []; + + // Contract function arguments are pushed in reverse order + if (isContractUnlocker(unlocker)) { + return unlocker.params.flatMap((param, i, params) => (isEcdsaTemplate(param) ? [params.length - 1 - i] : [])); + } + + return []; +} diff --git a/packages/cashscript/src/network/ElectrumNetworkProvider.ts b/packages/cashscript/src/network/ElectrumNetworkProvider.ts index eafe772c8..812c55309 100644 --- a/packages/cashscript/src/network/ElectrumNetworkProvider.ts +++ b/packages/cashscript/src/network/ElectrumNetworkProvider.ts @@ -9,6 +9,7 @@ import { SpendableUtxo, Network } from '../interfaces.js'; import NetworkProvider from './NetworkProvider.js'; import { addressToLockScript } from '../utils.js'; import { + NetworkProviderError, NetworkProviderMissingInputsError, NetworkProviderMempoolConflictError, NetworkProviderTransactionAlreadySubmittedError, @@ -249,7 +250,7 @@ const RELATIVE_TIMELOCK_PATTERNS = [ 'non-BIP68-final', ]; -function classifyNetworkProviderError(errorMessage: string): Error { +function classifyNetworkProviderError(errorMessage: string): NetworkProviderError { if (MISSING_INPUTS_PATTERNS.some((pattern) => errorMessage.includes(pattern))) { return new NetworkProviderMissingInputsError(errorMessage); } @@ -270,7 +271,7 @@ function classifyNetworkProviderError(errorMessage: string): Error { return new NetworkProviderRelativeTimelockError(errorMessage); } - return new Error(errorMessage); + return new NetworkProviderError(errorMessage, errorMessage); } function lockingBytecodeToElectrumScriptHash(lockingBytecode: Uint8Array): string { diff --git a/packages/cashscript/src/network/MockNetworkProvider.ts b/packages/cashscript/src/network/MockNetworkProvider.ts index c86dabb04..7d9c8907c 100644 --- a/packages/cashscript/src/network/MockNetworkProvider.ts +++ b/packages/cashscript/src/network/MockNetworkProvider.ts @@ -4,7 +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'; +import { NetworkProviderAbsoluteTimelockError, NetworkProviderMissingInputsError } from './errors.js'; /** * Options accepted by the `MockNetworkProvider` constructor. @@ -130,9 +130,8 @@ export default class MockNetworkProvider implements NetworkProvider { 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) { - throw new Error(`UTXO not found for input ${input.outpointIndex} of transaction ${txid}`); + throw new NetworkProviderMissingInputsError(`UTXO not found for input ${input.outpointIndex} of transaction ${txid}`); } return remainingUtxoEntries.splice(utxoIndex, 1)[0]; diff --git a/packages/cashscript/src/transaction-utils.ts b/packages/cashscript/src/transaction-utils.ts index 1373ef2c6..25d81cfe9 100644 --- a/packages/cashscript/src/transaction-utils.ts +++ b/packages/cashscript/src/transaction-utils.ts @@ -54,7 +54,7 @@ export function gatherFungibleTokenUtxos( utxos: U[], tokenCategory: string, amount: bigint, ): GatherUtxosResult { const sortedTokenUtxos = utxos - .filter((utxo) => isFungibleTokenUtxo(utxo) && utxo.token!.category === tokenCategory) + .filter((utxo) => isFungibleTokenUtxo(utxo) && utxo.token!.category.toLowerCase() === tokenCategory.toLowerCase()) .toSorted((a, b) => Number(b.token!.amount - a.token!.amount)); const targetUtxos: U[] = []; diff --git a/packages/cashscript/src/utils.ts b/packages/cashscript/src/utils.ts index e8a2744bf..61cf9c5af 100644 --- a/packages/cashscript/src/utils.ts +++ b/packages/cashscript/src/utils.ts @@ -76,7 +76,8 @@ export function validateInput(utxo: Utxo, changeLocks: Record): // A UTXO/unlocker mismatch would be rejected by the network, so we catch it locally with a descriptive error export function validateUnlocker(utxo: SpendableUtxo, unlocker: Unlocker, inputIndex: number, network: Network): void { const unlockerLockingBytecode = getUnlockerLockingBytecode(unlocker); - if (unlockerLockingBytecode === undefined || utxo.lockingBytecode === unlockerLockingBytecode) return; + if (unlockerLockingBytecode === undefined) return; + if (utxo.lockingBytecode.toLowerCase() === unlockerLockingBytecode.toLowerCase()) return; throw new UnlockerLockingBytecodeMismatchError( inputIndex, @@ -171,7 +172,8 @@ function validateChangeLocks(changeLocks: Record, category?: st throw new OutputBchChangeLockedError(); } - if (category && changeLocks[category]) { + // Token categories are hex strings, which are compared case-insensitively + if (category && changeLocks[category.toLowerCase()]) { throw new OutputTokenChangeLockedError(category); } } @@ -317,9 +319,14 @@ export function createOpReturnOutput( } function toBin(output: string): Uint8Array { - const data = output.replace(/^0x/, ''); - const encode = data === output ? utf8ToBin : hexToBin; - return encode(data); + if (!output.startsWith('0x')) return utf8ToBin(output); + + const hex = output.slice(2); + if (!isHex(hex)) { + throw new Error(`OP_RETURN chunk should be a valid hex string with an even number of digits, found '${output}'`); + } + + return hexToBin(hex); } // BCH consensus requires the fork id flag on every signing serialization diff --git a/packages/cashscript/test/TransactionBuilder.test.ts b/packages/cashscript/test/TransactionBuilder.test.ts index 3395bc4b7..493ede5fb 100644 --- a/packages/cashscript/test/TransactionBuilder.test.ts +++ b/packages/cashscript/test/TransactionBuilder.test.ts @@ -1,4 +1,4 @@ -import { decodeTransactionUnsafe, hexToBin, stringify } from '@bitauth/libauth'; +import { binToHex, decodeTransactionUnsafe, hexToBin, stringify } from '@bitauth/libauth'; import { Contract, SignatureTemplate, ElectrumNetworkProvider, MockNetworkProvider, placeholderP2PKHUnlocker, placeholderPublicKey, placeholderSignature } from '../src/index.js'; import { bobAddress, @@ -14,8 +14,8 @@ import { carolTokenAddress, alicePriv, } from './fixture/vars.js'; -import { Network, SpendableUtxo, Utxo } from '../src/interfaces.js'; -import { utxoComparator, calculateDust, randomUtxo, randomToken, isNonTokenUtxo, isFungibleTokenUtxo } from '../src/utils.js'; +import { Network, SignatureAlgorithm, SighashType, SpendableUtxo, Utxo } from '../src/interfaces.js'; +import { utxoComparator, calculateDust, randomUtxo, randomToken, isNonTokenUtxo, isFungibleTokenUtxo, addressToLockScript } from '../src/utils.js'; import p2pkhArtifact from './fixture/p2pkh.artifact.js'; import twtArtifact from './fixture/transfer_with_timeout.artifact.js'; import { TransactionBuilder } from '../src/TransactionBuilder.js'; @@ -30,6 +30,8 @@ import { UnlockerLockingBytecodeMismatchError, } from '../src/Errors.js'; import { FailingMockNetworkProvider } from '../src/network/MockNetworkProvider.js'; +import { NetworkProviderError, NetworkProviderMissingInputsError } from '../src/network/errors.js'; +import { ElectrumClient, type ElectrumClientEvents } from '@electrum-cash/network'; describe('Transaction Builder', () => { const provider = process.env.TESTS_USE_CHIPNET @@ -189,6 +191,14 @@ describe('Transaction Builder', () => { expect(() => buildWithFee(size)).not.toThrow(); }); + it('should fail when an OP_RETURN chunk is not valid hex', async () => { + const builder = new TransactionBuilder({ provider }); + + expect(() => builder.addOpReturnOutput(['0xabc'])).toThrow("OP_RETURN chunk should be a valid hex string with an even number of digits, found '0xabc'"); + expect(() => builder.addOpReturnOutput(['0xzz'])).toThrow("found '0xzz'"); + expect(() => builder.addOpReturnOutput(['0x', '0xABCD', 'abc'])).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 () => { @@ -273,6 +283,13 @@ describe('Transaction Builder', () => { )).toThrow(UnlockerLockingBytecodeMismatchError); }); + it('should compare the UTXO and unlocker locking bytecode case-insensitively', async () => { + const utxo = randomUtxo({ lockingBytecode: p2pkhInstance.lockingBytecode.toUpperCase() }); + const unlocker = p2pkhInstance.unlock.spend(carolPub, new SignatureTemplate(carolPriv)); + + expect(() => new TransactionBuilder({ provider }).addInput(utxo, unlocker)).not.toThrow(); + }); + it('should fail when adding a UTXO without a lockingBytecode', async () => { const utxoWithoutLockingBytecode = randomUtxo() as SpendableUtxo; @@ -395,6 +412,41 @@ describe('Transaction Builder', () => { expect((await error).message).toMatch(/^Transaction [0-9a-f]{64} was broadcast, but its details could not be retrieved/); }); + it('should throw network provider errors from send() unchanged', async () => { + const mockProvider = new MockNetworkProvider(); + const utxo = mockProvider.addUtxo(aliceAddress, randomUtxo({ satoshis: 100_000n })); + const createTransaction = (amount: bigint): TransactionBuilder => new TransactionBuilder({ provider: mockProvider }) + .addInput(utxo, new SignatureTemplate(alicePriv).unlockP2PKH()) + .addOutput({ to: aliceAddress, amount }); + + await createTransaction(90_000n).send(); + + // The UTXO was spent by the first transaction, so a second transaction spending it is a double spend + const error = await createTransaction(80_000n).send().catch((e) => e); + expect(error).toBeInstanceOf(NetworkProviderMissingInputsError); + expect(error.message).toContain('UTXO not found'); + }); + + it('should throw a NetworkProviderError from send() for an unrecognised Electrum rejection', async () => { + const electrum = { + connect: vi.fn(async () => {}), + disconnect: vi.fn(async () => true), + request: vi.fn(async () => new Error('some-unrecognised-rejection')), + }; + const electrumProvider = new ElectrumNetworkProvider(Network.CHIPNET, { + electrum: electrum as unknown as ElectrumClient, + }); + + const utxo = randomUtxo({ lockingBytecode: p2pkhInstance.lockingBytecode }); + const transaction = new TransactionBuilder({ provider: electrumProvider }) + .addInput(utxo, p2pkhInstance.unlock.spend(carolPub, new SignatureTemplate(carolPriv))) + .addOutput({ to: carolAddress, amount: 1_000n }); + + const error = await transaction.send().catch((e) => e); + expect(error).toBeInstanceOf(NetworkProviderError); + expect(error.originalError).toBe('some-unrecognised-rejection'); + }); + it('should preserve the Bitauth URI when broadcast fails', async () => { const failingProvider = new FailingMockNetworkProvider(); const contract = new Contract(p2pkhArtifact, [carolPkh], { provider: failingProvider }); @@ -479,6 +531,25 @@ describe('Transaction Builder', () => { }); }); + describe('addBchChangeOutputIfNeeded', () => { + it('should never pay less than the fee rate with ECDSA signatures', () => { + const ecdsaTemplate = new SignatureTemplate(carolPriv, SighashType.SIGHASH_ALL, SignatureAlgorithm.ECDSA); + const carolLockingBytecode = binToHex(addressToLockScript(carolAddress)); + + // ECDSA signatures vary in length, so signing again after adding the change output can make the transaction + // larger than the transaction that the change was calculated for + for (let i = 0; i < 100; i += 1) { + const builder = new TransactionBuilder({ provider }) + .addInput(randomUtxo({ lockingBytecode: carolLockingBytecode }), ecdsaTemplate.unlockP2PKH()) + .addInput(randomContractUtxo(), p2pkhInstance.unlock.spend(carolPub, ecdsaTemplate)) + .addOutput({ to: bobAddress, amount: 10_000n }) + .addBchChangeOutputIfNeeded({ to: aliceAddress, feeRate: 1 }); + + expect(() => builder.build()).not.toThrow(); + } + }); + }); + describe('token change lock', () => { it('should prevent further inputs or outputs of the same category after a token change output was added', () => { const token = randomToken(); @@ -559,6 +630,30 @@ describe('Transaction Builder', () => { expect(carolChange!.token).toEqual({ amount: 3000n, category: tokenB.category }); }); + it('should compare token categories case-insensitively', () => { + const token = randomToken({ amount: 1000n }); + const upperCaseCategory = token.category.toUpperCase(); + + const builder = new TransactionBuilder({ provider, allowImplicitFungibleTokenBurn: true }) + .addInput(randomContractUtxo({ token }), carolUnlocker()) + .addOutput({ to: bobTokenAddress, amount: 1000n, token: { amount: 400n, category: upperCaseCategory } }) + .addTokenChangeOutputIfNeeded({ category: upperCaseCategory, to: aliceTokenAddress }); + + // The change output is added, so no tokens are burned + const txOutputs = getTxOutputs(decodeTransactionUnsafe(hexToBin(builder.build()))); + const changeOutput = txOutputs.find((o) => o.to === aliceTokenAddress); + expect(changeOutput!.token).toEqual({ amount: 600n, category: token.category }); + + // The category is locked in either case + expect(() => builder.addInput(randomContractUtxo({ token }), carolUnlocker())).toThrow(OutputTokenChangeLockedError); + + // Without implicit burns, tokens sent to the upper case category are not counted as burned + expect(() => new TransactionBuilder({ provider }) + .addInput(randomContractUtxo({ token }), carolUnlocker()) + .addOutput({ to: bobTokenAddress, amount: 1000n, token: { amount: 1000n, category: upperCaseCategory } }) + .build()).not.toThrow(); + }); + it('should fail when the change address does not support tokens', () => { const token = randomToken(); const builder = new TransactionBuilder({ provider }) diff --git a/website/docs/sdk/transaction-builder.md b/website/docs/sdk/transaction-builder.md index 525042d34..dd0276f17 100644 --- a/website/docs/sdk/transaction-builder.md +++ b/website/docs/sdk/transaction-builder.md @@ -157,7 +157,7 @@ transactionBuilder.addOutputs([ transactionBuilder.addOpReturnOutput(chunks: string[]): this ``` -Adds an OP_RETURN output to the transaction with the provided data chunks in string format. If the string is `0x`-prefixed, it is treated as a hex string. Otherwise it is treated as a UTF-8 string. +Adds an OP_RETURN output to the transaction with the provided data chunks in string format. If the string is `0x`-prefixed, it is treated as a hex string, and an error is thrown if it is not valid hex. Otherwise it is treated as a UTF-8 string. #### Example ```ts @@ -174,6 +174,8 @@ Adds a change output to the transaction if the transaction has enough funds to c After a BCH change output has been added, no more inputs or outputs can be added to the transaction. This is enforced by the SDK to prevent accidentally invalidating the change calculation. +The fee is calculated as if every ECDSA signature in the transaction has its maximum length (73 bytes), since ECDSA signatures vary in length and are generated again once the change output is added. Inputs with a `placeholderP2PKHUnlocker()` are sized for a 65-byte Schnorr signature, so a wallet that signs them with ECDSA makes the transaction larger than the fee was calculated for. + ```ts interface BchChangeOutputOptions { to: string | Uint8Array; @@ -432,4 +434,6 @@ interface FailedRequireError { If you are using an artifact compiled with an older version of `cashc`, the error will always be of the type `FailedTransactionError`. In this case, you can use the `reason` property of the error to determine the reason for the failure. +When the network provider rejects the transaction with a `NetworkProviderError` or one of its subclasses (see [Error Handling](/docs/sdk/electrum-network-provider#error-handling)), `send()` throws that error as it is, e.g. a `NetworkProviderMissingInputsError` when an input was already spent. Other errors while broadcasting are thrown as a `FailedTransactionError`. + [bitcoin-wiki-timelocks]: https://en.bitcoin.it/wiki/Timelock From 230d098b402ca37c853d263037516b802e6718cd Mon Sep 17 00:00:00 2001 From: Mathieu Geukens Date: Tue, 29 Sep 2026 00:06:15 +0200 Subject: [PATCH 3/3] fix: throw on unknown opcodes and invalid data in asmToBytecode asmToBytecode encoded an unknown opcode name as OP_0 and decoded invalid hex data tokens leniently. An artifact that uses an opcode the installed libauth version does not know would silently get a different bytecode and address, and funds sent there may be unspendable. Unknown opcode names and data tokens that are not hex with an even number of digits now throw an error. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/utils/src/script.ts | 14 +++++++++++--- packages/utils/test/script.test.ts | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/utils/src/script.ts b/packages/utils/src/script.ts index 115baeb8c..0535e1217 100644 --- a/packages/utils/src/script.ts +++ b/packages/utils/src/script.ts @@ -1,6 +1,7 @@ import { encodeDataPush, hexToBin, + isHex, disassembleBytecodeBch, flattenBinArray, encodeAuthenticationInstructions, @@ -68,14 +69,21 @@ export function asmToBytecode(asm: string): Uint8Array { if (asm === '') return new Uint8Array(); // Convert the ASM tokens to AuthenticationInstructions + // Unknown opcodes and invalid hex would otherwise silently be encoded as different bytecode const instructions = asm.split(' ').map((token) => { - // Even though the OpcodesBch type allows for { [key: number]: string }, we know that the keys are always the opcodes - // so we can safely cast to the AuthenticationInstruction type if (token.startsWith('OP_')) { - return { opcode: Op[token as keyof typeof Op] } as AuthenticationInstruction; + const opcode = Op[token as keyof typeof Op]; + if (typeof opcode !== 'number') { + throw new Error(`Unknown opcode '${token}' in ASM`); + } + + return { opcode } as AuthenticationInstruction; } const data = token.replace(/<|>/g, '').replace(/^0x/, ''); + if (!isHex(data)) { + throw new Error(`Invalid data '${token}' in ASM, expected a hex string with an even number of digits`); + } return decodeAuthenticationInstructions(encodeDataPush(hexToBin(data)))[0]; }); diff --git a/packages/utils/test/script.test.ts b/packages/utils/test/script.test.ts index fa7ae8f4a..8a19427a9 100644 --- a/packages/utils/test/script.test.ts +++ b/packages/utils/test/script.test.ts @@ -7,6 +7,7 @@ import { calculateBytesize, countOpcodes, encodeNullDataScript, + Op, scriptToAsm, scriptToBytecode, } from '../src/index.js'; @@ -53,6 +54,26 @@ describe('script utils', () => { }); }); + describe('asmToBytecode() with invalid ASM', () => { + it('should throw on an unknown opcode instead of encoding it as OP_0', () => { + expect(() => asmToBytecode('OP_1 OP_NOT_AN_OPCODE')).toThrow("Unknown opcode 'OP_NOT_AN_OPCODE' in ASM"); + expect(() => asmToBytecode('OP_toString')).toThrow("Unknown opcode 'OP_toString' in ASM"); + }); + + it('should throw on data that is not hex or has an odd number of digits', () => { + expect(() => asmToBytecode('OP_1 zz')).toThrow("Invalid data 'zz' in ASM"); + expect(() => asmToBytecode('abc')).toThrow("Invalid data 'abc' in ASM"); + expect(() => asmToBytecode('<0xabc>')).toThrow("Invalid data '<0xabc>' in ASM"); + }); + + it('should encode every opcode name that bytecodeToAsm produces', () => { + for (let opcode = Op.OP_1NEGATE; opcode <= 0xff; opcode += 1) { + const bytecode = Uint8Array.of(opcode); + expect(asmToBytecode(bytecodeToAsm(bytecode))).toEqual(bytecode); + } + }); + }); + describe('bytecodeToAsm()', () => { fixtures.forEach(({ name, asm, bytecode }) => { it(`should convert bytecode to asm for "${name}"`, () => {