From c3602372ccb5a18b363790738d872f18ddde4021 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 19 Mar 2019 11:55:51 +0100 Subject: [PATCH] Implement retroactive IS locking of transactions first seen in blocks instead of mempool (#2770) * Don't rely on UTXO set in CheckCanLock The UTXO set only works for TXs in the mempool and won't work when we try to retroactively lock unlocked TXs from blocks. This is safe as ProcessTx is only called when a TX was accepted into the mempool or connected in a block, which means that all input checks were good. * Rename RetryLockMempoolTxs to RetryLockTxs and let it retry connected TXs * Instead of manually calling ProcessTx, let SyncTransaction handle all cases SyncTransaction is called from AcceptToMemoryPool and when transactions got connected in a block. So this is the time we want to run TXs through ProcessTx. This also enables retroactive signing of TXs that were unknown before a new block appeared. * Test retroactive signing and safe TXs in LLMQ ChainLocks tests * Also test for retroactive signing of chained TXs * Honor lockedParentTx when looking for TXs to retry signing * Stop scanning for TXs to retry after a depth of 6 * Generate 6 block to avoid retroactive signing overloading Travis * Avoid retroactive signing * Don't rely on NewPoWValidBlock and use SyncTransaction to build blockTxs NewPoWValidBlock is not guaranteed to be called when blocks come in fast. When a block is accepted in AcceptBlock, NewPoWValidBlock is only called when the new block is a successor of the currently active tip. This is not the case when after the first block a second block is accepted immediately as the first block is not connected yet. This might be a bug actually in the handling of NewPoWValidBlock, so we might need to check/fix this later, but currently I prefer to not touch that part. Instead, we now use SyncTransaction to gather TXs for blockTxs. This works because SyncTransaction is called for all transactions in a freshly connected block in one go. The call also happens before UpdatedBlockTip is called, so it's fine with the existing logic. * Use tx.IsCoinBase() instead of checking index 0 Also check for empty vin. --- qa/rpc-tests/autois-mempool.py | 16 ++++- qa/rpc-tests/llmq-chainlocks.py | 50 +++++++++++++++ qa/rpc-tests/p2p-autoinstantsend.py | 4 ++ qa/rpc-tests/p2p-instantsend.py | 4 ++ src/dsnotificationinterface.cpp | 5 -- src/dsnotificationinterface.h | 1 - src/llmq/quorums_chainlocks.cpp | 47 ++++++-------- src/llmq/quorums_chainlocks.h | 1 - src/llmq/quorums_instantsend.cpp | 99 +++++++++++++++++++++++------ src/llmq/quorums_instantsend.h | 2 +- src/net_processing.cpp | 6 -- src/rpc/rawtransaction.cpp | 1 - src/wallet/wallet.cpp | 2 - 13 files changed, 174 insertions(+), 64 deletions(-) diff --git a/qa/rpc-tests/autois-mempool.py b/qa/rpc-tests/autois-mempool.py index 7a63cd0ec8480..71b23727defdd 100755 --- a/qa/rpc-tests/autois-mempool.py +++ b/qa/rpc-tests/autois-mempool.py @@ -56,6 +56,10 @@ def run_test(self): self.log.info("Test old InstantSend") self.test_auto(); + # Generate 6 block to avoid retroactive signing overloading Travis + self.nodes[0].generate(6) + sync_blocks(self.nodes) + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0) self.wait_for_sporks_same() @@ -100,9 +104,17 @@ def test_auto(self, new_is = False): assert(not self.send_regular_instantsend(sender, receiver, False) if new_is else self.send_regular_instantsend(sender, receiver)) # generate one block to clean up mempool and retry auto and regular IS - # generate 2 more blocks to have enough confirmations for IS - self.nodes[0].generate(3) + # generate 5 more blocks to avoid retroactive signing (which would overload Travis) + set_mocktime(get_mocktime() + 1) + set_node_times(self.nodes, get_mocktime()) + self.nodes[0].spork("SPORK_2_INSTANTSEND_ENABLED", 4070908800) + self.wait_for_sporks_same() + self.nodes[0].generate(6) self.sync_all() + set_mocktime(get_mocktime() + 1) + set_node_times(self.nodes, get_mocktime()) + self.nodes[0].spork("SPORK_2_INSTANTSEND_ENABLED", 0) + self.wait_for_sporks_same() assert(self.send_simple_tx(sender, receiver)) assert(self.send_regular_instantsend(sender, receiver, not new_is)) diff --git a/qa/rpc-tests/llmq-chainlocks.py b/qa/rpc-tests/llmq-chainlocks.py index 6cbcc6cdc108a..26ebe6fd3c2b1 100755 --- a/qa/rpc-tests/llmq-chainlocks.py +++ b/qa/rpc-tests/llmq-chainlocks.py @@ -88,6 +88,38 @@ def run_test(self): self.wait_for_chainlock(self.nodes[0], self.nodes[1].getbestblockhash()) assert(self.nodes[0].getbestblockhash() == self.nodes[1].getbestblockhash()) + # Enable LLMQ bases InstantSend, which also enables checks for "safe" transactions + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0) + self.wait_for_sporks_same() + + # Isolate a node and let it create some transactions which won't get IS locked + self.nodes[0].setnetworkactive(False) + txs = [] + for i in range(3): + txs.append(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)) + txs += self.create_chained_txs(self.nodes[0], 1) + # Assert that after block generation these TXs are NOT included (as they are "unsafe") + self.nodes[0].generate(1) + for txid in txs: + tx = self.nodes[0].getrawtransaction(txid, 1) + assert("confirmations" not in tx) + sleep(1) + assert(not self.nodes[0].getblock(self.nodes[0].getbestblockhash())["chainlock"]) + # Disable LLMQ based InstantSend for a very short time (this never gets propagated to other nodes) + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 4070908800) + # Now the TXs should be included + self.nodes[0].generate(1) + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0) + # Assert that TXs got included now + for txid in txs: + tx = self.nodes[0].getrawtransaction(txid, 1) + assert("confirmations" in tx and tx["confirmations"] > 0) + # Enable network on first node again, which will cause the blocks to propagate and IS locks to happen retroactively + # for the mined TXs, which will then allow the network to create a CLSIG + self.nodes[0].setnetworkactive(True) + connect_nodes(self.nodes[0], 1) + self.wait_for_chainlock(self.nodes[0], self.nodes[1].getbestblockhash()) + def wait_for_chainlock_tip_all_nodes(self): for node in self.nodes: tip = node.getbestblockhash() @@ -110,6 +142,24 @@ def wait_for_chainlock(self, node, block_hash): sleep(0.1) raise AssertionError("wait_for_chainlock timed out") + def create_chained_txs(self, node, amount): + txid = node.sendtoaddress(node.getnewaddress(), amount) + tx = node.getrawtransaction(txid, 1) + inputs = [] + valueIn = 0 + for txout in tx["vout"]: + inputs.append({"txid": txid, "vout": txout["n"]}) + valueIn += txout["value"] + outputs = { + node.getnewaddress(): round(float(valueIn) - 0.0001, 6) + } + + rawtx = node.createrawtransaction(inputs, outputs) + rawtx = node.signrawtransaction(rawtx) + rawtxid = node.sendrawtransaction(rawtx["hex"]) + + return [txid, rawtxid] + if __name__ == '__main__': LLMQChainLocksTest().main() diff --git a/qa/rpc-tests/p2p-autoinstantsend.py b/qa/rpc-tests/p2p-autoinstantsend.py index 4af1e7640f9da..707265a1c2624 100755 --- a/qa/rpc-tests/p2p-autoinstantsend.py +++ b/qa/rpc-tests/p2p-autoinstantsend.py @@ -39,6 +39,10 @@ def run_test(self): self.log.info("Test old InstantSend") self.test_auto(); + # Generate 6 block to avoid retroactive signing overloading Travis + self.nodes[0].generate(6) + sync_blocks(self.nodes) + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0) self.wait_for_sporks_same() diff --git a/qa/rpc-tests/p2p-instantsend.py b/qa/rpc-tests/p2p-instantsend.py index 39a75cc40cce3..5be59689410b0 100755 --- a/qa/rpc-tests/p2p-instantsend.py +++ b/qa/rpc-tests/p2p-instantsend.py @@ -28,6 +28,10 @@ def run_test(self): self.log.info("Test old InstantSend") self.test_doublespend() + # Generate 6 block to avoid retroactive signing overloading Travis + self.nodes[0].generate(6) + sync_blocks(self.nodes) + self.nodes[0].spork("SPORK_20_INSTANTSEND_LLMQ_BASED", 0) self.wait_for_sporks_same() diff --git a/src/dsnotificationinterface.cpp b/src/dsnotificationinterface.cpp index 418f1f9e2879d..c758191f5f64d 100644 --- a/src/dsnotificationinterface.cpp +++ b/src/dsnotificationinterface.cpp @@ -71,11 +71,6 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); } -void CDSNotificationInterface::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr& block) -{ - llmq::chainLocksHandler->NewPoWValidBlock(pindex, block); -} - void CDSNotificationInterface::SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) { llmq::quorumInstantSendManager->SyncTransaction(tx, pindex, posInBlock); diff --git a/src/dsnotificationinterface.h b/src/dsnotificationinterface.h index c345551153e38..1dc83f7d085e2 100644 --- a/src/dsnotificationinterface.h +++ b/src/dsnotificationinterface.h @@ -21,7 +21,6 @@ class CDSNotificationInterface : public CValidationInterface void AcceptedBlockHeader(const CBlockIndex *pindexNew) override; void NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; - void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) override; void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) override; void NotifyMasternodeListChanged(const CDeterministicMNList& newList) override; void NotifyChainLock(const CBlockIndex* pindex) override; diff --git a/src/llmq/quorums_chainlocks.cpp b/src/llmq/quorums_chainlocks.cpp index f9a73c539108d..40bf14004b943 100644 --- a/src/llmq/quorums_chainlocks.cpp +++ b/src/llmq/quorums_chainlocks.cpp @@ -316,41 +316,34 @@ void CChainLocksHandler::TrySignChainTip() quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash); } -void CChainLocksHandler::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr& block) +void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) { - LOCK(cs); - if (blockTxs.count(pindex->GetBlockHash())) { - // should actually not happen (blocks are only written once to disk and this is when NewPoWValidBlock is called) - // but be extra safe here in case this behaviour changes. - return; + bool handleTx = true; + if (tx.IsCoinBase() || tx.vin.empty()) { + handleTx = false; } - int64_t curTime = GetAdjustedTime(); + LOCK(cs); - // We listen for NewPoWValidBlock so that we can collect all TX ids of all included TXs of newly received blocks + if (handleTx) { + int64_t curTime = GetAdjustedTime(); + txFirstSeenTime.emplace(tx.GetHash(), curTime); + } + + // We listen for SyncTransaction so that we can collect all TX ids of all included TXs of newly received blocks // We need this information later when we try to sign a new tip, so that we can determine if all included TXs are // safe. - - auto txs = std::make_shared>(); - for (const auto& tx : block->vtx) { - if (tx->IsCoinBase() || tx->vin.empty()) { - continue; + if (pindex && posInBlock != CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK) { + auto it = blockTxs.find(pindex->GetBlockHash()); + if (it == blockTxs.end()) { + // we want this to be run even if handleTx == false, so that the coinbase TX triggers creation of an empty entry + it = blockTxs.emplace(pindex->GetBlockHash(), std::make_shared>()).first; + } + if (handleTx) { + auto& txs = *it->second; + txs.emplace(tx.GetHash()); } - txs->emplace(tx->GetHash()); - txFirstSeenTime.emplace(tx->GetHash(), curTime); - } - blockTxs[pindex->GetBlockHash()] = txs; -} - -void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) -{ - if (tx.IsCoinBase() || tx.vin.empty()) { - return; } - - LOCK(cs); - int64_t curTime = GetAdjustedTime(); - txFirstSeenTime.emplace(tx.GetHash(), curTime); } bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid) diff --git a/src/llmq/quorums_chainlocks.h b/src/llmq/quorums_chainlocks.h index 660a983a71186..6d0714bbc7b83 100644 --- a/src/llmq/quorums_chainlocks.h +++ b/src/llmq/quorums_chainlocks.h @@ -87,7 +87,6 @@ class CChainLocksHandler : public CRecoveredSigsListener void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash); void AcceptedBlockHeader(const CBlockIndex* pindexNew); void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork); - void NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr& block); void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock); void TrySignChainTip(); void EnforceBestChainLock(); diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 7db72015c5ab2..2078f9f4d0ce9 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -294,20 +294,23 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu return false; } - Coin coin; - const CBlockIndex* pindexMined = nullptr; + CTransactionRef tx; + uint256 hashBlock; + // this relies on enabled txindex and won't work if we ever try to remove the requirement for txindex for masternodes + if (!GetTransaction(outpoint.hash, tx, params, hashBlock, false)) { + if (printDebug) { + LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: failed to find parent TX %s\n", __func__, + txHash.ToString(), outpoint.hash.ToString()); + } + return false; + } + + const CBlockIndex* pindexMined; int nTxAge; { LOCK(cs_main); - if (!pcoinsTip->GetCoin(outpoint, coin) || coin.IsSpent()) { - if (printDebug) { - LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s: failed to find UTXO %s\n", __func__, - txHash.ToString(), outpoint.ToStringShort()); - } - return false; - } - pindexMined = chainActive[coin.nHeight]; - nTxAge = chainActive.Height() - coin.nHeight + 1; + pindexMined = mapBlockIndex.at(hashBlock); + nTxAge = chainActive.Height() - pindexMined->nHeight + 1; } if (nTxAge < nInstantSendConfirmationsRequired) { @@ -321,7 +324,7 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu } if (retValue) { - *retValue = coin.out.nValue; + *retValue = tx->vout[outpoint.n].nValue; } return true; @@ -673,7 +676,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has g_connman->RelayInv(inv); RemoveMempoolConflictsForLock(hash, islock); - RetryLockMempoolTxs(islock.txid); + RetryLockTxs(islock.txid); UpdateWalletTransaction(islock.txid, tx); } @@ -708,8 +711,17 @@ void CInstantSendManager::SyncTransaction(const CTransaction& tx, const CBlockIn return; } - if (IsLocked(tx.GetHash())) { - RetryLockMempoolTxs(tx.GetHash()); + if (tx.IsCoinBase() || tx.vin.empty()) { + // coinbase can't and TXs with no inputs be locked + return; + } + + bool locked = IsLocked(tx.GetHash()); + bool chainlocked = pindex && chainLocksHandler->HasChainLock(pindex->nHeight, pindex->GetBlockHash()); + if (locked || chainlocked) { + RetryLockTxs(tx.GetHash()); + } else { + ProcessTx(tx, Params().GetConsensus()); } } @@ -756,7 +768,7 @@ void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock) db.WriteLastChainLockBlock(pindexChainLock->GetBlockHash()); } - RetryLockMempoolTxs(uint256()); + RetryLockTxs(uint256()); } void CInstantSendManager::RemoveFinalISLock(const uint256& hash, const CInstantSendLockPtr& islock) @@ -795,9 +807,9 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, con } } -void CInstantSendManager::RetryLockMempoolTxs(const uint256& lockedParentTx) +void CInstantSendManager::RetryLockTxs(const uint256& lockedParentTx) { - // Let's retry all mempool TXs which don't have an islock yet and where the parents got ChainLocked now + // Let's retry all unlocked TXs from mempool and and recently connected blocks std::unordered_map txs; @@ -817,6 +829,57 @@ void CInstantSendManager::RetryLockMempoolTxs(const uint256& lockedParentTx) } } } + + uint256 lastChainLockBlock; + const CBlockIndex* pindexLastChainLockBlock = nullptr; + const CBlockIndex* pindexWalk = nullptr; + { + LOCK(cs); + lastChainLockBlock = db.GetLastChainLockBlock(); + } + { + LOCK(cs_main); + if (!lastChainLockBlock.IsNull()) { + pindexLastChainLockBlock = mapBlockIndex.at(lastChainLockBlock); + pindexWalk = chainActive.Tip(); + } + } + + // scan blocks until we hit the last chainlocked block we know of. Also stop scanning after a depth of 6 to avoid + // signing thousands of TXs at once. Also, after a depth of 6, blocks get eligible for ChainLocking even if unsafe + // TXs are included, so there is no need to retroactively sign these. + int depth = 0; + while (pindexWalk && pindexWalk != pindexLastChainLockBlock && depth < 6) { + CBlock block; + { + LOCK(cs_main); + if (!ReadBlockFromDisk(block, pindexWalk, Params().GetConsensus())) { + pindexWalk = pindexWalk->pprev; + continue; + } + } + + for (const auto& tx : block.vtx) { + if (lockedParentTx.IsNull()) { + txs.emplace(tx->GetHash(), tx); + } else { + bool isChild = false; + for (auto& in : tx->vin) { + if (in.prevout.hash == lockedParentTx) { + isChild = true; + break; + } + } + if (isChild) { + txs.emplace(tx->GetHash(), tx); + } + } + } + + pindexWalk = pindexWalk->pprev; + depth++; + } + for (auto& p : txs) { auto& tx = p.second; { diff --git a/src/llmq/quorums_instantsend.h b/src/llmq/quorums_instantsend.h index bd689d641213d..da9e61b65cfa6 100644 --- a/src/llmq/quorums_instantsend.h +++ b/src/llmq/quorums_instantsend.h @@ -124,7 +124,7 @@ class CInstantSendManager : public CRecoveredSigsListener void RemoveFinalISLock(const uint256& hash, const CInstantSendLockPtr& islock); void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock); - void RetryLockMempoolTxs(const uint256& lockedParentTx); + void RetryLockTxs(const uint256& lockedParentTx); bool AlreadyHave(const CInv& inv); bool GetInstantSendLockByHash(const uint256& hash, CInstantSendLock& ret); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 957f51e9c17b0..70b5e45593aa3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2164,10 +2164,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr instantsend.Vote(tx.GetHash(), connman); } - if (nInvType != MSG_TXLOCK_REQUEST) { - llmq::quorumInstantSendManager->ProcessTx(tx, chainparams.GetConsensus()); - } - mempool.check(pcoinsTip); connman.RelayTransaction(tx); for (unsigned int i = 0; i < tx.vout.size(); i++) { @@ -2212,8 +2208,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr vWorkQueue.emplace_back(orphanHash, i); } vEraseQueue.push_back(orphanHash); - - llmq::quorumInstantSendManager->ProcessTx(orphanTx, chainparams.GetConsensus()); } else if (!fMissingInputs2) { diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 8e598553756c6..8ece8720fc62a 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1020,7 +1020,6 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) throw JSONRPCError(RPC_TRANSACTION_ERROR, state.GetRejectReason()); } } - llmq::quorumInstantSendManager->ProcessTx(*tx, Params().GetConsensus()); } else if (fHaveChain) { throw JSONRPCError(RPC_TRANSACTION_ALREADY_IN_CHAIN, "transaction already in block chain"); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2b63bb16eb81..b668a60f1f5c9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1921,8 +1921,6 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman, const std::string& str } } - llmq::quorumInstantSendManager->ProcessTx(*this->tx, Params().GetConsensus()); - if (connman) { connman->RelayTransaction((CTransaction)*this); return true;