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.

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

Conversation

Anunayj
Copy link
Contributor

@Anunayj Anunayj commented Feb 28, 2022

Mempool inclusion policy does not consider descendant fee when
checking transaction for minimum fee, this would allow you to
incentivize the miner to include your transaction without having
to wait for a block with low weight or your transaction to get
evicted.

Network policy would still disallow relaying transactions < minFeeRate, but this would allow CPFP to be used as a fee bumping mechanism in case a low fee transaction does get into mempool.

Mempool inclusion policy does not consider descendant fee when
checking transaction for minimum fee, this would allow you to
incentivize the miner to include your transaction without having
to wait for a block with low weight or your transaction to get
evicted.
@Anunayj Anunayj added mempool part of the codebase mining part of the codebase tests part of the codebase labels Feb 28, 2022
@coveralls
Copy link

coveralls commented Feb 28, 2022

Pull Request Test Coverage Report for Build 1909571704

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.005%) to 62.736%

Files with Coverage Reduction New Missed Lines %
lib/mempool/mempoolentry.js 1 65.0%
Totals Coverage Status
Change from base Build 1813406438: 0.005%
Covered Lines: 21335
Relevant Lines: 31814

💛 - Coveralls

Comment on lines +213 to +234
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);
});
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?

Comment on lines +239 to +240
// Fee should be enough for both the first transation and second transaction
assert(fee > 140 + 108);
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

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

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.

@pinheadmz
Copy link
Member

I'm still thinking this through... would be nice to see another test or two to cover edge cases, for example:

A CPFP chain of 3, 4, 5 txs long? Maybe even a MEMPOOL_MAX_ANCESTORS or MEMPOOL_MAX_ANCESTORS + 1 length CPFP chain? This would probably be a mempool test, not a miner test. I think this limit is covered in wallet test but not yet in mempool.

What happens if a tx in a CPFP chain is confirmed but the rest is not? What if its the first or even middle TX in a CPFP chain? We should make sure that a TX's descendant is removed from the mempool for any reason that we do indeed re-adjust that TX's descSize and descFee. We might even need to evict a TX if it becomes free.

Same applies to "orphan" TXs. We may get parent / child TXs in reverse order -- what happens then?

@Anunayj
Copy link
Contributor Author

Anunayj commented Mar 11, 2022

I think at this point, we should just make a separate test for and call it fee-bumping.js or something. This itself is getting too long for being in miner-test.js

@pinheadmz pinheadmz added this to the hsd 5.0.0 milestone Oct 6, 2022
@pinheadmz pinheadmz modified the milestones: hsd 5.0.0, hsd 5.0.1 Nov 29, 2022
@nodech nodech removed this from the hsd 5.0.1 milestone Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mempool part of the codebase mining part of the codebase tests part of the codebase waiting for the author modifier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants