From be45af33e26ee73bbe630656a5a23fbc0b6445e3 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Mon, 2 Oct 2017 18:33:22 -0700 Subject: [PATCH] PayChan and Escrow should ignore DisallowXRP (RIPD-1462) --- src/ripple/app/tx/impl/Escrow.cpp | 6 +++- src/ripple/app/tx/impl/PayChan.cpp | 16 +++++++--- src/test/app/Escrow_test.cpp | 35 ++++++++++++++++++---- src/test/app/PayChan_test.cpp | 47 ++++++++++++++++++++---------- 4 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 8b28be74f62..32434e2bfba 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -235,7 +235,11 @@ EscrowCreate::doApply() if (((*sled)[sfFlags] & lsfRequireDestTag) && ! ctx_.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - if ((*sled)[sfFlags] & lsfDisallowXRP) + + // Obeying the lsfDissalowXRP flag was a bug. Piggyback on + // featureDepositAuth to remove the bug. + if (! ctx_.view().rules().enabled(featureDepositAuth) && + ((*sled)[sfFlags] & lsfDisallowXRP)) return tecNO_TARGET; } diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index d8ee7f7e710..c104a70846e 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -206,7 +206,11 @@ PayChanCreate::preclaim(PreclaimContext const &ctx) if (((*sled)[sfFlags] & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - if ((*sled)[sfFlags] & lsfDisallowXRP) + + // Obeying the lsfDisallowXRP flag was a bug. Piggyback on + // featureDepositAuth to remove the bug. + if (!ctx.view.rules().enabled(featureDepositAuth) && + ((*sled)[sfFlags] & lsfDisallowXRP)) return tecNO_TARGET; } @@ -457,15 +461,19 @@ PayChanClaim::doApply() if (!sled) return terNO_ACCOUNT; - if (txAccount == src && (sled->getFlags() & lsfDisallowXRP)) + // Obeying the lsfDisallowXRP flag was a bug. Piggyback on + // featureDepositAuth to remove the bug. + bool const depositAuth {ctx_.view().rules().enabled(featureDepositAuth)}; + if (!depositAuth && + (txAccount == src && (sled->getFlags() & lsfDisallowXRP))) return tecNO_TARGET; // Check whether the destination account requires deposit authorization if (txAccount != dst) { - if (ctx_.view().rules().enabled(featureDepositAuth) && + if (depositAuth && ((sled->getFlags() & lsfDepositAuth) == lsfDepositAuth)) - return tecNO_PERMISSION; + return tecNO_PERMISSION; } (*slep)[sfBalance] = ctx_.tx[sfBalance]; diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 154cb25483c..debcfdcc6c6 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -253,6 +253,35 @@ struct Escrow_test : public beast::unit_test::suite BEAST_EXPECT((*sle)[sfDestinationTag] == 2); } + void + testDisallowXRP() + { + testcase ("Disallow XRP"); + + using namespace jtx; + using namespace std::chrono; + + { + // Respect the "asfDisallowXRP" account flag: + Env env(*this, supported_amendments() - featureDepositAuth); + + env.fund(XRP(5000), "bob", "george"); + env(fset("george", asfDisallowXRP)); + env(lockup("bob", "george", XRP(10), env.now() + 1s), + ter (tecNO_TARGET)); + } + { + // Ignore the "asfDisallowXRP" account flag, which we should + // have been doing before. + Env env(*this); + + env.fund(XRP(5000), "bob", "george"); + env(fset("george", asfDisallowXRP)); + env(lockup("bob", "george", XRP(10), env.now() + 1s)); + } + + } + void testFails() { @@ -331,11 +360,6 @@ struct Escrow_test : public beast::unit_test::suite env.fund (accountReserve, "frank"); env(lockup("frank", "bob", XRP(1), env.now() + 1s), ter (tecINSUFFICIENT_RESERVE)); - // Respect the "asfDisallowXRP" account flag: - env.fund (accountReserve + accountIncrement, "george"); - env(fset("george", asfDisallowXRP)); - env(lockup("bob", "george", XRP(10), env.now() + 1s), ter (tecNO_TARGET)); - { // Specify incorrect sequence number env.fund (XRP(5000), "hannah"); auto const seq = env.seq("hannah"); @@ -980,6 +1004,7 @@ struct Escrow_test : public beast::unit_test::suite { testEnablement(); testTags(); + testDisallowXRP(); testFails(); testLockup(); testEscrowConditions(); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 8df75572cae..72f2c73822d 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -629,40 +629,57 @@ struct PayChan_test : public beast::unit_test::suite testcase ("Disallow XRP"); using namespace jtx; using namespace std::literals::chrono_literals; + + auto const alice = Account ("alice"); + auto const bob = Account ("bob"); { // Create a channel where dst disallows XRP - Env env (*this); - auto const alice = Account ("alice"); - auto const bob = Account ("bob"); + Env env (*this, supported_amendments() - featureDepositAuth); env.fund (XRP (10000), alice, bob); env (fset (bob, asfDisallowXRP)); - auto const pk = alice.pk (); - auto const settleDelay = 3600s; - auto const channelFunds = XRP (1000); - env (create (alice, bob, channelFunds, settleDelay, pk), + env (create (alice, bob, XRP (1000), 3600s, alice.pk()), ter (tecNO_TARGET)); auto const chan = channel (*env.current (), alice, bob); BEAST_EXPECT (!channelExists (*env.current (), chan)); } + { + // Create a channel where dst disallows XRP. Ignore that flag, + // since it's just advisory. + Env env (*this); + env.fund (XRP (10000), alice, bob); + env (fset (bob, asfDisallowXRP)); + env (create (alice, bob, XRP (1000), 3600s, alice.pk())); + auto const chan = channel (*env.current (), alice, bob); + BEAST_EXPECT (channelExists (*env.current (), chan)); + } + { // Claim to a channel where dst disallows XRP // (channel is created before disallow xrp is set) - Env env (*this); - auto const alice = Account ("alice"); - auto const bob = Account ("bob"); + Env env (*this, supported_amendments() - featureDepositAuth); env.fund (XRP (10000), alice, bob); - auto const pk = alice.pk (); - auto const settleDelay = 3600s; - auto const channelFunds = XRP (1000); - env (create (alice, bob, channelFunds, settleDelay, pk)); + env (create (alice, bob, XRP (1000), 3600s, alice.pk())); auto const chan = channel (*env.current (), alice, bob); BEAST_EXPECT (channelExists (*env.current (), chan)); env (fset (bob, asfDisallowXRP)); - auto const preBob = env.balance (bob); auto const reqBal = XRP (500).value(); env (claim (alice, chan, reqBal, reqBal), ter(tecNO_TARGET)); } + { + // Claim to a channel where dst disallows XRP (channel is + // created before disallow xrp is set). Ignore that flag + // since it is just advisory. + Env env (*this); + env.fund (XRP (10000), alice, bob); + env (create (alice, bob, XRP (1000), 3600s, alice.pk())); + auto const chan = channel (*env.current (), alice, bob); + BEAST_EXPECT (channelExists (*env.current (), chan)); + + env (fset (bob, asfDisallowXRP)); + auto const reqBal = XRP (500).value(); + env (claim (alice, chan, reqBal, reqBal)); + } } void