BIP125: Enable replace-by-fee in mempool #811
Conversation
pinheadmz
added
advanced
|
I'd personally like to run The relevant codepath in the wallet: |
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
==========================================
+ Coverage 61.67% 61.84% +0.16%
==========================================
Files 155 155
Lines 25971 26008 +37
==========================================
+ Hits 16018 16085 +67
+ Misses 9953 9923 -30
Continue to review full report at Codecov.
|
|
Also want to call attention to this: Lines 966 to 969 in 7b47913 This is NOT a protocol rule. TX version 2 is defined by BIP 112 (OP_CSV) to add new functionality to the I think this line was added so that bcoin (rejecting RBF by default) would NOT reject OP_CSV transactions. Maybe once the wallet is RBF enabled and if we decide to RBF acceptance to |
lib/mempool/mempool.js
Outdated
| * @method | ||
| * @param {TX} tx | ||
| * @param {CoinView} view | ||
| * @returns {Promise} | ||
| * @returns {Promise} - Returns {@link MempoolEntry}[] |
There was a problem hiding this comment.
| * @returns {Promise} - Returns {@link MempoolEntry}[] | |
| * @returns {Promise} - Returns {@link MempoolEntry}[] |
| * @returns {Promise} - Returns {@link MempoolEntry}[] | |
| * @returns {Promise} - Returns {@link MempoolEntry[]} |
lib/mempool/mempool.js
Outdated
| } | ||
| } | ||
|
|
||
| // Replacement TXs must not evict/replace more than 100 descendants |
There was a problem hiding this comment.
I think this function should be inside the for loop at line 986 to avoid unnecessary check of the remaining tx altogether.
lib/mempool/mempool.js
Outdated
| * Get an array of all transactions currently in the mempool that | ||
| * spend one or more of the same outputs as an incoming transaction. | ||
| * @param {TX} tx | ||
| * @returns {Promise} - Returns (@link MempoolEntry}[]. |
There was a problem hiding this comment.
| * @returns {Promise} - Returns (@link MempoolEntry}[]. | |
| * @returns {Promise} - Returns (@link MempoolEntry}[]. |
| * @returns {Promise} - Returns (@link MempoolEntry}[]. | |
| * @returns {Promise} - Returns (@link MempoolEntry[]}. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #811 +/- ##
==========================================
+ Coverage 70.41% 70.56% +0.14%
==========================================
Files 174 174
Lines 27515 27566 +51
==========================================
+ Hits 19376 19451 +75
+ Misses 8139 8115 -24
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Finished a first-pass code review. Nice work.
Left some comments about adding descriptive logs in addition to throwing errors.
Apart from the comments, I have 2 suggestions:
- Should we preserve the order of checks like Core? I don't have a strong opinion on this. As long as we catch all the cases, any order should be fine but it's definitely something worth looking into.
- I think it would be a good idea to move all RBF checks to a different function like Core's
MemPoolAccept::ReplacementChecks
lib/mempool/mempool.js
Outdated
| getConflicts(tx) { | ||
| const conflicts = []; | ||
| const hashes = new BufferSet(); | ||
|
|
||
| for (const {prevout} of tx.inputs) { | ||
| const {hash, index} = prevout; | ||
| const conflict = this.getSpent(hash, index); | ||
|
|
||
| if (conflict && !hashes.has(hash)) { | ||
| hashes.add(hash); | ||
| conflicts.push(conflict); | ||
| } | ||
| } | ||
|
|
||
| return conflicts; | ||
| } |
There was a problem hiding this comment.
why not just do this?
| getConflicts(tx) { | |
| const conflicts = []; | |
| const hashes = new BufferSet(); | |
| for (const {prevout} of tx.inputs) { | |
| const {hash, index} = prevout; | |
| const conflict = this.getSpent(hash, index); | |
| if (conflict && !hashes.has(hash)) { | |
| hashes.add(hash); | |
| conflicts.push(conflict); | |
| } | |
| } | |
| return conflicts; | |
| } | |
| function getConflicts(tx) { | |
| const conflicts = new Set(); | |
| for (const { prevout: { hash, index } } of tx.inputs) { | |
| const conflict = this.getSpent(hash, index); | |
| if (conflict) { | |
| conflicts.add(conflict); | |
| } | |
| } | |
| return Array.from(conflicts); | |
| } |
lib/mempool/mempool.js
Outdated
| if (totalEvictions > 100) { | ||
| throw new VerifyError(tx, | ||
| 'nonstandard', | ||
| 'too many potential replacements', |
There was a problem hiding this comment.
We should add logs here like core
return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
txid.ToString(),
nConflictingCount,
MAX_REPLACEMENT_CANDIDATES);
lib/mempool/mempool.js
Outdated
| throw new VerifyError(tx, | ||
| 'insufficientfee', | ||
| 'insufficient fee', | ||
| 0); |
There was a problem hiding this comment.
adding comments to make life easier
return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
txid.ToString(),
replacement_feerate.ToString(),
original_feerate.ToString());
lib/mempool/mempool.js
Outdated
| throw new VerifyError(tx, | ||
| 'insufficientfee', | ||
| 'insufficient fee', | ||
| 0); |
There was a problem hiding this comment.
return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
lib/mempool/mempool.js
Outdated
| throw new VerifyError(tx, | ||
| 'insufficientfee', | ||
| 'insufficient fee', | ||
| 0); |
There was a problem hiding this comment.
return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
txid.ToString(),
FormatMoney(additional_fees),
FormatMoney(relay_fee.GetFee(replacement_vsize)));
lib/mempool/mempool.js
Outdated
| throw new VerifyError(tx, | ||
| 'nonstandard', | ||
| 'replacement-adds-unconfirmed', | ||
| 0); |
There was a problem hiding this comment.
return strprintf("replacement %s adds unconfirmed input, idx %d",
tx.GetHash().ToString(), j);
lib/mempool/mempool.js
Outdated
| totalEvictions += this.countDescendants(conflict) + 1; | ||
|
|
||
| // Replacement TXs must not evict/replace more than 100 descendants | ||
| if (totalEvictions > 100) { |
There was a problem hiding this comment.
Maybe move 100 into policy and use const from there for the 100.
lib/mempool/mempool.js
Outdated
| // Replacement TX must also pay for the total fees of all descendant | ||
| // transactions that will be evicted if an ancestor is replaced. | ||
| // Thus the replacement "pays for the bandwidth" of all the conflicts. | ||
| if (entry.deltaFee <= conflictingFees) { |
There was a problem hiding this comment.
This check is not strictly necessary as the next check covers this as well. If deltaFee <= conflictingFees the feeRemaining will be negative.
lib/mempool/mempool.js
Outdated
| 0); | ||
| } | ||
|
|
||
| // Replacement TXs can not consume any unconfirmed outputs that were not |
There was a problem hiding this comment.
Not request to change, but I used it to better see what was happening, so basically asking if I understood what's up here:
// Replacement TXs can not consume any unconfirmed outputs that were not
// already included in the original transactions. They can only add
// confirmed UTXO, saving the trouble of checking new mempool ancestors.
// Rule #2 checks
for (const {prevout} of tx.inputs) {
// We don't have the transaction in the mempool and it is not an
// orphan, it means we are spending from the chain, no issues with
// Rules #2.
if (!this.map.has(prevout.hash))
continue;
// If the prevout is in the mempool and it was already being spent
// by other transactions (which we are in conflict with) - we
// are not violating rule #2.
if (this.isSpent(prevout.hash, prevout.index))
continue;
// TX is spending new unconfirmed coin, which violates #2.
throw new VerifyError(tx,
'nonstandard',
'replacement-adds-unconfirmed',
0);
}
"Quick and dirty" isDoubleSpend() check evaluates to false if the conflicting tx has opted in to RBF. More verbose checks are still required for BIP125. General RBF tests added.
Implements and tests BIP125 rules specifically pertaining to fee requirements for replacement transactions. Valid replacements must pay a higher fee rate than the original TX, and also must pay for the bandwidth of all potentially evicted transactions in addition to its own bandwidth.
Implements two extra rules defined in BIP125: - The replacement transaction may only include an unconfirmed input if that input was included in one of the original transactions. - The number of original transactions to be replaced and their descendant transactions which will be evicted from the mempool must not exceed a total of 100 transactions.
All the BIP125 rules have been implemented to reject invalid replacement TXs. This is where the actual replacement happens, adding the replacement TX and evicting all the conflicts and their descendants.
|
@IMPranshu @nodech @theanmolsharma thanks so much for your reviews. I addressed all the comments and rebased on master in latest push. Also added a new commit that covers an edge case I hadn't thought of before: if we replace a tx that was spending an unconfirmed mempool output, the mempool would not "find" that coin and not only consider the replacement an orphan but also not even save it as an orphan. |
There was a problem hiding this comment.
After another review and with the recent fixes to the rbf getting weirldy orphaned (but not really orphaned) - I noticed that hasCoin in the CoinView construction enables transactions that are not valid and currently can't be caught.
First let's start with the test case description:
TX_A -> 2 outs
TX_B -> spends TX_A.out[0]
TX_C -> spends TX_B.out[0]
TX_D -> spends TX_A.out[0]
-> spends TX_B.out[0]
This is the example of the transaction (TX_D) that becomes valid and avoids CoinView/isDoubleSpend checks but is invalid. Test case will be provided below.
The situation occurs because TX_D does not really contradict any of the RBF rules itself:
- It passes isDoubleSpend/hasCoin checks because of the RBF checks.
- It does not spend form NEW unconfirmed things, TX_B.out[0] and TX_A.out[0] are both spent already so this rule is good.
Fees and other stuff is properly managed.
But because the coins in the CoinView are not retroactively removed, when TX_A, TX_B, TX_C are prepared for removal, TX_D verification passes. After that stuff breaks in following code when we are trying to evict entries (but that's beside the point here)
So ideally, when we are processing RBFs, we want to retroactively remove the coins and transactions from the CoinView and do proper verification checks that Coins still exist for the new Transaction after "prepared to get removed" transactions have been subtracted from the coinview. (Ideally we should have similar abstraction as bitcoin where they have processing state with all necessary info that can be reverted easily, CoinView is the only thing that does that, mempool txs/spents etc are not easily modifiable on the fly like coinview is)
Test case
// Recursive bad:
// A -> 2 outs
// B -> spends A.out[0]
// C -> spends B.out[0]
// D -> spends A.out[0]
// -> spends B.out[0]
it('should not accept for recursive spends', async () => {
mempool.options.replaceByFee = true;
const addr1 = chaincoins.createReceive().getAddress();
const coin0 = chaincoins.getCoins()[0];
// Generate parent TX
const mtx0 = new MTX();
mtx0.addCoin(coin0);
mtx0.addOutput(addr1, coin0.value - 200);
chaincoins.sign(mtx0);
assert(mtx0.verify());
const tx0 = mtx0.toTX();
await mempool.addTX(tx0);
const mtx1 = new MTX();
const coin1 = Coin.fromTX(tx0, 0, -1);
mtx1.addCoin(coin1);
mtx1.inputs[0].sequence = 0xfffffffd;
mtx1.addOutput(addr1, coin1.value - 200);
chaincoins.sign(mtx1);
assert(mtx1.verify());
const tx1 = mtx1.toTX();
await mempool.addTX(tx1);
const mtx2 = new MTX();
const coin2 = Coin.fromTX(tx1, 0, -1);
mtx2.addCoin(coin2);
mtx2.inputs[0].sequence = 0xfffffffd;
mtx2.addOutput(addr1, coin2.value - 200);
chaincoins.sign(mtx2);
assert(mtx2.verify());
const tx2 = mtx2.toTX();
await mempool.addTX(tx2);
// Send attempted replacement
const mtx3 = new MTX();
mtx3.addCoin(coin1);
mtx3.addCoin(coin2);
mtx3.addOutput(addr1, coin2.value + coin1.value - 1000);
chaincoins.sign(mtx3);
assert(mtx3.verify());
const tx3 = mtx3.toTX();
let err;
try {
await mempool.addTX(tx3);
} catch (e) {
err = e;
}
assert(err);
console.log(err);
// await assert.rejects(async () => {
// }, {
// type: 'VerifyError',
// reason: 'bad-txns-inputs-spent'
// });
});This currently hits assertion error in "unrelated" place:
AssertionError [ERR_ASSERTION]: Assertion failed.
at Mempool.untrackEntry (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:1963:5)
at Mempool.removeEntry (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:1219:10)
at Mempool.evictEntry (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:1238:10)
at Mempool.insertTX (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:867:14)
at async Mempool._addTX (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:715:17)
at async Mempool.addTX (/home/nd/dev/bcoin-org/bcoin/lib/mempool/mempool.js:694:14)
at async Context.<anonymous> (/home/nd/dev/bcoin-org/bcoin/test/mempool-test.js:1621:9) {
type: 'AssertionError',
code: 'ERR_ASSERTION',
generatedMessage: true,
actual: false,
expected: true,
operator: '=='
}```
lib/mempool/mempool.js
Outdated
| // If the TX that spent this coin signals | ||
| // replaceability, then even though it is "spent" | ||
| // we still "have" it. | ||
| if (spender) { |
There was a problem hiding this comment.
I think hasCoin becomes a not-ideal name for this if it depends on many things. Also this will ruin compatibility with the getCoin - even though it is not used for anything important.
One solution to this (and others I will mention) could be to pass new flag "rbf" to make it old behavior the default one and only use rbf when necessary and make it clearer. Alternatively, change the name of the method ?
| @@ -1659,13 +1781,35 @@ class Mempool extends EventEmitter { | |||
| isDoubleSpend(tx) { | |||
| for (const {prevout} of tx.inputs) { | |||
| const {hash, index} = prevout; | |||
| if (this.isSpent(hash, index)) | |||
There was a problem hiding this comment.
Similar to above, I think it's good to have pure double spend vs rbf double spent methods separate or behind a flag. This is not critical in this specific method, but just a note I wanted to mention.
We only need to call the "quick and dirty test" if the user has set mempool RBF option to false. Do the Rule #1 check in verifyRBF() instead where it belongs. This leaves one loose end in maybeOrphan() that must be caught so we don't treat double-spends like orphans.
The current implementation improperly assumed that any unconfirmed inputs spent by the replacement TX were also spent by its direct conflict. It therefore did not account for the case where the replacement was spending an unconfirmed CHILD of its direct replacement. This would cause it to replace its own inputs which is an invalid mempool state and led to unexpected errors.
|
Awesome review @nodech and great catch! There was an ugly bug in Rule 2. I also addressed your two other comments by restoring the existing implementations of mempool: fix RBF Rule 2mempool: restore original isDoubleSpend() implementationmempool: allow replacement of tx spending unconfirmed utxoThis commit was refactored so the "is spender replaceable" logic is now in Tests were updated with ASCII art as well ;-) |
lib/mempool/mempool.js
Outdated
| // must also be spent by the conflict. | ||
| for (const {prevout: conflictPrevout} of conflict.tx.inputs) { | ||
| if (conflictPrevout.hash.equals(prevout.hash) | ||
| && conflictPrevout.input === prevout.input) { |
There was a problem hiding this comment.
Should this be .index? .input does not exist on Outpoint.
There was a problem hiding this comment.
🤦 🤦 🤦 🤦 🤦 🤦 🤦 🤦 🤦
Fixed this in a new commit and added a test that fails without the patch
This is an initial implementation of mempool replace-by-fee as described in BIP125:
This implementation is based on Bitcoin Core PR bitcoin/bitcoin#6871 with one subtle difference: Bitcoin Core has an extra rule that rejects replacement transactions that spend an output they would be replacing. That also applies to outputs of descendant transactions that would be evicted from the mempool if their parent was replaced. Obviously this is an invalid transaction because its acceptance in to the mempool would cause its own input to stop existing. However, rule # 2 above already catches this edge case. The only reason it is explicit in Bitcoin Core is because rule # 2 could be bypassed by policy configuration whereas the extra rule applies to a subset of absolutely invalid transactions. It's a bit of a tricky check because ancestors and conflicts all need to be cross-compared, and I don't think we need it.
Options like this are apparently available in Bitcoin Knots.
We should also discuss whether to eliminate the
replace-by-feeoption altogether. It is not a config option in Bitcoin Core. Maybe this can be discussed after the bcoin wallet is ready for RBF.--
new TODO: this PR followed bitcoin core's implementation and therefore is currently subject to the same vulnerability described here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-May/018893.html