diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp index 5afb9478f0..d9291b1c11 100644 --- a/src/catchup/ApplyCheckpointWork.cpp +++ b/src/catchup/ApplyCheckpointWork.cpp @@ -98,13 +98,14 @@ ApplyCheckpointWork::openInputFiles() mHeaderHistoryEntry = LedgerHeaderHistoryEntry(); if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) { - mTxResultIn.close(); + mTxResultIn = std::make_optional(); FileTransferInfo tri(mDownloadDir, FileType::HISTORY_FILE_TYPE_RESULTS, mCheckpoint); CLOG_DEBUG(History, "Replaying transaction results from {}", tri.localPath_nogz()); - mTxResultIn.open(tri.localPath_nogz()); - mTxHistoryResultEntry = TransactionHistoryResultEntry{}; + mTxResultIn->open(tri.localPath_nogz()); + mTxHistoryResultEntry = + std::make_optional(); } mFilesOpen = true; } @@ -157,29 +158,33 @@ ApplyCheckpointWork::getCurrentTxResultSet() ZoneScoped; auto& lm = mApp.getLedgerManager(); auto seq = lm.getLastClosedLedgerNum() + 1; - // Check mTxResultSet prior to loading next result set. // This order is important because it accounts for ledger "gaps" // in the history archives (which are caused by ledgers with empty tx // sets, as those are not uploaded). - do + while (mTxResultIn) { - if (mTxHistoryResultEntry.ledgerSeq < seq) - { - CLOG_DEBUG(History, "Advancing past txresultset for ledger {}", - mTxHistoryResultEntry.ledgerSeq); - } - else if (mTxHistoryResultEntry.ledgerSeq > seq) - { - break; - } - else + if (mTxHistoryResultEntry) + { - releaseAssert(mTxHistoryResultEntry.ledgerSeq == seq); - CLOG_DEBUG(History, "Loaded txresultset for ledger {}", seq); - return std::make_optional(mTxHistoryResultEntry.txResultSet); + if (mTxHistoryResultEntry->ledgerSeq < seq) + { + CLOG_DEBUG(History, "Advancing past txresultset for ledger {}", + mTxHistoryResultEntry->ledgerSeq); + } + else if (mTxHistoryResultEntry->ledgerSeq > seq) + { + break; + } + else + { + releaseAssert(mTxHistoryResultEntry->ledgerSeq == seq); + CLOG_DEBUG(History, "Loaded txresultset for ledger {}", seq); + return std::make_optional(mTxHistoryResultEntry->txResultSet); + } } - } while (mTxResultIn && mTxResultIn.readOne(mTxHistoryResultEntry)); + mTxResultIn->readOne(*mTxHistoryResultEntry); + } CLOG_DEBUG(History, "No txresultset for ledger {}", seq); return std::nullopt; } diff --git a/src/catchup/ApplyCheckpointWork.h b/src/catchup/ApplyCheckpointWork.h index 4e15055215..abf2f6780f 100644 --- a/src/catchup/ApplyCheckpointWork.h +++ b/src/catchup/ApplyCheckpointWork.h @@ -52,9 +52,9 @@ class ApplyCheckpointWork : public BasicWork XDRInputFileStream mHdrIn; XDRInputFileStream mTxIn; - XDRInputFileStream mTxResultIn; + std::optional mTxResultIn; TransactionHistoryEntry mTxHistoryEntry; - TransactionHistoryResultEntry mTxHistoryResultEntry; + std::optional mTxHistoryResultEntry; LedgerHeaderHistoryEntry mHeaderHistoryEntry; OnFailureCallback mOnFailure; diff --git a/src/catchup/CatchupConfiguration.h b/src/catchup/CatchupConfiguration.h index 3008d64000..b48ad93669 100644 --- a/src/catchup/CatchupConfiguration.h +++ b/src/catchup/CatchupConfiguration.h @@ -32,7 +32,7 @@ namespace stellar // doing offline commandline catchups with stellar-core catchup command. // // Catchup can be done in two modes - ONLINE and OFFLINE. In ONLINE mode, the -// node is connected to the network. If receives ledgers during catchup and +// node is connected to the network. It receives ledgers during catchup and // applies them after history is applied. Also, an additional closing ledger is // required to mark catchup as complete and the node as synced. In OFFLINE mode, // the node is not connected to network, so new ledgers are not being diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index 37b0b5e8db..e18b829683 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -43,40 +43,59 @@ DownloadApplyTxsWork::yieldMoreWork() { throw std::runtime_error("Work has no more children to iterate over!"); } - std::vector fileTypesToDownload{ - FileType::HISTORY_FILE_TYPE_TRANSACTIONS}; - std::vector> downloadSeq; - std::vector filesToTransfer; - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) - { - fileTypesToDownload.emplace_back(FileType::HISTORY_FILE_TYPE_RESULTS); - } - for (auto const& fileType : fileTypesToDownload) - { - CLOG_INFO(History, - "Downloading, unzipping and applying {} for checkpoint {}", - typeString(fileType), mCheckpointToQueue); - FileTransferInfo ft(mDownloadDir, fileType, mCheckpointToQueue); - filesToTransfer.emplace_back(ft); - downloadSeq.emplace_back( - std::make_shared(mApp, ft, mArchive)); - } - - OnFailureCallback cb = [archive = mArchive, filesToTransfer]() { - for (auto const& ft : filesToTransfer) - { - CLOG_ERROR(History, "Archive {} maybe contains corrupt file {}", - archive->getName(), ft.remoteName()); - } - }; + CLOG_INFO(History, + "Downloading, unzipping and applying {} for checkpoint {}", + typeString(FileType::HISTORY_FILE_TYPE_TRANSACTIONS), + mCheckpointToQueue); + FileTransferInfo ft(mDownloadDir, FileType::HISTORY_FILE_TYPE_TRANSACTIONS, + mCheckpointToQueue); + auto getAndUnzip = + std::make_shared(mApp, ft, mArchive); auto const& hm = mApp.getHistoryManager(); auto low = hm.firstLedgerInCheckpointContaining(mCheckpointToQueue); auto high = std::min(mCheckpointToQueue, mRange.last()); + TmpDir const& dir = mDownloadDir; + uint32_t checkpoint = mCheckpointToQueue; + auto getFileWeak = std::weak_ptr(getAndUnzip); + + OnFailureCallback cb = [getFileWeak, checkpoint, &dir]() { + auto getFile = getFileWeak.lock(); + if (getFile) + { + auto archive = getFile->getArchive(); + if (archive) + { + FileTransferInfo ti( + dir, FileType::HISTORY_FILE_TYPE_TRANSACTIONS, checkpoint); + CLOG_ERROR(History, "Archive {} maybe contains corrupt file {}", + archive->getName(), ti.remoteName()); + } + } + }; + auto apply = std::make_shared( mApp, mDownloadDir, LedgerRange::inclusive(low, high), cb); + std::vector> seq{getAndUnzip}; + std::vector filesToTransfer{ft}; + + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + { + CLOG_INFO(History, + "Downloading, unzipping and applying {} for checkpoint {}", + typeString(FileType::HISTORY_FILE_TYPE_RESULTS), + mCheckpointToQueue); + + FileTransferInfo resultsFile(mDownloadDir, + FileType::HISTORY_FILE_TYPE_RESULTS, + mCheckpointToQueue); + seq.emplace_back(std::make_shared( + mApp, resultsFile, mArchive)); + filesToTransfer.push_back(resultsFile); + } + auto maybeWaitForMerges = [](Application& app) { if (app.getConfig().CATCHUP_WAIT_MERGES_TX_APPLY_FOR_TESTING) { @@ -94,10 +113,8 @@ DownloadApplyTxsWork::yieldMoreWork() { auto prev = mLastYieldedWork; bool pqFellBehind = false; - auto applyName = apply->getName(); auto predicate = [prev, pqFellBehind, waitForPublish = mWaitForPublish, - maybeWaitForMerges, - applyName](Application& app) mutable { + maybeWaitForMerges](Application& app) mutable { if (!prev) { throw std::runtime_error("Download and apply txs: related Work " @@ -128,33 +145,39 @@ DownloadApplyTxsWork::yieldMoreWork() } return res && maybeWaitForMerges(app); }; - downloadSeq.push_back(std::make_shared( + seq.push_back(std::make_shared( mApp, "conditional-" + apply->getName(), predicate, apply)); } else { - downloadSeq.push_back(std::make_shared( + seq.push_back(std::make_shared( mApp, "wait-merges" + apply->getName(), maybeWaitForMerges, apply)); } - downloadSeq.push_back(std::make_shared( - mApp, "delete-transactions-" + std::to_string(mCheckpointToQueue), + seq.push_back(std::make_shared( + mApp, + "delete-transactions-" + + (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS + ? std::string("and-results-") + : "") + + std::to_string(mCheckpointToQueue), [filesToTransfer](Application& app) { for (auto const& ft : filesToTransfer) { - CLOG_DEBUG(History, "Deleting transactions {}", + CLOG_DEBUG(History, "Deleting {} {}", ft.getTypeString(), ft.localPath_nogz()); try { std::filesystem::remove( std::filesystem::path(ft.localPath_nogz())); - CLOG_DEBUG(History, "Deleted transactions {}", + CLOG_DEBUG(History, "Deleted {} {}", ft.getTypeString(), ft.localPath_nogz()); } catch (std::filesystem::filesystem_error const& e) { - CLOG_ERROR(History, "Could not delete transactions {}: {}", - ft.localPath_nogz(), e.what()); + CLOG_ERROR(History, "Could not delete {} {}: {}", + ft.getTypeString(), ft.localPath_nogz(), + e.what()); return false; } } @@ -162,8 +185,8 @@ DownloadApplyTxsWork::yieldMoreWork() })); auto nextWork = std::make_shared( - mApp, "download-apply-" + std::to_string(mCheckpointToQueue), - downloadSeq, BasicWork::RETRY_NEVER); + mApp, "download-apply-" + std::to_string(mCheckpointToQueue), seq, + BasicWork::RETRY_NEVER); mCheckpointToQueue += mApp.getHistoryManager().getCheckpointFrequency(); mLastYieldedWork = nextWork; return nextWork; diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 3d08008e5b..efd12ebc61 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -915,13 +915,14 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData) // first, prefetch source accounts for txset, then charge fees prefetchTxSourceIds(txs); auto const mutableTxResults = - processFeesSeqNums(txs, ltx, *applicableTxSet, ledgerCloseMeta); + processFeesSeqNums(txs, ltx, *applicableTxSet, ledgerCloseMeta, + ledgerData.getExpectedResults()); TransactionResultSet txResultSet; txResultSet.results.reserve(txs.size()); // Subtle: after this call, `header` is invalidated, and is not safe to use applyTransactions(*applicableTxSet, txs, mutableTxResults, ltx, txResultSet, - ledgerData.getExpectedResults(), ledgerCloseMeta); + ledgerCloseMeta); if (mApp.getConfig().MODE_STORES_HISTORY_MISC) { auto ledgerSeq = ltx.loadHeader().current().ledgerSeq; @@ -1348,7 +1349,8 @@ std::vector LedgerManagerImpl::processFeesSeqNums( std::vector const& txs, AbstractLedgerTxn& ltxOuter, ApplicableTxSetFrame const& txSet, - std::unique_ptr const& ledgerCloseMeta) + std::unique_ptr const& ledgerCloseMeta, + std::optional const& expectedResults) { ZoneScoped; std::vector txResults; @@ -1361,6 +1363,16 @@ LedgerManagerImpl::processFeesSeqNums( auto header = ltx.loadHeader().current(); std::map accToMaxSeq; + // If we have expected results, we assign them to the mutable tx results + // here. + std::optional::const_iterator> + expectedResultsIter = std::nullopt; + if (expectedResults) + { + expectedResultsIter = + std::make_optional(expectedResults->results.begin()); + } + bool mergeSeen = false; for (auto tx : txs) { @@ -1368,6 +1380,30 @@ LedgerManagerImpl::processFeesSeqNums( txResults.push_back( tx->processFeeSeqNum(ltxTx, txSet.getTxBaseFee(tx, header))); + if (expectedResultsIter) + { + releaseAssert(*expectedResultsIter != + expectedResults->results.end()); + releaseAssert((*expectedResultsIter)->transactionHash == + tx->getContentsHash()); + if ((*expectedResultsIter)->result.result.code() == + TransactionResultCode::txSUCCESS) + { + CLOG_DEBUG( + Tx, "Skipping replay of successful transaction: tx {}", + binToHex(tx->getContentsHash())); + txResults.back()->setReplaySuccessfulTransactionResult( + (*expectedResultsIter)->result); + } + else + { + CLOG_DEBUG(Tx, "Replaying failing transaction: tx {}", + binToHex(tx->getContentsHash())); + txResults.back()->setReplayFailingTransactionResult( + (*expectedResultsIter)->result); + } + ++(*expectedResultsIter); + } if (protocolVersionStartsFrom( ltxTx.loadHeader().current().ledgerVersion, @@ -1499,7 +1535,6 @@ LedgerManagerImpl::applyTransactions( std::vector const& txs, std::vector const& mutableTxResults, AbstractLedgerTxn& ltx, TransactionResultSet& txResultSet, - std::optional const& expectedResults, std::unique_ptr const& ledgerCloseMeta) { ZoneNamedN(txsZone, "applyTransactions", true); @@ -1522,17 +1557,6 @@ LedgerManagerImpl::applyTransactions( prefetchTransactionData(txs); - std::optional::const_iterator> - expectedResultsIter = std::nullopt; - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS && expectedResults) - { - auto const& resVec = expectedResults->results; - CLOG_DEBUG(Tx, "Will skip replaying known results: {} txs, {} results", - txs.size(), resVec.size()); - releaseAssertOrThrow(txs.size() == resVec.size()); - expectedResultsIter = std::make_optional(resVec.begin()); - } - Hash sorobanBasePrngSeed = txSet.getContentsHash(); uint64_t txNum{0}; uint64_t txSucceeded{0}; @@ -1566,37 +1590,6 @@ LedgerManagerImpl::applyTransactions( TransactionResultPair results; results.transactionHash = tx->getContentsHash(); - if (expectedResultsIter) - { - releaseAssert(*expectedResultsIter != - expectedResults->results.end()); - while ((*expectedResultsIter)->transactionHash != - results.transactionHash) - { - (*expectedResultsIter)++; - releaseAssert(*expectedResultsIter != - expectedResults->results.end()); - } - releaseAssert((*expectedResultsIter)->transactionHash == - results.transactionHash); - if ((*expectedResultsIter)->result.result.code() == - TransactionResultCode::txSUCCESS) - { - CLOG_DEBUG(Tx, - "Skipping signature verification for known " - "successful tx#{}", - index); - tx->setReplaySuccessfulTransactionResult( - (*expectedResultsIter)->result); - } - else - { - CLOG_DEBUG(Tx, "Skipping replay of known failed tx#{}", index); - tx->setReplayFailingTransactionResult( - (*expectedResultsIter)->result); - } - } - tx->apply(mApp.getAppConnector(), ltx, tm, mutableTxResult, subSeed); tx->processPostApply(mApp.getAppConnector(), ltx, tm, mutableTxResult); diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index d3b4eca48d..87fde6e551 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -77,14 +77,14 @@ class LedgerManagerImpl : public LedgerManager std::vector processFeesSeqNums( std::vector const& txs, AbstractLedgerTxn& ltxOuter, ApplicableTxSetFrame const& txSet, - std::unique_ptr const& ledgerCloseMeta); + std::unique_ptr const& ledgerCloseMeta, + std::optional const& expectedResults); void applyTransactions( ApplicableTxSetFrame const& txSet, std::vector const& txs, std::vector const& mutableTxResults, AbstractLedgerTxn& ltx, TransactionResultSet& txResultSet, - std::optional const& expectedResults, std::unique_ptr const& ledgerCloseMeta); // initialLedgerVers must be the ledger version at the start of the ledger. diff --git a/src/rust/soroban/p22 b/src/rust/soroban/p22 index 1cd8b8dca9..8911531dfb 160000 --- a/src/rust/soroban/p22 +++ b/src/rust/soroban/p22 @@ -1 +1 @@ -Subproject commit 1cd8b8dca9aeeca9ce45b129cd923992b32dc258 +Subproject commit 8911531dfb9a4331bc308ff8934c2223bc4e81ed diff --git a/src/test/FuzzerImpl.cpp b/src/test/FuzzerImpl.cpp index b60edf3cc3..02123c4d8b 100644 --- a/src/test/FuzzerImpl.cpp +++ b/src/test/FuzzerImpl.cpp @@ -921,14 +921,13 @@ class FuzzTransactionFrame : public TransactionFrame // attempt application of transaction without processing the fee or // committing the LedgerTxn - auto sc = - SignatureCheckerImpl{ltx.loadHeader().current().ledgerVersion, - getContentsHash(), mEnvelope.v1().signatures}; - SignatureChecker& signatureChecker = sc; + auto signatureChecker = std::make_unique( + ltx.loadHeader().current().ledgerVersion, getContentsHash(), + mEnvelope.v1().signatures); LedgerSnapshot ltxStmt(ltx); // if any ill-formed Operations, do not attempt transaction application auto isInvalidOperation = [&](auto const& op, auto& opResult) { - return !op->checkValid(app.getAppConnector(), signatureChecker, + return !op->checkValid(app.getAppConnector(), *signatureChecker, ltxStmt, false, opResult, mTxResult->getSorobanData()); }; @@ -951,7 +950,7 @@ class FuzzTransactionFrame : public TransactionFrame loadSourceAccount(ltx, ltx.loadHeader()); processSeqNum(ltx); TransactionMetaFrame tm(2); - applyOperations(signatureChecker, app.getAppConnector(), ltx, tm, + applyOperations(*signatureChecker, app.getAppConnector(), ltx, tm, *mTxResult, Hash{}); if (mTxResult->getResultCode() == txINTERNAL_ERROR) { diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp index eb24128b43..a0883bef6a 100644 --- a/src/transactions/FeeBumpTransactionFrame.cpp +++ b/src/transactions/FeeBumpTransactionFrame.cpp @@ -136,9 +136,9 @@ FeeBumpTransactionFrame::processPostApply(AppConnector& app, } bool -FeeBumpTransactionFrame::checkSignature(SignatureChecker& signatureChecker, - LedgerEntryWrapper const& account, - int32_t neededWeight) const +FeeBumpTransactionFrame::checkSignature( + AbstractSignatureChecker& signatureChecker, + LedgerEntryWrapper const& account, int32_t neededWeight) const { auto& acc = account.current().data.account(); std::vector signers; @@ -170,17 +170,16 @@ FeeBumpTransactionFrame::checkValid(AppConnector& app, LedgerSnapshot const& ls, auto txResult = createSuccessResultWithFeeCharged( ls.getLedgerHeader().current(), minBaseFee, false); - auto sc = - SignatureCheckerImpl{ls.getLedgerHeader().current().ledgerVersion, - getContentsHash(), mEnvelope.feeBump().signatures}; - SignatureChecker& signatureChecker = sc; - if (commonValid(signatureChecker, ls, false, *txResult) != + auto signatureChecker = std::make_unique( + ls.getLedgerHeader().current().ledgerVersion, getContentsHash(), + mEnvelope.feeBump().signatures); + if (commonValid(*signatureChecker, ls, false, *txResult) != ValidationType::kFullyValid) { return txResult; } - if (!signatureChecker.checkAllSignaturesUsed()) + if (!signatureChecker->checkAllSignaturesUsed()) { txResult->setResultCode(txBAD_AUTH_EXTRA); return txResult; @@ -266,8 +265,8 @@ FeeBumpTransactionFrame::commonValidPreSeqNum( FeeBumpTransactionFrame::ValidationType FeeBumpTransactionFrame::commonValid( - SignatureChecker& signatureChecker, LedgerSnapshot const& ls, bool applying, - MutableTransactionResultBase& txResult) const + AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, + bool applying, MutableTransactionResultBase& txResult) const { ValidationType res = ValidationType::kInvalid; @@ -576,16 +575,4 @@ FeeBumpTransactionFrame::toStellarMessage() const msg->transaction() = mEnvelope; return msg; } - -void -FeeBumpTransactionFrame::setReplayFailingTransactionResult( - TransactionResult const&) const -{ /* NO OP*/ -} -void -FeeBumpTransactionFrame::setReplaySuccessfulTransactionResult( - TransactionResult const&) const -{ /* NO OP*/ -} - } diff --git a/src/transactions/FeeBumpTransactionFrame.h b/src/transactions/FeeBumpTransactionFrame.h index d31f21dae9..85e97f42df 100644 --- a/src/transactions/FeeBumpTransactionFrame.h +++ b/src/transactions/FeeBumpTransactionFrame.h @@ -11,7 +11,7 @@ namespace stellar { class AbstractLedgerTxn; class Application; -class SignatureChecker; +class AbstractSignatureChecker; class FeeBumpTransactionFrame : public TransactionFrameBase { @@ -27,7 +27,7 @@ class FeeBumpTransactionFrame : public TransactionFrameBase mutable Hash mContentsHash; mutable Hash mFullHash; - bool checkSignature(SignatureChecker& signatureChecker, + bool checkSignature(AbstractSignatureChecker& signatureChecker, LedgerEntryWrapper const& account, int32_t neededWeight) const; @@ -42,7 +42,7 @@ class FeeBumpTransactionFrame : public TransactionFrameBase kFullyValid }; - ValidationType commonValid(SignatureChecker& signatureChecker, + ValidationType commonValid(AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, bool applying, MutableTransactionResultBase& txResult) const; @@ -139,10 +139,5 @@ class FeeBumpTransactionFrame : public TransactionFrameBase SorobanResources const& sorobanResources() const override; virtual int64 declaredSorobanResourceFee() const override; virtual bool XDRProvidesValidFee() const override; - - void setReplayFailingTransactionResult( - TransactionResult const& failing) const override; - void setReplaySuccessfulTransactionResult( - TransactionResult const& successful) const override; }; } diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 4dccb4da68..70279f0d50 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -314,6 +314,32 @@ MutableTransactionResult::isSuccess() const return getResult().result.code() == txSUCCESS; } +void +MutableTransactionResult::setReplaySuccessfulTransactionResult( + TransactionResult const& success) +{ + mReplaySuccessfulTransactionResult = std::make_optional(success); +} + +void +MutableTransactionResult::setReplayFailingTransactionResult( + TransactionResult const& failing) +{ + mReplayFailingTransactionResult = std::make_optional(failing); +} + +std::optional const& +MutableTransactionResult::getReplaySuccessfulTransactionResult() const +{ + return mReplaySuccessfulTransactionResult; +} + +std::optional const& +MutableTransactionResult::getReplayFailingTransactionResult() const +{ + return mReplayFailingTransactionResult; +} + FeeBumpMutableTransactionResult::FeeBumpMutableTransactionResult( MutableTxResultPtr innerTxResult) : MutableTransactionResultBase(), mInnerTxResult(innerTxResult) @@ -447,4 +473,33 @@ FeeBumpMutableTransactionResult::isSuccess() const { return mTxResult->result.code() == txFEE_BUMP_INNER_SUCCESS; } +void +FeeBumpMutableTransactionResult::setReplaySuccessfulTransactionResult( + TransactionResult const& successful) +{ + // no-op +} + +void +FeeBumpMutableTransactionResult::setReplayFailingTransactionResult( + TransactionResult const& failing) +{ + // no-op + // TODO maybe this should be no-op + mReplayFailingTransactionResult = std::make_optional(failing); +} + +std::optional const& +FeeBumpMutableTransactionResult::getReplaySuccessfulTransactionResult() const +{ + // no -op? + return mReplaySuccessfulTransactionResult; +} + +std::optional const& +FeeBumpMutableTransactionResult::getReplayFailingTransactionResult() const +{ + return mReplayFailingTransactionResult; +} + } \ No newline at end of file diff --git a/src/transactions/MutableTransactionResult.h b/src/transactions/MutableTransactionResult.h index 3527f20274..b76ceb6b9b 100644 --- a/src/transactions/MutableTransactionResult.h +++ b/src/transactions/MutableTransactionResult.h @@ -71,6 +71,10 @@ class MutableTransactionResultBase : public NonMovableOrCopyable { protected: std::unique_ptr mTxResult; + std::optional mReplaySuccessfulTransactionResult{ + std::nullopt}; + std::optional mReplayFailingTransactionResult{ + std::nullopt}; MutableTransactionResultBase(); MutableTransactionResultBase(MutableTransactionResultBase&& rhs); @@ -97,8 +101,15 @@ class MutableTransactionResultBase : public NonMovableOrCopyable virtual void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) = 0; - + virtual std::optional const& + getReplaySuccessfulTransactionResult() const = 0; + virtual std::optional const& + getReplayFailingTransactionResult() const = 0; virtual bool isSuccess() const = 0; + virtual void + setReplayFailingTransactionResult(TransactionResult const& failing) = 0; + virtual void + setReplaySuccessfulTransactionResult(TransactionResult const& success) = 0; }; class MutableTransactionResult : public MutableTransactionResultBase @@ -124,6 +135,14 @@ class MutableTransactionResult : public MutableTransactionResultBase TransactionResult const& getResult() const override; TransactionResultCode getResultCode() const override; void setResultCode(TransactionResultCode code) override; + void setReplayFailingTransactionResult( + TransactionResult const& failing) override; + void setReplaySuccessfulTransactionResult( + TransactionResult const& success) override; + std::optional const& + getReplaySuccessfulTransactionResult() const override; + std::optional const& + getReplayFailingTransactionResult() const override; OperationResult& getOpResultAt(size_t index) override; std::shared_ptr getSorobanData() override; @@ -177,5 +196,14 @@ class FeeBumpMutableTransactionResult : public MutableTransactionResultBase void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) override; bool isSuccess() const override; + + void setReplayFailingTransactionResult( + TransactionResult const& failing) override; + void setReplaySuccessfulTransactionResult( + TransactionResult const& success) override; + std::optional const& + getReplaySuccessfulTransactionResult() const override; + std::optional const& + getReplayFailingTransactionResult() const override; }; } \ No newline at end of file diff --git a/src/transactions/OperationFrame.cpp b/src/transactions/OperationFrame.cpp index 69c2660c60..00e2cd307e 100644 --- a/src/transactions/OperationFrame.cpp +++ b/src/transactions/OperationFrame.cpp @@ -135,7 +135,8 @@ OperationFrame::OperationFrame(Operation const& op, } bool -OperationFrame::apply(AppConnector& app, SignatureChecker& signatureChecker, +OperationFrame::apply(AppConnector& app, + AbstractSignatureChecker& signatureChecker, AbstractLedgerTxn& ltx, Hash const& sorobanBasePrngSeed, OperationResult& res, std::shared_ptr sorobanData) const @@ -168,7 +169,7 @@ OperationFrame::isOpSupported(LedgerHeader const&) const } bool -OperationFrame::checkSignature(SignatureChecker& signatureChecker, +OperationFrame::checkSignature(AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, OperationResult& res, bool forApply) const { @@ -218,7 +219,7 @@ OperationFrame::getSourceID() const // verifies that the operation is well formed (operation specific) bool OperationFrame::checkValid(AppConnector& app, - SignatureChecker& signatureChecker, + AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, bool forApply, OperationResult& res, std::shared_ptr sorobanData) const diff --git a/src/transactions/OperationFrame.h b/src/transactions/OperationFrame.h index 5cc8aa6641..7ee1f54978 100644 --- a/src/transactions/OperationFrame.h +++ b/src/transactions/OperationFrame.h @@ -20,7 +20,7 @@ class LedgerManager; class LedgerTxnEntry; class LedgerTxnHeader; -class SignatureChecker; +class AbstractSignatureChecker; class TransactionFrame; class MutableTransactionResultBase; @@ -67,18 +67,19 @@ class OperationFrame OperationFrame(OperationFrame const&) = delete; virtual ~OperationFrame() = default; - bool checkSignature(SignatureChecker& signatureChecker, + bool checkSignature(AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, OperationResult& res, bool forApply) const; AccountID getSourceID() const; - bool checkValid(AppConnector& app, SignatureChecker& signatureChecker, + bool checkValid(AppConnector& app, + AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, bool forApply, OperationResult& res, std::shared_ptr sorobanData) const; - bool apply(AppConnector& app, SignatureChecker& signatureChecker, + bool apply(AppConnector& app, AbstractSignatureChecker& signatureChecker, AbstractLedgerTxn& ltx, Hash const& sorobanBasePrngSeed, OperationResult& res, std::shared_ptr sorobanData) const; diff --git a/src/transactions/SignatureChecker.cpp b/src/transactions/SignatureChecker.cpp index 327c49bdad..6a8636f6ba 100644 --- a/src/transactions/SignatureChecker.cpp +++ b/src/transactions/SignatureChecker.cpp @@ -17,7 +17,7 @@ namespace stellar { -SignatureCheckerImpl::SignatureCheckerImpl( +SignatureChecker::SignatureChecker( uint32_t protocolVersion, Hash const& contentsHash, xdr::xvector const& signatures) : mProtocolVersion{protocolVersion} @@ -28,8 +28,8 @@ SignatureCheckerImpl::SignatureCheckerImpl( } bool -SignatureCheckerImpl::checkSignature(std::vector const& signersV, - int neededWeight) +SignatureChecker::checkSignature(std::vector const& signersV, + int neededWeight) { #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION return true; @@ -136,7 +136,7 @@ SignatureCheckerImpl::checkSignature(std::vector const& signersV, } bool -SignatureCheckerImpl::checkAllSignaturesUsed() const +SignatureChecker::checkAllSignaturesUsed() const { #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION return true; diff --git a/src/transactions/SignatureChecker.h b/src/transactions/SignatureChecker.h index 69e05261f9..2c06879f8c 100644 --- a/src/transactions/SignatureChecker.h +++ b/src/transactions/SignatureChecker.h @@ -16,20 +16,17 @@ namespace stellar { -class SignatureChecker +class AbstractSignatureChecker { public: - virtual ~SignatureChecker() - { - } - + virtual ~AbstractSignatureChecker() = default; virtual bool checkSignature(std::vector const&, int32_t) = 0; virtual bool checkAllSignaturesUsed() const = 0; }; -class SignatureCheckerImpl : public SignatureChecker +class SignatureChecker : public AbstractSignatureChecker { public: - explicit SignatureCheckerImpl( + explicit SignatureChecker( uint32_t protocolVersion, Hash const& contentsHash, xdr::xvector const& signatures); @@ -45,12 +42,12 @@ class SignatureCheckerImpl : public SignatureChecker std::vector mUsedSignatures; }; -class AlwaysValidSignatureChecker : public SignatureChecker +class AlwaysValidSignatureChecker : public AbstractSignatureChecker { public: AlwaysValidSignatureChecker(uint32_t, Hash const&, xdr::xvector const&) - : SignatureChecker() + : AbstractSignatureChecker() { } diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 107b62ef83..46c8120868 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -266,7 +266,7 @@ TransactionFrame::getFee(LedgerHeader const& header, } bool -TransactionFrame::checkSignature(SignatureChecker& signatureChecker, +TransactionFrame::checkSignature(AbstractSignatureChecker& signatureChecker, LedgerEntryWrapper const& account, int32_t neededWeight) const { @@ -284,8 +284,9 @@ TransactionFrame::checkSignature(SignatureChecker& signatureChecker, } bool -TransactionFrame::checkSignatureNoAccount(SignatureChecker& signatureChecker, - AccountID const& accountID) const +TransactionFrame::checkSignatureNoAccount( + AbstractSignatureChecker& signatureChecker, + AccountID const& accountID) const { ZoneScoped; std::vector signers; @@ -295,7 +296,8 @@ TransactionFrame::checkSignatureNoAccount(SignatureChecker& signatureChecker, } bool -TransactionFrame::checkExtraSigners(SignatureChecker& signatureChecker) const +TransactionFrame::checkExtraSigners( + AbstractSignatureChecker& signatureChecker) const { ZoneScoped; if (extraSignersExist()) @@ -1087,7 +1089,7 @@ TransactionFrame::processSeqNum(AbstractLedgerTxn& ltx) const bool TransactionFrame::processSignatures( - ValidationType cv, SignatureChecker& signatureChecker, + ValidationType cv, AbstractSignatureChecker& signatureChecker, AbstractLedgerTxn& ltxOuter, MutableTransactionResultBase& txResult) const { ZoneScoped; @@ -1152,22 +1154,6 @@ TransactionFrame::processSignatures( return maybeValid; } -// These functions are const so that we can call them from a const context but -// they modify mutable members. -void -TransactionFrame::setReplaySuccessfulTransactionResult( - TransactionResult const& successful) const -{ - mReplaySuccessfulTransactionResult = std::make_optional(successful); -} - -void -TransactionFrame::setReplayFailingTransactionResult( - TransactionResult const& failing) const -{ - mReplayFailingTransactionResult = std::make_optional(failing); -} - bool TransactionFrame::isBadSeq(LedgerHeaderWrapper const& header, int64_t seqNum) const @@ -1197,7 +1183,7 @@ TransactionFrame::isBadSeq(LedgerHeaderWrapper const& header, TransactionFrame::ValidationType TransactionFrame::commonValid(AppConnector& app, - SignatureChecker& signatureChecker, + AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, SequenceNumber current, bool applying, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, @@ -1450,9 +1436,10 @@ TransactionFrame::checkValidWithOptionallyChargedFee( auto txResult = createSuccessResultWithFeeCharged( ls.getLedgerHeader().current(), minBaseFee, false); releaseAssert(txResult); - auto sc = SignatureCheckerImpl{ls.getLedgerHeader().current().ledgerVersion, - getContentsHash(), getSignatures(mEnvelope)}; - SignatureChecker& signatureChecker = sc; + auto signatureChecker = std::make_unique( + ls.getLedgerHeader().current().ledgerVersion, getContentsHash(), + getSignatures(mEnvelope)); + std::optional sorobanResourceFee; if (protocolVersionStartsFrom(ls.getLedgerHeader().current().ledgerVersion, SOROBAN_PROTOCOL_VERSION) && @@ -1462,9 +1449,9 @@ TransactionFrame::checkValidWithOptionallyChargedFee( ls.getLedgerHeader().current().ledgerVersion, app.getLedgerManager().getSorobanNetworkConfig(), app.getConfig()); } - bool res = commonValid(app, signatureChecker, ls, current, false, chargeFee, - lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset, - sorobanResourceFee, + bool res = commonValid(app, *signatureChecker, ls, current, false, + chargeFee, lowerBoundCloseTimeOffset, + upperBoundCloseTimeOffset, sorobanResourceFee, txResult) == ValidationType::kMaybeValid; if (res) { @@ -1473,7 +1460,7 @@ TransactionFrame::checkValidWithOptionallyChargedFee( auto const& op = mOperations[i]; auto& opResult = txResult->getOpResultAt(i); - if (!op->checkValid(app, signatureChecker, ls, false, opResult, + if (!op->checkValid(app, *signatureChecker, ls, false, opResult, txResult->getSorobanData())) { // it's OK to just fast fail here and not try to call @@ -1484,7 +1471,7 @@ TransactionFrame::checkValidWithOptionallyChargedFee( } } - if (!signatureChecker.checkAllSignaturesUsed()) + if (!signatureChecker->checkAllSignaturesUsed()) { txResult->setInnermostResultCode(txBAD_AUTH_EXTRA); } @@ -1582,21 +1569,23 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, } bool -TransactionFrame::applyOperations(SignatureChecker& signatureChecker, +TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, AppConnector& app, AbstractLedgerTxn& ltx, TransactionMetaFrame& outerMeta, MutableTransactionResultBase& txResult, Hash const& sorobanBasePrngSeed) const { ZoneScoped; - if (mReplayFailingTransactionResult.has_value()) + // TODO maybe encapsulate this in a function + // in MutableTransactionResultBase + auto const& failingResult = txResult.getReplayFailingTransactionResult(); + if (failingResult) { // Sub-zone for skips ZoneScopedN("skipped failed"); - txResult.setResultCode(mReplayFailingTransactionResult->result.code()); - txResult.getResult().result.results() = - mReplayFailingTransactionResult->result.results(); + txResult.setResultCode(failingResult->result.code()); + txResult.getResult().result.results() = failingResult->result.results(); return false; } @@ -1816,17 +1805,19 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, { mCachedAccountPreProtocol8.reset(); uint32_t ledgerVersion = ltx.loadHeader().current().ledgerVersion; - auto skipMode = mReplayFailingTransactionResult.has_value() || - mReplaySuccessfulTransactionResult.has_value(); - std::unique_ptr signatureChecker; - if (skipMode) + std::unique_ptr signatureChecker; + // If the txResult has a replay result (catchup in skip mode is + // enabled), + // we do not perform signature verification. + if (txResult->getReplayFailingTransactionResult() || + txResult->getReplaySuccessfulTransactionResult()) { signatureChecker = std::make_unique( ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); } else { - signatureChecker = std::make_unique( + signatureChecker = std::make_unique( ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); } diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index 1ed945120c..1489c89349 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -38,7 +38,7 @@ class LedgerManager; class LedgerTxnEntry; class LedgerTxnHeader; class SecretKey; -class SignatureChecker; +class AbstractSignatureChecker; class MutableTransactionResultBase; class SorobanTxData; class XDROutputFileStream; @@ -70,10 +70,6 @@ class TransactionFrame : public TransactionFrameBase mutable Hash mFullHash; // the hash of the contents and the sig. std::vector> mOperations; - mutable std::optional mReplaySuccessfulTransactionResult{ - std::nullopt}; - mutable std::optional mReplayFailingTransactionResult{ - std::nullopt}; LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, LedgerTxnHeader const& header) const; @@ -109,7 +105,7 @@ class TransactionFrame : public TransactionFrameBase int64_t seqNum) const; ValidationType commonValid(AppConnector& app, - SignatureChecker& signatureChecker, + AbstractSignatureChecker& signatureChecker, LedgerSnapshot const& ls, SequenceNumber current, bool applying, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, @@ -123,7 +119,7 @@ class TransactionFrame : public TransactionFrameBase AccountID const& accountID, SignerKey const& signerKey) const; - bool applyOperations(SignatureChecker& checker, AppConnector& app, + bool applyOperations(AbstractSignatureChecker& checker, AppConnector& app, AbstractLedgerTxn& ltx, TransactionMetaFrame& meta, MutableTransactionResultBase& txResult, Hash const& sorobanBasePrngSeed) const; @@ -131,7 +127,7 @@ class TransactionFrame : public TransactionFrameBase void processSeqNum(AbstractLedgerTxn& ltx) const; bool processSignatures(ValidationType cv, - SignatureChecker& signatureChecker, + AbstractSignatureChecker& signatureChecker, AbstractLedgerTxn& ltxOuter, MutableTransactionResultBase& txResult) const; @@ -203,13 +199,13 @@ class TransactionFrame : public TransactionFrameBase std::optional baseFee, bool applying) const override; - bool checkSignature(SignatureChecker& signatureChecker, + bool checkSignature(AbstractSignatureChecker& signatureChecker, LedgerEntryWrapper const& account, int32_t neededWeight) const; - bool checkSignatureNoAccount(SignatureChecker& signatureChecker, + bool checkSignatureNoAccount(AbstractSignatureChecker& signatureChecker, AccountID const& accountID) const; - bool checkExtraSigners(SignatureChecker& signatureChecker) const; + bool checkExtraSigners(AbstractSignatureChecker& signatureChecker) const; MutableTxResultPtr checkValidWithOptionallyChargedFee( AppConnector& app, LedgerSnapshot const& ls, SequenceNumber current, @@ -292,10 +288,6 @@ class TransactionFrame : public TransactionFrameBase virtual int64 declaredSorobanResourceFee() const override; virtual bool XDRProvidesValidFee() const override; - void setReplayFailingTransactionResult( - TransactionResult const& failing) const override; - void setReplaySuccessfulTransactionResult( - TransactionResult const& successful) const override; #ifdef BUILD_TESTS friend class TransactionTestFrame; #endif diff --git a/src/transactions/TransactionFrameBase.h b/src/transactions/TransactionFrameBase.h index 8d66a3da4b..707fd10cf9 100644 --- a/src/transactions/TransactionFrameBase.h +++ b/src/transactions/TransactionFrameBase.h @@ -114,10 +114,5 @@ class TransactionFrameBase virtual SorobanResources const& sorobanResources() const = 0; virtual int64 declaredSorobanResourceFee() const = 0; virtual bool XDRProvidesValidFee() const = 0; - - virtual void - setReplaySuccessfulTransactionResult(TransactionResult const&) const = 0; - virtual void - setReplayFailingTransactionResult(TransactionResult const&) const = 0; }; } diff --git a/src/transactions/test/TransactionTestFrame.cpp b/src/transactions/test/TransactionTestFrame.cpp index 9cf0fd4c69..8ed5958299 100644 --- a/src/transactions/test/TransactionTestFrame.cpp +++ b/src/transactions/test/TransactionTestFrame.cpp @@ -350,14 +350,4 @@ TransactionTestFrame::XDRProvidesValidFee() const return mTransactionFrame->XDRProvidesValidFee(); } -void -TransactionTestFrame::setReplayFailingTransactionResult( - TransactionResult const&) const -{ /* NO OP*/ -} -void -TransactionTestFrame::setReplaySuccessfulTransactionResult( - TransactionResult const&) const -{ /* NO OP*/ -} } \ No newline at end of file diff --git a/src/transactions/test/TransactionTestFrame.h b/src/transactions/test/TransactionTestFrame.h index 82991da88c..910d5e89d7 100644 --- a/src/transactions/test/TransactionTestFrame.h +++ b/src/transactions/test/TransactionTestFrame.h @@ -135,10 +135,6 @@ class TransactionTestFrame : public TransactionFrameBase SorobanResources const& sorobanResources() const override; int64 declaredSorobanResourceFee() const override; bool XDRProvidesValidFee() const override; - void setReplayFailingTransactionResult( - TransactionResult const& failing) const override; - void setReplaySuccessfulTransactionResult( - TransactionResult const& successful) const override; bool isTestTx() const override {