Fix CPFP when tx fee is less than MIN_RELAY #694
Conversation
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
added
mempool
Pull Request Test Coverage Report for Build 1909571704
💛 - Coveralls |
| 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); | ||
| }); |
There was a problem hiding this comment.
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?
| // Fee should be enough for both the first transation and second transaction | ||
| assert(fee > 140 + 108); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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 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 Same applies to "orphan" TXs. We may get parent / child TXs in reverse order -- what happens then? |
|
I think at this point, we should just make a separate test for and call it |
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.