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

BIP125: Enable replace-by-fee in mempool #811

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
231 changes: 213 additions & 18 deletions lib/mempool/mempool.js
Original file line number Diff line number Diff line change
Expand Up @@ -817,15 +817,19 @@
0);
}

// Quick and dirty test to verify we're
// not double-spending an output in the
// mempool.
if (this.isDoubleSpend(tx)) {
this.emit('conflict', tx);
throw new VerifyError(tx,
'duplicate',
'bad-txns-inputs-spent',
0);
if (!this.options.replaceByFee) {
// Quick and dirty test to verify we're
// not double-spending an output in the
// mempool.
// If we are verifying RBF, we will do a
// slow and clean test later...
if (this.isDoubleSpend(tx)) {
this.emit('conflict', tx);
throw new VerifyError(tx,

Check warning on line 828 in lib/mempool/mempool.js

View check run for this annotation

Codecov / codecov/patch

lib/mempool/mempool.js#L827-L828

Added lines #L827 - L828 were not covered by tests
'duplicate',
'bad-txns-inputs-spent',
0);
}
}

// Get coin viewpoint as it
Expand All @@ -844,7 +848,20 @@
const entry = MempoolEntry.fromTX(tx, view, height);

// Contextual verification.
await this.verify(entry, view);
const conflicts = await this.verify(entry, view);

// RBF only: Remove conflicting TXs and their descendants
if (!this.options.replaceByFee) {
assert(conflicts.length === 0);
} else {
for (const conflict of conflicts) {
this.logger.debug(
'Replacing tx %h with %h',
conflict.tx.hash(),
tx.hash());
this.evictEntry(conflict);
}
}

// Add and index the entry.
await this.addEntry(entry, view);
Expand All @@ -862,10 +879,11 @@

/**
* Verify a transaction with mempool standards.
* Returns an array of conflicting TXs
* @method
* @param {MempoolEntry} entry
* @param {CoinView} view
* @returns {Promise}
* @returns {Promise} - Returns {@link MempoolEntry[]}
*/

async verify(entry, view) {
Expand Down Expand Up @@ -953,6 +971,16 @@
0);
}

// If we reject RBF transactions altogether we can skip these checks,
// because incoming conflicts are already rejected as double spends.
let conflicts = [];
if (this.options.replaceByFee)
conflicts = this.getConflicts(tx);

if (conflicts.length) {
tynes marked this conversation as resolved.
Show resolved Hide resolved
this.verifyRBF(entry, view, conflicts);
}

// Contextual sanity checks.
const [fee, reason, score] = tx.checkInputs(view, height);

Expand Down Expand Up @@ -995,6 +1023,111 @@
assert(await this.verifyResult(tx, view, flags),
'BUG: Verify failed for mandatory but not standard.');
}

return conflicts;
}

/**
* Verify BIP 125 replaceability
* https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki
* Also see documented discrepancy between specification and implementation
* regarding Rule #1:
* https://www.cve.org/CVERecord?id=CVE-2021-31876
* Bitcoin Core policy may also change regularly, see:
* https://github.com/bitcoin/bitcoin/blob/master/doc/policy/
* mempool-replacements.md
* @method
* @param {MempoolEntry} entry
* @param {CoinView} view
* @param {MempoolEntry[]} conflicts
* @returns {Boolean}
*/

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

let conflictingFees = 0;
let totalEvictions = 0;

for (const conflict of conflicts) {
// Rule #1 - as implemented in Bitcoin Core (not BIP 125)
// We do not replace TXs that do not signal opt-in RBF
if (!conflict.tx.isRBF()) {
throw new VerifyError(tx,
'duplicate',
'bad-txns-inputs-spent',
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) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be .index? .input does not exist on Outpoint.

Copy link
Member Author

@pinheadmz pinheadmz Aug 21, 2023

Choose a reason for hiding this comment

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

🤦 🤦 🤦 🤦 🤦 🤦 🤦 🤦 🤦

Fixed this in a new commit and added a test that fails without the patch

// 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;

// Rule #5
// Replacement TXs must not evict/replace more than 100 descendants
if (totalEvictions > policy.MEMPOOL_MAX_REPLACEMENTS) {
throw new VerifyError(tx,
'nonstandard',
'too many potential replacements',
0);
}

// Rule #6 - currently implemented by Bitcoin Core but not in BIP 125
// The replacement transaction must have a higher feerate than its
// direct conflicts.
// We compare using deltaFee to account for prioritiseTransaction()
if (entry.getDeltaRate() <= conflict.getDeltaRate()) {
throw new VerifyError(tx,
'insufficientfee',
'insufficient fee: must not reduce total mempool fee rate',
0);
}
}

// Rule #3 and #4
// Replacement TX must 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.
// Once the conflicts are all paid for, the replacement TX fee
// still needs to cover it's own bandwidth.
// We use our policy min fee, Bitcoin Core has a separate incremental fee
const minFee = policy.getMinFee(entry.size, this.options.minRelay);
const feeRemaining = entry.deltaFee - conflictingFees;
if (feeRemaining < minFee) {
throw new VerifyError(tx,
'insufficientfee',
'insufficient fee: must pay for fees including conflicts',
0);
}
}

/**
Expand Down Expand Up @@ -1425,9 +1558,22 @@
const missing = [];

for (const {prevout} of tx.inputs) {
// We assume the view came from mempool.getCoinView()
// which will only exclude spent coins if the spender
// is not replaceable
if (view.hasEntry(prevout))
continue;

// If this prevout is missing from the view because
// it has a spender in the mempool it's not an orphan,
// it's a double-spend.
if (this.isSpent(prevout.hash, prevout.index)) {
throw new VerifyError(tx,
'duplicate',
'bad-txns-inputs-spent',
0);
}

if (this.hasReject(prevout.hash)) {
this.logger.debug(
'Not storing orphan %h (rejected parents).',
Expand Down Expand Up @@ -1659,13 +1805,35 @@
isDoubleSpend(tx) {
for (const {prevout} of tx.inputs) {
const {hash, index} = prevout;
if (this.isSpent(hash, index))
Copy link
Member

Choose a reason for hiding this comment

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

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.

const conflict = this.getSpent(hash, index);

if (conflict)
return true;
}

return false;
}

/**
* 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[]}.
*/

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);
}

/**
* Get coin viewpoint (lock).
* Note: this does not return
Expand Down Expand Up @@ -1713,7 +1881,7 @@
}

/**
* Get coin viewpoint (no lock).
* Get coin viewpoint as it pertains to mempool (no lock).
* @method
* @param {TX} tx
* @returns {Promise} - Returns {@link CoinView}.
Expand All @@ -1724,14 +1892,41 @@

for (const {prevout} of tx.inputs) {
const {hash, index} = prevout;
const tx = this.getTX(hash);

if (tx) {
if (this.hasCoin(hash, index))
view.addIndex(tx, index, -1);
continue;
// First check mempool for the TX
// that created the coin we need for the view
const parentTX = this.getTX(hash);

if (parentTX) {
// Does parent TX even have the output index?
if (index >= parentTX.outputs.length)
continue;

Check warning on line 1903 in lib/mempool/mempool.js

View check run for this annotation

Codecov / codecov/patch

lib/mempool/mempool.js#L1903

Added line #L1903 was not covered by tests

// Check to see if this output is already spent
// by another unconfirmed TX in the mempool
const spender = this.getSpent(hash, index);

if (!spender) {
// Parent TX output is unspent, add it to the view
view.addIndex(parentTX, index, -1);
continue;
}

// If the spender TX signals opt-in RBF, then we
// can still consider the parent TX output as spendable.
// Note that at this point we are not fully checking all the
// replaceability rules and the tx initally passed to this
// function may not be actually allowed to replace the spender.
if (spender.tx.isRBF()) {
// If any TX in the mempool signals opt-in RBF
// we already know this option is set.
assert(this.options.replaceByFee);
view.addIndex(parentTX, index, -1);
}
}

// Parent TX is not in mempool.
// Check the chain (UTXO set) for the coin.
const coin = await this.chain.readCoin(prevout);

if (coin)
Expand Down
7 changes: 7 additions & 0 deletions lib/protocol/policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,13 @@ exports.MEMPOOL_EXPIRY_TIME = 72 * 60 * 60;

exports.MEMPOOL_MAX_ORPHANS = 100;

/**
* BIP125 Rule #5
* Maximum number of transactions that can be replaced.
*/

exports.MEMPOOL_MAX_REPLACEMENTS = 100;

/**
* Minimum block size to create. Block will be
* filled with free transactions until block
Expand Down
Loading