Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CPFP when tx fee is less than MIN_RELAY #694

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix CPFP when tx fee is less than MIN_RELAY #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec53743
Fix CPFP when tx fee is less than MIN_RELAY
Anunayj Feb 28, 2022
28b1486
test: Child pays for parent
Anunayj Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/mining/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ class BlockEntry {
item.fee = entry.getFee();
item.rate = entry.getDeltaRate();
item.priority = entry.getPriority(attempt.height);
item.free = entry.getDeltaFee() < policy.getMinFee(entry.size);
item.free = entry.descFee < policy.getMinFee(entry.descSize);
item.sigops = entry.sigops;
item.descRate = entry.getDescRate();
return item;
Expand Down
80 changes: 79 additions & 1 deletion test/miner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const Mempool = require('../lib/mempool/mempool');
const Miner = require('../lib/mining/miner');
const Address = require('../lib/primitives/address');
const MTX = require('../lib/primitives/mtx');
const Coin = require('../lib/primitives/coin');
const MemWallet = require('./util/memwallet');
const {BufferSet} = require('buffer-map');

Expand Down Expand Up @@ -77,6 +78,7 @@ describe('Miner', function() {

let walletAddr;
const txids = new BufferSet();
let coin, parentTX;

it('should generate 20 blocks to wallet address', async () => {
walletAddr = wallet.createReceive().getAddress();
Expand Down Expand Up @@ -110,7 +112,6 @@ describe('Miner', function() {
}

assert.strictEqual(mempool.map.size, 10);

const block = await miner.mineBlock(chain.tip, addr);
await chain.add(block);

Expand Down Expand Up @@ -180,4 +181,81 @@ describe('Miner', function() {
assert(txids.has(block.txs[i].hash()));
}
});

it('should not include free transaction in a block', async () => {
// Miner does not have any room for free TXs
miner.options.minWeight = 0;

const value = 1 * 1e6;
const fee = 0;

// Get a change address
const change = wallet.createChange().getAddress();
const mtx = new MTX();
coin = wallet.getCoins()[0];
mtx.addCoin(coin);
mtx.addOutput(addr, value);
mtx.addOutput(change, coin.value - value - fee); // no fee
wallet.sign(mtx);
parentTX = mtx.toTX();

await mempool.addTX(parentTX, -1);
assert.strictEqual(mempool.map.size, 1);

const block = await miner.mineBlock(chain.tip, addr);
await chain.add(block);

// TX is still in mempool, nothing in block except coinbase
assert.strictEqual(mempool.map.size, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test block should begin either with a mempool.reset() or assertion that mempool is empty. What might be even better is starting a new describe block called CPFP or something. Especially since the "should not include free transaction in a block" test is basically a duplicate. We know we can't include free TXs in blocks from L127-L165.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think a separate describe block is better, since this itself got way too long for just miner test. or maybe a complete different test file for CPFP and in future RBF (and other fee bumping mechanisms) might be better.

assert.strictEqual(block.txs.length, 1);
});

it('should fail to double spend the coin - duplicate tx in mempool', async () => {
const value = 1 * 1e6;
const fee = 1000;
const mtx = new MTX();

const change = wallet.createChange().getAddress();
mtx.addCoin(coin);
mtx.addOutput(addr, value);
mtx.addOutput(change, coin.value - value - fee);
wallet.sign(mtx);
const tx = mtx.toTX();

assert.rejects(
async () => await mempool.addTX(tx, -1),
{
code: 'duplicate',
reason: 'bad-txns-inputs-spent'
}
);
// orignal tx is still in mempool
assert.strictEqual(mempool.map.size, 1);
});
Comment on lines +213 to +234
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks more like a mempool test than a miner test. Do we need to add it? Is this error already covered in the mempool tests?


it('should include child transaction if child pays enough fee (CPFP)', async () => {
const fee = 1000;

// Fee should be enough for both the first transation and second transaction
assert(fee > 140 + 108);
Comment on lines +239 to +240
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get these values from (140 and 108)? It might be more clear to see the values pulled from the MempoolEntry itself is possible. In addition, asserting that the descSize and descFee of the parent TX is being updated when child is added to mempool and that the value is correct.

Copy link
Contributor Author

@Anunayj Anunayj Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea, I'll fix that, sorry mb there, I was supposed to replace that with calculation for fee for both transactions, that was just supposed to be a placeholder


const mtx = new MTX();
const change = wallet.createChange().getAddress();
const coin = Coin.fromTX(parentTX, 1, -1);

mtx.addCoin(coin);
mtx.addOutput(change, coin.value - fee);
wallet.sign(mtx);
const tx = mtx.toTX();
await mempool.addTX(tx, -1);
// Both transactions in mempool
assert.strictEqual(mempool.map.size, 2);

const block = await miner.mineBlock(chain.tip, addr);
await chain.add(block);

// Both transactions should get mined into the block
assert.strictEqual(mempool.map.size, 0);
assert.strictEqual(block.txs.length, 3);
});
});