From 51379b026973fe81527b28c32e498123a1a2ea49 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Tue, 9 Jul 2024 18:37:37 -0700 Subject: [PATCH] WIP --- src/test/FuzzerImpl.cpp | 14 ++++-- src/transactions/OperationFrame.cpp | 67 ++++++++++++++++----------- src/transactions/OperationFrame.h | 15 +++--- src/transactions/TransactionFrame.cpp | 49 ++++++++++++++++++-- src/transactions/TransactionFrame.h | 4 ++ 5 files changed, 107 insertions(+), 42 deletions(-) diff --git a/src/test/FuzzerImpl.cpp b/src/test/FuzzerImpl.cpp index 9eda55d2bd..f28f94fe04 100644 --- a/src/test/FuzzerImpl.cpp +++ b/src/test/FuzzerImpl.cpp @@ -918,17 +918,21 @@ class FuzzTransactionFrame : public TransactionFrame // reset results of operations mTxResult = createSuccessResultWithFeeCharged(ltx.getHeader(), 0, true); + auto header = ltx.loadHeader(); + // attempt application of transaction without processing the fee or // committing the LedgerTxn - SignatureChecker signatureChecker{ - ltx.loadHeader().current().ledgerVersion, getContentsHash(), - mEnvelope.v1().signatures}; + SignatureChecker signatureChecker{header.current().ledgerVersion, + getContentsHash(), + mEnvelope.v1().signatures}; // if any ill-formed Operations, do not attempt transaction application auto isInvalidOperation = [&](auto const& op, auto& opResult) { + auto ltxe = op->loadSourceAccount(ltx, header); + auto sourceAccount = LtxReadOnlyResult::create(std::move(ltxe)); return !op->checkValid( &app.getLedgerManager().getSorobanNetworkConfig(), - app.getConfig(), signatureChecker, ltx, false, opResult, - mTxResult->getSorobanData()); + app.getConfig(), signatureChecker, sourceAccount, + header.current(), false, opResult, mTxResult->getSorobanData()); }; auto const& ops = getOperations(); diff --git a/src/transactions/OperationFrame.cpp b/src/transactions/OperationFrame.cpp index 406e9abec4..02db7163a7 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: @@ -149,8 +149,16 @@ OperationFrame::apply(Application& app, SignatureChecker& signatureChecker, sorobanCfg = &app.getLedgerManager().getSorobanNetworkConfig(); } - bool applyRes = checkValid(sorobanCfg, app.getConfig(), signatureChecker, - ltx, true, res, sorobanData); + bool applyRes; + { + LedgerTxn cvLtx(ltx); + auto header = cvLtx.loadHeader(); + auto sourceAccount = + LtxReadOnlyResult::create(loadSourceAccount(cvLtx, header)); + applyRes = + checkValid(sorobanCfg, app.getConfig(), signatureChecker, + sourceAccount, header.current(), true, res, sorobanData); + } if (applyRes) { if (isSoroban()) @@ -194,24 +202,12 @@ OperationFrame::isOpSupported(LedgerHeader const&) const bool OperationFrame::checkSignature(SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltx, OperationResult& res, + ReadOnlyResultPtr sourceAccount, + LedgerHeader const& header, OperationResult& res, bool forApply) const { ZoneScoped; - auto header = ltx.loadHeader(); - auto sourceAccount = loadSourceAccount(ltx, header); - if (sourceAccount) - { - auto neededThreshold = - getNeededThreshold(sourceAccount, getThresholdLevel()); - if (!mParentTx.checkSignature(signatureChecker, sourceAccount.current(), - neededThreshold)) - { - res.code(opBAD_AUTH); - return false; - } - } - else + if (sourceAccount->isDead()) { if (forApply || !mOperation.sourceAccount) { @@ -226,6 +222,17 @@ OperationFrame::checkSignature(SignatureChecker& signatureChecker, return false; } } + else + { + auto neededThreshold = + getNeededThreshold(sourceAccount->entry(), getThresholdLevel()); + if (!mParentTx.checkSignature(signatureChecker, sourceAccount->entry(), + neededThreshold)) + { + res.code(opBAD_AUTH); + return false; + } + } return true; } @@ -245,24 +252,24 @@ bool OperationFrame::checkValid(SorobanNetworkConfig const* const sorobanCfg, Config const& cfg, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, bool forApply, + ReadOnlyResultPtr sourceAccount, + LedgerHeader const& header, 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(signatureChecker, sourceAccount, header, res, + forApply)) { return false; } @@ -271,7 +278,7 @@ OperationFrame::checkValid(SorobanNetworkConfig const* const sorobanCfg, { // 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 (sourceAccount->isDead()) { res.code(opNO_ACCOUNT); return false; @@ -312,6 +319,14 @@ OperationFrame::loadSourceAccount(AbstractLedgerTxn& ltx, return mParentTx.loadAccount(ltx, header.current(), getSourceID()); } +ReadOnlyResultPtr +OperationFrame::loadSourceAccount(ReadOnlyState& roState, + LedgerHeader const& header) const +{ + ZoneScoped; + return mParentTx.loadReadOnlyAccount(roState, header, getSourceID()); +} + void OperationFrame::resetResultSuccess(OperationResult& res) const { diff --git a/src/transactions/OperationFrame.h b/src/transactions/OperationFrame.h index 20160df2c9..624d2e8637 100644 --- a/src/transactions/OperationFrame.h +++ b/src/transactions/OperationFrame.h @@ -64,14 +64,16 @@ class OperationFrame // header flags virtual bool isOpSupported(LedgerHeader const& header) const; - LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, - LedgerTxnHeader const& header) const; - public: static std::shared_ptr makeHelper(Operation const& op, TransactionFrame const& parentTx, uint32_t index); + LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, + LedgerTxnHeader const& header) const; + ReadOnlyResultPtr loadSourceAccount(ReadOnlyState& roState, + LedgerHeader const& header) const; + OperationFrame(Operation const& op, TransactionFrame const& parentTx); OperationFrame(OperationFrame const&) = delete; virtual ~OperationFrame() = default; @@ -80,15 +82,16 @@ class OperationFrame void resetResultSuccess(OperationResult& res) const; bool checkSignature(SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltx, OperationResult& res, + ReadOnlyResultPtr sourceAccount, + LedgerHeader const& header, OperationResult& res, bool forApply) const; AccountID getSourceID() const; bool checkValid(SorobanNetworkConfig const* const sorobanCfg, Config const& cfg, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, bool forApply, - OperationResult& res, + ReadOnlyResultPtr sourceAccount, LedgerHeader const& header, + bool forApply, OperationResult& res, std::shared_ptr sorobanData) const; bool apply(Application& app, SignatureChecker& signatureChecker, diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 0472a0129d..3b88b77c3a 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -392,6 +392,27 @@ TransactionFrame::loadAccount(AbstractLedgerTxn& ltx, } } +ReadOnlyResultPtr +TransactionFrame::loadReadOnlyAccount(ReadOnlyState& roState, + LedgerHeader const& header, + AccountID const& accountID) const +{ + ZoneScoped; + + // While this function should be truly read-only, we need to preserve a + // buggy cache from older versions of the protocol, resulting in this messy + // cast and potential write via loadSourceAccount. + if (protocolVersionIsBefore(header.ledgerVersion, ProtocolVersion::V_8)) + { + releaseAssert(threadIsMain()); + auto& ltx = static_cast(roState).getLedgerTxn(); + auto ltxe = loadAccount(ltx, header, accountID); + return LtxReadOnlyResult::create(std::move(ltxe)); + } + + return roState.loadEntry(accountKey(accountID)); +} + bool TransactionFrame::hasDexOperations() const { @@ -1136,11 +1157,17 @@ TransactionFrame::processSignatures( { // scope here to avoid potential side effects of loading source accounts LedgerTxn ltx(ltxOuter); + auto header = ltx.loadHeader(); + for (size_t i = 0; i < mOperations.size(); ++i) { auto const& op = mOperations[i]; auto& opResult = txResult.getOpResultAt(i); - if (!op->checkSignature(signatureChecker, ltx, opResult, false)) + + auto sourceAccount = + LtxReadOnlyResult::create(op->loadSourceAccount(ltx, header)); + if (!op->checkSignature(signatureChecker, sourceAccount, + header.current(), opResult, false)) { allOpsValid = false; } @@ -1444,15 +1471,27 @@ TransactionFrame::checkValidWithOptionallyChargedFee( txResult) == ValidationType::kMaybeValid; if (res) { - // TODO: Pipe sourceAccount directly to op - auto& ltx = static_cast(roState).getLedgerTxn(); for (size_t i = 0; i < mOperations.size(); ++i) { auto const& op = mOperations[i]; auto& opResult = txResult->getOpResultAt(i); - if (!op->checkValid(sorobanCfg, cfg, signatureChecker, ltx, false, - opResult, txResult->getSorobanData())) + bool cv; + if (getSourceID() == op->getSourceID()) + { + cv = op->checkValid(sorobanCfg, cfg, signatureChecker, + sourceAccount, header, false, opResult, + txResult->getSorobanData()); + } + else + { + auto opSourceAccount = op->loadSourceAccount(roState, header); + cv = op->checkValid(sorobanCfg, cfg, signatureChecker, + opSourceAccount, header, false, opResult, + txResult->getSorobanData()); + } + + if (!cv) { // it's OK to just fast fail here and not try to call // checkValid on all operations as the resulting object diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index 34081ca284..9e4285a543 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -279,6 +279,10 @@ class TransactionFrame : public TransactionFrameBase LedgerHeader const& header, AccountID const& accountID) const; + ReadOnlyResultPtr loadReadOnlyAccount(ReadOnlyState& roState, + LedgerHeader const& header, + AccountID const& accountID) const; + std::optional const getMinSeqNum() const override; Duration getMinSeqAge() const override; uint32 getMinSeqLedgerGap() const override;