Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check XRP endpoints for circular paths (RIPD-1781): #3209

Closed
wants to merge 1 commit into from
Closed

Check XRP endpoints for circular paths (RIPD-1781): #3209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/ripple/app/paths/impl/XRPEndpointStep.cpp
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@
#include <ripple/basics/Log.h>
#include <ripple/basics/XRPAmount.h>
#include <ripple/ledger/PaymentSandbox.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/Quality.h>

#include <boost/container/flat_set.hpp>
Expand Down Expand Up @@ -359,6 +360,18 @@ XRPEndpointStep<TDerived>::check (StrandContext const& ctx) const
if (ter != tesSUCCESS)
return ter;

if (ctx.view.rules().enabled(fix1781))
{
auto const issuesIndex = isLast_ ? 0 : 1;
if (!ctx.seenDirectIssues[issuesIndex].insert(xrpIssue()).second)
seelabs marked this conversation as resolved.
Show resolved Hide resolved
{
JLOG(j_.debug())
<< "XRPEndpointStep: loop detected: Index: " << ctx.strandSize
<< ' ' << *this;
return temBAD_PATH_LOOP;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run code_cov over the new code, this line returning temBAD_PATH_LOOP isn't hit. Is there a (not too painful) way to hit this line with a unit test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I can make a test that will hit this. The reason is the book steps also check for a path, and the book step will detect the loop before the endpoint will.

I'd still like to leave this code in, but in practice I don't think it can be hit.

}
}

return tesSUCCESS;
}

Expand Down
2 changes: 2 additions & 0 deletions src/ripple/protocol/Feature.h
View file
Open in desktop
Expand Up @@ -111,6 +111,7 @@ class FeatureCollections
// fixQualityUpperBound should be activated before FlowCross
"fixQualityUpperBound",
"RequireFullyCanonicalSig",
"fix1781", // XRPEndpointSteps should be included in the circular payment check
};

std::vector<uint256> features;
Expand Down Expand Up @@ -399,6 +400,7 @@ extern uint256 const fixPayChanRecipientOwnerDir;
extern uint256 const featureDeletableAccounts;
extern uint256 const fixQualityUpperBound;
extern uint256 const featureRequireFullyCanonicalSig;
extern uint256 const fix1781;

} // ripple

Expand Down
2 changes: 2 additions & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Expand Up @@ -130,6 +130,7 @@ detail::supportedAmendments ()
"DeletableAccounts",
"fixQualityUpperBound",
"RequireFullyCanonicalSig",
"fix1781",
};
return supported;
}
Expand Down Expand Up @@ -189,5 +190,6 @@ uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRec
uint256 const featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts");
uint256 const fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound");
uint256 const featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig");
uint256 const fix1781 = *getRegisteredFeature("fix1781");

} // ripple
97 changes: 96 additions & 1 deletion src/test/app/Flow_test.cpp
View file
Open in desktop
Expand Up @@ -1180,6 +1180,100 @@ struct Flow_test : public beast::unit_test::suite
ter(temBAD_PATH));
}

void
testXRPPathLoop()
{
testcase("Circular XRP");

using namespace jtx;
auto const alice = Account("alice");
auto const bob = Account("bob");
auto const gw = Account("gw");
auto const USD = gw["USD"];
auto const EUR = gw["EUR"];

for (auto const withFix : {true, false})
{
auto const feats = [&withFix]() -> FeatureBitset {
if (withFix)
return supported_amendments();
return supported_amendments() - FeatureBitset{fix1781};
}();
{
// Payment path starting with XRP
Env env(*this, feats);
env.fund(XRP(10000), alice, bob, gw);
env.trust(USD(1000), alice, bob);
env.trust(EUR(1000), alice, bob);
env(pay(gw, alice, USD(100)));
env(pay(gw, alice, EUR(100)));
env.close();

env(offer(alice, XRP(100), USD(100)), txflags(tfPassive));
env(offer(alice, USD(100), XRP(100)), txflags(tfPassive));
env(offer(alice, XRP(100), EUR(100)), txflags(tfPassive));
env.close();

TER const expectedTer =
withFix ? TER{temBAD_PATH_LOOP} : TER{tesSUCCESS};
env(pay(alice, bob, EUR(1)),
path(~USD, ~XRP, ~EUR),
sendmax(XRP(1)),
txflags(tfNoRippleDirect),
ter(expectedTer));
}
pass();
}
{
// Payment path ending with XRP
Env env(*this);
env.fund(XRP(10000), alice, bob, gw);
env.trust(USD(1000), alice, bob);
env.trust(EUR(1000), alice, bob);
env(pay(gw, alice, USD(100)));
env(pay(gw, alice, EUR(100)));
env.close();

env(offer(alice, XRP(100), USD(100)), txflags(tfPassive));
env(offer(alice, EUR(100), XRP(100)), txflags(tfPassive));
env.close();
// EUR -> //XRP -> //USD ->XRP
env(pay(alice, bob, XRP(1)),
path(~XRP, ~USD, ~XRP),
sendmax(EUR(1)),
txflags(tfNoRippleDirect),
ter(temBAD_PATH_LOOP));
}
{
// Payment where loop is formed in the middle of the path, not on an
// endpoint
auto const JPY = gw["JPY"];
Env env(*this);
env.fund(XRP(10000), alice, bob, gw);
env.close();
env.trust(USD(1000), alice, bob);
env.trust(EUR(1000), alice, bob);
env.trust(JPY(1000), alice, bob);
env.close();
env(pay(gw, alice, USD(100)));
env(pay(gw, alice, EUR(100)));
env(pay(gw, alice, JPY(100)));
env.close();

env(offer(alice, USD(100), XRP(100)), txflags(tfPassive));
env(offer(alice, XRP(100), EUR(100)), txflags(tfPassive));
env(offer(alice, EUR(100), XRP(100)), txflags(tfPassive));
env(offer(alice, XRP(100), JPY(100)), txflags(tfPassive));
env.close();

env(pay(alice, bob, JPY(1)),
path(~XRP, ~EUR, ~XRP, ~JPY),
sendmax(USD(1)),
txflags(tfNoRippleDirect),
ter(temBAD_PATH_LOOP));
}
}

void testWithFeats(FeatureBitset features)
{
using namespace jtx;
Expand All @@ -1204,6 +1298,7 @@ struct Flow_test : public beast::unit_test::suite
void run() override
{
testLimitQuality();
testXRPPathLoop();
testRIPD1443();
testRIPD1449();

Expand Down Expand Up @@ -1231,7 +1326,7 @@ struct Flow_manual_test : public Flow_test
testWithFeats(all - flowCross - f1513);
testWithFeats(all - flowCross );
testWithFeats(all - f1513);
testWithFeats(all );
testWithFeats(all );

testEmptyStrand(all - f1513);
testEmptyStrand(all );
Expand Down
2 changes: 1 addition & 1 deletion src/test/app/PayStrand_test.cpp
View file
Open in desktop
Expand Up @@ -703,7 +703,7 @@ struct PayStrand_test : public beast::unit_test::suite
alice,
/*deliver*/ xrpIssue(),
/*limitQuality*/ boost::none,
/*sendMaxIssue*/ xrpIssue(),
/*sendMaxIssue*/ EUR.issue(),
path,
true,
false,
Expand Down