From 096f5b306485fda337e0a6dd2bd589bbdc73a2e9 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Wed, 3 Jul 2024 11:50:21 -0700 Subject: [PATCH] wip --- src/test/FuzzerImpl.cpp | 24 ++++- src/transactions/FeeBumpTransactionFrame.cpp | 2 +- src/transactions/OperationFrame.cpp | 100 ++++++++++++++----- src/transactions/OperationFrame.h | 21 +++- src/transactions/TransactionFrame.cpp | 63 +++++++----- src/transactions/TransactionFrame.h | 16 +-- 6 files changed, 156 insertions(+), 70 deletions(-) diff --git a/src/test/FuzzerImpl.cpp b/src/test/FuzzerImpl.cpp index e3f08b75b7..00e3e88d9f 100644 --- a/src/test/FuzzerImpl.cpp +++ b/src/test/FuzzerImpl.cpp @@ -924,10 +924,24 @@ class FuzzTransactionFrame : public TransactionFrame ltx.loadHeader().current().ledgerVersion, getContentsHash(), mEnvelope.v1().signatures}; // if any ill-formed Operations, do not attempt transaction application - auto isInvalidOperation = [&](auto const& op, auto& opResult) { - return !op->checkValid(app, signatureChecker, ltx, false, opResult, - mTxResult->getSorobanData()); - }; + auto isInvalidOperation = + [&](std::shared_ptr const& op, + auto& opResult) { + std::optional sorobanCfg{}; + if (protocolVersionStartsFrom( + ltx.loadHeader().current().ledgerVersion, + SOROBAN_PROTOCOL_VERSION)) + { + sorobanCfg = + app.getLedgerManager().getSorobanNetworkConfig(); + } + + auto ret = op->checkValid( + ltx, app.getConfig(), sorobanCfg, + ltx.loadHeader().current(), signatureChecker, false, + opResult, mTxResult->getSorobanData()); + return !ret; + }; auto const& ops = getOperations(); for (size_t i = 0; i < ops.size(); ++i) @@ -944,7 +958,7 @@ class FuzzTransactionFrame : public TransactionFrame // while the following method's result is not captured, regardless, for // protocols < 8, this triggered buggy caching, and potentially may do // so in the future - loadSourceAccount(ltx, ltx.loadHeader()); + loadSourceAccount(ltx, ltx.loadHeader().current()); processSeqNum(ltx); TransactionMetaFrame tm(2); applyOperations(signatureChecker, app, ltx, tm, *mTxResult, Hash{}); diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp index 3b0638510b..d6a75f5931 100644 --- a/src/transactions/FeeBumpTransactionFrame.cpp +++ b/src/transactions/FeeBumpTransactionFrame.cpp @@ -208,7 +208,7 @@ FeeBumpTransactionFrame::checkValidInternal( SignatureChecker signatureChecker{header.ledgerVersion, getContentsHash(), mEnvelope.feeBump().signatures}; - if (commonValid(signatureChecker, dbLoader, header, false, *txResult) != + if (commonValid(signatureChecker, dbLoader, header, false, *txResult) != ValidationType::kFullyValid) { return txResult; diff --git a/src/transactions/OperationFrame.cpp b/src/transactions/OperationFrame.cpp index cfa9c7097f..91d3a24b35 100644 --- a/src/transactions/OperationFrame.cpp +++ b/src/transactions/OperationFrame.cpp @@ -45,9 +45,9 @@ namespace stellar using namespace std; static int32_t -getNeededThreshold(LedgerTxnEntry const& account, ThresholdLevel const level) +getNeededThreshold(LedgerEntry const& account, ThresholdLevel const level) { - auto const& acc = account.current().data.account(); + auto const& acc = account.data.account(); switch (level) { case ThresholdLevel::LOW: @@ -142,8 +142,22 @@ OperationFrame::apply(Application& app, SignatureChecker& signatureChecker, { ZoneScoped; CLOG_TRACE(Tx, "{}", xdrToCerealString(mOperation, "Operation")); - bool applyRes = - checkValid(app, signatureChecker, ltx, true, res, sorobanData); + std::optional sorobanCfg{}; + if (protocolVersionStartsFrom(ltx.loadHeader().current().ledgerVersion, + SOROBAN_PROTOCOL_VERSION)) + { + sorobanCfg = app.getLedgerManager().getSorobanNetworkConfig(); + } + + bool applyRes; + { + LedgerTxn checkValidLtx(ltx); + applyRes = checkValid( + checkValidLtx, app.getConfig(), sorobanCfg, + checkValidLtx.loadHeader().current(), signatureChecker, true, res, + sorobanData); + } + if (applyRes) { if (isSoroban()) @@ -185,19 +199,19 @@ OperationFrame::isOpSupported(LedgerHeader const&) const return true; } +template bool -OperationFrame::checkSignature(SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltx, OperationResult& res, - bool forApply) const +OperationFrame::checkSignature(T& dbLoader, LedgerHeader const& header, + SignatureChecker& signatureChecker, + OperationResult& res, bool forApply) const { ZoneScoped; - auto header = ltx.loadHeader(); - auto sourceAccount = loadSourceAccount(ltx, header); + auto sourceAccount = loadReadOnlySourceAccount(dbLoader, header); if (sourceAccount) { auto neededThreshold = - getNeededThreshold(sourceAccount, getThresholdLevel()); - if (!mParentTx.checkSignature(signatureChecker, sourceAccount.current(), + getNeededThreshold(*sourceAccount, getThresholdLevel()); + if (!mParentTx.checkSignature(signatureChecker, *sourceAccount, neededThreshold)) { res.code(opBAD_AUTH); @@ -234,26 +248,28 @@ OperationFrame::getSourceID() const // called when determining if we should flood // make sure sig is correct // verifies that the operation is well formed (operation specific) +template bool -OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, bool forApply, - OperationResult& res, - std::shared_ptr sorobanData) const +OperationFrame::checkValid( + T& dbLoader, Config const& cfg, + std::optional const& sorobanCfg, + LedgerHeader const& header, SignatureChecker& signatureChecker, + bool forApply, OperationResult& res, + std::shared_ptr sorobanData) const { ZoneScoped; - // Note: ltx is always rolled back so checkValid never modifies the ledger - LedgerTxn ltx(ltxOuter); - if (!isOpSupported(ltx.loadHeader().current())) + if (!isOpSupported(header)) { res.code(opNOT_SUPPORTED); return false; } - auto ledgerVersion = ltx.loadHeader().current().ledgerVersion; + auto ledgerVersion = header.ledgerVersion; if (!forApply || protocolVersionIsBefore(ledgerVersion, ProtocolVersion::V_10)) { - if (!checkSignature(signatureChecker, ltx, res, forApply)) + if (!checkSignature(dbLoader, header, signatureChecker, res, + forApply)) { return false; } @@ -262,7 +278,7 @@ OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker, { // for ledger versions >= 10 we need to load account here, as for // previous versions it is done in checkSignature call - if (!loadSourceAccount(ltx, ltx.loadHeader())) + if (!loadReadOnlySourceAccount(dbLoader, header)) { res.code(opNO_ACCOUNT); return false; @@ -275,11 +291,9 @@ OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker, isSoroban()) { releaseAssertOrThrow(sorobanData); - auto const& sorobanConfig = - app.getLedgerManager().getSorobanNetworkConfig(); - - return doCheckValidForSoroban(sorobanConfig, app.getConfig(), - ledgerVersion, res, *sorobanData); + releaseAssertOrThrow(sorobanCfg); + return doCheckValidForSoroban(*sorobanCfg, cfg, ledgerVersion, res, + *sorobanData); } else { @@ -287,6 +301,21 @@ OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker, } } +template bool OperationFrame::checkValid( + AbstractLedgerTxn& dbLoader, Config const& cfg, + std::optional const& sorobanCfg, + LedgerHeader const& header, SignatureChecker& signatureChecker, + bool forApply, OperationResult& res, + std::shared_ptr sorobanData) const; + +template bool +OperationFrame::checkValid>( + std::shared_ptr& dbLoader, Config const& cfg, + std::optional const& sorobanCfg, + LedgerHeader const& header, SignatureChecker& signatureChecker, + bool forApply, OperationResult& res, + std::shared_ptr sorobanData) const; + bool OperationFrame::doCheckValidForSoroban(SorobanNetworkConfig const& config, Config const& appConfig, @@ -302,7 +331,24 @@ OperationFrame::loadSourceAccount(AbstractLedgerTxn& ltx, LedgerTxnHeader const& header) const { ZoneScoped; - return mParentTx.loadAccount(ltx, header, getSourceID()); + return mParentTx.loadAccount(ltx, header.current(), getSourceID()); +} + +std::shared_ptr +OperationFrame::loadReadOnlySourceAccount( + std::shared_ptr bl, + LedgerHeader const& header) const +{ + ZoneScoped; + return mParentTx.loadReadOnlySourceAccount(bl, header); +} + +std::shared_ptr +OperationFrame::loadReadOnlySourceAccount(AbstractLedgerTxn& ltx, + LedgerHeader const& header) const +{ + ZoneScoped; + return mParentTx.loadReadOnlySourceAccount(ltx, header); } void diff --git a/src/transactions/OperationFrame.h b/src/transactions/OperationFrame.h index f161f1ca0e..0d41e11b56 100644 --- a/src/transactions/OperationFrame.h +++ b/src/transactions/OperationFrame.h @@ -67,6 +67,13 @@ class OperationFrame LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, LedgerTxnHeader const& header) const; + std::shared_ptr + loadReadOnlySourceAccount(std::shared_ptr bl, + LedgerHeader const& header) const; + std::shared_ptr + loadReadOnlySourceAccount(AbstractLedgerTxn& ltx, + LedgerHeader const& header) const; + public: static std::shared_ptr makeHelper(Operation const& op, TransactionFrame const& parentTx, @@ -79,14 +86,18 @@ class OperationFrame // given an operation result, gives a default value representing "success" void resetResultSuccess(OperationResult& res) const; - bool checkSignature(SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltx, OperationResult& res, - bool forApply) const; + template + bool checkSignature(T& dbLoader, LedgerHeader const& header, + SignatureChecker& signatureChecker, + OperationResult& res, bool forApply) const; AccountID getSourceID() const; - bool checkValid(Application& app, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, bool forApply, + template + bool checkValid(T& dbLoader, Config const& cfg, + std::optional const& sorobanCfg, + LedgerHeader const& header, + SignatureChecker& signatureChecker, bool forApply, OperationResult& res, std::shared_ptr sorobanData) const; diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 58c2465b37..39659d8161 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -321,15 +321,18 @@ TransactionFrame::checkExtraSigners(SignatureChecker& signatureChecker) const std::shared_ptr TransactionFrame::loadReadOnlySourceAccount( - std::shared_ptr bl) const + std::shared_ptr bl, + LedgerHeader const& header) const { + releaseAssert(false); return bl->getLedgerEntry(accountKey(getSourceID())); } std::shared_ptr -TransactionFrame::loadReadOnlySourceAccount(AbstractLedgerTxn& ltx) const +TransactionFrame::loadReadOnlySourceAccount(AbstractLedgerTxn& ltx, + LedgerHeader const& header) const { - auto ret = loadSourceAccount(ltx, ltx.loadHeader()); + auto ret = loadSourceAccount(ltx, header); if (ret) { return std::make_shared(ret.current()); @@ -340,12 +343,11 @@ TransactionFrame::loadReadOnlySourceAccount(AbstractLedgerTxn& ltx) const LedgerTxnEntry TransactionFrame::loadSourceAccount(AbstractLedgerTxn& ltx, - LedgerTxnHeader const& header) const + LedgerHeader const& header) const { ZoneScoped; auto res = loadAccount(ltx, header, getSourceID()); - if (protocolVersionIsBefore(header.current().ledgerVersion, - ProtocolVersion::V_8)) + if (protocolVersionIsBefore(header.ledgerVersion, ProtocolVersion::V_8)) { // this is buggy caching that existed in old versions of the protocol if (res) @@ -363,12 +365,11 @@ TransactionFrame::loadSourceAccount(AbstractLedgerTxn& ltx, LedgerTxnEntry TransactionFrame::loadAccount(AbstractLedgerTxn& ltx, - LedgerTxnHeader const& header, + LedgerHeader const& header, AccountID const& accountID) const { ZoneScoped; - if (protocolVersionIsBefore(header.current().ledgerVersion, - ProtocolVersion::V_8) && + if (protocolVersionIsBefore(header.ledgerVersion, ProtocolVersion::V_8) && mCachedAccountPreProtocol8 && mCachedAccountPreProtocol8->ledgerEntry().data.account().accountID == accountID) @@ -699,7 +700,7 @@ TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter, auto header = ltx.loadHeader(); // The fee source could be from a Fee-bump, so it needs to be forwarded here // instead of using TransactionFrame's getFeeSource() method - auto feeSourceAccount = loadAccount(ltx, header, feeSource); + auto feeSourceAccount = loadAccount(ltx, header.current(), feeSource); if (!feeSourceAccount) { // Account was merged (shouldn't be possible) @@ -1074,7 +1075,7 @@ TransactionFrame::commonValidPreSeqNum( return false; } - if (!loadReadOnlySourceAccount(dbLoader)) + if (!loadReadOnlySourceAccount(dbLoader, header)) { txResult->setInnermostResultCode(txNO_ACCOUNT); return false; @@ -1091,7 +1092,7 @@ TransactionFrame::processSeqNum(AbstractLedgerTxn& ltx) const if (protocolVersionStartsFrom(header.current().ledgerVersion, ProtocolVersion::V_10)) { - auto sourceAccount = loadSourceAccount(ltx, header); + auto sourceAccount = loadSourceAccount(ltx, header.current()); if (sourceAccount.current().data.account().seqNum > getSeqNum()) { throw std::runtime_error("unexpected sequence number"); @@ -1143,7 +1144,9 @@ TransactionFrame::processSignatures( { auto const& op = mOperations[i]; auto& opResult = txResult.getOpResultAt(i); - if (!op->checkSignature(signatureChecker, ltx, opResult, false)) + if (!op->checkSignature( + ltx, ltx.loadHeader().current(), signatureChecker, opResult, + false)) { allOpsValid = false; } @@ -1222,7 +1225,7 @@ TransactionFrame::commonValid( return res; } - auto sourceAccount = *loadReadOnlySourceAccount(dbLoader); + auto sourceAccount = *loadReadOnlySourceAccount(dbLoader, header); // in older versions, the account's sequence number is updated when taking // fees @@ -1297,7 +1300,7 @@ TransactionFrame::processFeeSeqNum(AbstractLedgerTxn& ltx, createSuccessResultWithFeeCharged(header.current(), baseFee, true); releaseAssert(txResult); - auto sourceAccount = loadSourceAccount(ltx, header); + auto sourceAccount = loadSourceAccount(ltx, header.current()); if (!sourceAccount) { throw std::runtime_error("Unexpected database state"); @@ -1450,15 +1453,16 @@ TransactionFrame::checkValidWithOptionallyChargedFee( auto const& op = mOperations[i]; auto& opResult = txResult->getOpResultAt(i); - // if (!op->checkValid(app, signatureChecker, ltx, false, opResult, - // txResult->getSorobanData())) - // { - // // it's OK to just fast fail here and not try to call - // // checkValid on all operations as the resulting object - // // is only used by applications - // txResult->setInnermostResultCode(txFAILED); - // return txResult; - // } + if (!op->checkValid(dbLoader, cfg, sorobanCfg, header, + signatureChecker, false, opResult, + txResult->getSorobanData())) + { + // it's OK to just fast fail here and not try to call + // checkValid on all operations as the resulting object + // is only used by applications + txResult->setInnermostResultCode(txFAILED); + return txResult; + } } if (!signatureChecker.checkAllSignaturesUsed()) @@ -1471,6 +1475,15 @@ TransactionFrame::checkValidWithOptionallyChargedFee( return txResult; } +template MutableTxResultPtr +TransactionFrame::checkValidWithOptionallyChargedFee< + std::shared_ptr>( + std::shared_ptr& dbLoader, Config const& cfg, + std::optional const& sorobanCfg, + LedgerHeader const& header, SequenceNumber current, bool chargeFee, + uint64_t lowerBoundCloseTimeOffset, + uint64_t upperBoundCloseTimeOffset) const; + MutableTxResultPtr TransactionFrame::checkValid(std::shared_ptr bl, Config const cfg, @@ -1501,7 +1514,7 @@ TransactionFrame::checkValid(Application& app, AbstractLedgerTxn& ltxOuter, sorobanCfg = app.getLedgerManager().getSorobanNetworkConfig(); } - return checkValidWithOptionallyChargedFee( + return checkValidWithOptionallyChargedFee( ltx, app.getConfig(), sorobanCfg, header, current, true, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); } diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index b774f2eb78..6e34ac6090 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -71,13 +71,8 @@ class TransactionFrame : public TransactionFrameBase std::vector> mOperations; - std::shared_ptr loadReadOnlySourceAccount( - std::shared_ptr bl) const; - std::shared_ptr - loadReadOnlySourceAccount(AbstractLedgerTxn& ltx) const; - LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, - LedgerTxnHeader const& header) const; + LedgerHeader const& header) const; enum ValidationType { @@ -178,6 +173,13 @@ class TransactionFrame : public TransactionFrameBase return mOperations; } + std::shared_ptr + loadReadOnlySourceAccount(std::shared_ptr bl, + LedgerHeader const& header) const; + std::shared_ptr + loadReadOnlySourceAccount(AbstractLedgerTxn& ltx, + LedgerHeader const& header) const; + #ifdef BUILD_TESTS TransactionEnvelope& getMutableEnvelope() const override; void clearCached() const override; @@ -287,7 +289,7 @@ class TransactionFrame : public TransactionFrameBase std::shared_ptr toStellarMessage() const override; LedgerTxnEntry loadAccount(AbstractLedgerTxn& ltx, - LedgerTxnHeader const& header, + LedgerHeader const& header, AccountID const& accountID) const; std::optional const getMinSeqNum() const override;