From 6121196f2745df34df8fc103af23756558217aa0 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Tue, 9 Jul 2024 14:43:39 -0700 Subject: [PATCH] Refactored checkValid with ReadOnlyState --- src/ledger/ReadOnlyState.cpp | 92 ++++++++++++++++++++ src/ledger/ReadOnlyState.h | 88 +++++++++++++++++++ src/transactions/FeeBumpTransactionFrame.cpp | 5 +- src/transactions/TransactionFrame.cpp | 79 ++++++++++++----- src/transactions/TransactionFrame.h | 12 ++- 5 files changed, 248 insertions(+), 28 deletions(-) create mode 100644 src/ledger/ReadOnlyState.cpp create mode 100644 src/ledger/ReadOnlyState.h diff --git a/src/ledger/ReadOnlyState.cpp b/src/ledger/ReadOnlyState.cpp new file mode 100644 index 0000000000..588bb62493 --- /dev/null +++ b/src/ledger/ReadOnlyState.cpp @@ -0,0 +1,92 @@ +// Copyright 2024 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 "bucket/BucketListSnapshot.h" +#include "ledger/LedgerTxn.h" +#include "ledger/ReadOnlyState.h" +#include "util/GlobalChecks.h" + +namespace stellar +{ + +LtxReadOnlyResult::LtxReadOnlyResult(LedgerTxnEntry&& entry) + : mEntry(std::move(entry)) +{ +} + +ReadOnlyResultPtr +LtxReadOnlyResult::create(LedgerTxnEntry&& entry) +{ + return ReadOnlyResultPtr(new LtxReadOnlyResult(std::move(entry))); +} + +LedgerEntry const& +LtxReadOnlyResult::entry() const +{ + return mEntry.current(); +} + +bool +LtxReadOnlyResult::isDead() const +{ + return !static_cast(mEntry); +} + +BucketListReadOnlyResult::BucketListReadOnlyResult( + std::shared_ptr entry) + : mEntry(entry) +{ +} + +ReadOnlyResultPtr +BucketListReadOnlyResult::create(std::shared_ptr entry) +{ + return ReadOnlyResultPtr(new BucketListReadOnlyResult(entry)); +} + +LedgerEntry const& +BucketListReadOnlyResult::entry() const +{ + releaseAssertOrThrow(mEntry); + return *mEntry; +} + +bool +BucketListReadOnlyResult::isDead() const +{ + return !static_cast(mEntry); +} + +BucketListReadOnlyState::BucketListReadOnlyState( + SearchableBucketListSnapshot& snapshot) + : mSnapshot(snapshot) +{ +} + +ReadOnlyResultPtr +BucketListReadOnlyState::loadEntry(LedgerKey const& key) +{ + auto result = mSnapshot.getLedgerEntry(key); + return BucketListReadOnlyResult::create(result); +} + +LtxReadOnlyState::LtxReadOnlyState(AbstractLedgerTxn& ltx) : mLtx(ltx) +{ + releaseAssert(threadIsMain()); +} + +ReadOnlyResultPtr +LtxReadOnlyState::loadEntry(LedgerKey const& key) +{ + releaseAssert(threadIsMain()); + auto result = mLtx.load(key); + return LtxReadOnlyResult::create(std::move(result)); +} + +AbstractLedgerTxn& +LtxReadOnlyState::getLedgerTxn() +{ + return mLtx; +} +} \ No newline at end of file diff --git a/src/ledger/ReadOnlyState.h b/src/ledger/ReadOnlyState.h new file mode 100644 index 0000000000..fa1db63365 --- /dev/null +++ b/src/ledger/ReadOnlyState.h @@ -0,0 +1,88 @@ +#pragma once + +// Copyright 2024 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 "ledger/LedgerTxnEntry.h" +#include "util/NonCopyable.h" +#include "util/types.h" + +#include + +namespace stellar +{ + +class AbstractLedgerTxn; +class SearchableBucketListSnapshot; +class ReadOnlyResult; +class ReadOnlyState; + +typedef std::shared_ptr ReadOnlyResultPtr; + +// ReadOnlyResult is a generic wrapper for the result of loading a LedgerEntry +// loaded either from a LedgerTxn or from a BucketList snapshot. +class ReadOnlyResult +{ + public: + virtual LedgerEntry const& entry() const = 0; + virtual bool isDead() const = 0; +}; + +class LtxReadOnlyResult : public ReadOnlyResult +{ + private: + LedgerTxnEntry mEntry; + LtxReadOnlyResult(LedgerTxnEntry&& entry); + + public: + static ReadOnlyResultPtr create(LedgerTxnEntry&& entry); + + LedgerEntry const& entry() const override; + bool isDead() const override; +}; + +class BucketListReadOnlyResult : public ReadOnlyResult +{ + private: + std::shared_ptr mEntry; + BucketListReadOnlyResult(std::shared_ptr entry); + + public: + static ReadOnlyResultPtr create(std::shared_ptr entry); + + LedgerEntry const& entry() const override; + bool isDead() const override; +}; + +// ReadOnlyState is an interface for loading LedgerEntries from either a +// LedgerTxn object or a SearchableBucketListSnapshot +class ReadOnlyState +{ + public: + virtual ReadOnlyResultPtr loadEntry(LedgerKey const& key) = 0; +}; + +class BucketListReadOnlyState : public ReadOnlyState +{ + private: + SearchableBucketListSnapshot& mSnapshot; + + public: + BucketListReadOnlyState(SearchableBucketListSnapshot& snapshot); + + ReadOnlyResultPtr loadEntry(LedgerKey const& key) override; +}; + +class LtxReadOnlyState : public ReadOnlyState +{ + private: + AbstractLedgerTxn& mLtx; + + public: + LtxReadOnlyState(AbstractLedgerTxn& ledgerTxn); + + ReadOnlyResultPtr loadEntry(LedgerKey const& key) override; + AbstractLedgerTxn& getLedgerTxn(); +}; +} \ No newline at end of file diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp index 66fcb9cc3c..aff1ae941e 100644 --- a/src/transactions/FeeBumpTransactionFrame.cpp +++ b/src/transactions/FeeBumpTransactionFrame.cpp @@ -191,10 +191,11 @@ FeeBumpTransactionFrame::checkValid(Application& app, auto header = ltx.loadHeader().current(); auto sorobanCfg = app.getLedgerManager().maybeGetSorobanNetworkConfigPtr( header.ledgerVersion); + LtxReadOnlyState roState(ltx); auto innerTxResult = mInnerTx->checkValidWithOptionallyChargedFee( - ltx, cfg, sorobanCfg, header, current, false, lowerBoundCloseTimeOffset, - upperBoundCloseTimeOffset); + roState, cfg, sorobanCfg, header, current, false, + lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); auto finalTxResult = createSuccessResultWithNewInnerTx( std::move(txResult), std::move(innerTxResult), mInnerTx); diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 672266c694..0472a0129d 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -318,6 +318,26 @@ TransactionFrame::checkExtraSigners(SignatureChecker& signatureChecker) const return true; } +ReadOnlyResultPtr +TransactionFrame::loadReadOnlySourceAccount(ReadOnlyState& roState, + LedgerHeader const& header) 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 = loadSourceAccount(ltx, header); + return LtxReadOnlyResult::create(std::move(ltxe)); + } + + return roState.loadEntry(accountKey(getSourceID())); +} + LedgerTxnEntry TransactionFrame::loadSourceAccount(AbstractLedgerTxn& ltx, LedgerHeader const& header) const @@ -867,7 +887,7 @@ TransactionFrame::isTooEarlyForAccount(LedgerHeader const& header, bool TransactionFrame::commonValidPreSeqNum( - AbstractLedgerTxn& ltx, Config const& cfg, + ReadOnlyResultPtr sourceAccount, Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset, @@ -1051,7 +1071,7 @@ TransactionFrame::commonValidPreSeqNum( return false; } - if (!loadSourceAccount(ltx, header)) + if (sourceAccount->isDead()) { txResult->setInnermostResultCode(txNO_ACCOUNT); return false; @@ -1173,7 +1193,7 @@ TransactionFrame::ValidationType TransactionFrame::commonValid( Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, SequenceNumber current, bool applying, + ReadOnlyResultPtr sourceAccount, SequenceNumber current, bool applying, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset, std::optional sorobanResourceFee, @@ -1181,7 +1201,6 @@ TransactionFrame::commonValid( { ZoneScoped; releaseAssertOrThrow(txResult); - LedgerTxn ltx(ltxOuter); ValidationType res = ValidationType::kInvalid; if (applying && @@ -1191,14 +1210,15 @@ TransactionFrame::commonValid( "Applying transaction with non-current closeTime"); } - if (!commonValidPreSeqNum( - ltx, cfg, sorobanCfg, header, chargeFee, lowerBoundCloseTimeOffset, - upperBoundCloseTimeOffset, sorobanResourceFee, txResult)) + if (!commonValidPreSeqNum(sourceAccount, cfg, sorobanCfg, header, chargeFee, + lowerBoundCloseTimeOffset, + upperBoundCloseTimeOffset, sorobanResourceFee, + txResult)) { return res; } - auto sourceAccount = loadSourceAccount(ltx, header); + releaseAssertOrThrow(!sourceAccount->isDead()); // in older versions, the account's sequence number is updated when taking // fees @@ -1208,7 +1228,7 @@ TransactionFrame::commonValid( { if (current == 0) { - current = sourceAccount.current().data.account().seqNum; + current = sourceAccount->entry().data.account().seqNum; } if (isBadSeq(header, current)) { @@ -1219,7 +1239,7 @@ TransactionFrame::commonValid( res = ValidationType::kInvalidUpdateSeqNum; - if (isTooEarlyForAccount(header, sourceAccount.current(), + if (isTooEarlyForAccount(header, sourceAccount->entry(), lowerBoundCloseTimeOffset)) { txResult->setInnermostResultCode(txBAD_MIN_SEQ_AGE_OR_GAP); @@ -1227,8 +1247,8 @@ TransactionFrame::commonValid( } if (!checkSignature( - signatureChecker, sourceAccount.current(), - sourceAccount.current().data.account().thresholds[THRESHOLD_LOW])) + signatureChecker, sourceAccount->entry(), + sourceAccount->entry().data.account().thresholds[THRESHOLD_LOW])) { txResult->setInnermostResultCode(txBAD_AUTH); return res; @@ -1255,7 +1275,7 @@ TransactionFrame::commonValid( // don't let the account go below the reserve after accounting for // liabilities if (chargeFee && - getAvailableBalance(header, sourceAccount.current()) < feeToPay) + getAvailableBalance(header, sourceAccount->entry()) < feeToPay) { txResult->setInnermostResultCode(txINSUFFICIENT_BALANCE); return res; @@ -1379,7 +1399,7 @@ TransactionFrame::removeAccountSigner(AbstractLedgerTxn& ltxOuter, MutableTxResultPtr TransactionFrame::checkValidWithOptionallyChargedFee( - AbstractLedgerTxn& ltxOuter, Config const& cfg, + ReadOnlyState& roState, Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, SequenceNumber current, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset) const @@ -1394,7 +1414,6 @@ TransactionFrame::checkValidWithOptionallyChargedFee( return txResult; } - LedgerTxn ltx(ltxOuter); int64_t minBaseFee = header.baseFee; if (!chargeFee) { @@ -1416,12 +1435,17 @@ TransactionFrame::checkValidWithOptionallyChargedFee( sorobanResourceFee = computePreApplySorobanResourceFee( header.ledgerVersion, *sorobanCfg, cfg); } - bool res = commonValid(cfg, sorobanCfg, header, signatureChecker, ltx, - current, false, chargeFee, lowerBoundCloseTimeOffset, - upperBoundCloseTimeOffset, sorobanResourceFee, - txResult) == ValidationType::kMaybeValid; + + auto sourceAccount = loadReadOnlySourceAccount(roState, header); + bool res = + commonValid(cfg, sorobanCfg, header, signatureChecker, sourceAccount, + current, false, chargeFee, lowerBoundCloseTimeOffset, + upperBoundCloseTimeOffset, sorobanResourceFee, + 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]; @@ -1457,9 +1481,10 @@ TransactionFrame::checkValid(Application& app, AbstractLedgerTxn& ltxOuter, auto& header = ltxOuter.loadHeader().current(); auto sorobanCfg = app.getLedgerManager().maybeGetSorobanNetworkConfigPtr( header.ledgerVersion); + LtxReadOnlyState roState(ltxOuter); return checkValidWithOptionallyChargedFee( - ltxOuter, app.getConfig(), sorobanCfg, header, current, true, + roState, app.getConfig(), sorobanCfg, header, current, true, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset); } @@ -1751,10 +1776,18 @@ TransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx, declaredSorobanResourceFee() - sorobanResourceFee->non_refundable_fee); } + LedgerTxn ltxTx(ltx); - auto cv = commonValid(app.getConfig(), sorobanCfg, header, - signatureChecker, ltxTx, 0, true, chargeFee, 0, 0, - sorobanResourceFee, txResult); + TransactionFrame::ValidationType cv; + { + LedgerTxn cvLtx(ltxTx); + auto sourceAccount = + LtxReadOnlyResult::create(loadSourceAccount(cvLtx, header)); + + cv = commonValid(app.getConfig(), sorobanCfg, header, + signatureChecker, sourceAccount, 0, true, + chargeFee, 0, 0, sorobanResourceFee, txResult); + } if (cv >= ValidationType::kInvalidUpdateSeqNum) { processSeqNum(ltxTx); diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h index 8b04d3137b..34081ca284 100644 --- a/src/transactions/TransactionFrame.h +++ b/src/transactions/TransactionFrame.h @@ -6,6 +6,7 @@ #include "ledger/InternalLedgerEntry.h" #include "ledger/NetworkConfig.h" +#include "ledger/ReadOnlyState.h" #include "main/Config.h" #include "overlay/StellarXDR.h" #include "rust/RustBridge.h" @@ -73,6 +74,10 @@ class TransactionFrame : public TransactionFrameBase LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx, LedgerHeader const& header) const; + ReadOnlyResultPtr + loadReadOnlySourceAccount(ReadOnlyState& roState, + LedgerHeader const& header) const; + enum ValidationType { kInvalid, // transaction is not valid at all @@ -92,7 +97,8 @@ class TransactionFrame : public TransactionFrameBase LedgerEntry const& sourceAccount, uint64_t lowerBoundCloseTimeOffset) const; - bool commonValidPreSeqNum(AbstractLedgerTxn& ltx, Config const& cfg, + bool commonValidPreSeqNum(ReadOnlyResultPtr sourceAccount, + Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, @@ -105,7 +111,7 @@ class TransactionFrame : public TransactionFrameBase ValidationType commonValid(Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, SignatureChecker& signatureChecker, - AbstractLedgerTxn& ltxOuter, SequenceNumber current, + ReadOnlyResultPtr sourceAccount, SequenceNumber current, bool applying, bool chargeFee, uint64_t lowerBoundCloseTimeOffset, uint64_t upperBoundCloseTimeOffset, @@ -206,7 +212,7 @@ class TransactionFrame : public TransactionFrameBase bool checkExtraSigners(SignatureChecker& signatureChecker) const; MutableTxResultPtr checkValidWithOptionallyChargedFee( - AbstractLedgerTxn& ltxOuter, Config const& cfg, + ReadOnlyState& roState, Config const& cfg, SorobanNetworkConfig const* const sorobanCfg, LedgerHeader const& header, SequenceNumber current, bool chargeFee, uint64_t lowerBoundCloseTimeOffset,