-
Notifications
You must be signed in to change notification settings - Fork 282
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
base: master
Are you sure you want to change the base?
Changes from all commits
d62a61c
ccc0b40
359eb09
0c7d0a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
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); | ||
} | ||
|
||
/** | ||
|
@@ -193,13 +210,6 @@ class Mempool extends EventEmitter { | |
*/ | ||
|
||
async _addBlock(block, txs, view) { | ||
if (this.map.size === 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove this ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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]; | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to There needs to be additional check when the transaction that we have not seen is removed by Same as I believe all evictEntry callers are included here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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); | ||
} | ||
|
||
|
@@ -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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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(); | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} catch (err) { | ||
this.logger.debug( | ||
'Could not reconnect spender %x: %s.', | ||
tx.hash(), err.message); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -3098,7 +3201,7 @@ class Orphan { | |
* Create an orphan. | ||
* @constructor | ||
* @param {TX} tx | ||
* @param {Hash[]} missing | ||
* @param {Number} missing | ||
* @param {Number} id | ||
*/ | ||
|
||
|
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); | ||
}); | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.