Skip to content

Commit

Permalink
fix: bitcoin fees underestimation, closes #4777
Browse files Browse the repository at this point in the history
  • Loading branch information
@kyranjamie
kyranjamie committed Apr 23, 2024
1 parent b0276ec commit e5a6206
Show file tree
Hide file tree
Showing 13 changed files with 225 additions and 128 deletions.
73 changes: 50 additions & 23 deletions src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,52 @@ const demoUtxos = [
{ value: 909 },
];

function generate10kSpendWithDummyUtxoSet(recipient: string) {
return determineUtxosForSpend({
utxos: demoUtxos as any,
amount: 10_000,
feeRate: 20,
recipient,
});
}

describe(determineUtxosForSpend.name, () => {
function generate10kSpendWithTestData(recipient: string) {
return determineUtxosForSpend({
utxos: demoUtxos as any,
amount: 10_000,
feeRate: 20,
recipient,
describe('Estimated size', () => {
test('that Native Segwit, 1 input 2 outputs weighs 140 vBytes', () => {
const estimation = determineUtxosForSpend({
utxos: [{ value: 50_000 }] as any[],
amount: 40_000,
recipient: 'tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m',
feeRate: 20,
});
console.log(estimation);
expect(estimation.txVBytes).toBeGreaterThan(140);
expect(estimation.txVBytes).toBeLessThan(142);
});
}

describe('sorting algorithm (biggest first and no dust)', () => {
test('that Native Segwit, 2 input 2 outputs weighs 200vBytes', () => {
const estimation = determineUtxosForSpend({
utxos: [{ value: 50_000 }, { value: 50_000 }] as any[],
amount: 60_000,
recipient: 'tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m',
feeRate: 20,
});
console.log(estimation);
expect(estimation.txVBytes).toBeGreaterThan(208);
expect(estimation.txVBytes).toBeLessThan(209);
});
});

describe('sorting algorithm', () => {
test('that it filters out dust utxos', () => {
const result = generate10kSpendWithTestData('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
const result = generate10kSpendWithDummyUtxoSet('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
console.log(result);
const hasDust = result.filteredUtxos.some(utxo => utxo.value <= BTC_P2WPKH_DUST_AMOUNT);
expect(hasDust).toBeFalsy();
});

test('that it sorts utxos in decending order', () => {
const result = generate10kSpendWithTestData('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
const result = generate10kSpendWithDummyUtxoSet('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
result.inputs.forEach((u, i) => {
const nextUtxo = result.inputs[i + 1];
if (!nextUtxo) return;
Expand All @@ -50,41 +77,41 @@ describe(determineUtxosForSpend.name, () => {

test('that it accepts a wrapped segwit address', () =>
expect(() =>
generate10kSpendWithTestData('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH')
generate10kSpendWithDummyUtxoSet('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH')
).not.toThrowError());

test('that it accepts a legacy addresses', () =>
expect(() =>
generate10kSpendWithTestData('15PyZveQd28E2SHZu2ugkWZBp6iER41vXj')
generate10kSpendWithDummyUtxoSet('15PyZveQd28E2SHZu2ugkWZBp6iER41vXj')
).not.toThrowError());

test('that it throws an error with non-legit address', () => {
expect(() =>
generate10kSpendWithTestData('whoop-de-da-boop-da-de-not-a-bitcoin-address')
generate10kSpendWithDummyUtxoSet('whoop-de-da-boop-da-de-not-a-bitcoin-address')
).toThrowError();
});

test('that given a set of utxos, legacy is more expensive', () => {
const legacy = generate10kSpendWithTestData('15PyZveQd28E2SHZu2ugkWZBp6iER41vXj');
const segwit = generate10kSpendWithTestData('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH');
expect(legacy.fee).toBeGreaterThan(segwit.fee);
const legacy = generate10kSpendWithDummyUtxoSet('15PyZveQd28E2SHZu2ugkWZBp6iER41vXj');
const segwit = generate10kSpendWithDummyUtxoSet('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH');
expect(legacy.estimatedFee).toBeGreaterThan(segwit.estimatedFee);
});

test('that given a set of utxos, wrapped segwit is more expensive than native', () => {
const segwit = generate10kSpendWithTestData('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH');
const native = generate10kSpendWithTestData('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
expect(segwit.fee).toBeGreaterThan(native.fee);
const segwit = generate10kSpendWithDummyUtxoSet('33SVjoCHJovrXxjDKLFSXo1h3t5KgkPzfH');
const native = generate10kSpendWithDummyUtxoSet('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
expect(segwit.estimatedFee).toBeGreaterThan(native.estimatedFee);
});

test('that given a set of utxos, taproot is more expensive than native segwit', () => {
// Non-obvious behaviour.
// P2TR outputs = 34 vBytes
// P2WPKH outputs = 22 vBytes
const native = generate10kSpendWithTestData('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
const taproot = generate10kSpendWithTestData(
const native = generate10kSpendWithDummyUtxoSet('tb1qt28eagxcl9gvhq2rpj5slg7dwgxae2dn2hk93m');
const taproot = generate10kSpendWithDummyUtxoSet(
'tb1parwmj7533de3k2fw2kntyqacspvhm67qnjcmpqnnpfvzu05l69nsczdywd'
);
expect(taproot.fee).toBeGreaterThan(native.fee);
expect(taproot.estimatedFee).toBeGreaterThan(native.estimatedFee);
});

test('against a random set of generated utxos', () => {
Expand All @@ -102,7 +129,7 @@ describe(determineUtxosForSpend.name, () => {

expect(result.outputs[1].value.toString()).toEqual(
sumNumbers(result.inputs.map(i => i.value))
.minus(result.fee)
.minus(result.estimatedFee)
.minus(amount.toString())
.toString()
);
Expand Down
78 changes: 48 additions & 30 deletions src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import BigNumber from 'bignumber.js';
import { validate } from 'bitcoin-address-validation';

import type { RpcSendTransferRecipient } from '@shared/rpc/methods/send-transfer';

import { sumNumbers } from '@app/common/math/helpers';
import { UtxoResponseItem } from '@app/query/bitcoin/bitcoin-client';

import {
filterUneconomicalUtxos,
filterUneconomicalUtxosMultipleRecipients,
getSizeInfo,
getBitcoinTxSizeEstimation,
getSizeInfoMultipleRecipients,
} from '../utils';

Expand All @@ -33,25 +35,29 @@ export function determineUtxosForSpendAll({
if (!validate(recipient)) throw new Error('Cannot calculate spend of invalid address type');
const filteredUtxos = filterUneconomicalUtxos({ utxos, feeRate, address: recipient });

const sizeInfo = getSizeInfo({
inputLength: filteredUtxos.length,
outputLength: 1,
const sizeInfo = getBitcoinTxSizeEstimation({
inputCount: filteredUtxos.length,
outputCount: 1,
recipient,
});

// Fee has already been deducted from the amount with send all
const outputs = [{ value: BigInt(amount), address: recipient }];

const fee = Math.ceil(sizeInfo.txVBytes * feeRate);
const estimatedFee = Math.ceil(sizeInfo.txVBytes * feeRate);

return {
inputs: filteredUtxos,
outputs,
size: sizeInfo.txVBytes,
fee,
estimatedFee,
};
}

function getUtxoTotal(utxos: UtxoResponseItem[]) {
return sumNumbers(utxos.map(utxo => utxo.value));
}

export function determineUtxosForSpend({
amount,
feeRate,
Expand All @@ -60,47 +66,59 @@ export function determineUtxosForSpend({
}: DetermineUtxosForSpendArgs) {
if (!validate(recipient)) throw new Error('Cannot calculate spend of invalid address type');

const orderedUtxos = utxos.sort((a, b) => b.value - a.value);

const filteredUtxos = filterUneconomicalUtxos({
utxos: orderedUtxos,
const filteredUtxos: UtxoResponseItem[] = filterUneconomicalUtxos({
utxos: utxos.sort((a, b) => b.value - a.value),
feeRate,
address: recipient,
});

const neededUtxos = [];
let sum = 0n;
let sizeInfo = null;
if (!filteredUtxos.length) throw new InsufficientFundsError();

for (const utxo of filteredUtxos) {
sizeInfo = getSizeInfo({
inputLength: neededUtxos.length,
outputLength: 2,
// Prepopulate with first UTXO, at least one is needed
const neededUtxos: UtxoResponseItem[] = [filteredUtxos[0]];

function estimateTransactionSize() {
return getBitcoinTxSizeEstimation({
inputCount: neededUtxos.length,
outputCount: 2,
recipient,
});
if (sum >= BigInt(amount) + BigInt(Math.ceil(sizeInfo.txVBytes * feeRate))) break;
}

sum += BigInt(utxo.value);
neededUtxos.push(utxo);
function hasSufficientUtxosForTx() {
const txEstimation = estimateTransactionSize();
const neededAmount = new BigNumber(txEstimation.txVBytes * feeRate).plus(amount);
return getUtxoTotal(neededUtxos).isGreaterThanOrEqualTo(neededAmount);
}

if (!sizeInfo) throw new InsufficientFundsError();
function getRemainingUnspentUtxos() {
return filteredUtxos.filter(utxo => !neededUtxos.includes(utxo));
}

const fee = Math.ceil(sizeInfo.txVBytes * feeRate);
while (!hasSufficientUtxosForTx()) {
const [nextUtxo] = getRemainingUnspentUtxos();
if (!nextUtxo) throw new InsufficientFundsError();
neededUtxos.push(nextUtxo);
}

const estimatedFee = Math.ceil(
new BigNumber(estimateTransactionSize().txVBytes).multipliedBy(feeRate).toNumber()
);

const outputs = [
// outputs[0] = the desired amount going to recipient
{ value: BigInt(amount), address: recipient },
// outputs[1] = the remainder to be returned to a change address
{ value: sum - BigInt(amount) - BigInt(fee) },
{ value: BigInt(getUtxoTotal(neededUtxos).toString()) - BigInt(amount) - BigInt(estimatedFee) },
];

return {
filteredUtxos,
inputs: neededUtxos,
outputs,
size: sizeInfo.txVBytes,
fee,
size: estimateTransactionSize().txVBytes,
...estimateTransactionSize(),
estimatedFee,
};
}

Expand Down Expand Up @@ -140,13 +158,13 @@ export function determineUtxosForSpendAllMultipleRecipients({
address,
}));

const fee = Math.ceil(sizeInfo.txVBytes * feeRate);
const estimatedFee = Math.ceil(sizeInfo.txVBytes * feeRate);

return {
inputs: filteredUtxos,
outputs,
size: sizeInfo.txVBytes,
fee,
estimatedFee,
};
}

Expand Down Expand Up @@ -186,7 +204,7 @@ export function determineUtxosForSpendMultipleRecipients({

if (!sizeInfo) throw new InsufficientFundsError();

const fee = Math.ceil(sizeInfo.txVBytes * feeRate);
const estimatedFee = Math.ceil(sizeInfo.txVBytes * feeRate);

const outputs: {
value: bigint;
Expand All @@ -198,14 +216,14 @@ export function determineUtxosForSpendMultipleRecipients({
address,
})),
// outputs[recipients.length] = the remainder to be returned to a change address
{ value: sum - BigInt(amount) - BigInt(fee) },
{ value: sum - BigInt(amount) - BigInt(estimatedFee) },
];

return {
filteredUtxos,
inputs: neededUtxos,
outputs,
size: sizeInfo.txVBytes,
fee,
estimatedFee,
};
}
12 changes: 8 additions & 4 deletions src/app/common/transactions/bitcoin/use-generate-bitcoin-tx.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ export function useGenerateUnsignedNativeSegwitSingleRecipientTx() {
utxos,
};

const { inputs, outputs, fee } = isSendingMax
const {
inputs,
outputs,
estimatedFee: fee,
} = isSendingMax
? determineUtxosForSpendAll(determineUtxosArgs)
: determineUtxosForSpend(determineUtxosArgs);

Expand Down Expand Up @@ -127,11 +131,11 @@ export function useGenerateUnsignedNativeSegwitMultipleRecipientsTx() {
utxos,
};

const { inputs, outputs, fee } = isSendingMax
const { inputs, outputs, estimatedFee } = isSendingMax
? determineUtxosForSpendAllMultipleRecipients(determineUtxosArgs)
: determineUtxosForSpendMultipleRecipients(determineUtxosArgs);

logger.info('Coin selection', { inputs, outputs, fee });
logger.info('Coin selection', { inputs, outputs, estimatedFee });

if (!inputs.length) throw new Error('No inputs to sign');
if (!outputs.length) throw new Error('No outputs to sign');
Expand Down Expand Up @@ -166,7 +170,7 @@ export function useGenerateUnsignedNativeSegwitMultipleRecipientsTx() {
tx.addOutputAddress(output.address, BigInt(output.value), networkMode);
});

return { hex: tx.hex, fee, psbt: tx.toPSBT(), inputs };
return { hex: tx.hex, fee: estimatedFee, psbt: tx.toPSBT(), inputs };
} catch (e) {
// eslint-disable-next-line no-console
console.log('Error signing bitcoin transaction', e);
Expand Down
18 changes: 9 additions & 9 deletions src/app/common/transactions/bitcoin/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ export function getSpendableAmount({
}) {
const balance = utxos.map(utxo => utxo.value).reduce((prevVal, curVal) => prevVal + curVal, 0);

const size = getSizeInfo({
inputLength: utxos.length,
outputLength: 1,
const size = getBitcoinTxSizeEstimation({
inputCount: utxos.length,
outputCount: 1,
recipient: address,
});
const fee = Math.ceil(size.txVBytes * feeRate);
Expand Down Expand Up @@ -80,22 +80,22 @@ export function filterUneconomicalUtxos({
return filteredUtxos;
}

export function getSizeInfo(payload: {
inputLength: number;
outputLength: number;
export function getBitcoinTxSizeEstimation(payload: {
inputCount: number;
outputCount: number;
recipient: string;
}) {
const { inputLength, recipient, outputLength } = payload;
const { inputCount, recipient, outputCount } = payload;
const addressInfo = validate(recipient) ? getAddressInfo(recipient) : null;
const outputAddressTypeWithFallback = addressInfo ? addressInfo.type : 'p2wpkh';

const txSizer = new BtcSizeFeeEstimator();
const sizeInfo = txSizer.calcTxSize({
// Only p2wpkh is supported by the wallet
input_script: 'p2wpkh',
input_count: inputLength,
input_count: inputCount,
// From the address of the recipient, we infer the output type
[outputAddressTypeWithFallback + '_output_count']: outputLength,
[outputAddressTypeWithFallback + '_output_count']: outputCount,
});

return sizeInfo;
Expand Down
Loading

0 comments on commit e5a6206

Please sign in to comment.