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

Mempool: handle covenant conflicts during reorg and save TXs for later if possible #773

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 122 additions & 19 deletions lib/mempool/mempool.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,37 @@ class Mempool extends EventEmitter {
this.tip = this.network.genesis.hash;
this.nextState = this.chain.state;

this.waiting = new BufferMap();
this.orphans = new BufferMap();
this.map = new BufferMap();
this.spents = new BufferMap();
this.claims = new BufferMap();
this.airdrops = new BufferMap();
this.airdropIndex = new Map();
this.claimNames = new BufferMap();
// "The" mempool
this.map = new BufferMap(); // hash -> MempoolEntry
this.claims = new BufferMap(); // hash -> ClaimEntry
this.airdrops = new BufferMap(); // hash -> AirdropEntry

// Orphans and missing parents
this.waiting = new BufferMap(); // parent hash -> BufferSet[spender hashes]
this.orphans = new BufferMap(); // orphan tx hash -> Orphan

// Prevent double-spends
this.spents = new BufferMap(); // prevout key -> MempoolEntry of spender
this.claimNames = new BufferMap(); // namehash -> ClaimEntry
this.airdropIndex = new Map(); // airdrop position -> AirdropEntry

// Track namestates to validate incoming covenants
this.contracts = new ContractState(this.network);

// Recently rejected txs by hash
this.rejects = new RollingFilter(120000, 0.000001);

// TXs removed from the mempool because their
// parent TX was removed from a block and their
// covenants prevent them from joining in a block or mempool.
// Will be valid again once the parent is re-confirmed.
this.disconnected =
new BufferMap(); // parent hash -> BufferSet[child hashes]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would it be better to have the mapping description above the definition, so we don't have this kinda weird lines?

    // Will be valid again once the parent is re-confirmed.

    // parent hash -> BufferSet[child hashes]
    this.disconnected = new BufferMap();
    // child tx hash -> TX (not MempoolEntry)
    this.children = new BufferMap();

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like children should be called else: disconnectedTXMap or something.

this.children = new BufferMap(); // child tx hash -> TX (not MempoolEntry)

// Extensions of blockchain indexes by tx hash for API
this.coinIndex = new CoinIndex();
this.txIndex = new TXIndex();

this.contracts = new ContractState(this.network);
}

/**
Expand Down Expand Up @@ -193,13 +210,6 @@ class Mempool extends EventEmitter {
*/

async _addBlock(block, txs, view) {
if (this.map.size === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #319

I don't think there was any reason for this to be here besides early-bcoin-style laziness and maybe even an accidental copy-paste from _removeBlock()where it used to make more sense, to just avoid complicated mempool issues.

What the function does is resolve orphans whose missing parents were just confirmed. There is no reason to NOT do that just because the current mempool is empty.

&& this.claims.size === 0
&& this.airdrops.size === 0) {
this.tip = block.hash;
return;
}

const entries = [];
const cb = txs[0];

Expand All @@ -209,18 +219,27 @@ class Mempool extends EventEmitter {
const entry = this.getEntry(hash);

if (!entry) {
// Confirmed TX was not in mempool
// maybe clear other references and conflicts
this.removeOrphan(hash);
this.removeDoubleSpends(tx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to removeDoubleSpends:

There needs to be additional check when the transaction that we have not seen is removed by removeDoubleSpends. I think evictEntry is a good place to clean up disconnects, because evicting the original transaction that we are waiting to reconnect will leave them there w/o any way to clean up.

Same as removeDoubleSpends it could happen on covenant invalidation, e.g. unconfirmed disconnector transaction, that we are waiting for got invalidated in the mempool (e.g. expire). This also leads to evictEntry.

I believe all evictEntry callers are included here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing that we need to track is double spends to the .children transactions. Any of the children can get double spent in the block, but since they are not part of the normal transaction map and are not tracked via getSpent we will need to track them separately.

this.removeDoubleOpens(tx);

// Confirmed TX resolves an orphan
if (this.waiting.has(hash))
await this.handleOrphans(tx);
this.removeDoubleOpens(tx);

continue;
}

this.removeEntry(entry);

this.emit('confirmed', tx, block);

// Confirmed TX reconnects a child
if (this.disconnected.has(hash))
await this.reconnectSpenders(tx);

entries.push(entry);
}

Expand Down Expand Up @@ -382,6 +401,18 @@ class Mempool extends EventEmitter {
if (this.hasEntry(hash))
continue;

// Some covenants can only be used once per name per block.
// If the TX we want to re-insert into the mempool conflicts
// with another TX already in the mempool because of this rule,
// the solution is to evict the child TX (the TX already in the
// mempool) and then insert the parent TX (from the disconnected block).
// Since the child TX spends the output of the parent TX, evicting the
// parent TX but keeping the child TX would leave the mempool in an
// invalid state, and the miner would produce invalid blocks.
// We can hang on to those evicted child TXs until they are valid again.
if (this.contracts.hasNames(tx))
this.disconnectSpenders(tx);

Copy link
Contributor

Choose a reason for hiding this comment

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

In the catch of the following block, we need to try and reconnect those disconnected names. Otherwise they will get stuck in the disconnected until node is restarted, if unconfirmed transaction can't be inserted into the mempool.

try {
await this.insertTX(tx, -1);
total += 1;
Expand Down Expand Up @@ -557,6 +588,9 @@ class Mempool extends EventEmitter {
this.claimNames.clear();
this.spents.clear();
this.contracts.clear();
this.disconnected.clear();
this.children.clear();

this.coinIndex.reset();
this.txIndex.reset();

Expand Down Expand Up @@ -1851,6 +1885,75 @@ class Mempool extends EventEmitter {
}
}

/**
* Recursively disconnect spenders of a transaction.
* @private
* @param {TX} parent
*/

disconnectSpenders(parent) {
const parentHash = parent.hash();
const children = new BufferSet();
const names = new BufferSet();
rules.addNames(parent, names);

for (let i = 0; i < parent.outputs.length; i++) {
const spender = this.getSpent(parentHash, i); // MempoolEntry

// No child spending this output
if (!spender)
continue;

const {tx} = spender;

// Child is not linked by name.
// Covenant rules don't prevent it from
// staying in the mempool.
if (!rules.hasNames(tx, names))
continue;

// This child has to be removed from the mempool
// but it will be valid again once its parent confirms.
this.disconnectSpenders(tx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case where this will do anything ?

If spender was in the mempool it should not have disconnectable spenders?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point about this, I need to go back down this rabbit hole. it's possible i was clearing out ANY children of the disconnected tx, which would apply to like, if a child spent the change output of a name-linked tx

children.add(tx.hash());
this.children.set(tx.hash(), tx);
this.removeEntry(spender);
}

if (children.size)
this.disconnected.set(parentHash, children);
}

/**
* Recursively reconnect spenders of a transaction.
* @private
* @param {TX} parent
*/

async reconnectSpenders(parent) {
const hashes = this.disconnected.get(parent.hash());

if (!hashes)
return;

for (const hash of hashes) {
const tx = this.children.get(hash);
assert(tx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this assert will no longer hold true if we track double spends for children.


try {
const missing = await this.insertTX(tx, -1);
if (missing)
throw new Error('spender is orphan.');
// Recurse only on success
await this.reconnectSpenders(tx);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should reconnect spenders here again.. For example, if we have update -> update -> update chain in there, we only need to move into a single layer only, not in depth. Otherwise we will encounter unique name issues?

} catch (err) {
this.logger.debug(
'Could not reconnect spender %x: %s.',
tx.hash(), err.message);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove parent->[child hashes] from the disconnected list and the single layer of children from the children.

}

/**
* Count the highest number of
* ancestors a transaction may have.
Expand Down Expand Up @@ -3098,7 +3201,7 @@ class Orphan {
* Create an orphan.
* @constructor
* @param {TX} tx
* @param {Hash[]} missing
* @param {Number} missing
* @param {Number} id
*/

Expand Down
168 changes: 168 additions & 0 deletions test/mempool-reorg-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
'use strict';

const assert = require('bsert');
const Network = require('../lib/protocol/network');
const {Resource} = require('../lib/dns/resource');
const FullNode = require('../lib/node/fullnode');
const plugin = require('../lib/wallet/plugin');

const network = Network.get('regtest');
const {
treeInterval,
biddingPeriod,
revealPeriod
} = network.names;

describe('Mempool Covenant Reorg', function () {
const node = new FullNode({
network: 'regtest'
});
node.use(plugin);

let wallet, name;

before(async () => {
await node.open();
wallet = node.get('walletdb').wdb.primary;
});

after(async () => {
await node.close();
});

let counter = 0;
function makeResource() {
return Resource.fromJSON({
records: [{type: 'TXT', txt: [`${counter++}`]}]
});
}

async function getConfirmedResource() {
const res = await node.rpc.getNameResource([name]);
return res.records[0].txt[0];
}

function getMempoolResource() {
if (!node.mempool.map.size)
return null;

assert.strictEqual(node.mempool.map.size, 1);
const {tx} = node.mempool.map.toValues()[0];
const res = Resource.decode(tx.outputs[0].covenant.items[2]);
return res.records[0].txt[0];
}

it('should fund wallet and win name', async () => {
await node.rpc.generate([10]);
name = await node.rpc.grindName([3]);
await wallet.sendOpen(name, true);
await node.rpc.generate([treeInterval + 1]);
await wallet.sendBid(name, 10000, 20000);
await node.rpc.generate([biddingPeriod]);
await wallet.sendReveal(name);
await node.rpc.generate([revealPeriod]);
await node.rpc.generate([1]);
await wallet.sendUpdate(name, makeResource());
await node.rpc.generate([1]);

assert.strictEqual(await getConfirmedResource(), '0');
});

it('should generate UPDATE chain', async () => {
for (let i = 0; i < 10; i++) {
await wallet.sendUpdate(
name,
makeResource(),
{selection: 'age'} // avoid spending coinbase early
);
await node.rpc.generate([1]);
}
});

it('should shallow reorg chain', async () => {
// Initial state
assert.strictEqual(await getConfirmedResource(), '10');

// Mempool is empty
assert.strictEqual(getMempoolResource(), null);

// Do not reorg beyond tree interval
assert(node.chain.height % treeInterval === 3);

// Reorganize
const waiter = new Promise((resolve) => {
node.once('reorganize', () => {
resolve();
});
});

const depth = 3;
let entry = await node.chain.getEntryByHeight(node.chain.height - depth);
for (let i = 0; i <= depth; i++) {
const block = await node.miner.cpu.mineBlock(entry);
entry = await node.chain.add(block);
}
await waiter;

// State after reorg
assert.strictEqual(await getConfirmedResource(), '7');

// Mempool is NOT empty, "next" tx is waiting
assert.strictEqual(getMempoolResource(), '8');

// This next block would be invalid in our own chain
// if mempool was corrupted with the wrong tx from the reorg.
await node.rpc.generate([1]);

// State after new block
assert.strictEqual(await getConfirmedResource(), '8');

// Mempool is again NOT empty, "next NEXT" tx is waiting
assert.strictEqual(getMempoolResource(), '9');

// One more
await node.rpc.generate([1]);
assert.strictEqual(await getConfirmedResource(), '9');
assert.strictEqual(getMempoolResource(), '10');

// Finally
await node.rpc.generate([1]);
assert.strictEqual(await getConfirmedResource(), '10');
assert.strictEqual(getMempoolResource(), null);
});

it('should deep reorg chain', async () => {
// Initial state
assert.strictEqual(await getConfirmedResource(), '10');

// Mempool is empty
assert.strictEqual(getMempoolResource(), null);

// Reorganize beyond tree interval
const waiter = new Promise((resolve) => {
node.once('reorganize', () => {
resolve();
});
});

const depth = 12;
let entry = await node.chain.getEntryByHeight(node.chain.height - depth);
// Intentionally forking from historical tree interval requires dirty hack
const {treeRoot} = await node.chain.getEntryByHeight(node.chain.height - depth + 1);
await node.chain.db.tree.inject(treeRoot);
for (let i = 0; i <= depth; i++) {
const block = await node.miner.cpu.mineBlock(entry);
entry = await node.chain.add(block);
}
await waiter;

// State after reorg
assert.strictEqual(await getConfirmedResource(), '2');
assert.strictEqual(getMempoolResource(), '3');

// Confirm entire update chain one by one
await node.rpc.generate([8]);
assert.strictEqual(await getConfirmedResource(), '10');
assert.strictEqual(getMempoolResource(), null);
});
});
Loading