Skip to content

Commit

Permalink
mempool: fix RBF Rule #2
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@pinheadmz
pinheadmz committed Aug 20, 2023
1 parent 5e0cd19 commit 6f04c0d
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 44 deletions.
59 changes: 33 additions & 26 deletions lib/mempool/mempool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -978,7 +978,7 @@ class Mempool extends EventEmitter {
conflicts = this.getConflicts(tx);

if (conflicts.length) {
this.verifyRBF(entry, conflicts);
this.verifyRBF(entry, view, conflicts);
}

// Contextual sanity checks.
Expand Down Expand Up @@ -1038,11 +1038,12 @@ class Mempool extends EventEmitter {
* mempool-replacements.md
* @method
* @param {MempoolEntry} entry
* @param {CoinView} view
* @param {MempoolEntry[]} conflicts
* @returns {Boolean}
*/

verifyRBF(entry, conflicts) {
verifyRBF(entry, view, conflicts) {
const tx = entry.tx;

let conflictingFees = 0;
Expand All @@ -1058,6 +1059,36 @@ class Mempool extends EventEmitter {
0);
}

// Rule #2
// 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.
// This also prevents the case of replacing our own inputs.
PREVOUTS: for (const {prevout} of tx.inputs) {
// Confirmed inputs are not relevant to Rule #2
if (view.getHeight(prevout) > 0) {
continue;
}

// Any unconfirmed inputs spent by the replacement
// 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) {
// Once we find a match we don't need to check any more
// of the conflict's inputs. Continue by testing the
// next input in the potential replacement tx.
continue PREVOUTS;
}
}

// Rule #2 violation
throw new VerifyError(tx,
'nonstandard',
'replacement-adds-unconfirmed',
0);
}

conflictingFees += conflict.descFee;
totalEvictions += this.countDescendants(conflict) + 1;

Expand Down Expand Up @@ -1097,30 +1128,6 @@ class Mempool extends EventEmitter {
'insufficient fee: must pay for fees including conflicts',
0);
}

// Rule #2
// 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.
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.
// Rule #2 does not apply.
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 rule #2.
throw new VerifyError(tx,
'nonstandard',
'replacement-adds-unconfirmed',
0);
}
}

/**
Expand Down
110 changes: 92 additions & 18 deletions test/mempool-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1362,38 +1362,42 @@ describe('Mempool', function() {
});

it('should reject replacement including new unconfirmed UTXO', async() => {
// {confirmed coin 1} {confirmed coin 2}
// | | |
// | tx 1 tx 2 {output}
// | |
// | +--------------------------------+
// | |
// tx 3 is invalid!

mempool.options.replaceByFee = true;

const coin1 = chaincoins.getCoins()[0];
const coin2 = chaincoins.getCoins()[1];

// tx 1 spends a confirmed coin
const mtx1 = new MTX();
const mtx2 = new MTX();
mtx1.addCoin(coin1);
mtx2.addCoin(coin2);

mtx1.inputs[0].sequence = 0xfffffffd;

const addr1 = chaincoins.createReceive().getAddress();
mtx1.addOutput(addr1, coin1.value - 1000);
chaincoins.sign(mtx1);
assert(mtx1.verify());
const tx1 = mtx1.toTX();
assert(tx1.isRBF());
await mempool.addTX(tx1);

// tx 2 spends a different confirmed coin
const mtx2 = new MTX();
mtx2.addCoin(coin2);
const addr2 = chaincoins.createReceive().getAddress();
mtx2.addOutput(addr2, coin2.value - 1000);

chaincoins.sign(mtx1);
chaincoins.sign(mtx2);

assert(mtx1.verify());
assert(mtx2.verify());
const tx1 = mtx1.toTX();
const tx2 = mtx2.toTX();

assert(tx1.isRBF());

await mempool.addTX(tx1);
await mempool.addTX(tx2);

// Attempt to replace tx1 and include the unconfirmed output of tx2
// Attempt to replace tx 1 and include the unconfirmed output of tx 2
const mtx3 = new MTX();
mtx3.addCoin(coin1);
const coin3 = Coin.fromTX(tx2, 0, -1);
Expand Down Expand Up @@ -1483,12 +1487,20 @@ describe('Mempool', function() {
});

it('should accept replacement spending an unconfirmed output', async () => {
// {confirmed coin 1}
// |
// tx 0 {output}
// | |
// tx 1 |
// |
// tx 2

mempool.options.replaceByFee = true;

const addr1 = chaincoins.createReceive().getAddress();
const coin0 = chaincoins.getCoins()[0];

// Generate parent TX
// Generate parent tx 0
const mtx0 = new MTX();
mtx0.addCoin(coin0);
mtx0.addOutput(addr1, coin0.value - 200);
Expand All @@ -1497,7 +1509,7 @@ describe('Mempool', function() {
const tx0 = mtx0.toTX();
await mempool.addTX(tx0);

// Spend unconfirmed output to replaceable child
// Spend unconfirmed output to replaceable child tx 1
const mtx1 = new MTX();
const coin1 = Coin.fromTX(tx0, 0, -1);
mtx1.addCoin(coin1);
Expand All @@ -1508,7 +1520,7 @@ describe('Mempool', function() {
const tx1 = mtx1.toTX();
await mempool.addTX(tx1);

// Send replacement
// Send replacement tx 2
const mtx2 = new MTX();
mtx2.addCoin(coin1);
mtx2.addOutput(addr1, coin1.value - 400);
Expand All @@ -1517,7 +1529,7 @@ describe('Mempool', function() {
const tx2 = mtx2.toTX();
await mempool.addTX(tx2);

// Unconfirmed parent and replacement in mempool together
// Unconfirmed parent tx 0 and replacement tx 2 are in mempool together
assert(mempool.has(tx0.hash()));
assert(!mempool.has(tx1.hash()));
assert(mempool.has(tx2.hash()));
Expand Down Expand Up @@ -1564,5 +1576,67 @@ describe('Mempool', function() {
reason: 'bad-txns-inputs-spent'
});
});

it('should not accept replacement that evicts its own inputs', async () => {
// {confirmed coin 1}
// |
// tx 0 {output}
// | |
// | tx 1 {output}
// | |
// | +-------+
// | |
// tx 2 is invalid!

mempool.options.replaceByFee = true;

const addr1 = chaincoins.createReceive().getAddress();
const coin0 = chaincoins.getCoins()[0];

// Generate tx 0 which spends a confirmed coin
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);

// Generate tx 1 which spends an output of tx 0
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);

// Send tx 2 which attempts to:
// - replace tx 1 by spending an output of tx 0
// - ALSO spend an output of tx 1
// This is obviously invalid because if tx 1 is replaced,
// its output no longer exists so it can not be spent by tx 2.
const mtx2 = new MTX();
mtx2.addCoin(coin1);
const coin2 = Coin.fromTX(tx1, 0, -1);
mtx2.addCoin(coin2);
mtx2.addOutput(addr1, coin2.value + coin1.value - 1000);
chaincoins.sign(mtx2);
assert(mtx2.verify());
const tx2 = mtx2.toTX();

await assert.rejects(async () => {
await mempool.addTX(tx2);
}, {
type: 'VerifyError',
reason: 'replacement-adds-unconfirmed'
});

assert(mempool.has(tx0.hash()));
assert(mempool.has(tx1.hash()));
assert(!mempool.has(tx2.hash()));
});
});
});

0 comments on commit 6f04c0d

Please sign in to comment.