Mempool: handle covenant conflicts during reorg and save TXs for later if possible #773
Conversation
| // 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] |
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
I feel like children should be called else: disconnectedTXMap or something.
|
|
||
| // 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.
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.
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
| // 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.
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.
| if (missing) | ||
| throw new Error('spender is orphan.'); | ||
| // Recurse only on success | ||
| await this.reconnectSpenders(tx); |
There was a problem hiding this comment.
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?
| 'Could not reconnect spender %x: %s.', | ||
| tx.hash(), err.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should remove parent->[child hashes] from the disconnected list and the single layer of children from the children.
| @@ -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.
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.
There was a problem hiding this comment.
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.
|
|
||
| for (const hash of hashes) { | ||
| const tx = this.children.get(hash); | ||
| assert(tx); |
There was a problem hiding this comment.
Note: this assert will no longer hold true if we track double spends for children.
| @@ -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.
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 PR should improve UX and increase miner fee profits after reorgs and in some edge cases involving covenant conflicts.
There are some covenants that can not coexist in a block, for example
UPDATEcan only happen once per name per block. That also means that at any time in the mempool, there can only be oneUPDATEper name. After a reorginzation when transactions get removed from blocks and re-enter the mempool, we need to track these conflicts carefully and make sure that "the next valid transaction" in the covenant chain is the only covenant for a given name in the mempool.By storing the conflicting TXs outside the mempool in a new data structure, a miner can still earn fees LATER when the parent TXs confirm and the child TXs are once again valid.