From 7e1145068d90081102fd58ddc86dad02854b61d8 Mon Sep 17 00:00:00 2001 From: Kyon <32325790+kyonRay@users.noreply.github.com> Date: Sun, 28 Jan 2024 10:27:13 +0800 Subject: [PATCH] (txpool,pbft): fix txpool check nonce and block limit logic, fix pbft receive large view pre-prepare msg not reset tx bug. (#4198) --- bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.cpp | 54 ++++++++++++------- bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.h | 14 ++++- .../pbft/cache/PBFTCacheProcessor.cpp | 1 + .../bcos-pbft/pbft/engine/PBFTEngine.cpp | 13 ++++- bcos-sdk/bcos-cpp-sdk/SdkFactory.cpp | 2 +- .../bcos-txpool/sync/TransactionSync.cpp | 14 +++-- .../bcos-txpool/sync/TransactionSync.h | 2 +- .../interfaces/TxPoolStorageInterface.h | 5 +- .../txpool/interfaces/TxValidatorInterface.h | 15 +++--- .../txpool/storage/MemoryStorage.cpp | 35 ++++++++---- .../txpool/storage/MemoryStorage.h | 5 +- .../txpool/validator/TxValidator.cpp | 27 ++++++---- .../txpool/validator/TxValidator.h | 14 ++++- 13 files changed, 141 insertions(+), 60 deletions(-) diff --git a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.cpp b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.cpp index 3f43b156f4..aa8d68c911 100644 --- a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.cpp +++ b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.cpp @@ -304,7 +304,11 @@ void PBFTCache::resetCache(ViewType _curView) m_precommitted = false; PBFT_LOG(INFO) << LOG_DESC("resetCache") << LOG_KV("precommit", m_precommit ? "true" : "false") << LOG_KV("prePrepare", m_prePrepare ? "true" : "false") - << LOG_KV("curView", _curView); + << LOG_KV("prepareView", m_prePrepare ? m_prePrepare->view() : 0) + << LOG_KV("curView", _curView) + << ((m_prePrepare && m_prePrepare->consensusProposal()) ? + printPBFTProposal(m_prePrepare->consensusProposal()) : + "consensusProposal is null"); if (!m_precommit && m_prePrepare && m_prePrepare->consensusProposal() && m_prePrepare->view() < _curView) { @@ -315,40 +319,54 @@ void PBFTCache::resetCache(ViewType _curView) // reset the exceptioned txs to unsealed m_config->validator()->asyncResetTxsFlag(m_prePrepare->consensusProposal()->data(), false); m_prePrepare = nullptr; - m_exceptionPrePrepare = nullptr; } - if (m_exceptionPrePrepare) + resetExceptionCache(_curView); + // clear the expired prepare cache + resetCacheAfterViewChange(m_prepareCacheList, _curView); + // clear the expired commit cache + resetCacheAfterViewChange(m_commitCacheList, _curView); + + // recalculate m_prepareReqWeight + recalculateQuorum(m_prepareReqWeight, m_prepareCacheList); + // recalculate m_commitReqWeight + recalculateQuorum(m_commitReqWeight, m_commitCacheList); +} + +void PBFTCache::resetExceptionCache(ViewType _curView) +{ + if (m_exceptionPrePrepareList.empty()) [[likely]] + { + return; + } + for (auto exceptionPrePrepare = m_exceptionPrePrepareList.begin(); + exceptionPrePrepare != m_exceptionPrePrepareList.end();) { auto validPrePrepare = (m_precommit || (m_prePrepare && m_prePrepare->consensusProposal() && m_prePrepare->view() >= _curView)); if (validPrePrepare && - (m_prePrepare && m_prePrepare->hash() == m_exceptionPrePrepare->hash())) + (m_prePrepare && m_prePrepare->hash() == (*exceptionPrePrepare)->hash())) { if (c_fileLogLevel == TRACE) [[unlikely]] { PBFT_LOG(TRACE) << LOG_DESC("resetCache : exceptionPrePrepare but finally be valid") - << printPBFTProposal(m_exceptionPrePrepare->consensusProposal()); + << printPBFTProposal((*exceptionPrePrepare)->consensusProposal()); } } else { PBFT_LOG(INFO) << LOG_DESC("resetCache : asyncResetTxsFlag exceptionPrePrepare") - << printPBFTProposal(m_exceptionPrePrepare->consensusProposal()); + << printPBFTProposal((*exceptionPrePrepare)->consensusProposal()); m_config->validator()->asyncResetTxsFlag( - m_exceptionPrePrepare->consensusProposal()->data(), false); - m_prePrepare = nullptr; - m_exceptionPrePrepare = nullptr; + (*exceptionPrePrepare)->consensusProposal()->data(), false); + exceptionPrePrepare = m_exceptionPrePrepareList.erase(exceptionPrePrepare); + if (exceptionPrePrepare == m_exceptionPrePrepareList.end()) + { + m_prePrepare = nullptr; + break; + } } + exceptionPrePrepare++; } - // clear the expired prepare cache - resetCacheAfterViewChange(m_prepareCacheList, _curView); - // clear the expired commit cache - resetCacheAfterViewChange(m_commitCacheList, _curView); - - // recalculate m_prepareReqWeight - recalculateQuorum(m_prepareReqWeight, m_prepareCacheList); - // recalculate m_commitReqWeight - recalculateQuorum(m_commitReqWeight, m_commitCacheList); } void PBFTCache::setCheckPointProposal(PBFTProposalInterface::Ptr _proposal) diff --git a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.h b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.h index 21c675bdfc..5bbd1faeb2 100644 --- a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.h +++ b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCache.h @@ -62,6 +62,14 @@ class PBFTCache : public std::enable_shared_from_this { return; } + // NOTE: If prePrepare is not null, it means prePrepare is already exist, and it will be + // replaced without reset proposal txs to unsealed. So we need to add the old prePrepare to + // exception prePrepare list, and it will be processed resetTxs when finalizeConsensus and + // reachNewView. + if (m_prePrepare && m_prePrepare->consensusProposal()) + { + m_exceptionPrePrepareList.push_back(m_prePrepare); + } m_prePrepare = _prePrepareMsg; PBFT_LOG(INFO) << LOG_DESC("addPrePrepareCache") << printPBFTMsgInfo(_prePrepareMsg) << LOG_KV("sys", _prePrepareMsg->consensusProposal()->systemProposal()) @@ -70,7 +78,7 @@ class PBFTCache : public std::enable_shared_from_this void addExceptionPrePrepareCache(PBFTMessageInterface::Ptr _prePrepareMsg) { - m_exceptionPrePrepare = std::move(_prePrepareMsg); + m_exceptionPrePrepareList.push_back(std::move(_prePrepareMsg)); } bcos::protocol::BlockNumber index() const { return m_index; } @@ -90,6 +98,8 @@ class PBFTCache : public std::enable_shared_from_this // reset the cache after viewchange virtual void resetCache(ViewType _curView); + virtual void resetExceptionCache(ViewType _curView); + virtual void setCheckPointProposal(PBFTProposalInterface::Ptr _proposal); PBFTProposalInterface::Ptr checkPointProposal() { return m_checkpointProposal; } @@ -215,7 +225,7 @@ class PBFTCache : public std::enable_shared_from_this QuorumRecoderType m_commitReqWeight; PBFTMessageInterface::Ptr m_prePrepare = nullptr; - PBFTMessageInterface::Ptr m_exceptionPrePrepare = nullptr; + std::vector m_exceptionPrePrepareList = {}; PBFTMessageInterface::Ptr m_precommit = nullptr; PBFTMessageInterface::Ptr m_precommitWithoutData = nullptr; diff --git a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCacheProcessor.cpp b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCacheProcessor.cpp index 741e78d2cc..e9bd553de2 100644 --- a/bcos-pbft/bcos-pbft/pbft/cache/PBFTCacheProcessor.cpp +++ b/bcos-pbft/bcos-pbft/pbft/cache/PBFTCacheProcessor.cpp @@ -854,6 +854,7 @@ void PBFTCacheProcessor::removeConsensusedCache( if (pcache->first <= _consensusedNumber) { m_proposalsToStableConsensus.erase(pcache->first); + pcache->second->resetExceptionCache(_view); pcache = m_caches.erase(pcache); eraseSize++; continue; diff --git a/bcos-pbft/bcos-pbft/pbft/engine/PBFTEngine.cpp b/bcos-pbft/bcos-pbft/pbft/engine/PBFTEngine.cpp index ae9b270404..be258a3e71 100644 --- a/bcos-pbft/bcos-pbft/pbft/engine/PBFTEngine.cpp +++ b/bcos-pbft/bcos-pbft/pbft/engine/PBFTEngine.cpp @@ -431,6 +431,8 @@ void PBFTEngine::onRecvProposal(bool _containSysTxs, bytesConstRef _proposalData } else { + PBFT_LOG(INFO) << LOG_DESC("handlePrePrepareMsg failed") << printPBFTProposal(pbftMessage) + << LOG_KV("costT(ms)", utcSteadyTime() - beginHandleT); resetSealedTxs(pbftMessage); } } @@ -711,7 +713,13 @@ CheckResult PBFTEngine::checkPBFTMsgState(PBFTMessageInterface::Ptr _pbftReq) co << LOG_KV("proposalCommitted", proposalCommitted); return CheckResult::INVALID; } - // case index equal + // Note: Accept pbft message with larger view then local view, for other nodes may viewchange to + // a larger view, and the node-self is not aware of the viewchange. + // In normal case, it will not happen, node-self will recover the view from the viewchange, and + // soon will reach to new view. + // BUT in the Byzantium case, malicious node will send the pre-prepare message with a larger + // view, to lay down some specific txs. + // FIXME: to check this logic. if (_pbftReq->view() < m_config->view()) { PBFT_LOG(DEBUG) << LOG_DESC("checkPBFTMsgState: invalid pbftMsg for invalid view") @@ -890,6 +898,9 @@ bool PBFTEngine::handlePrePrepareMsg(PBFTMessageInterface::Ptr _prePrepareMsg, auto result = checkPrePrepareMsg(_prePrepareMsg); if (result == CheckResult::INVALID) { + PBFT_LOG(INFO) << LOG_DESC("handlePrePrepareMsg checkPrePrepareMsg failed") + << printPBFTMsgInfo(_prePrepareMsg) << m_config->printCurrentState() + << LOG_KV("utc", utcSteadyTime()); return false; } if (!_generatedFromNewView) diff --git a/bcos-sdk/bcos-cpp-sdk/SdkFactory.cpp b/bcos-sdk/bcos-cpp-sdk/SdkFactory.cpp index 7a260b28cb..72356a6914 100644 --- a/bcos-sdk/bcos-cpp-sdk/SdkFactory.cpp +++ b/bcos-sdk/bcos-cpp-sdk/SdkFactory.cpp @@ -136,7 +136,7 @@ bcos::cppsdk::jsonrpc::JsonRpcImpl::Ptr SdkFactory::buildJsonRpc( msg->setPayload(data); _service->asyncSendMessageByGroupAndNode(_group, _node, msg, Options(), - [_respFunc](auto&& _error, auto&& _msg, auto&& _session) { + [_respFunc](Error::Ptr _error, MessageFace::Ptr _msg, auto&& _session) { (void)_session; _respFunc(_error, _msg ? _msg->payload() : nullptr); }); diff --git a/bcos-txpool/bcos-txpool/sync/TransactionSync.cpp b/bcos-txpool/bcos-txpool/sync/TransactionSync.cpp index d66c63a62a..780d59465d 100644 --- a/bcos-txpool/bcos-txpool/sync/TransactionSync.cpp +++ b/bcos-txpool/bcos-txpool/sync/TransactionSync.cpp @@ -155,6 +155,7 @@ void TransactionSync::requestMissedTxs(PublicPtr _generatedNodeID, HashListPtr _ { return; } + // if _generatedNodeID == nullptr, then it means request txs in schduler if (!_generatedNodeID || _generatedNodeID->data() == txsSync->m_config->nodeID()->data()) { @@ -264,6 +265,7 @@ void TransactionSync::requestMissedTxsFromPeer(PublicPtr _generatedNodeID, HashL } auto networkT = utcTime() - startT; auto recordT = utcTime(); + // verify fetch txs response transactionSync->verifyFetchedTxs(_error, _nodeID, _data, _missedTxs, _verifiedProposal, [networkT, recordT, proposalHeader, _onVerifyFinished]( @@ -353,10 +355,10 @@ void TransactionSync::verifyFetchedTxs(Error::Ptr _error, NodeIDPtr _nodeID, byt _onVerifyFinished( BCOS_ERROR_PTR(CommonError::TransactionsMissing, "TransactionsMissing"), false); // try to import the transactions even when verify failed - importDownloadedTxs(transactions); + importDownloadedTxsByBlock(transactions); return; } - if (!importDownloadedTxs(transactions, _verifiedProposal)) + if (!importDownloadedTxsByBlock(transactions, _verifiedProposal)) { _onVerifyFinished(BCOS_ERROR_PTR(CommonError::TxsSignatureVerifyFailed, "invalid transaction for invalid signature or nonce or blockLimit"), @@ -383,9 +385,11 @@ void TransactionSync::verifyFetchedTxs(Error::Ptr _error, NodeIDPtr _nodeID, byt << LOG_KV("timecost", (utcTime() - recordT)); } -bool TransactionSync::importDownloadedTxs(Block::Ptr _txsBuffer, Block::Ptr _verifiedProposal) +bool TransactionSync::importDownloadedTxsByBlock( + Block::Ptr _txsBuffer, Block::Ptr _verifiedProposal) { auto txs = std::make_shared(); + txs->reserve(_txsBuffer->transactionsSize()); for (size_t i = 0; i < _txsBuffer->transactionsSize(); i++) { txs->emplace_back(std::const_pointer_cast(_txsBuffer->transaction(i))); @@ -403,6 +407,7 @@ bool TransactionSync::importDownloadedTxs(TransactionsPtr _txs, Block::Ptr _veri // Note: only need verify the signature for the transactions bool enforceImport = false; BlockHeader::Ptr proposalHeader = nullptr; + // if _verifiedProposal is null, it means import txs from ledger if (_verifiedProposal && _verifiedProposal->blockHeader()) { proposalHeader = _verifiedProposal->blockHeader(); @@ -410,7 +415,7 @@ bool TransactionSync::importDownloadedTxs(TransactionsPtr _txs, Block::Ptr _veri } auto recordT = utcTime(); auto startT = utcTime(); - // verify the transactions + // verify the transactions signature std::atomic_bool verifySuccess = {true}; tbb::parallel_for(tbb::blocked_range(0, txsSize), [&_txs, &_verifiedProposal, &proposalHeader, this, &verifySuccess]( @@ -433,6 +438,7 @@ bool TransactionSync::importDownloadedTxs(TransactionsPtr _txs, Block::Ptr _veri } try { + // verify failed, it will throw exception tx->verify(*m_hashImpl, *m_signatureImpl); } catch (std::exception const& e) diff --git a/bcos-txpool/bcos-txpool/sync/TransactionSync.h b/bcos-txpool/bcos-txpool/sync/TransactionSync.h index a80fa7a20f..85a9074210 100644 --- a/bcos-txpool/bcos-txpool/sync/TransactionSync.h +++ b/bcos-txpool/bcos-txpool/sync/TransactionSync.h @@ -84,7 +84,7 @@ class TransactionSync : public TransactionSyncInterface, Error::Ptr _error, bcos::protocol::TransactionsPtr _fetchedTxs, bcos::protocol::Block::Ptr _verifiedProposal, VerifyResponseCallback _onVerifyFinished); - virtual bool importDownloadedTxs(bcos::protocol::Block::Ptr _txsBuffer, + virtual bool importDownloadedTxsByBlock(bcos::protocol::Block::Ptr _txsBuffer, bcos::protocol::Block::Ptr _verifiedProposal = nullptr); virtual bool importDownloadedTxs(bcos::protocol::TransactionsPtr _txs, diff --git a/bcos-txpool/bcos-txpool/txpool/interfaces/TxPoolStorageInterface.h b/bcos-txpool/bcos-txpool/txpool/interfaces/TxPoolStorageInterface.h index c7614540fd..a9866985a5 100644 --- a/bcos-txpool/bcos-txpool/txpool/interfaces/TxPoolStorageInterface.h +++ b/bcos-txpool/bcos-txpool/txpool/interfaces/TxPoolStorageInterface.h @@ -46,7 +46,10 @@ class TxPoolStorageInterface protocol::Transaction::Ptr transaction, std::function afterInsertHook) = 0; virtual bcos::protocol::TransactionStatus insert(bcos::protocol::Transaction::Ptr _tx) = 0; - virtual void batchInsert(bcos::protocol::Transactions const& _txs) = 0; + [[deprecated( + "do not use raw insert tx pool without check, use " + "batchVerifyAndSubmitTransaction")]] virtual void + batchInsert(bcos::protocol::Transactions const& _txs) = 0; virtual bcos::protocol::Transaction::Ptr remove(bcos::crypto::HashType const& _txHash) = 0; virtual bcos::protocol::Transaction::Ptr removeSubmittedTx( diff --git a/bcos-txpool/bcos-txpool/txpool/interfaces/TxValidatorInterface.h b/bcos-txpool/bcos-txpool/txpool/interfaces/TxValidatorInterface.h index 2bb05df9ca..924ae02221 100644 --- a/bcos-txpool/bcos-txpool/txpool/interfaces/TxValidatorInterface.h +++ b/bcos-txpool/bcos-txpool/txpool/interfaces/TxValidatorInterface.h @@ -20,6 +20,7 @@ */ #pragma once #include "bcos-txpool/txpool/interfaces/NonceCheckerInterface.h" +#include "bcos-txpool/txpool/validator/LedgerNonceChecker.h" #include #include namespace bcos::txpool @@ -32,15 +33,11 @@ class TxValidatorInterface virtual ~TxValidatorInterface() = default; virtual bcos::protocol::TransactionStatus verify(bcos::protocol::Transaction::ConstPtr _tx) = 0; - virtual bcos::protocol::TransactionStatus submittedToChain( + virtual bcos::protocol::TransactionStatus checkLedgerNonceAndBlockLimit( bcos::protocol::Transaction::ConstPtr _tx) = 0; - virtual NonceCheckerInterface::Ptr ledgerNonceChecker() { return m_ledgerNonceChecker; } - virtual void setLedgerNonceChecker(NonceCheckerInterface::Ptr _ledgerNonceChecker) - { - m_ledgerNonceChecker = std::move(_ledgerNonceChecker); - } - -protected: - NonceCheckerInterface::Ptr m_ledgerNonceChecker; + virtual bcos::protocol::TransactionStatus checkTxpoolNonce( + bcos::protocol::Transaction::ConstPtr _tx) = 0; + virtual LedgerNonceChecker::Ptr ledgerNonceChecker() = 0; + virtual void setLedgerNonceChecker(LedgerNonceChecker::Ptr _ledgerNonceChecker) = 0; }; } // namespace bcos::txpool \ No newline at end of file diff --git a/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.cpp b/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.cpp index 1be7e90215..1fa9d9a554 100644 --- a/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.cpp +++ b/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.cpp @@ -129,7 +129,10 @@ task::Task MemoryStorage::submitTransact if (result != TransactionStatus::None) { - TXPOOL_LOG(DEBUG) << "Submit transaction failed! " << result; + TXPOOL_LOG(DEBUG) + << "Submit transaction failed! " + << LOG_KV("TxHash", m_transaction ? m_transaction->hash().hex() : "") + << LOG_KV("result", result); m_submitResult.emplace( BCOS_ERROR_PTR((int32_t)result, bcos::protocol::toString(result))); handle.resume(); @@ -217,7 +220,13 @@ TransactionStatus MemoryStorage::enforceSubmitTransaction(Transaction::Ptr _tx) auto txHash = _tx->hash(); // the transaction has already onChain, reject it { - auto result = m_config->txValidator()->submittedToChain(_tx); + // check txpool nonce + // check ledger tx + auto result = m_config->txValidator()->checkTxpoolNonce(_tx); + if (result == TransactionStatus::None) + { + result = m_config->txValidator()->checkLedgerNonceAndBlockLimit(_tx); + } Transaction::ConstPtr tx = nullptr; { TxsMap::ReadAccessor::Ptr accessor; @@ -227,7 +236,7 @@ TransactionStatus MemoryStorage::enforceSubmitTransaction(Transaction::Ptr _tx) tx = accessor->value(); } } - if (result == TransactionStatus::NonceCheckFail) + if (result == TransactionStatus::NonceCheckFail) [[unlikely]] { if (tx) { @@ -240,7 +249,8 @@ TransactionStatus MemoryStorage::enforceSubmitTransaction(Transaction::Ptr _tx) return TransactionStatus::NonceCheckFail; } - if (tx) + // tx already in txpool + if (tx) [[likely]] { if (!tx->sealed() || tx->batchHash() == HashType()) { @@ -724,7 +734,8 @@ void MemoryStorage::batchFetchTxs(Block::Ptr _txsList, Block::Ptr _sysTxsList, s // since the invalid nonce has already been checked before the txs import into the // txPool, the txs with duplicated nonce here are already-committed, but have not been // dropped - auto result = m_config->txValidator()->submittedToChain(tx); + // check txpool txs, no need to check txpool nonce + auto result = m_config->txValidator()->checkLedgerNonceAndBlockLimit(tx); if (result == TransactionStatus::NonceCheckFail) { // in case of the same tx notified more than once @@ -1157,7 +1168,7 @@ std::shared_ptr MemoryStorage::batchVerifyProposal(Block::Ptr _block) bool MemoryStorage::batchVerifyProposal(std::shared_ptr _txsHashList) { - bool has = true; + bool has = false; m_txsTable.batchFind( *_txsHashList, [&has](auto const& txHash, TxsMap::ReadAccessor::Ptr accessor) { has = (accessor != nullptr); @@ -1178,7 +1189,8 @@ HashListPtr MemoryStorage::getTxsHash(int _limit) { return true; } - auto result = m_config->txValidator()->submittedToChain(tx); + // check txpool txs, no need to check txpool nonce + auto result = m_config->txValidator()->checkLedgerNonceAndBlockLimit(tx); if (result != TransactionStatus::None) { TxsMap::WriteAccessor::Ptr writeAccessor; @@ -1258,7 +1270,8 @@ void MemoryStorage::cleanUpExpiredTransactions() return true; } } - auto result = m_config->txValidator()->submittedToChain(tx); + // check txpool txs, no need to check txpool nonce + auto result = m_config->txValidator()->checkLedgerNonceAndBlockLimit(tx); // blockLimit expired if (result != TransactionStatus::None) { @@ -1298,7 +1311,7 @@ void MemoryStorage::batchImportTxs(TransactionsPtr _txs) { continue; } - // not checkLimit when receive txs from p2p + // will check ledger nonce, txpool nonce and blockLimit when import txs auto ret = verifyAndSubmitTransaction(tx, nullptr, false, false); if (ret != TransactionStatus::None) { @@ -1338,8 +1351,8 @@ bool MemoryStorage::batchVerifyAndSubmitTransaction( << LOG_KV("tx", tx->hash().abridged()) << LOG_KV("result", result) << LOG_KV("txBatchID", tx->batchId()) << LOG_KV("txBatchHash", tx->batchHash().abridged()) - << LOG_KV("consIndex", _header->number()) - << LOG_KV("propHash", _header->hash().abridged()); + << LOG_KV("consIndex", _header ? _header->number() : -1) + << LOG_KV("propHash", _header ? _header->hash().abridged() : ""); return false; } } diff --git a/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.h b/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.h index 94a8973343..c6bdcb928b 100644 --- a/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.h +++ b/bcos-txpool/bcos-txpool/txpool/storage/MemoryStorage.h @@ -75,7 +75,10 @@ class MemoryStorage : public TxPoolStorageInterface, std::function afterInsertHook = nullptr) override; bcos::protocol::TransactionStatus insert(bcos::protocol::Transaction::Ptr transaction) override; - void batchInsert(bcos::protocol::Transactions const& _txs) override; + + [[deprecated( + "do not use raw insert tx pool without check, use batchVerifyAndSubmitTransaction")]] void + batchInsert(bcos::protocol::Transactions const& _txs) override; bcos::protocol::Transaction::Ptr remove(bcos::crypto::HashType const& _txHash) override; // invoke when scheduler finished block executed and notify txpool new block result diff --git a/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.cpp b/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.cpp index f1b465b2bc..1a8cd1f246 100644 --- a/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.cpp +++ b/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.cpp @@ -26,26 +26,27 @@ using namespace bcos::txpool; TransactionStatus TxValidator::verify(bcos::protocol::Transaction::ConstPtr _tx) { - if (_tx->invalid()) + if (_tx->invalid()) [[unlikely]] { return TransactionStatus::InvalidSignature; } // check groupId and chainId - if (_tx->groupId() != m_groupId) + if (_tx->groupId() != m_groupId) [[unlikely]] { return TransactionStatus::InvalidGroupId; } - if (_tx->chainId() != m_chainId) + if (_tx->chainId() != m_chainId) [[unlikely]] { return TransactionStatus::InvalidChainId; } - // compare with nonces cached in memory - auto status = m_txPoolNonceChecker->checkNonce(_tx, false); + // compare with nonces cached in memory, only check nonce in txpool + auto status = checkTxpoolNonce(_tx); if (status != TransactionStatus::None) { return status; } - status = submittedToChain(_tx); + // check ledger nonce and block limit + status = checkLedgerNonceAndBlockLimit(_tx); if (status != TransactionStatus::None) { return status; @@ -55,7 +56,7 @@ TransactionStatus TxValidator::verify(bcos::protocol::Transaction::ConstPtr _tx) { _tx->verify(*m_cryptoSuite->hashImpl(), *m_cryptoSuite->signatureImpl()); } - catch (std::exception const& e) + catch (...) { return TransactionStatus::InvalidSignature; } @@ -68,9 +69,10 @@ TransactionStatus TxValidator::verify(bcos::protocol::Transaction::ConstPtr _tx) return TransactionStatus::None; } -TransactionStatus TxValidator::submittedToChain(bcos::protocol::Transaction::ConstPtr _tx) +TransactionStatus TxValidator::checkLedgerNonceAndBlockLimit( + bcos::protocol::Transaction::ConstPtr _tx) { - // compare with nonces stored on-chain + // compare with nonces stored on-chain, and check block limit inside auto status = m_ledgerNonceChecker->checkNonce(_tx); if (status != TransactionStatus::None) { @@ -81,4 +83,9 @@ TransactionStatus TxValidator::submittedToChain(bcos::protocol::Transaction::Con _tx->setSystemTx(true); } return TransactionStatus::None; -} \ No newline at end of file +} + +TransactionStatus TxValidator::checkTxpoolNonce(bcos::protocol::Transaction::ConstPtr _tx) +{ + return m_txPoolNonceChecker->checkNonce(_tx, false); +} diff --git a/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.h b/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.h index 170da57c0c..722e3f5544 100644 --- a/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.h +++ b/bcos-txpool/bcos-txpool/txpool/validator/TxValidator.h @@ -42,8 +42,16 @@ class TxValidator : public TxValidatorInterface ~TxValidator() override = default; bcos::protocol::TransactionStatus verify(bcos::protocol::Transaction::ConstPtr _tx) override; - bcos::protocol::TransactionStatus submittedToChain( + bcos::protocol::TransactionStatus checkLedgerNonceAndBlockLimit( bcos::protocol::Transaction::ConstPtr _tx) override; + bcos::protocol::TransactionStatus checkTxpoolNonce( + bcos::protocol::Transaction::ConstPtr _tx) override; + + LedgerNonceChecker::Ptr ledgerNonceChecker() override { return m_ledgerNonceChecker; } + void setLedgerNonceChecker(LedgerNonceChecker::Ptr _ledgerNonceChecker) override + { + m_ledgerNonceChecker = std::move(_ledgerNonceChecker); + } protected: virtual inline bool isSystemTransaction(bcos::protocol::Transaction::ConstPtr const& _tx) @@ -52,7 +60,11 @@ class TxValidator : public TxValidatorInterface } private: + // check the transaction nonce in txpool NonceCheckerInterface::Ptr m_txPoolNonceChecker; + // check the transaction nonce in ledger, maintenance block number to nonce list mapping, and + // nonce list which already committed to ledger + LedgerNonceChecker::Ptr m_ledgerNonceChecker; bcos::crypto::CryptoSuite::Ptr m_cryptoSuite; std::string m_groupId; std::string m_chainId;