From 0b71f5f8fa2ff573b9c69e7a9b599894317ec99a Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sun, 13 Nov 2022 10:13:18 -0500 Subject: [PATCH 1/5] mtx: remove CoinSelector.MAX_FEE --- lib/primitives/mtx.js | 20 ++---- lib/wallet/wallet.js | 7 +- test/util/memwallet.js | 2 - test/wallet-coinselection-test.js | 106 ++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 24 deletions(-) create mode 100644 test/wallet-coinselection-test.js diff --git a/lib/primitives/mtx.js b/lib/primitives/mtx.js index bc42b11fb..ced667492 100644 --- a/lib/primitives/mtx.js +++ b/lib/primitives/mtx.js @@ -1662,13 +1662,10 @@ class CoinSelector { // This is mostly here for testing. // i.e. A fee rounded to the nearest // kb is easier to predict ahead of time. - if (this.round) { - const fee = policy.getRoundFee(size, this.rate); - return Math.min(fee, CoinSelector.MAX_FEE); - } + if (this.round) + return policy.getRoundFee(size, this.rate); - const fee = policy.getMinFee(size, this.rate); - return Math.min(fee, CoinSelector.MAX_FEE); + return policy.getMinFee(size, this.rate); } /** @@ -1803,7 +1800,7 @@ class CoinSelector { */ selectHard() { - this.fee = Math.min(this.hardFee, CoinSelector.MAX_FEE); + this.fee = this.hardFee; this.fund(); } } @@ -1826,15 +1823,6 @@ CoinSelector.FEE_RATE = 10000; CoinSelector.MIN_FEE = 10000; -/** - * Maximum fee to allow - * after coin selection. - * @const {Amount} - * @default - */ - -CoinSelector.MAX_FEE = consensus.COIN / 10; - /** * Funding Error * An error thrown from the coin selector. diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 7df1a7f64..41fe6315c 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1186,8 +1186,6 @@ class Wallet extends EventEmitter { maxFee: options.maxFee, estimate: prev => this.estimateSize(prev) }); - - assert(mtx.getFee() <= MTX.Selector.MAX_FEE, 'TX exceeds MAX_FEE.'); } /** @@ -4018,10 +4016,7 @@ class Wallet extends EventEmitter { const oldFee = tx.getFee(view); - let fee = tx.getMinFee(null, rate); - - if (fee > MTX.Selector.MAX_FEE) - fee = MTX.Selector.MAX_FEE; + const fee = tx.getMinFee(null, rate); if (oldFee >= fee) throw new Error('Fee is not increasing.'); diff --git a/test/util/memwallet.js b/test/util/memwallet.js index 258ee24b2..82c15d647 100644 --- a/test/util/memwallet.js +++ b/test/util/memwallet.js @@ -1669,8 +1669,6 @@ class MemWallet { async _create(mtx, options) { await this.fund(mtx, options); - assert(mtx.getFee() <= MTX.Selector.MAX_FEE, 'TX exceeds MAX_FEE.'); - mtx.sortMembers(); if (options && options.locktime != null) diff --git a/test/wallet-coinselection-test.js b/test/wallet-coinselection-test.js new file mode 100644 index 000000000..b823f2be8 --- /dev/null +++ b/test/wallet-coinselection-test.js @@ -0,0 +1,106 @@ +'use strict'; + +const assert = require('bsert'); +const { + MTX, + Network, + WalletDB +} = require('..'); + +// Use main instead of regtest because (deprecated) +// CoinSelector.MAX_FEE was network agnostic +const network = Network.get('main'); + +async function fundWallet(wallet, amounts) { + assert(Array.isArray(amounts)); + + const mtx = new MTX(); + const addr = await wallet.receiveAddress(); + for (const amt of amounts) { + mtx.addOutput(addr, amt); + } + + const height = wallet.wdb.height + 1; + const hash = Buffer.alloc(32); + hash.writeUInt16BE(height); + const dummyBlock = { + hash, + height, + time: Date.now() + }; + await wallet.wdb.addBlock(dummyBlock, [mtx.toTX()]); +} + +describe('Wallet Coin Selection', function () { + describe('Fees', function () { + const wdb = new WalletDB({network}); + let wallet; + + before(async () => { + await wdb.open(); + wdb.height = network.txStart + 1; + wdb.state.height = wdb.height; + wallet = wdb.primary; + }); + + after(async () => { + await wdb.close(); + }); + + it('should fund wallet', async () => { + await fundWallet(wallet, [100e6, 10e6, 1e6, 100000, 10000]); + const bal = await wallet.getBalance(); + assert.strictEqual(bal.confirmed, 111110000); + }); + + it('should pay default fee rate for small tx', async () => { + const address = await wallet.receiveAddress(); + const mtx = new MTX(); + mtx.addOutput(address, 5e6); + await wallet.fund(mtx); + await wallet.sign(mtx); + + assert.strictEqual(mtx.inputs.length, 1); + assert.strictEqual(mtx.outputs.length, 2); + + const rate = mtx.getRate(); + const fee = mtx.getFee(); + + assert.strictEqual(rate, network.feeRate); + assert(rate < network.maxFeeRate); + assert(fee > network.minRelay); + }); + + it('should pay default fee rate for maximum policy weight TX', async () => { + const address = await wallet.receiveAddress(); + const mtx = new MTX(); + for (let i = 0; i < 3120; i++) { + mtx.addOutput(address, 500); + } + // Add nulldata output to add precise amount of extra weight + mtx.addOutput( + { + version: 31, + hash: Buffer.alloc(38) + }, + 0 + ); + await wallet.fund(mtx); + await wallet.sign(mtx); + + // This is as close as we can get to + // policy.MAX_TX_WEIGHT (400000) using standard wallet + assert.strictEqual(mtx.getWeight(), 399997); + assert.strictEqual(mtx.inputs.length, 1); + + const rate = mtx.getRate(); + const fee = mtx.getFee(); + + assert.strictEqual(fee, 10e6); // 10 HNS + + assert.strictEqual(rate, network.feeRate); + assert(rate < network.maxFeeRate); + assert(fee > network.minRelay); + }); + }); +}); From efe34676a2fa649a32ec66bd725c12876c84f4a6 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sun, 13 Nov 2022 12:30:29 -0500 Subject: [PATCH 2/5] test: cover mempool fee too high and too low --- test/mempool-test.js | 83 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 2 deletions(-) diff --git a/test/mempool-test.js b/test/mempool-test.js index 419022119..2877ad2fa 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -63,7 +63,7 @@ const wallet = new MemWallet({ network }); let cachedTX = null; -function dummyInput(addr, hash) { +function dummyInput(addr, hash, value = 70000) { const coin = new Coin(); coin.height = 0; coin.value = 0; @@ -73,7 +73,7 @@ function dummyInput(addr, hash) { const fund = new MTX(); fund.addCoin(coin); - fund.addOutput(addr, 70000); + fund.addOutput(addr, value); const [tx, view] = fund.commit(); @@ -465,6 +465,85 @@ describe('Mempool', function() { } }); + it('should reject absurd fee', async () => { + const wallet = new MemWallet({ network }); + const addr = wallet.getAddress(); + const funds = 10000e6; + + const mtx = new MTX(); + mtx.addCoin( + dummyInput( + addr, + random.randomBytes(32), + funds + ) + ); + mtx.addOutput(wallet.getAddress(), 0); // temp + wallet.sign(mtx); + + const vsize = mtx.getVirtualSize(); + const minFee = (vsize / 1000) * network.minRelay; + const absurdFee = minFee * 10000; + + // Revise with exactly absurd fee + mtx.outputs[0].value = funds - absurdFee - 1; + mtx.inputs[0].witness.items.length = 0; + wallet.sign(mtx); + const tx1 = mtx.toTX(); + + await assert.rejects( + mempool.addTX(tx1), + {message: /absurdly-high-fee/} + ); + + // Revise again with just under absurd fee + mtx.outputs[0].value = funds - absurdFee; + mtx.inputs[0].witness.items.length = 0; + wallet.sign(mtx); + const tx2 = mtx.toTX(); + + await mempool.addTX(tx2); + }); + + it('should reject too-low fee', async () => { + const wallet = new MemWallet({ network }); + const addr = wallet.getAddress(); + const funds = 10000e6; + + const mtx = new MTX(); + mtx.addCoin( + dummyInput( + addr, + random.randomBytes(32), + funds + ) + ); + mtx.addOutput(wallet.getAddress(), 0); // temp + wallet.sign(mtx); + + const vsize = mtx.getVirtualSize(); + const minFee = (vsize / 1000) * network.minRelay; + + // Revise with just under minFee + mtx.outputs[0].value = funds - minFee + 1; + mtx.inputs[0].witness.items.length = 0; + wallet.sign(mtx); + const tx1 = mtx.toTX(); + + await assert.rejects( + mempool.addTX(tx1), + {message: /insufficient priority/} + ); + + // Revise again with exactly minFee + mtx.outputs[0].value = funds - minFee; + mtx.inputs[0].witness.items.length = 0; + wallet.sign(mtx); + const tx2 = mtx.toTX(); + + await mempool.addTX(tx2); + }); + it('should destroy mempool', async () => { await mempool.close(); await chain.close(); From 362d05de23f636aa3b06ddeb9ec99e1a439988ad Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sun, 13 Nov 2022 17:21:17 -0500 Subject: [PATCH 3/5] test: do not pass entire MTX to http wallet client argument The reason is that the MTX object will be converted to a generic JSON object using MTX.getJSON() and then stringified. This process is implemented in brq.RequestOptions.fromOptions(). The MTX object in these tests had properties such as `fee: 0` and `rate: 0` which get interpreted by the wallet node as options in mtx.fund(), resulting in unexpected funding by the wallet. --- test/wallet-http-test.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/wallet-http-test.js b/test/wallet-http-test.js index 1b571d313..0aafc14a1 100644 --- a/test/wallet-http-test.js +++ b/test/wallet-http-test.js @@ -251,10 +251,7 @@ describe('Wallet HTTP', function() { const output = openOutput(name, address); - const mtx = new MTX(); - mtx.outputs.push(output); - - const tx = await wallet.createTX(mtx); + const tx = await wallet.createTX({outputs: [output]}); assert.equal(tx.outputs[0].covenant.type, types.OPEN); }); @@ -263,10 +260,7 @@ describe('Wallet HTTP', function() { const output = openOutput(name, address); - const mtx = new MTX(); - mtx.outputs.push(output); - - const tx = await wallet.send(mtx); + const tx = await wallet.send({outputs: [output]});; assert.equal(tx.outputs[0].covenant.type, types.OPEN); }); From 48d93c452020562807b550119f96425b2eafa19c Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sun, 13 Nov 2022 17:56:31 -0500 Subject: [PATCH 4/5] wallet: check MTX for high and low fee errors along with policy limits --- lib/wallet/wallet.js | 10 ++++++ test/wallet-auction-test.js | 4 +-- test/wallet-coinselection-test.js | 54 +++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index 41fe6315c..c39d186c9 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -3958,6 +3958,16 @@ class Wallet extends EventEmitter { if (tx.getWeight() > policy.MAX_TX_WEIGHT) throw new Error('TX exceeds policy weight.'); + const minFee = policy.getMinFee( + mtx.getVirtualSize(), + this.network.minRelay + ); + const fee = mtx.getFee(); + if (fee < minFee) + throw new Error('Fee is below minimum relay limit.'); + if (fee > minFee * 10000) + throw new Error('Fee exceeds absurd limit.'); + const ancestors = await this.getPendingAncestors(tx); if (ancestors.size + 1 > this.maxAncestors) throw new Error('TX exceeds maximum unconfirmed ancestors.'); diff --git a/test/wallet-auction-test.js b/test/wallet-auction-test.js index 30a60676b..78d4b9eed 100644 --- a/test/wallet-auction-test.js +++ b/test/wallet-auction-test.js @@ -587,7 +587,7 @@ describe('Wallet Auction', function() { actions.push(['NONE', addr, 10000]); } - const batch = await wallet.sendBatch(actions, {hardFee: 10}); + const batch = await wallet.sendBatch(actions, {hardFee: 1000}); assert.strictEqual(batch.outputs.length, 11); @@ -597,7 +597,7 @@ describe('Wallet Auction', function() { const newBal = await wallet.getBalance(); - assert.strictEqual(oldBal.confirmed - newBal.confirmed, 100010); + assert.strictEqual(oldBal.confirmed - newBal.confirmed, 101000); }); it('should verify expected name properties', async () => { diff --git a/test/wallet-coinselection-test.js b/test/wallet-coinselection-test.js index b823f2be8..63ac188d9 100644 --- a/test/wallet-coinselection-test.js +++ b/test/wallet-coinselection-test.js @@ -102,5 +102,59 @@ describe('Wallet Coin Selection', function () { assert(rate < network.maxFeeRate); assert(fee > network.minRelay); }); + + it('should fail to pay absurd fee rate for small tx', async () => { + const address = await wallet.receiveAddress(); + await assert.rejects( + wallet.send({ + outputs: [{ + address, + value: 5e6 + }], + rate: 10001 * network.minRelay + }), + {message: 'Fee exceeds absurd limit.'} + ); + }); + + it('should pay fee just under the absurd limit', async () => { + const address = await wallet.receiveAddress(); + const tx = await wallet.send({ + outputs: [{ + address, + value: 5e6 + }], + rate: 10000 * network.minRelay + }); + const view = await wallet.getWalletCoinView(tx); + assert.strictEqual(tx.getRate(view), 10000 * network.minRelay); + }); + + it('should fail to pay too-low fee rate for small tx', async () => { + const address = await wallet.receiveAddress(); + await assert.rejects( + wallet.send({ + outputs: [{ + address, + value: 5e6 + }], + rate: network.minRelay - 1 + }), + {message: 'Fee is below minimum relay limit.'} + ); + }); + + it('should pay fee at the minimum relay limit', async () => { + const address = await wallet.receiveAddress(); + const tx = await wallet.send({ + outputs: [{ + address, + value: 5e6 + }], + rate: network.minRelay + }); + const view = await wallet.getWalletCoinView(tx); + assert.strictEqual(tx.getRate(view), network.minRelay); + }); }); }); From 5489e31d9cba5f70507b2266905e3e7741d1f05d Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 29 Nov 2022 09:41:09 -0500 Subject: [PATCH 5/5] policy: make absurd fee rate (factor) configurable --- lib/mempool/mempool.js | 11 ++++++++++- lib/protocol/policy.js | 8 ++++++++ lib/wallet/wallet.js | 13 ++++++++++++- test/mempool-test.js | 3 ++- test/wallet-coinselection-test.js | 10 +++++++--- 5 files changed, 39 insertions(+), 6 deletions(-) diff --git a/lib/mempool/mempool.js b/lib/mempool/mempool.js index e6eea0f8f..587b35701 100644 --- a/lib/mempool/mempool.js +++ b/lib/mempool/mempool.js @@ -1610,6 +1610,9 @@ class Mempool extends EventEmitter { // Make sure this guy gave a decent fee. const minFee = policy.getMinFee(entry.size, this.options.minRelay); + // Make sure this guy didn't overpay + const absurdFee = minFee * this.options.absurdFactor; + if (this.options.relayPriority && entry.fee < minFee) { if (!entry.isFree(height)) { throw new VerifyError(tx, @@ -1641,7 +1644,7 @@ class Mempool extends EventEmitter { } // Important safety feature. - if (this.options.rejectAbsurdFees && entry.fee > minFee * 10000) + if (this.options.rejectAbsurdFees && entry.fee > absurdFee) throw new VerifyError(tx, 'highfee', 'absurdly-high-fee', 0); // Why do we have this here? Nested transactions are cool. @@ -2725,6 +2728,7 @@ class MempoolOptions { this.maxAncestors = policy.MEMPOOL_MAX_ANCESTORS; this.expiryTime = policy.MEMPOOL_EXPIRY_TIME; this.minRelay = this.network.minRelay; + this.absurdFactor = policy.ABSURD_FEE_FACTOR; this.prefix = null; this.location = null; @@ -2828,6 +2832,11 @@ class MempoolOptions { this.minRelay = options.minRelay; } + if (options.absurdFactor != null) { + assert((options.absurdFactor >>> 0) === options.absurdFactor); + this.absurdFactor = options.absurdFactor; + } + if (options.prefix != null) { assert(typeof options.prefix === 'string'); this.prefix = options.prefix; diff --git a/lib/protocol/policy.js b/lib/protocol/policy.js index 088f9e381..60cafd6c4 100644 --- a/lib/protocol/policy.js +++ b/lib/protocol/policy.js @@ -61,6 +61,14 @@ exports.BYTES_PER_SIGOP = 20; exports.MIN_RELAY = 1000; +/** + * Maximum relay fee FACTOR for safety (policy). + * Multiplied by result of policy.getMinFee() + * @const {Number} + */ + +exports.ABSURD_FEE_FACTOR = 10000; + /** * Priority threshold for * free transactions (policy). diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index c39d186c9..e17b52c55 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -82,6 +82,7 @@ class Wallet extends EventEmitter { this.txdb = new TXDB(this.wdb); this.maxAncestors = policy.MEMPOOL_MAX_ANCESTORS; + this.absurdFactor = policy.ABSURD_FEE_FACTOR; if (options) this.fromOptions(options); @@ -155,6 +156,11 @@ class Wallet extends EventEmitter { this.maxAncestors = options.maxAncestors; } + if (options.absurdFactor != null) { + assert((options.absurdFactor >>> 0) === options.absurdFactor); + this.absurdFactor = options.absurdFactor; + } + if (!id) id = this.getID(); @@ -3962,10 +3968,15 @@ class Wallet extends EventEmitter { mtx.getVirtualSize(), this.network.minRelay ); + + const absurdFee = minFee * this.absurdFactor; + const fee = mtx.getFee(); + if (fee < minFee) throw new Error('Fee is below minimum relay limit.'); - if (fee > minFee * 10000) + + if (fee > absurdFee) throw new Error('Fee exceeds absurd limit.'); const ancestors = await this.getPendingAncestors(tx); diff --git a/test/mempool-test.js b/test/mempool-test.js index 2877ad2fa..2904bb1cd 100644 --- a/test/mempool-test.js +++ b/test/mempool-test.js @@ -22,6 +22,7 @@ const Witness = require('../lib/script/witness'); const CoinView = require('../lib/coins/coinview'); const util = require('../lib/utils/util'); const consensus = require('../lib/protocol/consensus'); +const policy = require('../lib/protocol/policy'); const MemWallet = require('./util/memwallet'); const ALL = Script.hashType.ALL; const common = require('../lib/blockchain/common'); @@ -483,7 +484,7 @@ describe('Mempool', function() { const vsize = mtx.getVirtualSize(); const minFee = (vsize / 1000) * network.minRelay; - const absurdFee = minFee * 10000; + const absurdFee = minFee * policy.ABSURD_FEE_FACTOR; // Revise with exactly absurd fee mtx.outputs[0].value = funds - absurdFee - 1; diff --git a/test/wallet-coinselection-test.js b/test/wallet-coinselection-test.js index 63ac188d9..f4cf9e940 100644 --- a/test/wallet-coinselection-test.js +++ b/test/wallet-coinselection-test.js @@ -4,7 +4,8 @@ const assert = require('bsert'); const { MTX, Network, - WalletDB + WalletDB, + policy } = require('..'); // Use main instead of regtest because (deprecated) @@ -111,7 +112,7 @@ describe('Wallet Coin Selection', function () { address, value: 5e6 }], - rate: 10001 * network.minRelay + rate: (policy.ABSURD_FEE_FACTOR + 1) * network.minRelay }), {message: 'Fee exceeds absurd limit.'} ); @@ -127,7 +128,10 @@ describe('Wallet Coin Selection', function () { rate: 10000 * network.minRelay }); const view = await wallet.getWalletCoinView(tx); - assert.strictEqual(tx.getRate(view), 10000 * network.minRelay); + assert.strictEqual( + tx.getRate(view), + policy.ABSURD_FEE_FACTOR * network.minRelay + ); }); it('should fail to pay too-low fee rate for small tx', async () => {