diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -929,6 +929,8 @@ queuedTx.get().erase(entry); } + bool isEmpty() { return queuedTx.empty(); } + void clear() { cachedInnerUsage = 0; queuedTx.clear(); diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2193,20 +2193,18 @@ return false; } - // If this block was deactivating the replay protection, then we need to - // remove transactions that are replay protected from the mempool. There is - // no easy way to do this so we'll just discard the whole mempool and then - // add the transaction of the block we just disconnected back. - if (IsReplayProtectionEnabled(config, pindexDelete) && - !IsReplayProtectionEnabled(config, pindexDelete->pprev)) { - LogPrint(BCLog::MEMPOOL, "Clearing mempool for reorg"); - - g_mempool.clear(); - // While not strictly necessary, clearing the disconnect pool is also - // beneficial so we don't try to reuse its content at the end of the - // reorg, which we know will fail. + // If this block is deactivating a fork, we move all mempool transactions + // in front of disconnectpool for reprocessing in a future + // updateMempoolForReorg call + if (GetBlockScriptFlags(config, pindexDelete) != + GetBlockScriptFlags(config, pindexDelete->pprev)) { + LogPrint(BCLog::MEMPOOL, + "%s mempool for a reorg crossing a fork boundary\n", + disconnectpool ? "Reprocessing" : "Clearing"); if (disconnectpool) { - disconnectpool->clear(); + disconnectpool->importMempool(g_mempool); + } else { + g_mempool.clear(); } } @@ -2474,17 +2472,21 @@ (nTime5 - nTime4) * MILLI, nTimeChainState * MICRO, nTimeChainState * MILLI / nBlocksTotal); - // If we just activated the replay protection with that block, it means - // transaction in the mempool are now invalid. As a result, we need to clear - // the mempool. - if (IsReplayProtectionEnabled(config, pindexNew) && - !IsReplayProtectionEnabled(config, pindexNew->pprev)) { - g_mempool.clear(); - } - // Remove conflicting transactions from the mempool.; g_mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight); disconnectpool.removeForBlock(blockConnecting.vtx); + + // If this block is activating a fork, we move all mempool transactions + // in front of disconnectpool for reprocessing in a future + // updateMempoolForReorg call + if (pindexNew->pprev != nullptr && + GetBlockScriptFlags(config, pindexNew) != + GetBlockScriptFlags(config, pindexNew->pprev)) { + LogPrint(BCLog::MEMPOOL, "Reprocessing mempool because new validation " + "rules are in effect\n"); + disconnectpool.importMempool(g_mempool); + } + // Update chainActive & related variables. UpdateTip(config, pindexNew); @@ -2753,9 +2755,11 @@ } } - if (fBlocksDisconnected) { - // If any blocks were disconnected, disconnectpool may be non empty. Add - // any disconnected transactions back to the mempool. + if (fBlocksDisconnected || !disconnectpool.isEmpty()) { + // If any blocks were disconnected, we need to update the mempool even + // if disconnectpool is empty. The disconnectpool may also be non-empty + // if the mempool was imported due to new validation rules being in + // effect disconnectpool.updateMempoolForReorg(config, true); } diff --git a/test/functional/abc-replay-protection.py b/test/functional/abc-replay-protection.py --- a/test/functional/abc-replay-protection.py +++ b/test/functional/abc-replay-protection.py @@ -169,6 +169,19 @@ return [txfund, txspend] + # Creates a chainable tx, spending a scriptPub=OP_TRUE coin into + # another + def create_chaining_tx(spend): + if isinstance(spend, CTransaction): + # After the first iteration, we start calling this function + tx = create_transaction( + spend, 0, b'', spend.vout[0].nValue - 1000, CScript([OP_TRUE])) + else: + tx = create_transaction( + spend.tx, spend.n, b'', 50 * COIN, CScript([OP_TRUE])) + tx.rehash() + return tx + def send_transaction_to_mempool(tx): tx_id = node.sendrawtransaction(ToHex(tx)) assert(tx_id in set(node.getrawmempool())) @@ -210,7 +223,8 @@ yield test # Check we are just before the activation time - assert_equal(node.getblockheader(node.getbestblockhash())['mediantime'], + assert_equal( + node.getblockheader(node.getbestblockhash())['mediantime'], REPLAY_PROTECTION_START_TIME - 1) # We are just before the fork, replay protected txns still are rejected @@ -230,17 +244,33 @@ send_transaction_to_mempool(txns[0]) tx_id = send_transaction_to_mempool(txns[1]) - # Activate the replay protection + # Send two always valid txns (pre and post-fork) to the mempool. The + # first one will be mined in the activation block, the 2nd will be kept + # in the mempool through the fork boundary + always_valid_txns_ids = [] + first_chained_tx = create_chaining_tx(out[3]) + always_valid_txns_ids.append( + send_transaction_to_mempool(first_chained_tx)) + last_chained_tx = create_chaining_tx(first_chained_tx) + always_valid_txns_ids.append( + send_transaction_to_mempool(last_chained_tx)) + + # Activate the replay protection while also mining the first chained tx block(5556) + update_block(5556, [first_chained_tx]) yield accepted() # Check we just activated the replay protection - assert_equal(node.getblockheader(node.getbestblockhash())['mediantime'], + assert_equal( + node.getblockheader(node.getbestblockhash())['mediantime'], REPLAY_PROTECTION_START_TIME) # Non replay protected transactions are not valid anymore, # so they should be removed from the mempool. assert(tx_id not in set(node.getrawmempool())) + # The 2nd chained transaction is still in the mempool after the fork + # activating block + assert(always_valid_txns_ids[1] in node.getrawmempool()) # Good old transactions are now invalid. send_transaction_to_mempool(txns[0]) @@ -278,30 +308,55 @@ assert(replay_tx0_id in set(node.getrawmempool())) assert(replay_tx1_id in set(node.getrawmempool())) - # They also can also be mined + # They also can also be mined, along with the 2nd chained tx b5 = block(5) - update_block(5, replay_txns) + update_block(5, replay_txns + [last_chained_tx]) yield accepted() + # Now we add a chain of 5 unconfirmed txns + for _ in range(5): + last_chained_tx = create_chaining_tx(last_chained_tx) + always_valid_txns_ids.append( + send_transaction_to_mempool(last_chained_tx)) + assert( + set(always_valid_txns_ids[2:]).issubset(set(node.getrawmempool()))) + # Ok, now we check if a reorg work properly accross the activation. postforkblockid = node.getbestblockhash() node.invalidateblock(postforkblockid) assert(replay_tx0_id in set(node.getrawmempool())) assert(replay_tx1_id in set(node.getrawmempool())) + # The 2nd chained txn, which was sent post fork, returned to the + # mempool + assert( + set(always_valid_txns_ids[1:]).issubset(set(node.getrawmempool()))) # Deactivating replay protection. forkblockid = node.getbestblockhash() node.invalidateblock(forkblockid) - assert(replay_tx0_id not in set(node.getrawmempool())) + assert(replay_tx0_id in set(node.getrawmempool())) assert(replay_tx1_id not in set(node.getrawmempool())) + # Now all chained txns are in the mempool, even those created after the + # fork activated + assert(set(always_valid_txns_ids).issubset(set(node.getrawmempool()))) # Check that we also do it properly on deeper reorg. node.reconsiderblock(forkblockid) node.reconsiderblock(postforkblockid) node.invalidateblock(forkblockid) - assert(replay_tx0_id not in set(node.getrawmempool())) + assert(replay_tx0_id in set(node.getrawmempool())) assert(replay_tx1_id not in set(node.getrawmempool())) + assert(set(always_valid_txns_ids).issubset(set(node.getrawmempool()))) + # Check that we do it properly on a real reorg to a different chain but + # still crossing the fork boundary + node.reconsiderblock(forkblockid) + tip(5104) + block(8) + block(9) + block(10) + yield TestInstance([[self.blocks[8], True], [self.blocks[9], True], [self.blocks[10], True]], False) + assert(set(always_valid_txns_ids).issubset(set(node.getrawmempool()))) if __name__ == '__main__': ReplayProtectionTest().main()