From 79eb3ba04a9bf36507514f86ddf8d283facc9a92 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Tue, 4 Feb 2025 18:02:51 -0800 Subject: [PATCH 1/9] POC prototype of DP dex memos --- src/ledger/LedgerManagerImpl.cpp | 2 + src/ledger/LedgerTxn.cpp | 1 + src/transactions/OfferExchange.cpp | 42 +++++++++-- .../PathPaymentStrictSendOpFrame.cpp | 73 ++++++++++++++++++- src/transactions/TransactionUtils.cpp | 2 + 5 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 873bb0664a..591f89602e 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1017,6 +1017,8 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData, emitNextMeta(); } + releaseAssertOrThrow(ledgerSeq != 53514768); + // The next 7 steps happen in a relatively non-obvious, subtle order. // This is unfortunate and it would be nice if we could make it not // be so subtle, but for the time being this is where we are. diff --git a/src/ledger/LedgerTxn.cpp b/src/ledger/LedgerTxn.cpp index 19228103df..f352d6201c 100644 --- a/src/ledger/LedgerTxn.cpp +++ b/src/ledger/LedgerTxn.cpp @@ -500,6 +500,7 @@ LedgerTxn::commit() noexcept void LedgerTxn::Impl::commit() noexcept { + ZoneNamedN(commitZone, "child_commit", true); maybeUpdateLastModifiedThenInvokeThenSeal([&](EntryMap const& entries) { // getEntryIterator has the strong exception safety guarantee // commitChild has the strong exception safety guarantee diff --git a/src/transactions/OfferExchange.cpp b/src/transactions/OfferExchange.cpp index c1f523966c..64066fea02 100644 --- a/src/transactions/OfferExchange.cpp +++ b/src/transactions/OfferExchange.cpp @@ -55,6 +55,7 @@ int64_t canSellAtMost(LedgerTxnHeader const& header, LedgerTxnEntry const& account, Asset const& asset, TrustLineWrapper const& trustLine) { + ZoneScoped; if (asset.type() == ASSET_TYPE_NATIVE) { // can only send above the minimum balance @@ -73,6 +74,7 @@ int64_t canSellAtMost(LedgerTxnHeader const& header, ConstLedgerTxnEntry const& account, Asset const& asset, ConstTrustLineWrapper const& trustLine) { + ZoneScoped; if (asset.type() == ASSET_TYPE_NATIVE) { // can only send above the minimum balance @@ -91,6 +93,7 @@ int64_t canBuyAtMost(LedgerTxnHeader const& header, LedgerTxnEntry const& account, Asset const& asset, TrustLineWrapper const& trustLine) { + ZoneScoped; if (asset.type() == ASSET_TYPE_NATIVE) { return std::max({getMaxAmountReceive(header, account), int64_t(0)}); @@ -107,6 +110,7 @@ int64_t canBuyAtMost(LedgerTxnHeader const& header, ConstLedgerTxnEntry const& account, Asset const& asset, ConstTrustLineWrapper const& trustLine) { + ZoneScoped; if (asset.type() == ASSET_TYPE_NATIVE) { return std::max({getMaxAmountReceive(header, account), int64_t(0)}); @@ -786,6 +790,7 @@ adjustOffer(LedgerTxnHeader const& header, LedgerTxnEntry& offer, TrustLineWrapper const& wheatLine, Asset const& sheep, TrustLineWrapper const& sheepLine) { + ZoneScoped; OfferEntry& oe = offer.current().data.offer(); int64_t maxWheatSend = std::min({oe.amount, canSellAtMost(header, account, wheat, wheatLine)}); @@ -1206,8 +1211,18 @@ crossOfferV10(AbstractLedgerTxn& ltx, LedgerTxnEntry& sellingWheatOffer, auto res = (offer.amount == 0) ? CrossOfferResult::eOfferTaken : CrossOfferResult::eOfferPartial; { + ZoneNamedN(commitZone, "offer_commit_parent", true); + // Construct == 238 MS LedgerTxn ltxInner(ltx); + + ZoneNamedN(constructZone, "offer_ltx_construct_end", true); + + // Copy header == 21 ms header = ltxInner.loadHeader(); + + ZoneNamedN(loadZone, "offer_ltx_header_end", true); + + // Body: 457 MS sellingWheatOffer = loadOffer(ltxInner, accountBID, offerID); if (res == CrossOfferResult::eOfferTaken) { @@ -1220,16 +1235,25 @@ crossOfferV10(AbstractLedgerTxn& ltx, LedgerTxnEntry& sellingWheatOffer, { acquireLiabilities(ltxInner, header, sellingWheatOffer); } - ltxInner.commit(); + + { + // Commit == 257 MS + ZoneNamedN(commitZone, "offer_commit_actual", true); + ltxInner.commit(); + } } // Note: The previous block creates a nested LedgerTxn so all entries are // deactivated at this point. Specifically, you cannot use sellingWheatOffer // or offer (which is a reference) since it is not active (and may have been // erased) at this point. - offerTrail.emplace_back( - makeClaimAtom(ltx.loadHeader().current().ledgerVersion, accountBID, - offerID, wheat, numWheatReceived, sheep, numSheepSend)); + { + + ZoneNamedN(claimAtomZone, "offer_trail", true); + offerTrail.emplace_back(makeClaimAtom( + ltx.loadHeader().current().ledgerVersion, accountBID, offerID, + wheat, numWheatReceived, sheep, numSheepSend)); + } return res; } @@ -1513,7 +1537,11 @@ convertWithOffers( while (needMore) { + ZoneNamedN(offerStart, "convert_with_offers_start", true); LedgerTxn ltx(ltxOuter); + + ZoneNamedN(convertOffersConstruct, "convert_with_offers_post_construct", + true); auto wheatOffer = ltx.loadBestOffer(sheep, wheat); if (!wheatOffer) { @@ -1583,7 +1611,11 @@ convertWithOffers( { return ConvertResult::ePartial; } - ltx.commit(); + + { + ZoneNamedN(commitZone, "convert_with_offers_commit", true); + ltx.commit(); + } sheepSend += numSheepSend; maxSheepSend -= numSheepSend; diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index 2820453453..4ee141e1e3 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -12,6 +12,9 @@ #include "util/XDROperators.h" #include +#include +#include + namespace stellar { @@ -43,7 +46,6 @@ PathPaymentStrictSendOpFrame::doApply( } pathStr += "->"; pathStr += assetToString(getDestAsset()); - ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); setResultSuccess(res); @@ -63,12 +65,61 @@ PathPaymentStrictSendOpFrame::doApply( return false; } + // Hash(subPath) -> (sendAmount -> set of receiveAmounts) + static std::map>> cache; + // build the full path to the destination, ending with destAsset std::vector fullPath; fullPath.insert(fullPath.end(), mPathPayment.path.begin(), mPathPayment.path.end()); fullPath.emplace_back(getDestAsset()); + SHA256 fullPathHasher; + for (auto const& asset : fullPath) + { + auto hash = getAssetHash(asset); + fullPathHasher.add( + ByteSlice(reinterpret_cast(&hash), sizeof(hash))); + } + + auto fullPathHash = fullPathHasher.finish(); + + if (auto iter = cache.find(fullPathHash); iter != cache.end()) + { + auto const& sendAmountToReceiveAmounts = iter->second; + + // Get set of receive amounts for which sendAmount is greater than or + // equal to our send amount + auto sendToReceiveAmountsIter = std::lower_bound( + sendAmountToReceiveAmounts.begin(), + sendAmountToReceiveAmounts.end(), mPathPayment.sendAmount, + [](const auto& pair, uint64_t value) { + // send amount -> set(minReceiveAmount) + return pair.first < value; + }); + + // For each op that has send the same or more than this op + for (; sendToReceiveAmountsIter != sendAmountToReceiveAmounts.end(); + ++sendToReceiveAmountsIter) + { + auto const& receiveAmounts = sendToReceiveAmountsIter->second; + + // If any received amount is less than or equal to destMin, we + // know the trade will fail since a previous trade sent more and + // received less than destMin but still failed + for (auto const& receiveAmount : receiveAmounts) + { + if (receiveAmount <= mPathPayment.destMin) + { + setResultConstraintNotMet(res); + pathStr += "-> hit"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); + return false; + } + } + } + } + // Walk the path Asset sendAsset = getSourceAsset(); int64_t maxAmountSend = mPathPayment.sendAmount; @@ -98,6 +149,12 @@ PathPaymentStrictSendOpFrame::doApply( amountSend, recvAsset, INT64_MAX, amountRecv, RoundingType::PATH_PAYMENT_STRICT_SEND, offerTrail, res)) { + static int i = 0; + CLOG_FATAL( + Bucket, + "PathPaymentStrictSendOpFrame::convert failed with {}, {}", + res.code(), i++); + return false; } @@ -111,15 +168,27 @@ PathPaymentStrictSendOpFrame::doApply( } if (maxAmountSend < mPathPayment.destMin) - { // make sure not over the max + { setResultConstraintNotMet(res); + + cache[fullPathHash][mPathPayment.sendAmount].insert( + mPathPayment.destMin); + + pathStr += "-> miss"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); return false; } if (!updateDestBalance(ltx, maxAmountSend, bypassIssuerCheck, res)) { + + pathStr += "-> miss"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); return false; } + + pathStr += "-> miss"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); innerResult(res).success().last = SimplePaymentResult(getDestID(), getDestAsset(), maxAmountSend); return true; diff --git a/src/transactions/TransactionUtils.cpp b/src/transactions/TransactionUtils.cpp index 15a177906b..eabdf929d5 100644 --- a/src/transactions/TransactionUtils.cpp +++ b/src/transactions/TransactionUtils.cpp @@ -556,6 +556,7 @@ addBalanceSkipAuthorization(LedgerTxnHeader const& header, bool addBalance(LedgerTxnHeader const& header, LedgerTxnEntry& entry, int64_t delta) { + ZoneScoped; if (entry.current().data.type() == ACCOUNT) { if (delta == 0) @@ -1142,6 +1143,7 @@ void releaseLiabilities(AbstractLedgerTxn& ltx, LedgerTxnHeader const& header, LedgerTxnEntry const& offer) { + ZoneScoped; acquireOrReleaseLiabilities(ltx, header, offer, false); } From 8757a7ab00f373ce1d8e88a00bc26ba56aeed16c Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Wed, 5 Feb 2025 13:07:04 -0800 Subject: [PATCH 2/9] With eviction optimization --- src/ledger/LedgerManager.h | 26 ++++++++ src/ledger/LedgerManagerImpl.cpp | 61 ++++++++++++++++++- src/ledger/LedgerManagerImpl.h | 20 ++++++ src/transactions/ManageOfferOpFrameBase.cpp | 25 ++++++++ .../PathPaymentStrictReceiveOpFrame.cpp | 8 +++ .../PathPaymentStrictSendOpFrame.cpp | 14 ++--- 6 files changed, 146 insertions(+), 8 deletions(-) diff --git a/src/ledger/LedgerManager.h b/src/ledger/LedgerManager.h index e10f11ab3d..d5a442daf8 100644 --- a/src/ledger/LedgerManager.h +++ b/src/ledger/LedgerManager.h @@ -16,6 +16,10 @@ class LedgerCloseData; class Database; class SorobanMetrics; +using PathPaymentStrictSendMap = + std::map>>; +using AssetToPathsMap = std::map>>; + /** * LedgerManager maintains, in memory, a logical pair of ledgers: * @@ -206,5 +210,27 @@ class LedgerManager } virtual bool isApplying() const = 0; + + // Clear the path payment strict send failure cache + virtual void clearPathPaymentStrictSendCache() = 0; + + // Cache a failed path payment strict send attempt + virtual void + cachePathPaymentStrictSendFailure(Hash const& pathHash, int64_t sendAmount, + int64_t receiveAmount, + std::vector const& assets) = 0; + + // Get cached failures for a path, or end iterator if not found + virtual PathPaymentStrictSendMap::const_iterator + getPathPaymentStrictSendCache(Hash const& pathHash) const = 0; + + // Get the end iterator for the path payment cache + virtual PathPaymentStrictSendMap::const_iterator + getPathPaymentStrictSendCacheEnd() const = 0; + + // Invalidate paths containing the given asset pair (in that order) + virtual void + invalidatePathPaymentCachesForAssetPair(Asset const& selling, + Asset const& buying) = 0; }; } diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 591f89602e..5873ac937c 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1017,7 +1017,7 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData, emitNextMeta(); } - releaseAssertOrThrow(ledgerSeq != 53514768); + // releaseAssertOrThrow(ledgerSeq != 53514768); // The next 7 steps happen in a relatively non-obvious, subtle order. // This is unfortunate and it would be nice if we could make it not @@ -1115,7 +1115,11 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData, CLOG_DEBUG(Perf, "Applied ledger {} in {} seconds", ledgerSeq, ledgerTimeSeconds.count()); FrameMark; + + // Clear the path payment cache at the start of processing a new ledger + clearPathPaymentStrictSendCache(); } + void LedgerManagerImpl::deleteOldEntries(Database& db, uint32_t ledgerSeq, uint32_t count) @@ -1876,4 +1880,59 @@ LedgerManagerImpl::ledgerClosed( return res; } + +void +LedgerManagerImpl::clearPathPaymentStrictSendCache() +{ + mPathPaymentStrictSendFailureCache.clear(); +} + +void +LedgerManagerImpl::cachePathPaymentStrictSendFailure( + Hash const& pathHash, int64_t sendAmount, int64_t receiveAmount, + std::vector const& assets) +{ + mPathPaymentStrictSendFailureCache[pathHash][sendAmount].insert( + receiveAmount); + + // Convert path into buy-sell pairs + std::vector> pairs; + for (size_t i = 0; i < assets.size() - 1; i++) + { + pairs.emplace_back(assets[i], assets[i + 1]); + } + + mAssetToPaths[pathHash] = pairs; +} + +PathPaymentStrictSendMap::const_iterator +LedgerManagerImpl::getPathPaymentStrictSendCache(Hash const& pathHash) const +{ + return mPathPaymentStrictSendFailureCache.find(pathHash); +} + +PathPaymentStrictSendMap::const_iterator +LedgerManagerImpl::getPathPaymentStrictSendCacheEnd() const +{ + return mPathPaymentStrictSendFailureCache.end(); +} + +void +LedgerManagerImpl::invalidatePathPaymentCachesForAssetPair(Asset const& selling, + Asset const& buying) +{ + // For each path in the cache + for (auto const& [pathHash, pairs] : mAssetToPaths) + { + // Check if this path contains the asset pair in sequence + for (size_t i = 0; i < pairs.size(); i++) + { + if (pairs[i].first == selling && pairs[i].second == buying) + { + mPathPaymentStrictSendFailureCache.erase(pathHash); + break; + } + } + } +} } diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index 3538021a04..ab8d9f6222 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -106,6 +106,13 @@ class LedgerManagerImpl : public LedgerManager // is currently closing a ledger or has ledgers queued to apply. bool mCurrentlyApplyingLedger{false}; + // Cache for path payment strict send operations + // Hash(subPath) -> (sendAmount -> set of receiveAmounts) + PathPaymentStrictSendMap mPathPaymentStrictSendFailureCache; + + // Maps assets to the paths that contain them + AssetToPathsMap mAssetToPaths; + static std::vector processFeesSeqNums( ApplicableTxSetFrame const& txSet, AbstractLedgerTxn& ltxOuter, std::unique_ptr const& ledgerCloseMeta, @@ -251,5 +258,18 @@ class LedgerManagerImpl : public LedgerManager { return mCurrentlyApplyingLedger; } + + void clearPathPaymentStrictSendCache() override; + void cachePathPaymentStrictSendFailure( + Hash const& pathHash, int64_t sendAmount, int64_t receiveAmount, + std::vector const& assets) override; + PathPaymentStrictSendMap::const_iterator + getPathPaymentStrictSendCache(Hash const& pathHash) const override; + + PathPaymentStrictSendMap::const_iterator + getPathPaymentStrictSendCacheEnd() const override; + + void invalidatePathPaymentCachesForAssetPair(Asset const& selling, + Asset const& buying) override; }; } diff --git a/src/transactions/ManageOfferOpFrameBase.cpp b/src/transactions/ManageOfferOpFrameBase.cpp index 38905dbde3..32eea55415 100644 --- a/src/transactions/ManageOfferOpFrameBase.cpp +++ b/src/transactions/ManageOfferOpFrameBase.cpp @@ -235,6 +235,9 @@ ManageOfferOpFrameBase::doApply( uint32_t flags = 0; LedgerEntry::_ext_t extension; + Price oldSellSheepOfferPrice(Price(0, 0)); + int64_t oldSheepAmount = 0; + if (mOfferID) { // modifying an old offer auto sellSheepOffer = stellar::loadOffer(ltx, getSourceID(), mOfferID); @@ -244,6 +247,9 @@ ManageOfferOpFrameBase::doApply( return false; } + oldSellSheepOfferPrice = sellSheepOffer.current().data.offer().price; + oldSheepAmount = sellSheepOffer.current().data.offer().amount; + // We are releasing the liabilites associated with this offer. This is // required in order to produce available balance for the offer to be // executed. Both trust lines must be reset since it is possible that @@ -539,6 +545,25 @@ ManageOfferOpFrameBase::doApply( } ltx.commit(); + + // Invalidate paths containing either asset in this offer + if (creatingNewOffer) + { + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mSheep, + mWheat); + } + // Deleting an offer does not invalidate any cached fails + else if (!isDeleteOffer()) + { + // Invalidate paths containing the asset pair if the offer is better + if (mPrice < oldSellSheepOfferPrice || + (mPrice == oldSellSheepOfferPrice && amount > oldSheepAmount)) + { + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + mSheep, mWheat); + } + } + return true; } diff --git a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp index 4507d68690..a74a678821 100644 --- a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp +++ b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp @@ -131,6 +131,14 @@ PathPaymentStrictReceiveOpFrame::doApply( { return false; } + + // Invalidate paths containing any asset pairs in the path + for (size_t i = 0; i < fullPath.size() - 1; i++) + { + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + fullPath[i], fullPath[i + 1]); + } + return true; } diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index 4ee141e1e3..51a7f83190 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -65,9 +65,6 @@ PathPaymentStrictSendOpFrame::doApply( return false; } - // Hash(subPath) -> (sendAmount -> set of receiveAmounts) - static std::map>> cache; - // build the full path to the destination, ending with destAsset std::vector fullPath; fullPath.insert(fullPath.end(), mPathPayment.path.begin(), @@ -84,7 +81,9 @@ PathPaymentStrictSendOpFrame::doApply( auto fullPathHash = fullPathHasher.finish(); - if (auto iter = cache.find(fullPathHash); iter != cache.end()) + auto iter = + app.getLedgerManager().getPathPaymentStrictSendCache(fullPathHash); + if (iter != app.getLedgerManager().getPathPaymentStrictSendCacheEnd()) { auto const& sendAmountToReceiveAmounts = iter->second; @@ -98,7 +97,7 @@ PathPaymentStrictSendOpFrame::doApply( return pair.first < value; }); - // For each op that has send the same or more than this op + // For each op that has sent the same or more than this op for (; sendToReceiveAmountsIter != sendAmountToReceiveAmounts.end(); ++sendToReceiveAmountsIter) { @@ -171,8 +170,9 @@ PathPaymentStrictSendOpFrame::doApply( { setResultConstraintNotMet(res); - cache[fullPathHash][mPathPayment.sendAmount].insert( - mPathPayment.destMin); + app.getLedgerManager().cachePathPaymentStrictSendFailure( + fullPathHash, mPathPayment.sendAmount, mPathPayment.destMin, + fullPath); pathStr += "-> miss"; ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); From 2f3cfb83b3eceecaaf15142c5bb98f65929774e9 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Wed, 5 Feb 2025 22:09:51 -0800 Subject: [PATCH 3/9] Working with invalidation properly --- src/ledger/LedgerManager.h | 7 ++- src/ledger/LedgerManagerImpl.cpp | 5 +- src/ledger/LedgerManagerImpl.h | 2 +- .../LiquidityPoolDepositOpFrame.cpp | 4 ++ .../LiquidityPoolWithdrawOpFrame.cpp | 5 ++ src/transactions/ManageOfferOpFrameBase.cpp | 28 +++++++--- .../PathPaymentStrictReceiveOpFrame.cpp | 6 ++- .../PathPaymentStrictSendOpFrame.cpp | 25 +++++++-- src/transactions/TransactionFrame.cpp | 52 ++++++++++++++++--- 9 files changed, 107 insertions(+), 27 deletions(-) diff --git a/src/ledger/LedgerManager.h b/src/ledger/LedgerManager.h index d5a442daf8..5073fa4ddb 100644 --- a/src/ledger/LedgerManager.h +++ b/src/ledger/LedgerManager.h @@ -215,10 +215,9 @@ class LedgerManager virtual void clearPathPaymentStrictSendCache() = 0; // Cache a failed path payment strict send attempt - virtual void - cachePathPaymentStrictSendFailure(Hash const& pathHash, int64_t sendAmount, - int64_t receiveAmount, - std::vector const& assets) = 0; + virtual void cachePathPaymentStrictSendFailure( + Hash const& pathHash, int64_t sendAmount, int64_t receiveAmount, + Asset const& source, std::vector const& assets) = 0; // Get cached failures for a path, or end iterator if not found virtual PathPaymentStrictSendMap::const_iterator diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 5873ac937c..75e3b916cf 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1890,13 +1890,15 @@ LedgerManagerImpl::clearPathPaymentStrictSendCache() void LedgerManagerImpl::cachePathPaymentStrictSendFailure( Hash const& pathHash, int64_t sendAmount, int64_t receiveAmount, - std::vector const& assets) + Asset const& source, std::vector const& assets) { + ZoneScoped; mPathPaymentStrictSendFailureCache[pathHash][sendAmount].insert( receiveAmount); // Convert path into buy-sell pairs std::vector> pairs; + pairs.emplace_back(source, assets[0]); for (size_t i = 0; i < assets.size() - 1; i++) { pairs.emplace_back(assets[i], assets[i + 1]); @@ -1921,6 +1923,7 @@ void LedgerManagerImpl::invalidatePathPaymentCachesForAssetPair(Asset const& selling, Asset const& buying) { + ZoneScoped; // For each path in the cache for (auto const& [pathHash, pairs] : mAssetToPaths) { diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index ab8d9f6222..78c3d2242a 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -262,7 +262,7 @@ class LedgerManagerImpl : public LedgerManager void clearPathPaymentStrictSendCache() override; void cachePathPaymentStrictSendFailure( Hash const& pathHash, int64_t sendAmount, int64_t receiveAmount, - std::vector const& assets) override; + Asset const& source, std::vector const& assets) override; PathPaymentStrictSendMap::const_iterator getPathPaymentStrictSendCache(Hash const& pathHash) const override; diff --git a/src/transactions/LiquidityPoolDepositOpFrame.cpp b/src/transactions/LiquidityPoolDepositOpFrame.cpp index f51876fe13..f910cccfa2 100644 --- a/src/transactions/LiquidityPoolDepositOpFrame.cpp +++ b/src/transactions/LiquidityPoolDepositOpFrame.cpp @@ -313,6 +313,10 @@ LiquidityPoolDepositOpFrame::doApply( throw std::runtime_error("insufficient liquidity pool limit"); } + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + cpp().assetA, cpp().assetB); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + cpp().assetB, cpp().assetA); innerResult(res).code(LIQUIDITY_POOL_DEPOSIT_SUCCESS); return true; } diff --git a/src/transactions/LiquidityPoolWithdrawOpFrame.cpp b/src/transactions/LiquidityPoolWithdrawOpFrame.cpp index 51cdac6d10..9aa54cdf1c 100644 --- a/src/transactions/LiquidityPoolWithdrawOpFrame.cpp +++ b/src/transactions/LiquidityPoolWithdrawOpFrame.cpp @@ -100,6 +100,11 @@ LiquidityPoolWithdrawOpFrame::doApply( throw std::runtime_error("insufficient reserveB"); } + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + constantProduct().params.assetA, constantProduct().params.assetB); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + constantProduct().params.assetB, constantProduct().params.assetA); + innerResult(res).code(LIQUIDITY_POOL_WITHDRAW_SUCCESS); return true; } diff --git a/src/transactions/ManageOfferOpFrameBase.cpp b/src/transactions/ManageOfferOpFrameBase.cpp index 32eea55415..a34200dd55 100644 --- a/src/transactions/ManageOfferOpFrameBase.cpp +++ b/src/transactions/ManageOfferOpFrameBase.cpp @@ -549,19 +549,33 @@ ManageOfferOpFrameBase::doApply( // Invalidate paths containing either asset in this offer if (creatingNewOffer) { + // Since the offer is new, we don't know which side of the trade has + // improved, so invalidate both app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mSheep, mWheat); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mWheat, + mSheep); } // Deleting an offer does not invalidate any cached fails else if (!isDeleteOffer()) { - // Invalidate paths containing the asset pair if the offer is better - if (mPrice < oldSellSheepOfferPrice || - (mPrice == oldSellSheepOfferPrice && amount > oldSheepAmount)) - { - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - mSheep, mWheat); - } + // // Invalidate paths containing the asset pair if the offer is better + // if (mPrice < oldSellSheepOfferPrice || + // (mPrice == oldSellSheepOfferPrice && amount > oldSheepAmount)) + // { + // // + // app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + // // mSheep, mWheat); + // // + // app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + // // mWheat, mSheep); + // } + + // TODO: Tighten this + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mSheep, + mWheat); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mWheat, + mSheep); } return true; diff --git a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp index a74a678821..50221a32e4 100644 --- a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp +++ b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp @@ -132,7 +132,11 @@ PathPaymentStrictReceiveOpFrame::doApply( return false; } - // Invalidate paths containing any asset pairs in the path + // Invalidate paths containing any asset pairs in the path, but reversed + // since the counter party is getting better + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + getDestAsset(), fullPath.front()); + for (size_t i = 0; i < fullPath.size() - 1; i++) { app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index 51a7f83190..797445f602 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -71,15 +71,19 @@ PathPaymentStrictSendOpFrame::doApply( mPathPayment.path.end()); fullPath.emplace_back(getDestAsset()); - SHA256 fullPathHasher; + SHA256 sourceAndPathHasher; + auto sourceAssetHash = getAssetHash(getSourceAsset()); + sourceAndPathHasher.add( + ByteSlice(reinterpret_cast(&sourceAssetHash), + sizeof(sourceAssetHash))); for (auto const& asset : fullPath) { auto hash = getAssetHash(asset); - fullPathHasher.add( + sourceAndPathHasher.add( ByteSlice(reinterpret_cast(&hash), sizeof(hash))); } - auto fullPathHash = fullPathHasher.finish(); + auto fullPathHash = sourceAndPathHasher.finish(); auto iter = app.getLedgerManager().getPathPaymentStrictSendCache(fullPathHash); @@ -102,6 +106,8 @@ PathPaymentStrictSendOpFrame::doApply( ++sendToReceiveAmountsIter) { auto const& receiveAmounts = sendToReceiveAmountsIter->second; + releaseAssert(sendToReceiveAmountsIter->first >= + mPathPayment.sendAmount); // If any received amount is less than or equal to destMin, we // know the trade will fail since a previous trade sent more and @@ -169,10 +175,9 @@ PathPaymentStrictSendOpFrame::doApply( if (maxAmountSend < mPathPayment.destMin) { setResultConstraintNotMet(res); - app.getLedgerManager().cachePathPaymentStrictSendFailure( fullPathHash, mPathPayment.sendAmount, mPathPayment.destMin, - fullPath); + getSourceAsset(), fullPath); pathStr += "-> miss"; ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); @@ -187,6 +192,16 @@ PathPaymentStrictSendOpFrame::doApply( return false; } + // Invalidate caches for filled offers, but in reverse because counter party + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + fullPath.front(), getSourceAsset()); + + for (size_t i = 0; i < fullPath.size() - 1; i++) + { + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + fullPath[i + 1], fullPath[i]); + } + pathStr += "-> miss"; ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); innerResult(res).success().last = diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 6ea4bec5aa..2146182e2a 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -5,6 +5,7 @@ #include "util/asio.h" #include "TransactionFrame.h" #include "OperationFrame.h" +#include "bucket/BucketIndexUtils.h" #include "crypto/Hex.h" #include "crypto/SHA.h" #include "crypto/SignerKey.h" @@ -37,6 +38,7 @@ #include "util/XDRStream.h" #include "xdr/Stellar-contract.h" #include "xdr/Stellar-ledger.h" +#include "xdr/Stellar-transaction.h" #include "xdrpp/marshal.h" #include "xdrpp/printer.h" #include @@ -1691,8 +1693,39 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, { ZoneScoped; #ifdef BUILD_TESTS + + bool skipTx = false; + + static int num_tx_skipped = 0; auto const& result = txResult.getReplayTransactionResult(); if (result && result->result.code() != txSUCCESS) + { + // Check if this is a PathPaymentStrictSend operation that failed + if (result->result.code() == txFAILED && + !result->result.results().empty()) + { + // Check each operation result + for (auto const& opRes : result->result.results()) + { + // Only skip TX if it contains a PathPaymentStrictSend with + // an error other than PATH_PAYMENT_STRICT_SEND_UNDER_DESTMIN. + // These are rare and the cache returns a different error, + // resulting in hash mismatch. Shouldn't impact perf values too + // much + if (opRes.code() == opINNER && + opRes.tr().type() == PATH_PAYMENT_STRICT_SEND && + opRes.tr().pathPaymentStrictSendResult().code() != + PATH_PAYMENT_STRICT_SEND_UNDER_DESTMIN) + { + skipTx = true; + CLOG_FATAL(Tx, "Skipping failed TX {}", num_tx_skipped++); + break; + } + } + } + } + + if (skipTx) { // Sub-zone for skips ZoneScopedN("skipped failed"); @@ -1935,21 +1968,24 @@ TransactionFrame::apply(AppConnector& app, AbstractLedgerTxn& ltx, uint32_t ledgerVersion = ltx.loadHeader().current().ledgerVersion; std::unique_ptr signatureChecker; #ifdef BUILD_TESTS + + // We are just replacing bad values, so still run signature for tests + // If the txResult has a replay result (catchup in skip mode is // enabled), // we do not perform signature verification. - if (txResult->getReplayTransactionResult()) - { - signatureChecker = std::make_unique( - ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); - } - else - { + // if (txResult->getReplayTransactionResult()) + // { + // signatureChecker = std::make_unique( + // ledgerVersion, getContentsHash(), getSignatures(mEnvelope)); + // } + // 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 From e1d7dfd06e70543d739f271ac54b3769d0df5139 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Wed, 5 Feb 2025 23:03:07 -0800 Subject: [PATCH 4/9] Optimized cache --- src/ledger/LedgerManager.h | 10 ++-- src/ledger/LedgerManagerImpl.cpp | 56 +++++++++++++------ src/ledger/LedgerManagerImpl.h | 4 +- .../LiquidityPoolDepositOpFrame.cpp | 4 +- .../LiquidityPoolWithdrawOpFrame.cpp | 8 +-- src/transactions/ManageOfferOpFrameBase.cpp | 16 +++--- .../PathPaymentStrictReceiveOpFrame.cpp | 4 +- .../PathPaymentStrictSendOpFrame.cpp | 4 +- 8 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/ledger/LedgerManager.h b/src/ledger/LedgerManager.h index 5073fa4ddb..27c4857406 100644 --- a/src/ledger/LedgerManager.h +++ b/src/ledger/LedgerManager.h @@ -17,8 +17,11 @@ class Database; class SorobanMetrics; using PathPaymentStrictSendMap = - std::map>>; -using AssetToPathsMap = std::map>>; + UnorderedMap>>; + +// Change map definition to use unordered_map with asset pair as key +using AssetToPathsMap = + UnorderedMap, AssetPairHash>; /** * LedgerManager maintains, in memory, a logical pair of ledgers: @@ -229,7 +232,6 @@ class LedgerManager // Invalidate paths containing the given asset pair (in that order) virtual void - invalidatePathPaymentCachesForAssetPair(Asset const& selling, - Asset const& buying) = 0; + invalidatePathPaymentCachesForAssetPair(AssetPair const& pair) = 0; }; } diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 75e3b916cf..bac16c9986 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -160,6 +160,9 @@ LedgerManagerImpl::LedgerManagerImpl(Application& app) { setupLedgerCloseMetaStream(); + + mPathPaymentStrictSendFailureCache.reserve(1000); + mAssetToPaths.reserve(1000); } void @@ -1885,6 +1888,7 @@ void LedgerManagerImpl::clearPathPaymentStrictSendCache() { mPathPaymentStrictSendFailureCache.clear(); + mAssetToPaths.clear(); } void @@ -1893,18 +1897,37 @@ LedgerManagerImpl::cachePathPaymentStrictSendFailure( Asset const& source, std::vector const& assets) { ZoneScoped; - mPathPaymentStrictSendFailureCache[pathHash][sendAmount].insert( - receiveAmount); + auto iter = mPathPaymentStrictSendFailureCache.find(pathHash); + if (iter == mPathPaymentStrictSendFailureCache.end()) + { + auto val = std::map>(); + val[sendAmount].insert(receiveAmount); + mPathPaymentStrictSendFailureCache.emplace(pathHash, std::move(val)); + } + else + { + auto& val = iter->second; + val[sendAmount].insert(receiveAmount); + } + + auto insert = [&](AssetPair const& pair) { + auto iter = mAssetToPaths.find(pair); + if (iter == mAssetToPaths.end()) + { + mAssetToPaths.emplace(pair, std::vector{pathHash}); + } + else + { + iter->second.push_back(pathHash); + } + }; // Convert path into buy-sell pairs - std::vector> pairs; - pairs.emplace_back(source, assets[0]); + insert(AssetPair{source, assets[0]}); for (size_t i = 0; i < assets.size() - 1; i++) { - pairs.emplace_back(assets[i], assets[i + 1]); + insert(AssetPair{assets[i], assets[i + 1]}); } - - mAssetToPaths[pathHash] = pairs; } PathPaymentStrictSendMap::const_iterator @@ -1920,22 +1943,19 @@ LedgerManagerImpl::getPathPaymentStrictSendCacheEnd() const } void -LedgerManagerImpl::invalidatePathPaymentCachesForAssetPair(Asset const& selling, - Asset const& buying) +LedgerManagerImpl::invalidatePathPaymentCachesForAssetPair( + AssetPair const& pair) { ZoneScoped; - // For each path in the cache - for (auto const& [pathHash, pairs] : mAssetToPaths) + + auto it = mAssetToPaths.find(pair); + if (it != mAssetToPaths.end()) { - // Check if this path contains the asset pair in sequence - for (size_t i = 0; i < pairs.size(); i++) + for (auto const& pathHash : it->second) { - if (pairs[i].first == selling && pairs[i].second == buying) - { - mPathPaymentStrictSendFailureCache.erase(pathHash); - break; - } + mPathPaymentStrictSendFailureCache.erase(pathHash); } + mAssetToPaths.erase(it); } } } diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h index 78c3d2242a..f9c73ca2a8 100644 --- a/src/ledger/LedgerManagerImpl.h +++ b/src/ledger/LedgerManagerImpl.h @@ -269,7 +269,7 @@ class LedgerManagerImpl : public LedgerManager PathPaymentStrictSendMap::const_iterator getPathPaymentStrictSendCacheEnd() const override; - void invalidatePathPaymentCachesForAssetPair(Asset const& selling, - Asset const& buying) override; + void + invalidatePathPaymentCachesForAssetPair(AssetPair const& pair) override; }; } diff --git a/src/transactions/LiquidityPoolDepositOpFrame.cpp b/src/transactions/LiquidityPoolDepositOpFrame.cpp index f910cccfa2..2e785619cd 100644 --- a/src/transactions/LiquidityPoolDepositOpFrame.cpp +++ b/src/transactions/LiquidityPoolDepositOpFrame.cpp @@ -314,9 +314,9 @@ LiquidityPoolDepositOpFrame::doApply( } app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - cpp().assetA, cpp().assetB); + AssetPair{cpp().assetA, cpp().assetB}); app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - cpp().assetB, cpp().assetA); + AssetPair{cpp().assetB, cpp().assetA}); innerResult(res).code(LIQUIDITY_POOL_DEPOSIT_SUCCESS); return true; } diff --git a/src/transactions/LiquidityPoolWithdrawOpFrame.cpp b/src/transactions/LiquidityPoolWithdrawOpFrame.cpp index 9aa54cdf1c..c3467118b4 100644 --- a/src/transactions/LiquidityPoolWithdrawOpFrame.cpp +++ b/src/transactions/LiquidityPoolWithdrawOpFrame.cpp @@ -100,10 +100,10 @@ LiquidityPoolWithdrawOpFrame::doApply( throw std::runtime_error("insufficient reserveB"); } - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - constantProduct().params.assetA, constantProduct().params.assetB); - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - constantProduct().params.assetB, constantProduct().params.assetA); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(AssetPair{ + constantProduct().params.assetA, constantProduct().params.assetB}); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(AssetPair{ + constantProduct().params.assetB, constantProduct().params.assetA}); innerResult(res).code(LIQUIDITY_POOL_WITHDRAW_SUCCESS); return true; diff --git a/src/transactions/ManageOfferOpFrameBase.cpp b/src/transactions/ManageOfferOpFrameBase.cpp index a34200dd55..6872dd0128 100644 --- a/src/transactions/ManageOfferOpFrameBase.cpp +++ b/src/transactions/ManageOfferOpFrameBase.cpp @@ -551,10 +551,10 @@ ManageOfferOpFrameBase::doApply( { // Since the offer is new, we don't know which side of the trade has // improved, so invalidate both - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mSheep, - mWheat); - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mWheat, - mSheep); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + AssetPair{mSheep, mWheat}); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + AssetPair{mWheat, mSheep}); } // Deleting an offer does not invalidate any cached fails else if (!isDeleteOffer()) @@ -572,10 +572,10 @@ ManageOfferOpFrameBase::doApply( // } // TODO: Tighten this - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mSheep, - mWheat); - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair(mWheat, - mSheep); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + AssetPair{mSheep, mWheat}); + app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( + AssetPair{mWheat, mSheep}); } return true; diff --git a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp index 50221a32e4..a51d209b8a 100644 --- a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp +++ b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp @@ -135,12 +135,12 @@ PathPaymentStrictReceiveOpFrame::doApply( // Invalidate paths containing any asset pairs in the path, but reversed // since the counter party is getting better app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - getDestAsset(), fullPath.front()); + AssetPair{getDestAsset(), fullPath.front()}); for (size_t i = 0; i < fullPath.size() - 1; i++) { app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - fullPath[i], fullPath[i + 1]); + AssetPair{fullPath[i], fullPath[i + 1]}); } return true; diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index 797445f602..4187854509 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -194,12 +194,12 @@ PathPaymentStrictSendOpFrame::doApply( // Invalidate caches for filled offers, but in reverse because counter party app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - fullPath.front(), getSourceAsset()); + AssetPair{fullPath.front(), getSourceAsset()}); for (size_t i = 0; i < fullPath.size() - 1; i++) { app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - fullPath[i + 1], fullPath[i]); + AssetPair{fullPath[i + 1], fullPath[i]}); } pathStr += "-> miss"; From 2a256e0d743541399d526b8a895e36009307f90b Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Fri, 7 Feb 2025 09:27:22 -0800 Subject: [PATCH 5/9] simplified path payment cache --- src/ledger/LedgerManager.h | 7 +++- src/ledger/LedgerManagerImpl.cpp | 24 +++++++++-- .../PathPaymentStrictSendOpFrame.cpp | 41 ++++++++----------- 3 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/ledger/LedgerManager.h b/src/ledger/LedgerManager.h index 27c4857406..b7a93ccbe5 100644 --- a/src/ledger/LedgerManager.h +++ b/src/ledger/LedgerManager.h @@ -16,8 +16,11 @@ class LedgerCloseData; class Database; class SorobanMetrics; -using PathPaymentStrictSendMap = - UnorderedMap>>; +// Maps the entire path hash to a map of {sendValue, minimumFailedRecvValue} +// Essentially, for the given path hash, we check all memos from TXs that have +// an equal or greater sendValue. If one of these asked to receive less than the +// current op and failed, we can guarantee the op will also fail. +using PathPaymentStrictSendMap = UnorderedMap>; // Change map definition to use unordered_map with asset pair as key using AssetToPathsMap = diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index bac16c9986..b4c7b0c4da 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1900,14 +1900,30 @@ LedgerManagerImpl::cachePathPaymentStrictSendFailure( auto iter = mPathPaymentStrictSendFailureCache.find(pathHash); if (iter == mPathPaymentStrictSendFailureCache.end()) { - auto val = std::map>(); - val[sendAmount].insert(receiveAmount); + // If path hash does not exist, populate + auto val = std::map(); + val[sendAmount] = receiveAmount; mPathPaymentStrictSendFailureCache.emplace(pathHash, std::move(val)); } else { - auto& val = iter->second; - val[sendAmount].insert(receiveAmount); + // If hash path exists, but this send amount does not exist, insert the + // new send amount + auto& sendAmountToMinReceiveAmount = iter->second; + if (auto sendToRecIter = sendAmountToMinReceiveAmount.find(sendAmount); + sendToRecIter == sendAmountToMinReceiveAmount.end()) + { + sendAmountToMinReceiveAmount[sendAmount] = receiveAmount; + } + else + { + // Else, update receive amount if it's lower than the current + // minimum for the given send amount + if (sendToRecIter->second > receiveAmount) + { + sendToRecIter->second = receiveAmount; + } + } } auto insert = [&](AssetPair const& pair) { diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index 4187854509..e6c1ac05a0 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -89,38 +89,34 @@ PathPaymentStrictSendOpFrame::doApply( app.getLedgerManager().getPathPaymentStrictSendCache(fullPathHash); if (iter != app.getLedgerManager().getPathPaymentStrictSendCacheEnd()) { - auto const& sendAmountToReceiveAmounts = iter->second; + auto const& sendAmountToMinReceiveAmount = iter->second; // Get set of receive amounts for which sendAmount is greater than or // equal to our send amount auto sendToReceiveAmountsIter = std::lower_bound( - sendAmountToReceiveAmounts.begin(), - sendAmountToReceiveAmounts.end(), mPathPayment.sendAmount, + sendAmountToMinReceiveAmount.begin(), + sendAmountToMinReceiveAmount.end(), mPathPayment.sendAmount, [](const auto& pair, uint64_t value) { - // send amount -> set(minReceiveAmount) + // Pair == {sendAmount, minReceiveAmount} return pair.first < value; }); // For each op that has sent the same or more than this op - for (; sendToReceiveAmountsIter != sendAmountToReceiveAmounts.end(); + for (; sendToReceiveAmountsIter != sendAmountToMinReceiveAmount.end(); ++sendToReceiveAmountsIter) { - auto const& receiveAmounts = sendToReceiveAmountsIter->second; releaseAssert(sendToReceiveAmountsIter->first >= mPathPayment.sendAmount); - // If any received amount is less than or equal to destMin, we - // know the trade will fail since a previous trade sent more and - // received less than destMin but still failed - for (auto const& receiveAmount : receiveAmounts) + // If minimum received amount is less than or equal to destMin, + // we know the trade will fail since a previous trade sent more and + // received less than this op but still failed + if (sendToReceiveAmountsIter->second <= mPathPayment.destMin) { - if (receiveAmount <= mPathPayment.destMin) - { - setResultConstraintNotMet(res); - pathStr += "-> hit"; - ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); - return false; - } + setResultConstraintNotMet(res); + pathStr += "-> hit"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); + return false; } } } @@ -154,12 +150,6 @@ PathPaymentStrictSendOpFrame::doApply( amountSend, recvAsset, INT64_MAX, amountRecv, RoundingType::PATH_PAYMENT_STRICT_SEND, offerTrail, res)) { - static int i = 0; - CLOG_FATAL( - Bucket, - "PathPaymentStrictSendOpFrame::convert failed with {}, {}", - res.code(), i++); - return false; } @@ -175,8 +165,11 @@ PathPaymentStrictSendOpFrame::doApply( if (maxAmountSend < mPathPayment.destMin) { setResultConstraintNotMet(res); + + // Bound as much as possible. mPathPayment.destMin failed, but so would + // maxAmountSend + 1. app.getLedgerManager().cachePathPaymentStrictSendFailure( - fullPathHash, mPathPayment.sendAmount, mPathPayment.destMin, + fullPathHash, mPathPayment.sendAmount, maxAmountSend + 1, getSourceAsset(), fullPath); pathStr += "-> miss"; From b0430395a5cc37f349236171ea6609576e5dad51 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Fri, 7 Feb 2025 09:48:45 -0800 Subject: [PATCH 6/9] Fixed buy-sell asset ordering in cache --- src/ledger/LedgerManagerImpl.cpp | 4 ++-- src/transactions/PathPaymentStrictReceiveOpFrame.cpp | 4 ++-- src/transactions/PathPaymentStrictSendOpFrame.cpp | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index b4c7b0c4da..0d0d65d219 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/src/ledger/LedgerManagerImpl.cpp @@ -1939,10 +1939,10 @@ LedgerManagerImpl::cachePathPaymentStrictSendFailure( }; // Convert path into buy-sell pairs - insert(AssetPair{source, assets[0]}); + insert(AssetPair{assets[0], source}); for (size_t i = 0; i < assets.size() - 1; i++) { - insert(AssetPair{assets[i], assets[i + 1]}); + insert(AssetPair{assets[i + 1], assets[i]}); } } diff --git a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp index a51d209b8a..894173f996 100644 --- a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp +++ b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp @@ -135,12 +135,12 @@ PathPaymentStrictReceiveOpFrame::doApply( // Invalidate paths containing any asset pairs in the path, but reversed // since the counter party is getting better app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{getDestAsset(), fullPath.front()}); + AssetPair{fullPath.front(), getDestAsset()}); for (size_t i = 0; i < fullPath.size() - 1; i++) { app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{fullPath[i], fullPath[i + 1]}); + AssetPair{fullPath[i + 1], fullPath[i]}); } return true; diff --git a/src/transactions/PathPaymentStrictSendOpFrame.cpp b/src/transactions/PathPaymentStrictSendOpFrame.cpp index e6c1ac05a0..658c4b28e5 100644 --- a/src/transactions/PathPaymentStrictSendOpFrame.cpp +++ b/src/transactions/PathPaymentStrictSendOpFrame.cpp @@ -186,13 +186,15 @@ PathPaymentStrictSendOpFrame::doApply( } // Invalidate caches for filled offers, but in reverse because counter party + // is getting better. We don't need to invalidate paths we filled, as + // filling them made the path strictly worse app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{fullPath.front(), getSourceAsset()}); + AssetPair{getSourceAsset(), fullPath.front()}); for (size_t i = 0; i < fullPath.size() - 1; i++) { app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{fullPath[i + 1], fullPath[i]}); + AssetPair{fullPath[i], fullPath[i + 1]}); } pathStr += "-> miss"; From 700b7a67494f62449819d964042cbb29510e1b64 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Fri, 7 Feb 2025 12:21:42 -0800 Subject: [PATCH 7/9] Shared cache between strict send and strict receive --- src/transactions/ManageOfferOpFrameBase.cpp | 29 ++------- .../PathPaymentStrictReceiveOpFrame.cpp | 60 +++++++++++++++++++ src/transactions/TransactionFrame.cpp | 10 ++++ 3 files changed, 75 insertions(+), 24 deletions(-) diff --git a/src/transactions/ManageOfferOpFrameBase.cpp b/src/transactions/ManageOfferOpFrameBase.cpp index 6872dd0128..aba98e23ce 100644 --- a/src/transactions/ManageOfferOpFrameBase.cpp +++ b/src/transactions/ManageOfferOpFrameBase.cpp @@ -546,32 +546,13 @@ ManageOfferOpFrameBase::doApply( ltx.commit(); - // Invalidate paths containing either asset in this offer - if (creatingNewOffer) - { - // Since the offer is new, we don't know which side of the trade has - // improved, so invalidate both - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{mSheep, mWheat}); - app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - AssetPair{mWheat, mSheep}); - } // Deleting an offer does not invalidate any cached fails - else if (!isDeleteOffer()) + if (!isDeleteOffer()) { - // // Invalidate paths containing the asset pair if the offer is better - // if (mPrice < oldSellSheepOfferPrice || - // (mPrice == oldSellSheepOfferPrice && amount > oldSheepAmount)) - // { - // // - // app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - // // mSheep, mWheat); - // // - // app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( - // // mWheat, mSheep); - // } - - // TODO: Tighten this + // Unfortunately, due to rounding errors we need to invalidate both side + // of the offer. Sometimes, even if an offer gets "worse", (like if the + // amount offer decreases), different rounding behavior can actually + // cause the "worse" offer to now favor the taker. app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( AssetPair{mSheep, mWheat}); app.getLedgerManager().invalidatePathPaymentCachesForAssetPair( diff --git a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp index 894173f996..e2bdd20319 100644 --- a/src/transactions/PathPaymentStrictReceiveOpFrame.cpp +++ b/src/transactions/PathPaymentStrictReceiveOpFrame.cpp @@ -72,6 +72,56 @@ PathPaymentStrictReceiveOpFrame::doApply( mPathPayment.path.rend()); fullPath.emplace_back(getSourceAsset()); + SHA256 pathHasher; + for (auto iter = fullPath.rbegin(); iter != fullPath.rend(); iter++) + { + auto hash = getAssetHash(*iter); + pathHasher.add( + ByteSlice(reinterpret_cast(&hash), sizeof(hash))); + } + + auto destHash = getAssetHash(getDestAsset()); + pathHasher.add(ByteSlice(reinterpret_cast(&destHash), + sizeof(destHash))); + + auto pathHash = pathHasher.finish(); + + // TODO: Optimize better by keeping max as well + auto iter = app.getLedgerManager().getPathPaymentStrictSendCache(pathHash); + if (iter != app.getLedgerManager().getPathPaymentStrictSendCacheEnd()) + { + auto const& sendAmountToMinReceiveAmount = iter->second; + + // Get set of receive amounts for which sendAmount is greater than or + // equal to our send amount + auto sendToReceiveAmountsIter = std::lower_bound( + sendAmountToMinReceiveAmount.begin(), + sendAmountToMinReceiveAmount.end(), mPathPayment.sendMax, + [](const auto& pair, uint64_t value) { + // Pair == {sendAmount, minReceiveAmount} + return pair.first < value; + }); + + // For each op that has sent the same or more than this op + for (; sendToReceiveAmountsIter != sendAmountToMinReceiveAmount.end(); + ++sendToReceiveAmountsIter) + { + releaseAssert(sendToReceiveAmountsIter->first >= + mPathPayment.sendMax); + + // If minimum received amount is less than or equal to destAmount, + // we know the trade will fail since a previous trade sent more and + // received less than this op but still failed + if (sendToReceiveAmountsIter->second <= mPathPayment.destAmount) + { + setResultConstraintNotMet(res); + pathStr += "-> hit"; + ZoneTextV(applyZone, pathStr.c_str(), pathStr.size()); + return false; + } + } + } + // Walk the path Asset recvAsset = getDestAsset(); int64_t maxAmountRecv = mPathPayment.destAmount; @@ -122,6 +172,16 @@ PathPaymentStrictReceiveOpFrame::doApply( if (maxAmountRecv > mPathPayment.sendMax) { // make sure not over the max + + // Convert to strict send format for cache purposes + std::vector path; + path.insert(path.end(), mPathPayment.path.begin(), + mPathPayment.path.end()); + path.emplace_back(getDestAsset()); + app.getLedgerManager().cachePathPaymentStrictSendFailure( + pathHash, mPathPayment.sendMax, mPathPayment.destAmount, + getSourceAsset(), path); + setResultConstraintNotMet(res); return false; } diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 2146182e2a..78e524b933 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1721,6 +1721,16 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, CLOG_FATAL(Tx, "Skipping failed TX {}", num_tx_skipped++); break; } + + if (opRes.code() == opINNER && + opRes.tr().type() == PATH_PAYMENT_STRICT_RECEIVE && + opRes.tr().pathPaymentStrictReceiveResult().code() != + PATH_PAYMENT_STRICT_RECEIVE_OVER_SENDMAX) + { + skipTx = true; + CLOG_FATAL(Tx, "Skipping failed TX {}", num_tx_skipped++); + break; + } } } } From 9ffa50b94ef6d028139f016302e7e2f803da706f Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Fri, 7 Feb 2025 12:42:37 -0800 Subject: [PATCH 8/9] Improve testing wrt error code divergence --- src/transactions/TransactionFrame.cpp | 36 ++++++++++++++++++++------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index 78e524b933..c19739149c 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1722,15 +1722,15 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, break; } - if (opRes.code() == opINNER && - opRes.tr().type() == PATH_PAYMENT_STRICT_RECEIVE && - opRes.tr().pathPaymentStrictReceiveResult().code() != - PATH_PAYMENT_STRICT_RECEIVE_OVER_SENDMAX) - { - skipTx = true; - CLOG_FATAL(Tx, "Skipping failed TX {}", num_tx_skipped++); - break; - } + // if (opRes.code() == opINNER && + // opRes.tr().type() == PATH_PAYMENT_STRICT_RECEIVE && + // opRes.tr().pathPaymentStrictReceiveResult().code() != + // PATH_PAYMENT_STRICT_RECEIVE_OVER_SENDMAX) + // { + // skipTx = true; + // CLOG_FATAL(Tx, "Skipping failed TX {}", + // num_tx_skipped++); break; + // } } } } @@ -1799,6 +1799,24 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, if (!txRes) { success = false; + + // Cache sometimes returns different error codes, is technically + // a protocol change. All success results should still succeed + // and all fails should still fail though. + if (result && result->result.code() == txFAILED && + !result->result.results().empty()) + { + auto const& correctRes = result->result.results().at(i); + if (correctRes.code() == opINNER && + correctRes.tr().type() == PATH_PAYMENT_STRICT_RECEIVE && + correctRes.tr() + .pathPaymentStrictReceiveResult() + .code() != + PATH_PAYMENT_STRICT_RECEIVE_OVER_SENDMAX) + { + opResult = correctRes; + } + } } // The operation meta will be empty if the transaction From 68e86b0ca712832174cfe43106d6096d28fbdbc8 Mon Sep 17 00:00:00 2001 From: Garand Tyson Date: Fri, 7 Feb 2025 14:19:59 -0800 Subject: [PATCH 9/9] Proper error code handling for strict send --- src/transactions/TransactionFrame.cpp | 65 +++++---------------------- 1 file changed, 10 insertions(+), 55 deletions(-) diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp index c19739149c..58155dd727 100644 --- a/src/transactions/TransactionFrame.cpp +++ b/src/transactions/TransactionFrame.cpp @@ -1693,62 +1693,7 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, { ZoneScoped; #ifdef BUILD_TESTS - - bool skipTx = false; - - static int num_tx_skipped = 0; auto const& result = txResult.getReplayTransactionResult(); - if (result && result->result.code() != txSUCCESS) - { - // Check if this is a PathPaymentStrictSend operation that failed - if (result->result.code() == txFAILED && - !result->result.results().empty()) - { - // Check each operation result - for (auto const& opRes : result->result.results()) - { - // Only skip TX if it contains a PathPaymentStrictSend with - // an error other than PATH_PAYMENT_STRICT_SEND_UNDER_DESTMIN. - // These are rare and the cache returns a different error, - // resulting in hash mismatch. Shouldn't impact perf values too - // much - if (opRes.code() == opINNER && - opRes.tr().type() == PATH_PAYMENT_STRICT_SEND && - opRes.tr().pathPaymentStrictSendResult().code() != - PATH_PAYMENT_STRICT_SEND_UNDER_DESTMIN) - { - skipTx = true; - CLOG_FATAL(Tx, "Skipping failed TX {}", num_tx_skipped++); - break; - } - - // if (opRes.code() == opINNER && - // opRes.tr().type() == PATH_PAYMENT_STRICT_RECEIVE && - // opRes.tr().pathPaymentStrictReceiveResult().code() != - // PATH_PAYMENT_STRICT_RECEIVE_OVER_SENDMAX) - // { - // skipTx = true; - // CLOG_FATAL(Tx, "Skipping failed TX {}", - // num_tx_skipped++); break; - // } - } - } - } - - if (skipTx) - { - // Sub-zone for skips - ZoneScopedN("skipped failed"); - CLOG_DEBUG(Tx, "Skipping replay of failed transaction: tx {}", - binToHex(getContentsHash())); - txResult.setResultCode(result->result.code()); - // 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( @@ -1816,6 +1761,16 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker, { opResult = correctRes; } + else if (correctRes.code() == opINNER && + correctRes.tr().type() == + PATH_PAYMENT_STRICT_SEND && + correctRes.tr() + .pathPaymentStrictSendResult() + .code() != + PATH_PAYMENT_STRICT_SEND_UNDER_DESTMIN) + { + opResult = correctRes; + } } }