Merge PR #778 from 'pinheadmz/maxfee'

Author: pinheadmz

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;

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.

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).

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();
 
@@ -1186,8 +1192,6 @@ class Wallet extends EventEmitter {
       maxFee: options.maxFee,
       estimate: prev => this.estimateSize(prev)
     });
-
-    assert(mtx.getFee() <= MTX.Selector.MAX_FEE, 'TX exceeds MAX_FEE.');
   }
 
   /**
@@ -3960,6 +3964,21 @@ 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 absurdFee = minFee * this.absurdFactor;
+
+    const fee = mtx.getFee();
+
+    if (fee < minFee)
+      throw new Error('Fee is below minimum relay limit.');
+
+    if (fee > absurdFee)
+      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.');
@@ -4018,10 +4037,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.');

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');
@@ -63,7 +64,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 +74,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 +466,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 * policy.ABSURD_FEE_FACTOR;
+
+    // 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();

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)

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 () => {