From aef823e578e5449425f770adeed3c8a6591ab4d1 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 3 Dec 2024 19:13:45 -0500 Subject: [PATCH 1/3] test: Unit tests to recreate invalid index logic error (#5242) * One hits the global cache, one does not. * Also some extra checking. Co-authored-by: Bronek Kozicki --- src/test/app/Regression_test.cpp | 98 ++++++++++++++++++++++++++ src/xrpld/ledger/detail/CachedView.cpp | 6 ++ 2 files changed, 104 insertions(+) diff --git a/src/test/app/Regression_test.cpp b/src/test/app/Regression_test.cpp index f54a88ace00..64fc2015c19 100644 --- a/src/test/app/Regression_test.cpp +++ b/src/test/app/Regression_test.cpp @@ -16,8 +16,11 @@ //============================================================================== #include +#include #include +#include #include +#include #include #include #include @@ -247,6 +250,100 @@ struct Regression_test : public beast::unit_test::suite jrReader.parse(jvRequest, buffers) && jvRequest.isObject()); } + void + testInvalidTxObjectIDType() + { + testcase("Invalid Transaction Object ID Type"); + // Crasher bug introduced in 2.0.1. Fixed in 2.3.0. + + using namespace jtx; + Env env(*this); + + Account alice("alice"); + Account bob("bob"); + env.fund(XRP(10'000), alice, bob); + env.close(); + + auto index = [&](auto account) { + Json::Value req(Json::objectValue); + req[jss::account] = account; + auto const resp = env.rpc("json", "account_info", to_string(req)); + BEAST_EXPECT( + resp.isMember(jss::result) && resp[jss::result].isObject()); + auto const& result = resp[jss::result]; + BEAST_EXPECT( + result.isMember(jss::account_data) && + result[jss::account_data].isObject()); + auto const& ad = result[jss::account_data]; + BEAST_EXPECT(ad.isMember(jss::index) && ad[jss::index].isString()); + uint256 index; + BEAST_EXPECT(index.parseHex(ad[jss::index].asString())); + return index; + }; + + { + auto const alice_index = index(alice.human()); + if (BEAST_EXPECT(alice_index.isNonZero())) + { + env(check::cash( + alice, alice_index, check::DeliverMin(XRP(100))), + ter(tecNO_ENTRY)); + } + } + + { + auto const bob_index = index(bob.human()); + + auto const digest = [&]() -> std::optional { + auto const& state = + env.app().getLedgerMaster().getClosedLedger()->stateMap(); + SHAMapHash digest; + if (!state.peekItem(bob_index, digest)) + return std::nullopt; + return digest.as_uint256(); + }(); + + auto const mapCounts = [&](CountedObjects::List const& list) { + std::map result; + for (auto const& e : list) + { + result[e.first] = e.second; + } + + return result; + }; + + if (BEAST_EXPECT(bob_index.isNonZero())) + { + if (BEAST_EXPECT(digest.has_value())) + { + auto& cache = env.app().cachedSLEs(); + cache.del(*digest, false); + auto const beforeCounts = + mapCounts(CountedObjects::getInstance().getCounts(0)); + + env(check::cash( + alice, bob_index, check::DeliverMin(XRP(100))), + ter(tecNO_ENTRY)); + + auto const afterCounts = + mapCounts(CountedObjects::getInstance().getCounts(0)); + + using namespace std::string_literals; + BEAST_EXPECT( + beforeCounts.at("CachedView::hit"s) == + afterCounts.at("CachedView::hit"s)); + BEAST_EXPECT( + beforeCounts.at("CachedView::hitExpired"s) + 1 == + afterCounts.at("CachedView::hitExpired"s)); + BEAST_EXPECT( + beforeCounts.at("CachedView::miss"s) == + afterCounts.at("CachedView::miss"s)); + } + } + } + } + void run() override { @@ -256,6 +353,7 @@ struct Regression_test : public beast::unit_test::suite testFeeEscalationAutofill(); testFeeEscalationExtremeConfig(); testJsonInvalid(); + testInvalidTxObjectIDType(); } }; diff --git a/src/xrpld/ledger/detail/CachedView.cpp b/src/xrpld/ledger/detail/CachedView.cpp index 645a2c79c13..eb791fc9321 100644 --- a/src/xrpld/ledger/detail/CachedView.cpp +++ b/src/xrpld/ledger/detail/CachedView.cpp @@ -57,6 +57,12 @@ CachedViewImpl::read(Keylet const& k) const baseRead = true; return base_.read(k); }); + { + // If the sle is null, then a failure must have occurred in base_.read() + XRPL_ASSERT( + sle || baseRead, + "ripple::CachedView::read : null SLE result from base"); + } if (cacheHit && baseRead) hitsexpired.increment(); else if (cacheHit) From b21851f6e76af331059a374bd897480a7085d660 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 13 Jan 2025 16:38:31 -0500 Subject: [PATCH 2/3] Apply suggestions from @bronek's code review Co-authored-by: Bronek Kozicki --- src/test/app/Regression_test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/app/Regression_test.cpp b/src/test/app/Regression_test.cpp index 64fc2015c19..4066612c176 100644 --- a/src/test/app/Regression_test.cpp +++ b/src/test/app/Regression_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include namespace ripple { @@ -282,7 +283,7 @@ struct Regression_test : public beast::unit_test::suite }; { - auto const alice_index = index(alice.human()); + auto const alice_index = keylet::account(alice).key; if (BEAST_EXPECT(alice_index.isNonZero())) { env(check::cash( @@ -292,7 +293,7 @@ struct Regression_test : public beast::unit_test::suite } { - auto const bob_index = index(bob.human()); + auto const bob_index = keylet::account(bob).key; auto const digest = [&]() -> std::optional { auto const& state = From e228cb2a9225abaee0d5310e5ecbee826ec86f61 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 13 Jan 2025 17:38:55 -0500 Subject: [PATCH 3/3] [FOLD] Remove the unnecessary "index" lambda, and some cleanup --- src/test/app/Regression_test.cpp | 66 +++++++++----------------- src/xrpld/ledger/detail/CachedView.cpp | 10 ++-- 2 files changed, 27 insertions(+), 49 deletions(-) diff --git a/src/test/app/Regression_test.cpp b/src/test/app/Regression_test.cpp index 4066612c176..ddaece88949 100644 --- a/src/test/app/Regression_test.cpp +++ b/src/test/app/Regression_test.cpp @@ -265,23 +265,6 @@ struct Regression_test : public beast::unit_test::suite env.fund(XRP(10'000), alice, bob); env.close(); - auto index = [&](auto account) { - Json::Value req(Json::objectValue); - req[jss::account] = account; - auto const resp = env.rpc("json", "account_info", to_string(req)); - BEAST_EXPECT( - resp.isMember(jss::result) && resp[jss::result].isObject()); - auto const& result = resp[jss::result]; - BEAST_EXPECT( - result.isMember(jss::account_data) && - result[jss::account_data].isObject()); - auto const& ad = result[jss::account_data]; - BEAST_EXPECT(ad.isMember(jss::index) && ad[jss::index].isString()); - uint256 index; - BEAST_EXPECT(index.parseHex(ad[jss::index].asString())); - return index; - }; - { auto const alice_index = keylet::account(alice).key; if (BEAST_EXPECT(alice_index.isNonZero())) @@ -314,33 +297,30 @@ struct Regression_test : public beast::unit_test::suite return result; }; - if (BEAST_EXPECT(bob_index.isNonZero())) + if (BEAST_EXPECT(bob_index.isNonZero()) && + BEAST_EXPECT(digest.has_value())) { - if (BEAST_EXPECT(digest.has_value())) - { - auto& cache = env.app().cachedSLEs(); - cache.del(*digest, false); - auto const beforeCounts = - mapCounts(CountedObjects::getInstance().getCounts(0)); - - env(check::cash( - alice, bob_index, check::DeliverMin(XRP(100))), - ter(tecNO_ENTRY)); - - auto const afterCounts = - mapCounts(CountedObjects::getInstance().getCounts(0)); - - using namespace std::string_literals; - BEAST_EXPECT( - beforeCounts.at("CachedView::hit"s) == - afterCounts.at("CachedView::hit"s)); - BEAST_EXPECT( - beforeCounts.at("CachedView::hitExpired"s) + 1 == - afterCounts.at("CachedView::hitExpired"s)); - BEAST_EXPECT( - beforeCounts.at("CachedView::miss"s) == - afterCounts.at("CachedView::miss"s)); - } + auto& cache = env.app().cachedSLEs(); + cache.del(*digest, false); + auto const beforeCounts = + mapCounts(CountedObjects::getInstance().getCounts(0)); + + env(check::cash(alice, bob_index, check::DeliverMin(XRP(100))), + ter(tecNO_ENTRY)); + + auto const afterCounts = + mapCounts(CountedObjects::getInstance().getCounts(0)); + + using namespace std::string_literals; + BEAST_EXPECT( + beforeCounts.at("CachedView::hit"s) == + afterCounts.at("CachedView::hit"s)); + BEAST_EXPECT( + beforeCounts.at("CachedView::hitExpired"s) + 1 == + afterCounts.at("CachedView::hitExpired"s)); + BEAST_EXPECT( + beforeCounts.at("CachedView::miss"s) == + afterCounts.at("CachedView::miss"s)); } } } diff --git a/src/xrpld/ledger/detail/CachedView.cpp b/src/xrpld/ledger/detail/CachedView.cpp index eb791fc9321..0463e5de05c 100644 --- a/src/xrpld/ledger/detail/CachedView.cpp +++ b/src/xrpld/ledger/detail/CachedView.cpp @@ -57,12 +57,10 @@ CachedViewImpl::read(Keylet const& k) const baseRead = true; return base_.read(k); }); - { - // If the sle is null, then a failure must have occurred in base_.read() - XRPL_ASSERT( - sle || baseRead, - "ripple::CachedView::read : null SLE result from base"); - } + // If the sle is null, then a failure must have occurred in base_.read() + XRPL_ASSERT( + sle || baseRead, + "ripple::CachedView::read : null SLE result from base"); if (cacheHit && baseRead) hitsexpired.increment(); else if (cacheHit)