From 222f2c0200e5cc57df8aa9a361585161b1dad053 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Wed, 16 Oct 2024 16:49:07 -0700 Subject: [PATCH 01/17] WIP --- src/catchup/ApplyCheckpointWork.cpp | 51 +++++++++- src/catchup/ApplyCheckpointWork.h | 27 ++++-- src/catchup/CatchupConfiguration.h | 15 +-- src/catchup/CatchupWork.h | 14 +-- src/catchup/DownloadApplyTxsWork.cpp | 97 ++++++++++--------- src/herder/LedgerCloseData.cpp | 9 +- src/herder/LedgerCloseData.h | 10 +- src/ledger/LedgerManagerImpl.cpp | 52 +++++++++- src/ledger/LedgerManagerImpl.h | 1 + .../test/LedgerCloseMetaStreamTests.cpp | 5 + src/main/Config.cpp | 2 + src/main/Config.h | 5 + src/test/FuzzerImpl.cpp | 7 +- src/transactions/FeeBumpTransactionFrame.cpp | 19 +++- src/transactions/FeeBumpTransactionFrame.h | 5 + src/transactions/SignatureChecker.cpp | 8 +- src/transactions/SignatureChecker.h | 37 ++++++- src/transactions/TransactionFrame.cpp | 60 ++++++++++-- src/transactions/TransactionFrame.h | 8 ++ src/transactions/TransactionFrameBase.h | 5 + .../test/TransactionTestFrame.cpp | 11 +++ src/transactions/test/TransactionTestFrame.h | 5 +- 22 files changed, 350 insertions(+), 103 deletions(-) diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp index 39180e9cdb..5afb9478f0 100644 --- a/src/catchup/ApplyCheckpointWork.cpp +++ b/src/catchup/ApplyCheckpointWork.cpp @@ -96,6 +96,16 @@ ApplyCheckpointWork::openInputFiles() mTxIn.open(ti.localPath_nogz()); mTxHistoryEntry = TransactionHistoryEntry(); mHeaderHistoryEntry = LedgerHeaderHistoryEntry(); + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + { + mTxResultIn.close(); + 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{}; + } mFilesOpen = true; } @@ -141,6 +151,39 @@ ApplyCheckpointWork::getCurrentTxSet() return TxSetXDRFrame::makeEmpty(lm.getLastClosedLedgerHeader()); } +std::optional +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 + { + 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)); + CLOG_DEBUG(History, "No txresultset for ledger {}", seq); + return std::nullopt; +} + std::shared_ptr ApplyCheckpointWork::getNextLedgerCloseData() { @@ -219,6 +262,12 @@ ApplyCheckpointWork::getNextLedgerCloseData() CLOG_DEBUG(History, "Ledger {} has {} transactions", header.ledgerSeq, txset->sizeTxTotal()); + std::optional txres = std::nullopt; + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + { + txres = getCurrentTxResultSet(); + } + // We've verified the ledgerHeader (in the "trusted part of history" // sense) in CATCHUP_VERIFY phase; we now need to check that the // txhash we're about to apply is the one denoted by that ledger @@ -249,7 +298,7 @@ ApplyCheckpointWork::getNextLedgerCloseData() return std::make_shared( header.ledgerSeq, txset, header.scpValue, - std::make_optional(mHeaderHistoryEntry.hash)); + std::make_optional(mHeaderHistoryEntry.hash), txres); } BasicWork::State diff --git a/src/catchup/ApplyCheckpointWork.h b/src/catchup/ApplyCheckpointWork.h index 93c4168b21..4e15055215 100644 --- a/src/catchup/ApplyCheckpointWork.h +++ b/src/catchup/ApplyCheckpointWork.h @@ -21,20 +21,24 @@ class TmpDir; struct LedgerHeaderHistoryEntry; /** - * This class is responsible for applying transactions stored in files on - * temporary directory (downloadDir) to local ledger. It requires two sets of - * files - ledgers and transactions - int .xdr format. Transaction files are - * used to read transactions that will be used and ledger files are used to + * This class is responsible for applying transactions stored in files in the + * temporary directory (downloadDir) to local the ledger. It requires two sets + * of files - ledgers and transactions - in .xdr format. Transaction files are + * used to read transactions that will be applied and ledger files are used to * check if ledger hashes are matching. * + * It may also require a third set of files - transaction results - to use in + * accelerated replay, where failed transactions are not applied and successful + * transactions are applied without verifying their signatures. + * * In each run it skips or applies transactions from one ledger. Skipping occurs - * when ledger to be applied is older than LCL from local ledger. At LCL - * boundary checks are made to confirm that ledgers from files knit up with - * LCL. If everything is OK, an apply ledger operation is performed. Then - * another check is made - if new local ledger matches corresponding ledger from - * file. + * when the ledger to be applied is older than the LCL of the local ledger. At + * LCL, boundary checks are made to confirm that the ledgers from the files knit + * up with LCL. If everything is OK, an apply ledger operation is performed. + * Then another check is made - if the new local ledger matches corresponding + * the ledger from file. * - * Constructor of this class takes some important parameters: + * The constructor of this class takes some important parameters: * * downloadDir - directory containing ledger and transaction files * * range - LedgerRange to apply, must be checkpoint-aligned, * and cover at most one checkpoint. @@ -48,7 +52,9 @@ class ApplyCheckpointWork : public BasicWork XDRInputFileStream mHdrIn; XDRInputFileStream mTxIn; + XDRInputFileStream mTxResultIn; TransactionHistoryEntry mTxHistoryEntry; + TransactionHistoryResultEntry mTxHistoryResultEntry; LedgerHeaderHistoryEntry mHeaderHistoryEntry; OnFailureCallback mOnFailure; @@ -57,6 +63,7 @@ class ApplyCheckpointWork : public BasicWork std::shared_ptr mConditionalWork; TxSetXDRFrameConstPtr getCurrentTxSet(); + std::optional getCurrentTxResultSet(); void openInputFiles(); std::shared_ptr getNextLedgerCloseData(); diff --git a/src/catchup/CatchupConfiguration.h b/src/catchup/CatchupConfiguration.h index 38465b96f9..3008d64000 100644 --- a/src/catchup/CatchupConfiguration.h +++ b/src/catchup/CatchupConfiguration.h @@ -13,7 +13,7 @@ namespace stellar { -// Each catchup can be configured by two parameters destination ledger +// Each catchup can be configured by two parameters: destination ledger // (and its hash, if known) and count of ledgers to apply. // Value of count can be adjusted in different ways during catchup. If applying // count ledgers would mean going before the last closed ledger - it is @@ -31,12 +31,13 @@ namespace stellar // and catchup to that instead of destination ledger. This is useful when // doing offline commandline catchups with stellar-core catchup command. // -// Catchup can be done in two modes - ONLINE nad OFFLINE. In ONLINE mode node -// is connected to the network. If receives ledgers during catchup and applies -// them after history is applied. Also additional closing ledger is required -// to mark catchup as complete and node as synced. In OFFLINE mode node is not -// connected to network, so new ledgers are not being externalized. Only -// buckets and transactions from history archives are applied. +// 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 +// 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 +// externalized. Only buckets and transactions from history archives are +// applied. class CatchupConfiguration { public: diff --git a/src/catchup/CatchupWork.h b/src/catchup/CatchupWork.h index ed36c75f5c..7607091ec2 100644 --- a/src/catchup/CatchupWork.h +++ b/src/catchup/CatchupWork.h @@ -24,22 +24,22 @@ using WorkSeqPtr = std::shared_ptr; // CatchupWork does all the necessary work to perform any type of catchup. // It accepts CatchupConfiguration structure to know from which ledger to which -// one do the catchup and if it involves only applying ledgers or ledgers and +// one to do the catchup and if it involves only applying ledgers or ledgers and // buckets. // -// First thing it does is to get a history state which allows to calculate -// proper destination ledger (in case CatchupConfiguration::CURRENT) was used -// and to get list of buckets that should be in database on that ledger. +// First, it gets a history state, which allows it to calculate a +// proper destination ledger (in case CatchupConfiguration::CURRENT) +// and get a list of buckets that should be in the database on that ledger. // -// Next step is downloading and verifying ledgers (if verifyMode is set to -// VERIFY_BUFFERED_LEDGERS it can also verify against ledgers currently +// Next, it downloads and verifies ledgers (if verifyMode is set to +// VERIFY_BUFFERED_LEDGERS, it can also verify against ledgers currently // buffered in LedgerManager). // // Then, depending on configuration, it can download, verify and apply buckets // (as in MINIMAL and RECENT catchups), and then download and apply // transactions (as in COMPLETE and RECENT catchups). // -// After that, catchup is done and node can replay buffered ledgers and take +// After that, catchup is done and the node can replay buffered ledgers and take // part in consensus protocol. class CatchupWork : public Work diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index 90538ce3fa..37b0b5e8db 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -43,44 +43,40 @@ 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)); + } - 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); + OnFailureCallback cb = [archive = mArchive, filesToTransfer]() { + for (auto const& ft : filesToTransfer) + { + CLOG_ERROR(History, "Archive {} maybe contains corrupt file {}", + archive->getName(), ft.remoteName()); + } + }; 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}; - auto maybeWaitForMerges = [](Application& app) { if (app.getConfig().CATCHUP_WAIT_MERGES_TX_APPLY_FOR_TESTING) { @@ -98,8 +94,10 @@ DownloadApplyTxsWork::yieldMoreWork() { auto prev = mLastYieldedWork; bool pqFellBehind = false; + auto applyName = apply->getName(); auto predicate = [prev, pqFellBehind, waitForPublish = mWaitForPublish, - maybeWaitForMerges](Application& app) mutable { + maybeWaitForMerges, + applyName](Application& app) mutable { if (!prev) { throw std::runtime_error("Download and apply txs: related Work " @@ -130,37 +128,42 @@ DownloadApplyTxsWork::yieldMoreWork() } return res && maybeWaitForMerges(app); }; - seq.push_back(std::make_shared( + downloadSeq.push_back(std::make_shared( mApp, "conditional-" + apply->getName(), predicate, apply)); } else { - seq.push_back(std::make_shared( + downloadSeq.push_back(std::make_shared( mApp, "wait-merges" + apply->getName(), maybeWaitForMerges, apply)); } - seq.push_back(std::make_shared( + downloadSeq.push_back(std::make_shared( mApp, "delete-transactions-" + std::to_string(mCheckpointToQueue), - [ft](Application& app) { - try + [filesToTransfer](Application& app) { + for (auto const& ft : filesToTransfer) { - std::filesystem::remove( - std::filesystem::path(ft.localPath_nogz())); - CLOG_DEBUG(History, "Deleted transactions {}", + CLOG_DEBUG(History, "Deleting transactions {}", ft.localPath_nogz()); - return true; - } - catch (std::filesystem::filesystem_error const& e) - { - CLOG_ERROR(History, "Could not delete transactions {}: {}", - ft.localPath_nogz(), e.what()); - return false; + try + { + std::filesystem::remove( + std::filesystem::path(ft.localPath_nogz())); + CLOG_DEBUG(History, "Deleted transactions {}", + ft.localPath_nogz()); + } + catch (std::filesystem::filesystem_error const& e) + { + CLOG_ERROR(History, "Could not delete transactions {}: {}", + ft.localPath_nogz(), e.what()); + return false; + } } + return true; })); auto nextWork = std::make_shared( - mApp, "download-apply-" + std::to_string(mCheckpointToQueue), seq, - BasicWork::RETRY_NEVER); + mApp, "download-apply-" + std::to_string(mCheckpointToQueue), + downloadSeq, BasicWork::RETRY_NEVER); mCheckpointToQueue += mApp.getHistoryManager().getCheckpointFrequency(); mLastYieldedWork = nextWork; return nextWork; diff --git a/src/herder/LedgerCloseData.cpp b/src/herder/LedgerCloseData.cpp index a4e60700e5..a00f1c757e 100644 --- a/src/herder/LedgerCloseData.cpp +++ b/src/herder/LedgerCloseData.cpp @@ -14,14 +14,15 @@ using namespace std; namespace stellar { -LedgerCloseData::LedgerCloseData(uint32_t ledgerSeq, - TxSetXDRFrameConstPtr txSet, - StellarValue const& v, - std::optional const& expectedLedgerHash) +LedgerCloseData::LedgerCloseData( + uint32_t ledgerSeq, TxSetXDRFrameConstPtr txSet, StellarValue const& v, + std::optional const& expectedLedgerHash, + std::optional const& expectedResults) : mLedgerSeq(ledgerSeq) , mTxSet(txSet) , mValue(v) , mExpectedLedgerHash(expectedLedgerHash) + , mExpectedResults(expectedResults) { releaseAssert(txSet->getContentsHash() == mValue.txSetHash); } diff --git a/src/herder/LedgerCloseData.h b/src/herder/LedgerCloseData.h index 31f9192277..a8a4039426 100644 --- a/src/herder/LedgerCloseData.h +++ b/src/herder/LedgerCloseData.h @@ -26,7 +26,9 @@ class LedgerCloseData public: LedgerCloseData( uint32_t ledgerSeq, TxSetXDRFrameConstPtr txSet, StellarValue const& v, - std::optional const& expectedLedgerHash = std::nullopt); + std::optional const& expectedLedgerHash = std::nullopt, + std::optional const& expectedResults = + std::nullopt); uint32_t getLedgerSeq() const @@ -48,6 +50,11 @@ class LedgerCloseData { return mExpectedLedgerHash; } + std::optional const& + getExpectedResults() const + { + return mExpectedResults; + } StoredDebugTransactionSet toXDR() const @@ -82,6 +89,7 @@ class LedgerCloseData TxSetXDRFrameConstPtr mTxSet; StellarValue mValue; std::optional mExpectedLedgerHash; + std::optional mExpectedResults; }; std::string stellarValueToString(Config const& c, StellarValue const& sv); diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 8e7c5841b2..3d08008e5b 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -59,6 +59,7 @@ #include #include +#include #include #include #include @@ -920,7 +921,7 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData) txResultSet.results.reserve(txs.size()); // Subtle: after this call, `header` is invalidated, and is not safe to use applyTransactions(*applicableTxSet, txs, mutableTxResults, ltx, txResultSet, - ledgerCloseMeta); + ledgerData.getExpectedResults(), ledgerCloseMeta); if (mApp.getConfig().MODE_STORES_HISTORY_MISC) { auto ledgerSeq = ltx.loadHeader().current().ledgerSeq; @@ -1498,6 +1499,7 @@ 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); @@ -1520,6 +1522,17 @@ 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}; @@ -1550,10 +1563,43 @@ LedgerManagerImpl::applyTransactions( } ++txNum; - tx->apply(mApp.getAppConnector(), ltx, tm, mutableTxResult, subSeed); - tx->processPostApply(mApp.getAppConnector(), ltx, tm, mutableTxResult); 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); + results.result = mutableTxResult->getResult(); if (results.result.result.code() == TransactionResultCode::txSUCCESS) { diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index 1d16f93f4e..d3b4eca48d 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -84,6 +84,7 @@ class LedgerManagerImpl : public LedgerManager 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/ledger/test/LedgerCloseMetaStreamTests.cpp b/src/ledger/test/LedgerCloseMetaStreamTests.cpp index efdff716d5..07aaa2dcd5 100644 --- a/src/ledger/test/LedgerCloseMetaStreamTests.cpp +++ b/src/ledger/test/LedgerCloseMetaStreamTests.cpp @@ -295,6 +295,11 @@ TEST_CASE("LedgerCloseMetaStream file descriptor - REPLAY_IN_MEMORY", cfg.RUN_STANDALONE = true; cfg.setInMemoryMode(); cfg.EXPERIMENTAL_PRECAUTION_DELAY_META = delayMeta; + SECTION("skip mode") + { + cfg.MODE_STORES_HISTORY_MISC = true; + cfg.CATCHUP_SKIP_KNOWN_RESULTS = true; + } VirtualClock clock; auto app = createTestApplication(clock, cfg, /*newdb=*/false); diff --git a/src/main/Config.cpp b/src/main/Config.cpp index 927883ee8d..201708cd01 100644 --- a/src/main/Config.cpp +++ b/src/main/Config.cpp @@ -1150,6 +1150,8 @@ Config::processConfig(std::shared_ptr t) CATCHUP_RECENT = readInt(item, 0, UINT32_MAX - 1); }}, + {"CATCHUP_SKIP_KNOWN_RESULTS", + [&]() { CATCHUP_SKIP_KNOWN_RESULTS = readBool(item); }}, {"ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING", [&]() { ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING = readBool(item); diff --git a/src/main/Config.h b/src/main/Config.h index 600a349259..ca7ffe659d 100644 --- a/src/main/Config.h +++ b/src/main/Config.h @@ -203,6 +203,11 @@ class Config : public std::enable_shared_from_this // If you want, say, a week of history, set this to 120000. uint32_t CATCHUP_RECENT; + // Mode for "accelerated" catchup. If set to true, the node will skip + // application of failed transactions and will not verify signatures of + // successful transactions. + bool CATCHUP_SKIP_KNOWN_RESULTS; + // Interval between automatic maintenance executions std::chrono::seconds AUTOMATIC_MAINTENANCE_PERIOD; diff --git a/src/test/FuzzerImpl.cpp b/src/test/FuzzerImpl.cpp index 23e4c400df..b60edf3cc3 100644 --- a/src/test/FuzzerImpl.cpp +++ b/src/test/FuzzerImpl.cpp @@ -921,9 +921,10 @@ class FuzzTransactionFrame : public TransactionFrame // attempt application of transaction without processing the fee or // committing the LedgerTxn - SignatureChecker signatureChecker{ - ltx.loadHeader().current().ledgerVersion, getContentsHash(), - mEnvelope.v1().signatures}; + auto sc = + SignatureCheckerImpl{ltx.loadHeader().current().ledgerVersion, + getContentsHash(), mEnvelope.v1().signatures}; + SignatureChecker& signatureChecker = sc; LedgerSnapshot ltxStmt(ltx); // if any ill-formed Operations, do not attempt transaction application auto isInvalidOperation = [&](auto const& op, auto& opResult) { diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp index ebddd16896..eb24128b43 100644 --- a/src/transactions/FeeBumpTransactionFrame.cpp +++ b/src/transactions/FeeBumpTransactionFrame.cpp @@ -170,9 +170,10 @@ FeeBumpTransactionFrame::checkValid(AppConnector& app, LedgerSnapshot const& ls, auto txResult = createSuccessResultWithFeeCharged( ls.getLedgerHeader().current(), minBaseFee, false); - SignatureChecker signatureChecker{ - ls.getLedgerHeader().current().ledgerVersion, getContentsHash(), - mEnvelope.feeBump().signatures}; + auto sc = + SignatureCheckerImpl{ls.getLedgerHeader().current().ledgerVersion, + getContentsHash(), mEnvelope.feeBump().signatures}; + SignatureChecker& signatureChecker = sc; if (commonValid(signatureChecker, ls, false, *txResult) != ValidationType::kFullyValid) { @@ -575,4 +576,16 @@ 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 6435c84eab..d31f21dae9 100644 --- a/src/transactions/FeeBumpTransactionFrame.h +++ b/src/transactions/FeeBumpTransactionFrame.h @@ -139,5 +139,10 @@ 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/SignatureChecker.cpp b/src/transactions/SignatureChecker.cpp index 6a8636f6ba..327c49bdad 100644 --- a/src/transactions/SignatureChecker.cpp +++ b/src/transactions/SignatureChecker.cpp @@ -17,7 +17,7 @@ namespace stellar { -SignatureChecker::SignatureChecker( +SignatureCheckerImpl::SignatureCheckerImpl( uint32_t protocolVersion, Hash const& contentsHash, xdr::xvector const& signatures) : mProtocolVersion{protocolVersion} @@ -28,8 +28,8 @@ SignatureChecker::SignatureChecker( } bool -SignatureChecker::checkSignature(std::vector const& signersV, - int neededWeight) +SignatureCheckerImpl::checkSignature(std::vector const& signersV, + int neededWeight) { #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION return true; @@ -136,7 +136,7 @@ SignatureChecker::checkSignature(std::vector const& signersV, } bool -SignatureChecker::checkAllSignaturesUsed() const +SignatureCheckerImpl::checkAllSignaturesUsed() const { #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION return true; diff --git a/src/transactions/SignatureChecker.h b/src/transactions/SignatureChecker.h index 15a8c33c35..69e05261f9 100644 --- a/src/transactions/SignatureChecker.h +++ b/src/transactions/SignatureChecker.h @@ -19,13 +19,23 @@ namespace stellar class SignatureChecker { public: - explicit SignatureChecker( + virtual ~SignatureChecker() + { + } + + virtual bool checkSignature(std::vector const&, int32_t) = 0; + virtual bool checkAllSignaturesUsed() const = 0; +}; +class SignatureCheckerImpl : public SignatureChecker +{ + public: + explicit SignatureCheckerImpl( uint32_t protocolVersion, Hash const& contentsHash, xdr::xvector const& signatures); bool checkSignature(std::vector const& signersV, - int32_t neededWeight); - bool checkAllSignaturesUsed() const; + int32_t neededWeight) override; + bool checkAllSignaturesUsed() const override; private: uint32_t mProtocolVersion; @@ -34,4 +44,25 @@ class SignatureChecker std::vector mUsedSignatures; }; + +class AlwaysValidSignatureChecker : public SignatureChecker +{ + public: + AlwaysValidSignatureChecker(uint32_t, Hash const&, + xdr::xvector const&) + : SignatureChecker() + { + } + + bool + checkSignature(std::vector const&, int32_t) + { + return true; + } + bool + checkAllSignaturesUsed() const + { + return true; + } }; +} diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 1eb017d768..107b62ef83 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1152,6 +1152,22 @@ 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 @@ -1434,10 +1450,9 @@ TransactionFrame::checkValidWithOptionallyChargedFee( auto txResult = createSuccessResultWithFeeCharged( ls.getLedgerHeader().current(), minBaseFee, false); releaseAssert(txResult); - - SignatureChecker signatureChecker{ - ls.getLedgerHeader().current().ledgerVersion, getContentsHash(), - getSignatures(mEnvelope)}; + auto sc = SignatureCheckerImpl{ls.getLedgerHeader().current().ledgerVersion, + getContentsHash(), getSignatures(mEnvelope)}; + SignatureChecker& signatureChecker = sc; std::optional sorobanResourceFee; if (protocolVersionStartsFrom(ls.getLedgerHeader().current().ledgerVersion, SOROBAN_PROTOCOL_VERSION) && @@ -1574,6 +1589,17 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, Hash const& sorobanBasePrngSeed) const { ZoneScoped; + if (mReplayFailingTransactionResult.has_value()) + { + // Sub-zone for skips + ZoneScopedN("skipped failed"); + + txResult.setResultCode(mReplayFailingTransactionResult->result.code()); + txResult.getResult().result.results() = + mReplayFailingTransactionResult->result.results(); + return false; + } + auto& internalErrorCounter = app.getMetrics().NewCounter( {"ledger", "transaction", "internal-error"}); bool reportInternalErrOnException = true; @@ -1691,6 +1717,11 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, SOROBAN_PROTOCOL_VERSION) && isSoroban()) { + // TODO this needs to execute in skip mode? + // We would have to execute the soroban ops to get the correct + // diagnostic info to publish to meta. This doesn't affect + // ledger state. + // If transaction fails, we don't charge for any // refundable resources. auto preApplyFee = computePreApplySorobanResourceFee( @@ -1785,8 +1816,19 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, { mCachedAccountPreProtocol8.reset(); uint32_t ledgerVersion = ltx.loadHeader().current().ledgerVersion; - SignatureChecker signatureChecker{ledgerVersion, getContentsHash(), - getSignatures(mEnvelope)}; + auto skipMode = mReplayFailingTransactionResult.has_value() || + mReplaySuccessfulTransactionResult.has_value(); + std::unique_ptr signatureChecker; + if (skipMode) + { + signatureChecker = std::make_unique( + ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); + } + else + { + signatureChecker = std::make_unique( + ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); + } // when applying, a failure during tx validation means that // we'll skip trying to apply operations but we'll still @@ -1809,7 +1851,7 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, } LedgerTxn ltxTx(ltx); LedgerSnapshot ltxStmt(ltxTx); - auto cv = commonValid(app, signatureChecker, ltxStmt, 0, true, + auto cv = commonValid(app, *signatureChecker, ltxStmt, 0, true, chargeFee, 0, 0, sorobanResourceFee, txResult); if (cv >= ValidationType::kInvalidUpdateSeqNum) { @@ -1817,7 +1859,7 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, } bool signaturesValid = - processSignatures(cv, signatureChecker, ltxTx, *txResult); + processSignatures(cv, *signatureChecker, ltxTx, *txResult); meta.pushTxChangesBefore(ltxTx.getChanges()); ltxTx.commit(); @@ -1835,7 +1877,7 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, updateSorobanMetrics(app); } - ok = applyOperations(signatureChecker, app, ltx, meta, + ok = applyOperations(*signatureChecker, app, ltx, meta, *txResult, sorobanBasePrngSeed); } return ok; diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index ad46e3b650..1ed945120c 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -70,6 +70,10 @@ 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; @@ -288,6 +292,10 @@ 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 707fd10cf9..8d66a3da4b 100644 --- a/src/transactions/TransactionFrameBase.h +++ b/src/transactions/TransactionFrameBase.h @@ -114,5 +114,10 @@ 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 8f4673ffcb..9cf0fd4c69 100644 --- a/src/transactions/test/TransactionTestFrame.cpp +++ b/src/transactions/test/TransactionTestFrame.cpp @@ -349,4 +349,15 @@ 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 0efca11fdf..82991da88c 100644 --- a/src/transactions/test/TransactionTestFrame.h +++ b/src/transactions/test/TransactionTestFrame.h @@ -135,7 +135,10 @@ 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 { From a8764e781f08852c187e312ba75688bab0f3d321 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Tue, 12 Nov 2024 19:56:46 -0800 Subject: [PATCH 02/17] Yolo test new mode without supercluster config --- src/main/CommandLine.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index be58b992c2..52e37c1201 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,6 +878,7 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; + config.CATCHUP_SKIP_KNOWN_RESULTS = true; if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && config.AUTOMATIC_MAINTENANCE_COUNT > 0) From 0f80cc888a1f6b57e576e207bc8726fdedc7d751 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Wed, 13 Nov 2024 11:21:24 -0800 Subject: [PATCH 03/17] Revert default to skip mode --- src/main/CommandLine.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index 52e37c1201..be58b992c2 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,7 +878,6 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; - config.CATCHUP_SKIP_KNOWN_RESULTS = true; if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && config.AUTOMATIC_MAINTENANCE_COUNT > 0) From 40701f86d26a65e27597b07fed40e9a71666ac64 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Fri, 15 Nov 2024 16:19:25 -0800 Subject: [PATCH 04/17] refactoring --- src/catchup/ApplyCheckpointWork.cpp | 43 ++++---- src/catchup/ApplyCheckpointWork.h | 4 +- src/catchup/CatchupConfiguration.h | 2 +- src/catchup/DownloadApplyTxsWork.cpp | 101 +++++++++++------- src/ledger/LedgerManagerImpl.cpp | 85 +++++++-------- src/ledger/LedgerManagerImpl.h | 4 +- src/rust/soroban/p22 | 2 +- src/test/FuzzerImpl.cpp | 11 +- src/transactions/FeeBumpTransactionFrame.cpp | 33 ++---- src/transactions/FeeBumpTransactionFrame.h | 11 +- src/transactions/MutableTransactionResult.cpp | 55 ++++++++++ src/transactions/MutableTransactionResult.h | 30 +++++- src/transactions/OperationFrame.cpp | 7 +- src/transactions/OperationFrame.h | 9 +- src/transactions/SignatureChecker.cpp | 8 +- src/transactions/SignatureChecker.h | 15 ++- src/transactions/TransactionFrame.cpp | 71 ++++++------ src/transactions/TransactionFrame.h | 22 ++-- src/transactions/TransactionFrameBase.h | 5 - .../test/TransactionTestFrame.cpp | 10 -- src/transactions/test/TransactionTestFrame.h | 4 - 21 files changed, 290 insertions(+), 242 deletions(-) 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 { From c2bc090abfd6f418ccfe3b970c1fa33b82eb5106 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Fri, 15 Nov 2024 16:43:11 -0800 Subject: [PATCH 05/17] update p22 soroban rust --- src/rust/soroban/p22 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust/soroban/p22 b/src/rust/soroban/p22 index 8911531dfb..1cd8b8dca9 160000 --- a/src/rust/soroban/p22 +++ b/src/rust/soroban/p22 @@ -1 +1 @@ -Subproject commit 8911531dfb9a4331bc308ff8934c2223bc4e81ed +Subproject commit 1cd8b8dca9aeeca9ce45b129cd923992b32dc258 From 92090fe46863cdea6120d223f1e47d44c75394b8 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Mon, 18 Nov 2024 13:42:58 -0800 Subject: [PATCH 06/17] CATCHUP_SKIP_KNOWN_RESULTS=true --- src/main/CommandLine.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index be58b992c2..52e37c1201 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,6 +878,7 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; + config.CATCHUP_SKIP_KNOWN_RESULTS = true; if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && config.AUTOMATIC_MAINTENANCE_COUNT > 0) From d6041b4a911562e2e7bf7151b54cd353d2df5660 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Tue, 19 Nov 2024 10:47:31 -0800 Subject: [PATCH 07/17] Downloading results is optional, failure is not fatal. Missing results is not fatal --- src/catchup/ApplyCheckpointWork.cpp | 7 +- src/catchup/DownloadApplyTxsWork.cpp | 45 ++++++++-- src/historywork/GetAndUnzipRemoteFileWork.cpp | 90 +++++++++++++++---- src/historywork/GetAndUnzipRemoteFileWork.h | 4 +- .../test/LedgerCloseMetaStreamTests.cpp | 2 +- src/main/CommandLine.cpp | 2 +- src/main/Config.cpp | 6 +- src/main/Config.h | 2 +- src/transactions/TransactionFrame.cpp | 7 -- src/work/WorkSequence.cpp | 15 +++- src/work/WorkSequence.h | 6 +- 11 files changed, 143 insertions(+), 43 deletions(-) diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp index d9291b1c11..269f138159 100644 --- a/src/catchup/ApplyCheckpointWork.cpp +++ b/src/catchup/ApplyCheckpointWork.cpp @@ -96,7 +96,7 @@ ApplyCheckpointWork::openInputFiles() mTxIn.open(ti.localPath_nogz()); mTxHistoryEntry = TransactionHistoryEntry(); mHeaderHistoryEntry = LedgerHeaderHistoryEntry(); - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { mTxResultIn = std::make_optional(); FileTransferInfo tri(mDownloadDir, FileType::HISTORY_FILE_TYPE_RESULTS, @@ -162,7 +162,7 @@ ApplyCheckpointWork::getCurrentTxResultSet() // 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). - while (mTxResultIn) + while (mTxResultIn && mTxResultIn->readOne(*mTxHistoryResultEntry)) { if (mTxHistoryResultEntry) @@ -183,7 +183,6 @@ ApplyCheckpointWork::getCurrentTxResultSet() return std::make_optional(mTxHistoryResultEntry->txResultSet); } } - mTxResultIn->readOne(*mTxHistoryResultEntry); } CLOG_DEBUG(History, "No txresultset for ledger {}", seq); return std::nullopt; @@ -268,7 +267,7 @@ ApplyCheckpointWork::getNextLedgerCloseData() txset->sizeTxTotal()); std::optional txres = std::nullopt; - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { txres = getCurrentTxResultSet(); } diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index e18b829683..02f2fc5703 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -80,8 +80,9 @@ DownloadApplyTxsWork::yieldMoreWork() std::vector> seq{getAndUnzip}; std::vector filesToTransfer{ft}; + std::vector> optionalDownloads; - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS) + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { CLOG_INFO(History, "Downloading, unzipping and applying {} for checkpoint {}", @@ -91,8 +92,40 @@ DownloadApplyTxsWork::yieldMoreWork() FileTransferInfo resultsFile(mDownloadDir, FileType::HISTORY_FILE_TYPE_RESULTS, mCheckpointToQueue); - seq.emplace_back(std::make_shared( - mApp, resultsFile, mArchive)); + auto getResultsWork = std::make_shared( + mApp, resultsFile, mArchive, /*logErrorOnFailure=*/false); + std::weak_ptr getResultsWorkWeak = + getResultsWork; + seq.emplace_back(getResultsWork); + // If the results file is not downloaded, the apply work should still + // proceed as long as the transactions file was downloaded successfully. + optionalDownloads.push_back(getResultsWork); + seq.emplace_back(std::make_shared( + mApp, "get-results-" + std::to_string(mCheckpointToQueue), + [apply, getResultsWorkWeak, checkpoint, &dir](Application& app) { + auto getResults = getResultsWorkWeak.lock(); + if (getResults && getResults->getState() != State::WORK_SUCCESS) + { + auto archive = getResults->getArchive(); + if (archive) + { + FileTransferInfo ti(dir, + FileType::HISTORY_FILE_TYPE_RESULTS, + checkpoint); + CLOG_WARNING( + History, + "Archive {} maybe contains corrupt results file " + "{}. " + "This is not fatal as long as the archive contains " + "valid transaction history. Catchup will proceed " + "but" + "the node will not be able to skip known results.", + archive->getName(), ti.remoteName()); + } + } + return true; + })); + filesToTransfer.push_back(resultsFile); } @@ -157,7 +190,7 @@ DownloadApplyTxsWork::yieldMoreWork() seq.push_back(std::make_shared( mApp, "delete-transactions-" + - (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS + (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING ? std::string("and-results-") : "") + std::to_string(mCheckpointToQueue), @@ -183,10 +216,10 @@ DownloadApplyTxsWork::yieldMoreWork() } return true; })); - auto nextWork = std::make_shared( mApp, "download-apply-" + std::to_string(mCheckpointToQueue), seq, - BasicWork::RETRY_NEVER); + BasicWork::RETRY_NEVER, true /*stop at first failure*/, + optionalDownloads); mCheckpointToQueue += mApp.getHistoryManager().getCheckpointFrequency(); mLastYieldedWork = nextWork; return nextWork; diff --git a/src/historywork/GetAndUnzipRemoteFileWork.cpp b/src/historywork/GetAndUnzipRemoteFileWork.cpp index fef3bc0a49..412d660e74 100644 --- a/src/historywork/GetAndUnzipRemoteFileWork.cpp +++ b/src/historywork/GetAndUnzipRemoteFileWork.cpp @@ -8,7 +8,6 @@ #include "historywork/GetRemoteFileWork.h" #include "historywork/GunzipFileWork.h" #include "util/GlobalChecks.h" -#include "util/Logging.h" #include #include @@ -17,11 +16,13 @@ namespace stellar GetAndUnzipRemoteFileWork::GetAndUnzipRemoteFileWork( Application& app, FileTransferInfo ft, - std::shared_ptr archive, size_t retry) + std::shared_ptr archive, size_t retry, + bool logErrorOnFailure) : Work(app, std::string("get-and-unzip-remote-file ") + ft.remoteName(), retry) , mFt(std::move(ft)) , mArchive(archive) + , mLogErrorOnFailure(logErrorOnFailure) { } @@ -56,8 +57,16 @@ GetAndUnzipRemoteFileWork::onFailureRaise() std::shared_ptr ar = getArchive(); if (ar) { - CLOG_ERROR(History, "Archive {}: file {} is maybe corrupt", - ar->getName(), mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR(History, "Archive {}: file {} is maybe corrupt", + ar->getName(), mFt.remoteName()); + } + else + { + CLOG_WARNING(History, "Archive {}: file {} is maybe corrupt", + ar->getName(), mFt.remoteName()); + } } Work::onFailureRaise(); } @@ -81,8 +90,18 @@ GetAndUnzipRemoteFileWork::doWork() auto state = mGunzipFileWork->getState(); if (state == State::WORK_SUCCESS && !fs::exists(mFt.localPath_nogz())) { - CLOG_ERROR(History, "Downloading and unzipping {}: .xdr not found", - mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR(History, + "Downloading and unzipping {}: .nogz not found", + mFt.remoteName()); + } + else + { + CLOG_WARNING(History, + "Downloading and unzipping {}: .nogz not found", + mFt.remoteName()); + } return State::WORK_FAILURE; } return state; @@ -119,8 +138,18 @@ GetAndUnzipRemoteFileWork::validateFile() ZoneScoped; if (!fs::exists(mFt.localPath_gz_tmp())) { - CLOG_ERROR(History, "Downloading and unzipping {}: .tmp file not found", - mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR(History, + "Downloading and unzipping {}: .tmp file not found", + mFt.remoteName()); + } + else + { + CLOG_WARNING(History, + "Downloading and unzipping {}: .tmp file not found", + mFt.remoteName()); + } return false; } @@ -129,18 +158,37 @@ GetAndUnzipRemoteFileWork::validateFile() if (fs::exists(mFt.localPath_gz()) && std::remove(mFt.localPath_gz().c_str())) { - CLOG_ERROR(History, - "Downloading and unzipping {}: failed to remove .gz", - mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR(History, + "Downloading and unzipping {}: failed to remove .gz", + mFt.remoteName()); + } + else + { + CLOG_WARNING(History, + "Downloading and unzipping {}: failed to remove .gz", + mFt.remoteName()); + } return false; } if (std::rename(mFt.localPath_gz_tmp().c_str(), mFt.localPath_gz().c_str())) { - CLOG_ERROR( - History, - "Downloading and unzipping {}: failed to rename .gz.tmp to .gz", - mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR( + History, + "Downloading and unzipping {}: failed to rename .gz.tmp to .gz", + mFt.remoteName()); + } + else + { + CLOG_WARNING( + History, + "Downloading and unzipping {}: failed to rename .gz.tmp to .gz", + mFt.remoteName()); + } return false; } @@ -149,8 +197,16 @@ GetAndUnzipRemoteFileWork::validateFile() if (!fs::exists(mFt.localPath_gz())) { - CLOG_ERROR(History, "Downloading and unzipping {}: .gz not found", - mFt.remoteName()); + if (mLogErrorOnFailure) + { + CLOG_ERROR(History, "Downloading and unzipping {}: .gz not found", + mFt.remoteName()); + } + else + { + CLOG_WARNING(History, "Downloading and unzipping {}: .gz not found", + mFt.remoteName()); + } return false; } diff --git a/src/historywork/GetAndUnzipRemoteFileWork.h b/src/historywork/GetAndUnzipRemoteFileWork.h index 55c9b4e779..b6350f2b91 100644 --- a/src/historywork/GetAndUnzipRemoteFileWork.h +++ b/src/historywork/GetAndUnzipRemoteFileWork.h @@ -20,6 +20,7 @@ class GetAndUnzipRemoteFileWork : public Work FileTransferInfo mFt; std::shared_ptr const mArchive; + bool mLogErrorOnFailure; bool validateFile(); @@ -29,7 +30,8 @@ class GetAndUnzipRemoteFileWork : public Work // retries. GetAndUnzipRemoteFileWork(Application& app, FileTransferInfo ft, std::shared_ptr archive = nullptr, - size_t retry = BasicWork::RETRY_A_LOT); + size_t retry = BasicWork::RETRY_A_LOT, + bool logErrorOnFailure = true); ~GetAndUnzipRemoteFileWork() = default; std::string getStatus() const override; std::shared_ptr getArchive() const; diff --git a/src/ledger/test/LedgerCloseMetaStreamTests.cpp b/src/ledger/test/LedgerCloseMetaStreamTests.cpp index 07aaa2dcd5..2daac8fa9f 100644 --- a/src/ledger/test/LedgerCloseMetaStreamTests.cpp +++ b/src/ledger/test/LedgerCloseMetaStreamTests.cpp @@ -298,7 +298,7 @@ TEST_CASE("LedgerCloseMetaStream file descriptor - REPLAY_IN_MEMORY", SECTION("skip mode") { cfg.MODE_STORES_HISTORY_MISC = true; - cfg.CATCHUP_SKIP_KNOWN_RESULTS = true; + cfg.CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = true; } VirtualClock clock; auto app = createTestApplication(clock, cfg, /*newdb=*/false); diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index 52e37c1201..581cb1d824 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,7 +878,7 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; - config.CATCHUP_SKIP_KNOWN_RESULTS = true; + config.CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = true; if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && config.AUTOMATIC_MAINTENANCE_COUNT > 0) diff --git a/src/main/Config.cpp b/src/main/Config.cpp index 201708cd01..d093084402 100644 --- a/src/main/Config.cpp +++ b/src/main/Config.cpp @@ -1150,8 +1150,10 @@ Config::processConfig(std::shared_ptr t) CATCHUP_RECENT = readInt(item, 0, UINT32_MAX - 1); }}, - {"CATCHUP_SKIP_KNOWN_RESULTS", - [&]() { CATCHUP_SKIP_KNOWN_RESULTS = readBool(item); }}, + {"CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING", + [&]() { + CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = readBool(item); + }}, {"ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING", [&]() { ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING = readBool(item); diff --git a/src/main/Config.h b/src/main/Config.h index ca7ffe659d..e17819f41b 100644 --- a/src/main/Config.h +++ b/src/main/Config.h @@ -206,7 +206,7 @@ class Config : public std::enable_shared_from_this // Mode for "accelerated" catchup. If set to true, the node will skip // application of failed transactions and will not verify signatures of // successful transactions. - bool CATCHUP_SKIP_KNOWN_RESULTS; + bool CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING; // Interval between automatic maintenance executions std::chrono::seconds AUTOMATIC_MAINTENANCE_PERIOD; diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 46c8120868..9e07c683d4 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1576,8 +1576,6 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, Hash const& sorobanBasePrngSeed) const { ZoneScoped; - // TODO maybe encapsulate this in a function - // in MutableTransactionResultBase auto const& failingResult = txResult.getReplayFailingTransactionResult(); if (failingResult) { @@ -1706,11 +1704,6 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, SOROBAN_PROTOCOL_VERSION) && isSoroban()) { - // TODO this needs to execute in skip mode? - // We would have to execute the soroban ops to get the correct - // diagnostic info to publish to meta. This doesn't affect - // ledger state. - // If transaction fails, we don't charge for any // refundable resources. auto preApplyFee = computePreApplySorobanResourceFee( diff --git a/src/work/WorkSequence.cpp b/src/work/WorkSequence.cpp index 35af97b1de..54fb1238f7 100644 --- a/src/work/WorkSequence.cpp +++ b/src/work/WorkSequence.cpp @@ -12,10 +12,13 @@ namespace stellar WorkSequence::WorkSequence(Application& app, std::string name, std::vector> sequence, - size_t maxRetries, bool stopAtFirstFailure) + size_t maxRetries, bool stopAtFirstFailure, + std::vector> optionalWork) : BasicWork(app, std::move(name), maxRetries) , mSequenceOfWork(std::move(sequence)) , mNextInSequence(mSequenceOfWork.begin()) + , mOptionalWork{optionalWork} + , mNextOptional{mOptionalWork.begin()} , mStopAtFirstFailure(stopAtFirstFailure) { } @@ -41,9 +44,17 @@ WorkSequence::onRun() else { auto state = w->getState(); + auto workIsOptional = + mNextOptional != mOptionalWork.end() && w == *mNextOptional; if (state == State::WORK_SUCCESS || - (state == State::WORK_FAILURE && !mStopAtFirstFailure)) + (state == State::WORK_FAILURE && + (!mStopAtFirstFailure || workIsOptional))) { + // If the work is optional, increment the optional work iterator. + if (workIsOptional) + { + ++mNextOptional; + } mCurrentExecuting = nullptr; ++mNextInSequence; return this->onRun(); diff --git a/src/work/WorkSequence.h b/src/work/WorkSequence.h index c040c7b391..cf660413b1 100644 --- a/src/work/WorkSequence.h +++ b/src/work/WorkSequence.h @@ -18,6 +18,9 @@ class WorkSequence : public BasicWork { std::vector> mSequenceOfWork; std::vector>::const_iterator mNextInSequence; + // Work that, if it fails, will not block the sequence from continuing. + std::vector> mOptionalWork; + std::vector>::const_iterator mNextOptional; std::shared_ptr mCurrentExecuting; bool const mStopAtFirstFailure; @@ -25,7 +28,8 @@ class WorkSequence : public BasicWork WorkSequence(Application& app, std::string name, std::vector> sequence, size_t maxRetries = RETRY_A_FEW, - bool stopAtFirstFailure = true); + bool stopAtFirstFailure = true, + std::vector> optionalWork = {}); ~WorkSequence() = default; std::string getStatus() const override; From 42242c0957c0846234fbe9ccc901965b6f38d4b2 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Tue, 19 Nov 2024 14:31:08 -0800 Subject: [PATCH 08/17] Handle resplay for feebumptransactionframe --- src/transactions/MutableTransactionResult.cpp | 7 ++----- src/transactions/TransactionFrame.cpp | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 70279f0d50..2253a4c98c 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -485,21 +485,18 @@ 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; + return std::nullopt; } std::optional const& FeeBumpMutableTransactionResult::getReplayFailingTransactionResult() const { - return mReplayFailingTransactionResult; + return std::nullopt; } } \ No newline at end of file diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 9e07c683d4..31c248d1cd 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1581,7 +1581,6 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, { // Sub-zone for skips ZoneScopedN("skipped failed"); - txResult.setResultCode(failingResult->result.code()); txResult.getResult().result.results() = failingResult->result.results(); return false; From e7e7b96579fa6d854216c734c114e2ed51c59934 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Tue, 19 Nov 2024 17:49:02 -0800 Subject: [PATCH 09/17] Set result of mutable fee bump txn result --- src/ledger/LedgerManagerImpl.cpp | 15 +++++++++------ src/main/CommandLine.cpp | 2 ++ src/transactions/MutableTransactionResult.cpp | 8 ++++---- src/transactions/TransactionFrame.cpp | 2 ++ 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index efd12ebc61..93a0a40542 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1386,18 +1386,21 @@ LedgerManagerImpl::processFeesSeqNums( expectedResults->results.end()); releaseAssert((*expectedResultsIter)->transactionHash == tx->getContentsHash()); - if ((*expectedResultsIter)->result.result.code() == - TransactionResultCode::txSUCCESS) + auto expectedResultCode = + (*expectedResultsIter)->result.result.code(); + if (expectedResultCode == TransactionResultCode::txSUCCESS || + expectedResultCode == + TransactionResultCode::txFEE_BUMP_INNER_SUCCESS) { - CLOG_DEBUG( - Tx, "Skipping replay of successful transaction: tx {}", - binToHex(tx->getContentsHash())); + CLOG_DEBUG(Tx, "Replaying successful transaction: tx {}", + binToHex(tx->getContentsHash())); txResults.back()->setReplaySuccessfulTransactionResult( (*expectedResultsIter)->result); } else { - CLOG_DEBUG(Tx, "Replaying failing transaction: tx {}", + CLOG_DEBUG(Tx, + "Skipping replay of failing transaction: tx {}", binToHex(tx->getContentsHash())); txResults.back()->setReplayFailingTransactionResult( (*expectedResultsIter)->result); diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index 581cb1d824..84aee37d88 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,6 +878,8 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; + + // SET TO TRUE FOR TESTING PURPOSES, will be removed pre-merge. config.CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = true; if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 2253a4c98c..de55ce060b 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -477,26 +477,26 @@ void FeeBumpMutableTransactionResult::setReplaySuccessfulTransactionResult( TransactionResult const& successful) { - // no-op + mInnerTxResult->setReplaySuccessfulTransactionResult(successful); } void FeeBumpMutableTransactionResult::setReplayFailingTransactionResult( TransactionResult const& failing) { - // no-op + mInnerTxResult->setReplayFailingTransactionResult(failing); } std::optional const& FeeBumpMutableTransactionResult::getReplaySuccessfulTransactionResult() const { - return std::nullopt; + return mInnerTxResult->getReplaySuccessfulTransactionResult(); } std::optional const& FeeBumpMutableTransactionResult::getReplayFailingTransactionResult() const { - return std::nullopt; + return mInnerTxResult->getReplayFailingTransactionResult(); } } \ No newline at end of file diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 31c248d1cd..ed1b25eb10 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1581,6 +1581,8 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, { // Sub-zone for skips ZoneScopedN("skipped failed"); + CLOG_DEBUG(Tx, "Skipping failed transaction: tx {}", + binToHex(getContentsHash())); txResult.setResultCode(failingResult->result.code()); txResult.getResult().result.results() = failingResult->result.results(); return false; From a30ae35e3a27797056e4b990c5b8643d533435da Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Tue, 19 Nov 2024 20:32:44 -0800 Subject: [PATCH 10/17] remove feebump special case --- src/ledger/LedgerManagerImpl.cpp | 11 ++++++----- src/transactions/MutableTransactionResult.cpp | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 93a0a40542..593881db22 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1388,11 +1388,11 @@ LedgerManagerImpl::processFeesSeqNums( tx->getContentsHash()); auto expectedResultCode = (*expectedResultsIter)->result.result.code(); - if (expectedResultCode == TransactionResultCode::txSUCCESS || - expectedResultCode == - TransactionResultCode::txFEE_BUMP_INNER_SUCCESS) + if (expectedResultCode == TransactionResultCode::txSUCCESS) { - CLOG_DEBUG(Tx, "Replaying successful transaction: tx {}", + CLOG_DEBUG(Tx, + "Skipping signature verification successful " + "transaction: tx {}", binToHex(tx->getContentsHash())); txResults.back()->setReplaySuccessfulTransactionResult( (*expectedResultsIter)->result); @@ -1400,7 +1400,8 @@ LedgerManagerImpl::processFeesSeqNums( else { CLOG_DEBUG(Tx, - "Skipping replay of failing transaction: tx {}", + "Skipping replay and signature verification of " + "failing transaction: tx {}", binToHex(tx->getContentsHash())); txResults.back()->setReplayFailingTransactionResult( (*expectedResultsIter)->result); diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index de55ce060b..9e1824c2fc 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -477,26 +477,26 @@ void FeeBumpMutableTransactionResult::setReplaySuccessfulTransactionResult( TransactionResult const& successful) { - mInnerTxResult->setReplaySuccessfulTransactionResult(successful); + /* NO-OP */ } void FeeBumpMutableTransactionResult::setReplayFailingTransactionResult( TransactionResult const& failing) { - mInnerTxResult->setReplayFailingTransactionResult(failing); + /* NO-OP */ } std::optional const& FeeBumpMutableTransactionResult::getReplaySuccessfulTransactionResult() const { - return mInnerTxResult->getReplaySuccessfulTransactionResult(); + return mReplaySuccessfulTransactionResult; } std::optional const& FeeBumpMutableTransactionResult::getReplayFailingTransactionResult() const { - return mInnerTxResult->getReplayFailingTransactionResult(); + return mReplayFailingTransactionResult; } } \ No newline at end of file From 6d2fa0bb1e08ce78d2b599d3351b08a02a89369a Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Wed, 20 Nov 2024 10:57:42 -0800 Subject: [PATCH 11/17] Just one variable for replay result --- src/ledger/LedgerManagerImpl.cpp | 23 ++-------- src/transactions/MutableTransactionResult.cpp | 44 ++++--------------- src/transactions/MutableTransactionResult.h | 35 +++++---------- src/transactions/TransactionFrame.cpp | 13 +++--- 4 files changed, 28 insertions(+), 87 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 593881db22..fcb45e6ba3 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1369,6 +1369,7 @@ LedgerManagerImpl::processFeesSeqNums( expectedResultsIter = std::nullopt; if (expectedResults) { + releaseAssert(mApp.getCatchupManager().isCatchupInitialized()); expectedResultsIter = std::make_optional(expectedResults->results.begin()); } @@ -1386,26 +1387,8 @@ LedgerManagerImpl::processFeesSeqNums( expectedResults->results.end()); releaseAssert((*expectedResultsIter)->transactionHash == tx->getContentsHash()); - auto expectedResultCode = - (*expectedResultsIter)->result.result.code(); - if (expectedResultCode == TransactionResultCode::txSUCCESS) - { - CLOG_DEBUG(Tx, - "Skipping signature verification successful " - "transaction: tx {}", - binToHex(tx->getContentsHash())); - txResults.back()->setReplaySuccessfulTransactionResult( - (*expectedResultsIter)->result); - } - else - { - CLOG_DEBUG(Tx, - "Skipping replay and signature verification of " - "failing transaction: tx {}", - binToHex(tx->getContentsHash())); - txResults.back()->setReplayFailingTransactionResult( - (*expectedResultsIter)->result); - } + txResults.back()->setReplayTransactionResult((*expectedResultsIter) + ->result); ++(*expectedResultsIter); } diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 9e1824c2fc..0298263602 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -315,29 +315,16 @@ MutableTransactionResult::isSuccess() const } void -MutableTransactionResult::setReplaySuccessfulTransactionResult( - TransactionResult const& success) +MutableTransactionResult::setReplayTransactionResult( + TransactionResult const& replayResult) { - mReplaySuccessfulTransactionResult = std::make_optional(success); -} - -void -MutableTransactionResult::setReplayFailingTransactionResult( - TransactionResult const& failing) -{ - mReplayFailingTransactionResult = std::make_optional(failing); + mReplayTransactionResult = std::make_optional(replayResult); } std::optional const& -MutableTransactionResult::getReplaySuccessfulTransactionResult() const +MutableTransactionResult::getReplayTransactionResult() const { - return mReplaySuccessfulTransactionResult; -} - -std::optional const& -MutableTransactionResult::getReplayFailingTransactionResult() const -{ - return mReplayFailingTransactionResult; + return mReplayTransactionResult; } FeeBumpMutableTransactionResult::FeeBumpMutableTransactionResult( @@ -473,30 +460,17 @@ 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) +FeeBumpMutableTransactionResult::setReplayTransactionResult( + TransactionResult const& replayResult) { /* NO-OP */ } std::optional const& -FeeBumpMutableTransactionResult::getReplaySuccessfulTransactionResult() const -{ - return mReplaySuccessfulTransactionResult; -} - -std::optional const& -FeeBumpMutableTransactionResult::getReplayFailingTransactionResult() const +FeeBumpMutableTransactionResult::getReplayTransactionResult() const { - return mReplayFailingTransactionResult; + return mReplayTransactionResult; } - } \ No newline at end of file diff --git a/src/transactions/MutableTransactionResult.h b/src/transactions/MutableTransactionResult.h index b76ceb6b9b..e39c6b9f4d 100644 --- a/src/transactions/MutableTransactionResult.h +++ b/src/transactions/MutableTransactionResult.h @@ -71,10 +71,7 @@ class MutableTransactionResultBase : public NonMovableOrCopyable { protected: std::unique_ptr mTxResult; - std::optional mReplaySuccessfulTransactionResult{ - std::nullopt}; - std::optional mReplayFailingTransactionResult{ - std::nullopt}; + std::optional mReplayTransactionResult{std::nullopt}; MutableTransactionResultBase(); MutableTransactionResultBase(MutableTransactionResultBase&& rhs); @@ -101,15 +98,11 @@ 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 std::optional const& + getReplayTransactionResult() const = 0; virtual void - setReplayFailingTransactionResult(TransactionResult const& failing) = 0; - virtual void - setReplaySuccessfulTransactionResult(TransactionResult const& success) = 0; + setReplayTransactionResult(TransactionResult const& replayResult) = 0; }; class MutableTransactionResult : public MutableTransactionResultBase @@ -135,14 +128,10 @@ 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; + void + setReplayTransactionResult(TransactionResult const& replayResult) override; std::optional const& - getReplayFailingTransactionResult() const override; + getReplayTransactionResult() const override; OperationResult& getOpResultAt(size_t index) override; std::shared_ptr getSorobanData() override; @@ -197,13 +186,9 @@ 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; + void + setReplayTransactionResult(TransactionResult const& replayResult) override; std::optional const& - getReplayFailingTransactionResult() const override; + getReplayTransactionResult() const override; }; } \ No newline at end of file diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index ed1b25eb10..50b4b07633 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1576,15 +1576,15 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, Hash const& sorobanBasePrngSeed) const { ZoneScoped; - auto const& failingResult = txResult.getReplayFailingTransactionResult(); - if (failingResult) + auto const& result = txResult.getReplayTransactionResult(); + if (result && result->result.code() != txSUCCESS) { // Sub-zone for skips ZoneScopedN("skipped failed"); - CLOG_DEBUG(Tx, "Skipping failed transaction: tx {}", + CLOG_DEBUG(Tx, "Skipping replay of failed transaction: tx {}", binToHex(getContentsHash())); - txResult.setResultCode(failingResult->result.code()); - txResult.getResult().result.results() = failingResult->result.results(); + txResult.setResultCode(result->result.code()); + txResult.getResult().result.results() = result->result.results(); return false; } @@ -1803,8 +1803,7 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, // If the txResult has a replay result (catchup in skip mode is // enabled), // we do not perform signature verification. - if (txResult->getReplayFailingTransactionResult() || - txResult->getReplaySuccessfulTransactionResult()) + if (txResult->getReplayTransactionResult()) { signatureChecker = std::make_unique( ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); From 3c2e52b9aa47f55a6dbcc7132a2c8cf676a1fdae Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Wed, 20 Nov 2024 11:02:19 -0800 Subject: [PATCH 12/17] format --- src/ledger/LedgerManagerImpl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index fcb45e6ba3..cb026b9d76 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1387,8 +1387,8 @@ LedgerManagerImpl::processFeesSeqNums( expectedResults->results.end()); releaseAssert((*expectedResultsIter)->transactionHash == tx->getContentsHash()); - txResults.back()->setReplayTransactionResult((*expectedResultsIter) - ->result); + txResults.back()->setReplayTransactionResult( + (*expectedResultsIter)->result); ++(*expectedResultsIter); } From c4514633e0448302a214a4287e2318dec1b716ff Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Thu, 21 Nov 2024 16:42:30 -0800 Subject: [PATCH 13/17] only access results() of tx results with code txFAILED, move skip mode to ifdef BUILDTEST --- src/catchup/ApplyCheckpointWork.cpp | 6 ++++++ src/catchup/ApplyCheckpointWork.h | 6 +++++- src/catchup/DownloadApplyTxsWork.cpp | 3 ++- src/herder/LedgerCloseData.h | 2 ++ src/historywork/GetAndUnzipRemoteFileWork.cpp | 1 + src/ledger/LedgerManagerImpl.cpp | 4 ++++ src/main/CommandLine.cpp | 3 ++- src/main/Config.cpp | 2 ++ src/main/Config.h | 2 ++ src/transactions/MutableTransactionResult.cpp | 4 ++++ src/transactions/MutableTransactionResult.h | 6 ++++++ src/transactions/SignatureChecker.h | 2 ++ src/transactions/TransactionFrame.cpp | 12 +++++++++++- 13 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp index 269f138159..f99add7be3 100644 --- a/src/catchup/ApplyCheckpointWork.cpp +++ b/src/catchup/ApplyCheckpointWork.cpp @@ -96,6 +96,7 @@ ApplyCheckpointWork::openInputFiles() mTxIn.open(ti.localPath_nogz()); mTxHistoryEntry = TransactionHistoryEntry(); mHeaderHistoryEntry = LedgerHeaderHistoryEntry(); +#ifdef BUILD_TESTS if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { mTxResultIn = std::make_optional(); @@ -107,6 +108,7 @@ ApplyCheckpointWork::openInputFiles() mTxHistoryResultEntry = std::make_optional(); } +#endif mFilesOpen = true; } @@ -152,6 +154,7 @@ ApplyCheckpointWork::getCurrentTxSet() return TxSetXDRFrame::makeEmpty(lm.getLastClosedLedgerHeader()); } +#ifdef BUILD_TESTS std::optional ApplyCheckpointWork::getCurrentTxResultSet() { @@ -187,6 +190,7 @@ ApplyCheckpointWork::getCurrentTxResultSet() CLOG_DEBUG(History, "No txresultset for ledger {}", seq); return std::nullopt; } +#endif // BUILD_TESTS std::shared_ptr ApplyCheckpointWork::getNextLedgerCloseData() @@ -267,10 +271,12 @@ ApplyCheckpointWork::getNextLedgerCloseData() txset->sizeTxTotal()); std::optional txres = std::nullopt; +#ifdef BUILD_TESTS if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { txres = getCurrentTxResultSet(); } +#endif // We've verified the ledgerHeader (in the "trusted part of history" // sense) in CATCHUP_VERIFY phase; we now need to check that the diff --git a/src/catchup/ApplyCheckpointWork.h b/src/catchup/ApplyCheckpointWork.h index abf2f6780f..f85f8c52bd 100644 --- a/src/catchup/ApplyCheckpointWork.h +++ b/src/catchup/ApplyCheckpointWork.h @@ -52,9 +52,11 @@ class ApplyCheckpointWork : public BasicWork XDRInputFileStream mHdrIn; XDRInputFileStream mTxIn; - std::optional mTxResultIn; TransactionHistoryEntry mTxHistoryEntry; +#ifdef BUILD_TESTS + std::optional mTxResultIn; std::optional mTxHistoryResultEntry; +#endif // BUILD_TESTS LedgerHeaderHistoryEntry mHeaderHistoryEntry; OnFailureCallback mOnFailure; @@ -63,7 +65,9 @@ class ApplyCheckpointWork : public BasicWork std::shared_ptr mConditionalWork; TxSetXDRFrameConstPtr getCurrentTxSet(); +#ifdef BUILD_TESTS std::optional getCurrentTxResultSet(); +#endif // BUILD_TESTS void openInputFiles(); std::shared_ptr getNextLedgerCloseData(); diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index 02f2fc5703..ab06cd500f 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -81,7 +81,7 @@ DownloadApplyTxsWork::yieldMoreWork() std::vector> seq{getAndUnzip}; std::vector filesToTransfer{ft}; std::vector> optionalDownloads; - +#ifdef BUILD_TESTS if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { CLOG_INFO(History, @@ -128,6 +128,7 @@ DownloadApplyTxsWork::yieldMoreWork() filesToTransfer.push_back(resultsFile); } +#endif // BUILD_TESTS auto maybeWaitForMerges = [](Application& app) { if (app.getConfig().CATCHUP_WAIT_MERGES_TX_APPLY_FOR_TESTING) diff --git a/src/herder/LedgerCloseData.h b/src/herder/LedgerCloseData.h index a8a4039426..4b57b5cc6b 100644 --- a/src/herder/LedgerCloseData.h +++ b/src/herder/LedgerCloseData.h @@ -50,11 +50,13 @@ class LedgerCloseData { return mExpectedLedgerHash; } +#ifdef BUILD_TESTS std::optional const& getExpectedResults() const { return mExpectedResults; } +#endif // BUILD_TESTS StoredDebugTransactionSet toXDR() const diff --git a/src/historywork/GetAndUnzipRemoteFileWork.cpp b/src/historywork/GetAndUnzipRemoteFileWork.cpp index 412d660e74..d25232b669 100644 --- a/src/historywork/GetAndUnzipRemoteFileWork.cpp +++ b/src/historywork/GetAndUnzipRemoteFileWork.cpp @@ -8,6 +8,7 @@ #include "historywork/GetRemoteFileWork.h" #include "historywork/GunzipFileWork.h" #include "util/GlobalChecks.h" +#include "util/Logging.h" #include #include diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index cb026b9d76..4996cf8530 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1363,6 +1363,7 @@ LedgerManagerImpl::processFeesSeqNums( auto header = ltx.loadHeader().current(); std::map accToMaxSeq; +#ifdef BUILD_TESTS // If we have expected results, we assign them to the mutable tx results // here. std::optional::const_iterator> @@ -1373,6 +1374,7 @@ LedgerManagerImpl::processFeesSeqNums( expectedResultsIter = std::make_optional(expectedResults->results.begin()); } +#endif bool mergeSeen = false; for (auto tx : txs) @@ -1381,6 +1383,7 @@ LedgerManagerImpl::processFeesSeqNums( txResults.push_back( tx->processFeeSeqNum(ltxTx, txSet.getTxBaseFee(tx, header))); +#ifdef BUILD_TESTS if (expectedResultsIter) { releaseAssert(*expectedResultsIter != @@ -1391,6 +1394,7 @@ LedgerManagerImpl::processFeesSeqNums( (*expectedResultsIter)->result); ++(*expectedResultsIter); } +#endif // BUILD_TESTS if (protocolVersionStartsFrom( ltxTx.loadHeader().current().ledgerVersion, diff --git a/src/main/CommandLine.cpp b/src/main/CommandLine.cpp index 84aee37d88..3a6feac424 100644 --- a/src/main/CommandLine.cpp +++ b/src/main/CommandLine.cpp @@ -878,9 +878,10 @@ runCatchup(CommandLineArgs const& args) config.RUN_STANDALONE = true; config.MANUAL_CLOSE = true; config.DISABLE_BUCKET_GC = disableBucketGC; - +#ifdef BUILD_TESTS // SET TO TRUE FOR TESTING PURPOSES, will be removed pre-merge. config.CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = true; +#endif // BUILD_TESTS if (config.AUTOMATIC_MAINTENANCE_PERIOD.count() > 0 && config.AUTOMATIC_MAINTENANCE_COUNT > 0) diff --git a/src/main/Config.cpp b/src/main/Config.cpp index d093084402..ea20daec07 100644 --- a/src/main/Config.cpp +++ b/src/main/Config.cpp @@ -1150,10 +1150,12 @@ Config::processConfig(std::shared_ptr t) CATCHUP_RECENT = readInt(item, 0, UINT32_MAX - 1); }}, +#ifdef BUILD_TESTS {"CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING", [&]() { CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING = readBool(item); }}, +#endif // BUILD_TESTS {"ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING", [&]() { ARTIFICIALLY_GENERATE_LOAD_FOR_TESTING = readBool(item); diff --git a/src/main/Config.h b/src/main/Config.h index e17819f41b..aef1a762ca 100644 --- a/src/main/Config.h +++ b/src/main/Config.h @@ -203,10 +203,12 @@ class Config : public std::enable_shared_from_this // If you want, say, a week of history, set this to 120000. uint32_t CATCHUP_RECENT; +#ifdef BUILD_TESTS // Mode for "accelerated" catchup. If set to true, the node will skip // application of failed transactions and will not verify signatures of // successful transactions. bool CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING; +#endif // BUILD_TESTS // Interval between automatic maintenance executions std::chrono::seconds AUTOMATIC_MAINTENANCE_PERIOD; diff --git a/src/transactions/MutableTransactionResult.cpp b/src/transactions/MutableTransactionResult.cpp index 0298263602..2de2bdcd19 100644 --- a/src/transactions/MutableTransactionResult.cpp +++ b/src/transactions/MutableTransactionResult.cpp @@ -314,6 +314,7 @@ MutableTransactionResult::isSuccess() const return getResult().result.code() == txSUCCESS; } +#ifdef BUILD_TESTS void MutableTransactionResult::setReplayTransactionResult( TransactionResult const& replayResult) @@ -326,6 +327,7 @@ MutableTransactionResult::getReplayTransactionResult() const { return mReplayTransactionResult; } +#endif // BUILD_TESTS FeeBumpMutableTransactionResult::FeeBumpMutableTransactionResult( MutableTxResultPtr innerTxResult) @@ -461,6 +463,7 @@ FeeBumpMutableTransactionResult::isSuccess() const return mTxResult->result.code() == txFEE_BUMP_INNER_SUCCESS; } +#ifdef BUILD_TESTS void FeeBumpMutableTransactionResult::setReplayTransactionResult( TransactionResult const& replayResult) @@ -473,4 +476,5 @@ FeeBumpMutableTransactionResult::getReplayTransactionResult() const { return mReplayTransactionResult; } +#endif // BUILD_TESTS } \ No newline at end of file diff --git a/src/transactions/MutableTransactionResult.h b/src/transactions/MutableTransactionResult.h index e39c6b9f4d..bfac8f7a90 100644 --- a/src/transactions/MutableTransactionResult.h +++ b/src/transactions/MutableTransactionResult.h @@ -99,10 +99,12 @@ class MutableTransactionResultBase : public NonMovableOrCopyable virtual void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) = 0; virtual bool isSuccess() const = 0; +#ifdef BUILD_TESTS virtual std::optional const& getReplayTransactionResult() const = 0; virtual void setReplayTransactionResult(TransactionResult const& replayResult) = 0; +#endif }; class MutableTransactionResult : public MutableTransactionResultBase @@ -128,10 +130,12 @@ class MutableTransactionResult : public MutableTransactionResultBase TransactionResult const& getResult() const override; TransactionResultCode getResultCode() const override; void setResultCode(TransactionResultCode code) override; +#ifdef BUILD_TESTS void setReplayTransactionResult(TransactionResult const& replayResult) override; std::optional const& getReplayTransactionResult() const override; +#endif // BUILD_TESTS OperationResult& getOpResultAt(size_t index) override; std::shared_ptr getSorobanData() override; @@ -186,9 +190,11 @@ class FeeBumpMutableTransactionResult : public MutableTransactionResultBase void refundSorobanFee(int64_t feeRefund, uint32_t ledgerVersion) override; bool isSuccess() const override; +#ifdef BUILD_TESTS void setReplayTransactionResult(TransactionResult const& replayResult) override; std::optional const& getReplayTransactionResult() const override; +#endif // BUILD_TESTS }; } \ No newline at end of file diff --git a/src/transactions/SignatureChecker.h b/src/transactions/SignatureChecker.h index 2c06879f8c..f3805901c8 100644 --- a/src/transactions/SignatureChecker.h +++ b/src/transactions/SignatureChecker.h @@ -42,6 +42,7 @@ class SignatureChecker : public AbstractSignatureChecker std::vector mUsedSignatures; }; +#ifdef BUILD_TESTS class AlwaysValidSignatureChecker : public AbstractSignatureChecker { public: @@ -62,4 +63,5 @@ class AlwaysValidSignatureChecker : public AbstractSignatureChecker return true; } }; +#endif // BUILD_TESTS } diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 50b4b07633..cf4a6f7781 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1576,6 +1576,7 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, Hash const& sorobanBasePrngSeed) const { ZoneScoped; +#ifdef BUILD_TESTS auto const& result = txResult.getReplayTransactionResult(); if (result && result->result.code() != txSUCCESS) { @@ -1584,9 +1585,14 @@ TransactionFrame::applyOperations(AbstractSignatureChecker& signatureChecker, CLOG_DEBUG(Tx, "Skipping replay of failed transaction: tx {}", binToHex(getContentsHash())); txResult.setResultCode(result->result.code()); - txResult.getResult().result.results() = result->result.results(); + // results field is only active if code is txFAILED or txSUCCESS + if (result->result.code() == txFAILED) + { + txResult.getResult().result.results() = result->result.results(); + } return false; } +#endif auto& internalErrorCounter = app.getMetrics().NewCounter( {"ledger", "transaction", "internal-error"}); @@ -1800,6 +1806,7 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, mCachedAccountPreProtocol8.reset(); uint32_t ledgerVersion = ltx.loadHeader().current().ledgerVersion; std::unique_ptr signatureChecker; +#ifdef BUILD_TESTS // If the txResult has a replay result (catchup in skip mode is // enabled), // we do not perform signature verification. @@ -1810,9 +1817,12 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, } else { +#endif // BUILD_TESTS signatureChecker = std::make_unique( ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); +#ifdef BUILD_TESTS } +#endif // BUILD_TESTS // when applying, a failure during tx validation means that // we'll skip trying to apply operations but we'll still From f02ab8733516e464fd927234e74c3bb21a1dc392 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Thu, 21 Nov 2024 20:26:52 -0800 Subject: [PATCH 14/17] ifdef --- src/catchup/DownloadApplyTxsWork.cpp | 13 ++++++++----- src/main/main.cpp | 1 - 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index ab06cd500f..b282a02a28 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -188,13 +188,16 @@ DownloadApplyTxsWork::yieldMoreWork() mApp, "wait-merges" + apply->getName(), maybeWaitForMerges, apply)); } + std::string deleteWorkName = "delete-transactions-"; + #ifdef BUILD_TESTS + if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) + { + deleteWorkName += "-and-results-"; + } + #endif // BUILD_TESTS seq.push_back(std::make_shared( mApp, - "delete-transactions-" + - (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING - ? std::string("and-results-") - : "") + - std::to_string(mCheckpointToQueue), + deleteWorkName + std::to_string(mCheckpointToQueue), [filesToTransfer](Application& app) { for (auto const& ft : filesToTransfer) { diff --git a/src/main/main.cpp b/src/main/main.cpp index edcd3ffb58..de25566195 100644 --- a/src/main/main.cpp +++ b/src/main/main.cpp @@ -1,7 +1,6 @@ // Copyright 2014 Stellar Development Foundation and contributors. Licensed // under the Apache License, Version 2.0. See the COPYING file at the root // of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 - #include "crypto/CryptoError.h" #include "invariant/InvariantDoesNotHold.h" #include "ledger/NonSociRelatedException.h" From 8d8b6a65c8e7484d1a45a8f9f1607990046e735e Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Thu, 21 Nov 2024 20:42:55 -0800 Subject: [PATCH 15/17] format --- src/catchup/DownloadApplyTxsWork.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index b282a02a28..394a656800 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -189,15 +189,14 @@ DownloadApplyTxsWork::yieldMoreWork() } std::string deleteWorkName = "delete-transactions-"; - #ifdef BUILD_TESTS +#ifdef BUILD_TESTS if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) { deleteWorkName += "-and-results-"; } - #endif // BUILD_TESTS +#endif // BUILD_TESTS seq.push_back(std::make_shared( - mApp, - deleteWorkName + std::to_string(mCheckpointToQueue), + mApp, deleteWorkName + std::to_string(mCheckpointToQueue), [filesToTransfer](Application& app) { for (auto const& ft : filesToTransfer) { From b23ffdc5ef3f6eb6a392aebbefd4f6c44d42585e Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Thu, 21 Nov 2024 20:56:40 -0800 Subject: [PATCH 16/17] ifdefs, moving results plumbing to one function' --- src/ledger/LedgerManagerImpl.cpp | 9 +++++---- src/ledger/LedgerManagerImpl.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 4996cf8530..39ad81bddc 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -914,9 +914,9 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData) // first, prefetch source accounts for txset, then charge fees prefetchTxSourceIds(txs); - auto const mutableTxResults = - processFeesSeqNums(txs, ltx, *applicableTxSet, ledgerCloseMeta, - ledgerData.getExpectedResults()); + + auto const mutableTxResults = processFeesSeqNums( + txs, ltx, *applicableTxSet, ledgerCloseMeta, ledgerData); TransactionResultSet txResultSet; txResultSet.results.reserve(txs.size()); @@ -1350,7 +1350,7 @@ LedgerManagerImpl::processFeesSeqNums( std::vector const& txs, AbstractLedgerTxn& ltxOuter, ApplicableTxSetFrame const& txSet, std::unique_ptr const& ledgerCloseMeta, - std::optional const& expectedResults) + LedgerCloseData const& ledgerData) { ZoneScoped; std::vector txResults; @@ -1368,6 +1368,7 @@ LedgerManagerImpl::processFeesSeqNums( // here. std::optional::const_iterator> expectedResultsIter = std::nullopt; + auto expectedResults = ledgerData.getExpectedResults(); if (expectedResults) { releaseAssert(mApp.getCatchupManager().isCatchupInitialized()); diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index 87fde6e551..b265220cc9 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -78,7 +78,7 @@ class LedgerManagerImpl : public LedgerManager std::vector const& txs, AbstractLedgerTxn& ltxOuter, ApplicableTxSetFrame const& txSet, std::unique_ptr const& ledgerCloseMeta, - std::optional const& expectedResults); + LedgerCloseData const& ledgerData); void applyTransactions( ApplicableTxSetFrame const& txSet, From fb725ab9b1da65fadbbd70a1bb23acedb8916738 Mon Sep 17 00:00:00 2001 From: Thomas Brady Date: Fri, 22 Nov 2024 20:23:18 -0800 Subject: [PATCH 17/17] refactor, extra checks around file access, remove optional work param from workseq --- src/catchup/ApplyCheckpointWork.cpp | 33 +++++++++++++++++++++++---- src/catchup/DownloadApplyTxsWork.cpp | 28 ++++++++--------------- src/historywork/GetRemoteFileWork.cpp | 1 + src/work/WorkSequence.cpp | 15 ++---------- src/work/WorkSequence.h | 6 +---- 5 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp index f99add7be3..7fdf4f8604 100644 --- a/src/catchup/ApplyCheckpointWork.cpp +++ b/src/catchup/ApplyCheckpointWork.cpp @@ -102,11 +102,34 @@ ApplyCheckpointWork::openInputFiles() 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 = - std::make_optional(); + if (!tri.localPath_nogz().empty() && + std::filesystem::exists(tri.localPath_nogz())) + { + CLOG_DEBUG(History, "Replaying transaction results from {}", + tri.localPath_nogz()); + + try + { + mTxResultIn->open(tri.localPath_nogz()); + } + catch (std::exception const& e) + { + CLOG_DEBUG(History, + "Failed to open transaction results file: {}. All " + "transactions will be applied.", + e.what()); + } + mTxHistoryResultEntry = + std::make_optional(); + } + else + { + CLOG_DEBUG(History, + "Results file {} not found for checkpoint {} . All " + "transactions will be applied for this checkpoint.", + tri.localPath_nogz(), mCheckpoint); + mTxHistoryResultEntry = std::nullopt; + } } #endif mFilesOpen = true; diff --git a/src/catchup/DownloadApplyTxsWork.cpp b/src/catchup/DownloadApplyTxsWork.cpp index 394a656800..3ecb25d59c 100644 --- a/src/catchup/DownloadApplyTxsWork.cpp +++ b/src/catchup/DownloadApplyTxsWork.cpp @@ -97,9 +97,6 @@ DownloadApplyTxsWork::yieldMoreWork() std::weak_ptr getResultsWorkWeak = getResultsWork; seq.emplace_back(getResultsWork); - // If the results file is not downloaded, the apply work should still - // proceed as long as the transactions file was downloaded successfully. - optionalDownloads.push_back(getResultsWork); seq.emplace_back(std::make_shared( mApp, "get-results-" + std::to_string(mCheckpointToQueue), [apply, getResultsWorkWeak, checkpoint, &dir](Application& app) { @@ -188,18 +185,12 @@ DownloadApplyTxsWork::yieldMoreWork() mApp, "wait-merges" + apply->getName(), maybeWaitForMerges, apply)); } - std::string deleteWorkName = "delete-transactions-"; -#ifdef BUILD_TESTS - if (mApp.getConfig().CATCHUP_SKIP_KNOWN_RESULTS_FOR_TESTING) + for (auto const& ft : filesToTransfer) { - deleteWorkName += "-and-results-"; - } -#endif // BUILD_TESTS - seq.push_back(std::make_shared( - mApp, deleteWorkName + std::to_string(mCheckpointToQueue), - [filesToTransfer](Application& app) { - for (auto const& ft : filesToTransfer) - { + auto deleteWorkName = "delete-" + ft.getTypeString() + "-" + + std::to_string(mCheckpointToQueue); + seq.push_back(std::make_shared( + mApp, deleteWorkName, [ft](Application& app) { CLOG_DEBUG(History, "Deleting {} {}", ft.getTypeString(), ft.localPath_nogz()); try @@ -216,13 +207,12 @@ DownloadApplyTxsWork::yieldMoreWork() e.what()); return false; } - } - return true; - })); + return true; + })); + } auto nextWork = std::make_shared( mApp, "download-apply-" + std::to_string(mCheckpointToQueue), seq, - BasicWork::RETRY_NEVER, true /*stop at first failure*/, - optionalDownloads); + BasicWork::RETRY_NEVER, true /*stop at first failure*/); mCheckpointToQueue += mApp.getHistoryManager().getCheckpointFrequency(); mLastYieldedWork = nextWork; return nextWork; diff --git a/src/historywork/GetRemoteFileWork.cpp b/src/historywork/GetRemoteFileWork.cpp index 044ea6f725..23a0d95405 100644 --- a/src/historywork/GetRemoteFileWork.cpp +++ b/src/historywork/GetRemoteFileWork.cpp @@ -42,6 +42,7 @@ GetRemoteFileWork::getCommand() releaseAssert(mCurrentArchive); releaseAssert(mCurrentArchive->hasGetCmd()); auto cmdLine = mCurrentArchive->getFileCmd(mRemote, mLocal); + CLOG_DEBUG(History, "Downloading file: cmd: {}", cmdLine); return CommandInfo{cmdLine, std::string()}; } diff --git a/src/work/WorkSequence.cpp b/src/work/WorkSequence.cpp index 54fb1238f7..35af97b1de 100644 --- a/src/work/WorkSequence.cpp +++ b/src/work/WorkSequence.cpp @@ -12,13 +12,10 @@ namespace stellar WorkSequence::WorkSequence(Application& app, std::string name, std::vector> sequence, - size_t maxRetries, bool stopAtFirstFailure, - std::vector> optionalWork) + size_t maxRetries, bool stopAtFirstFailure) : BasicWork(app, std::move(name), maxRetries) , mSequenceOfWork(std::move(sequence)) , mNextInSequence(mSequenceOfWork.begin()) - , mOptionalWork{optionalWork} - , mNextOptional{mOptionalWork.begin()} , mStopAtFirstFailure(stopAtFirstFailure) { } @@ -44,17 +41,9 @@ WorkSequence::onRun() else { auto state = w->getState(); - auto workIsOptional = - mNextOptional != mOptionalWork.end() && w == *mNextOptional; if (state == State::WORK_SUCCESS || - (state == State::WORK_FAILURE && - (!mStopAtFirstFailure || workIsOptional))) + (state == State::WORK_FAILURE && !mStopAtFirstFailure)) { - // If the work is optional, increment the optional work iterator. - if (workIsOptional) - { - ++mNextOptional; - } mCurrentExecuting = nullptr; ++mNextInSequence; return this->onRun(); diff --git a/src/work/WorkSequence.h b/src/work/WorkSequence.h index cf660413b1..c040c7b391 100644 --- a/src/work/WorkSequence.h +++ b/src/work/WorkSequence.h @@ -18,9 +18,6 @@ class WorkSequence : public BasicWork { std::vector> mSequenceOfWork; std::vector>::const_iterator mNextInSequence; - // Work that, if it fails, will not block the sequence from continuing. - std::vector> mOptionalWork; - std::vector>::const_iterator mNextOptional; std::shared_ptr mCurrentExecuting; bool const mStopAtFirstFailure; @@ -28,8 +25,7 @@ class WorkSequence : public BasicWork WorkSequence(Application& app, std::string name, std::vector> sequence, size_t maxRetries = RETRY_A_FEW, - bool stopAtFirstFailure = true, - std::vector> optionalWork = {}); + bool stopAtFirstFailure = true); ~WorkSequence() = default; std::string getStatus() const override;