diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index baeacb24d..614111e4e 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -1282,14 +1282,16 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel) if (bFillOrKill) { JLOG (j_.trace()) << "Fill or Kill: offer killed"; + if (sb.rules().enabled (fix1578)) + return { tecKILLED, false }; return { tesSUCCESS, false }; } // For 'immediate or cancel' offers, the amount remaining doesn't get - // placed - it gets cancelled and the operation succeeds. + // placed - it gets canceled and the operation succeeds. if (bImmediateOrCancel) { - JLOG (j_.trace()) << "Immediate or cancel: offer cancelled"; + JLOG (j_.trace()) << "Immediate or cancel: offer canceled"; return { tesSUCCESS, true }; } diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index 8a5365d15..c82b815e4 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -317,9 +317,14 @@ SetTrust::doApply () std::uint32_t const uFlagsIn (sleRippleState->getFieldU32 (sfFlags)); std::uint32_t uFlagsOut (uFlagsIn); - if (bSetNoRipple && !bClearNoRipple && (bHigh ? saHighBalance : saLowBalance) >= beast::zero) + if (bSetNoRipple && !bClearNoRipple) { - uFlagsOut |= (bHigh ? lsfHighNoRipple : lsfLowNoRipple); + if ((bHigh ? saHighBalance : saLowBalance) >= beast::zero) + uFlagsOut |= (bHigh ? lsfHighNoRipple : lsfLowNoRipple); + + else if (view().rules().enabled(fix1578)) + // Cannot set noRipple on a negative balance. + return tecNO_PERMISSION; } else if (bClearNoRipple && !bSetNoRipple) { diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 9fcd0e074..f8c90283d 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -634,14 +634,14 @@ Transactor::operator()() if (ctx_.size() > oversizeMetaDataCap) result = tecOVERSIZE; - if ((result == tecOVERSIZE) || + if ((result == tecOVERSIZE) || (result == tecKILLED) || (isTecClaimHardFail (result, view().flags()))) { JLOG(j_.trace()) << "reapplying because of " << transToken(result); std::vector removedOffers; - if (result == tecOVERSIZE) + if ((result == tecOVERSIZE) || (result == tecKILLED)) { ctx_.visit ( [&removedOffers]( @@ -668,7 +668,7 @@ Transactor::operator()() fee = reset(fee); // If necessary, remove any offers found unfunded during processing - if (result == tecOVERSIZE) + if ((result == tecOVERSIZE) || (result == tecKILLED)) removeUnfundedOffers (view(), removedOffers, ctx_.app.journal ("View")); applied = true; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index a66127582..8bf7abcd1 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -80,7 +80,8 @@ class FeatureCollections "fix1543", "fix1623", "DepositPreauth", - "fix1515" + "fix1515", + "fix1578" }; std::vector features; @@ -367,6 +368,7 @@ extern uint256 const fix1543; extern uint256 const fix1623; extern uint256 const featureDepositPreauth; extern uint256 const fix1515; +extern uint256 const fix1578; } // ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 1b4a9c8e2..b7473f145 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -263,6 +263,7 @@ enum TECcodes : TERUnderlyingType tecINVARIANT_FAILED = 147, tecEXPIRED = 148, tecDUPLICATE = 149, + tecKILLED = 150 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index dd5216f97..3d5c41910 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -111,9 +111,10 @@ detail::supportedAmendments () { "7117E2EC2DBF119CA55181D69819F1999ECEE1A0225A7FD2B9ED47940968479C fix1571" }, { "CA7C02118BA27599528543DFE77BA6838D1B0F43B447D4D7F53523CE6A0E9AC2 fix1543" }, { "58BE9B5968C4DA7C59BA900961828B113E5490699B21877DEF9A31E9D0FE5D5F fix1623" }, - { "3CBC5C4E630A1B82380295CDA84B32B49DD066602E74E39B85EF64137FA65194 DepositPreauth"}, + { "3CBC5C4E630A1B82380295CDA84B32B49DD066602E74E39B85EF64137FA65194 DepositPreauth" }, // Use liquidity from strands that consume max offers, but mark as dry - { "5D08145F0A4983F23AFFFF514E83FAD355C5ABFBB6CAB76FB5BC8519FF5F33BE fix1515"} + { "5D08145F0A4983F23AFFFF514E83FAD355C5ABFBB6CAB76FB5BC8519FF5F33BE fix1515" }, + { "FBD513F1B893AC765B78F250E6FFA6A11B573209D1842ADC787C850696741288 fix1578" } }; return supported; } @@ -168,5 +169,6 @@ uint256 const fix1543 = *getRegisteredFeature("fix1543"); uint256 const fix1623 = *getRegisteredFeature("fix1623"); uint256 const featureDepositPreauth = *getRegisteredFeature("DepositPreauth"); uint256 const fix1515 = *getRegisteredFeature("fix1515"); +uint256 const fix1578 = *getRegisteredFeature("fix1578"); } // ripple diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index aea0214e8..ad488ef8f 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -73,6 +73,7 @@ transResults() { tecINVARIANT_FAILED, { "tecINVARIANT_FAILED", "One or more invariants for the transaction were not satisfied." } }, { tecEXPIRED, { "tecEXPIRED", "Expiration time is passed." } }, { tecDUPLICATE, { "tecDUPLICATE", "Ledger object already exists." } }, + { tecKILLED, { "tecKILLED", "FillOrKill offer killed." } }, { tefALREADY, { "tefALREADY", "The exact transaction was already in this ledger." } }, { tefBAD_ADD_AUTH, { "tefBAD_ADD_AUTH", "Not authorized to add account." } }, diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 0624c2863..dcd5ccd8c 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -619,10 +619,16 @@ public: auto const bob = Account {"bob"}; auto const USD = gw["USD"]; - // Fill or Kill - unless we fully cross, just charge - // a fee and not place the offer on the books: + // Fill or Kill - unless we fully cross, just charge a fee and don't + // place the offer on the books. But also clean up expired offers + // that are discovered along the way. + // + // fix1578 changes the return code. Verify expected behavior + // without and with fix1578. + for (auto const tweakedFeatures : + {features - fix1578, features | fix1578}) { - Env env {*this, features}; + Env env {*this, tweakedFeatures}; auto const closeTime = fix1449Time () + 100 * env.closed ()->info ().closeTimeResolution; @@ -631,20 +637,41 @@ public: auto const f = env.current ()->fees ().base; env.fund (startBalance, gw, alice, bob); + + // bob creates an offer that expires before the next ledger close. + env (offer (bob, USD (500), XRP (500)), + json (sfExpiration.fieldName, lastClose(env) + 1), + ter(tesSUCCESS)); + + // The offer expires (it's not removed yet). + env.close (); + env.require ( + owners (bob, 1), + offers (bob, 1)); + + // bob creates the offer that will be crossed. env (offer (bob, USD (500), XRP (500)), ter(tesSUCCESS)); + env.close(); + env.require ( + owners (bob, 2), + offers (bob, 2)); + env (trust (alice, USD (1000)), ter(tesSUCCESS)); env (pay (gw, alice, USD (1000)), ter(tesSUCCESS)); - // Order that can't be filled: - env (offer (alice, XRP (1000), USD (1000)), - txflags (tfFillOrKill), ter(tesSUCCESS)); - + // Order that can't be filled but will remove bob's expired offer: + { + TER const killedCode {tweakedFeatures[fix1578] ? + TER {tecKILLED} : TER {tesSUCCESS}}; + env (offer (alice, XRP (1000), USD (1000)), + txflags (tfFillOrKill), ter(killedCode)); + } env.require ( - balance (alice, startBalance - f - f), + balance (alice, startBalance - (f * 2)), balance (alice, USD (1000)), owners (alice, 1), offers (alice, 0), - balance (bob, startBalance - f), + balance (bob, startBalance - (f * 2)), balance (bob, USD (none)), owners (bob, 1), offers (bob, 1)); @@ -654,11 +681,11 @@ public: txflags (tfFillOrKill), ter(tesSUCCESS)); env.require ( - balance (alice, startBalance - f - f - f + XRP (500)), + balance (alice, startBalance - (f * 3) + XRP (500)), balance (alice, USD (500)), owners (alice, 1), offers (alice, 0), - balance (bob, startBalance - f - XRP (500)), + balance (bob, startBalance - (f * 2) - XRP (500)), balance (bob, USD (500)), owners (bob, 1), offers (bob, 0)); @@ -682,7 +709,7 @@ public: // No cross: env (offer (alice, XRP (1000), USD (1000)), - txflags (tfImmediateOrCancel), ter(tesSUCCESS)); + txflags (tfImmediateOrCancel), ter(tesSUCCESS)); env.require ( balance (alice, startBalance - f - f), @@ -691,9 +718,9 @@ public: offers (alice, 0)); // Partially cross: - env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS)); + env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS)); env (offer (alice, XRP (1000), USD (1000)), - txflags (tfImmediateOrCancel), ter(tesSUCCESS)); + txflags (tfImmediateOrCancel), ter(tesSUCCESS)); env.require ( balance (alice, startBalance - f - f - f + XRP (50)), @@ -706,9 +733,9 @@ public: offers (bob, 0)); // Fully cross: - env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS)); + env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS)); env (offer (alice, XRP (50), USD (50)), - txflags (tfImmediateOrCancel), ter(tesSUCCESS)); + txflags (tfImmediateOrCancel), ter(tesSUCCESS)); env.require ( balance (alice, startBalance - f - f - f - f + XRP (100)), @@ -2739,6 +2766,10 @@ public: env.fund (XRP(10000000), gw, alice, bob); + // Code returned if an offer is killed. + TER const killedCode { + features[fix1578] ? TER {tecKILLED} : TER {tesSUCCESS}}; + // bob offers XRP for USD. env (trust(bob, USD(200))); env.close(); @@ -2748,8 +2779,8 @@ public: env.close(); { // alice submits a tfSell | tfFillOrKill offer that does not cross. - // It's still a tesSUCCESS, since the offer was successfully killed. - env (offer(alice, USD(21), XRP(2100), tfSell | tfFillOrKill)); + env (offer(alice, USD(21), XRP(2100), + tfSell | tfFillOrKill), ter (killedCode)); env.close(); env.require (balance (alice, USD(none))); env.require (offers (alice, 0)); @@ -2782,7 +2813,8 @@ public: // all of the offer is consumed. // We're using bob's left-over offer for XRP(500), USD(5) - env (offer(alice, USD(1), XRP(501), tfSell | tfFillOrKill)); + env (offer(alice, USD(1), XRP(501), + tfSell | tfFillOrKill), ter (killedCode)); env.close(); env.require (balance (alice, USD(35))); env.require (offers (alice, 0)); diff --git a/src/test/rpc/NoRipple_test.cpp b/src/test/rpc/NoRipple_test.cpp index 0ebf1ad3c..a4e1128ba 100644 --- a/src/test/rpc/NoRipple_test.cpp +++ b/src/test/rpc/NoRipple_test.cpp @@ -56,7 +56,8 @@ public: env.close(); // Check no-ripple flag on sender 'gateway' - auto lines = env.rpc("json", "account_lines", to_string(account_gw)); + Json::Value lines {env.rpc( + "json", "account_lines", to_string(account_gw))}; auto const& gline0 = lines[jss::result][jss::lines][0u]; BEAST_EXPECT(gline0[jss::no_ripple].asBool() == SetOrClear); @@ -72,45 +73,92 @@ public: testcase("Set noripple on a line with negative balance"); using namespace jtx; - Env env(*this, features); - auto const gw = Account("gateway"); auto const alice = Account("alice"); auto const bob = Account("bob"); auto const carol = Account("carol"); - env.fund(XRP(10000), gw, alice, bob, carol); + // fix1578 changes the return code. Verify expected behavior + // without and with fix1578. + for (auto const tweakedFeatures : + {features - fix1578, features | fix1578}) + { + Env env(*this, tweakedFeatures); - env.trust(alice["USD"](100), bob); - env.trust(bob["USD"](100), carol); - env.close(); + env.fund(XRP(10000), gw, alice, bob, carol); - env(pay(alice, carol, carol["USD"](50)), path(bob)); + env.trust(alice["USD"](100), bob); + env.trust(bob["USD"](100), carol); + env.close(); - env(trust(alice, bob["USD"](100), bob, tfSetNoRipple)); - env(trust(bob, carol["USD"](100), carol, tfSetNoRipple)); - env.close(); + // After this payment alice has a -50 USD balance with bob, and + // bob has a -50 USD balance with carol. So neither alice nor + // bob should be able to clear the noRipple flag. + env(pay(alice, carol, carol["USD"](50)), path(bob)); + env.close(); - Json::Value params; - params[jss::source_account] = alice.human(); - params[jss::destination_account] = carol.human(); - params[jss::destination_amount] = [] { - Json::Value dest_amt; - dest_amt[jss::currency] = "USD"; - dest_amt[jss::value] = "1"; - dest_amt[jss::issuer] = Account("carol").human(); - return dest_amt; - }(); + TER const terNeg {tweakedFeatures[fix1578] ? + TER {tecNO_PERMISSION} : TER {tesSUCCESS}}; - auto const resp = env.rpc("json", "ripple_path_find", to_string(params)); - BEAST_EXPECT(resp[jss::result][jss::alternatives].size()==1); + env(trust( + alice, bob["USD"](100), bob, tfSetNoRipple), ter(terNeg)); + env(trust( + bob, carol["USD"](100), carol, tfSetNoRipple), ter(terNeg)); + env.close(); - Json::Value account_alice; - account_alice[jss::account] = alice.human(); - auto const res = env.rpc("json", "account_lines", to_string(account_alice)); - auto const& lines = res[jss::result][jss::lines]; - BEAST_EXPECT(lines.size() == 1); - BEAST_EXPECT(!lines[0u].isMember(jss::no_ripple)); + Json::Value params; + params[jss::source_account] = alice.human(); + params[jss::destination_account] = carol.human(); + params[jss::destination_amount] = [] { + Json::Value dest_amt; + dest_amt[jss::currency] = "USD"; + dest_amt[jss::value] = "1"; + dest_amt[jss::issuer] = Account("carol").human(); + return dest_amt; + }(); + + auto const resp = env.rpc( + "json", "ripple_path_find", to_string(params)); + BEAST_EXPECT(resp[jss::result][jss::alternatives].size()==1); + + auto getAccountLines = [&env] (Account const& acct) + { + Json::Value jv; + jv[jss::account] = acct.human(); + auto const resp = + env.rpc("json", "account_lines", to_string(jv)); + return resp[jss::result][jss::lines]; + }; + { + auto const aliceLines = getAccountLines (alice); + BEAST_EXPECT(aliceLines.size() == 1); + BEAST_EXPECT(!aliceLines[0u].isMember(jss::no_ripple)); + + auto const bobLines = getAccountLines (bob); + BEAST_EXPECT(bobLines.size() == 2); + BEAST_EXPECT(!bobLines[0u].isMember(jss::no_ripple)); + BEAST_EXPECT(!bobLines[1u].isMember(jss::no_ripple)); + } + + // Now carol sends the 50 USD back to alice. Then alice and + // bob can set the noRipple flag. + env(pay(carol, alice, alice["USD"](50)), path(bob)); + env.close(); + + env(trust(alice, bob["USD"](100), bob, tfSetNoRipple)); + env(trust(bob, carol["USD"](100), carol, tfSetNoRipple)); + env.close(); + { + auto const aliceLines = getAccountLines (alice); + BEAST_EXPECT(aliceLines.size() == 1); + BEAST_EXPECT(aliceLines[0u].isMember(jss::no_ripple)); + + auto const bobLines = getAccountLines (bob); + BEAST_EXPECT(bobLines.size() == 2); + BEAST_EXPECT(bobLines[0u].isMember(jss::no_ripple_peer)); + BEAST_EXPECT(bobLines[1u].isMember(jss::no_ripple)); + } + } } void testPairwise(FeatureBitset features) @@ -144,7 +192,8 @@ public: return dest_amt; }(); - auto const resp = env.rpc("json", "ripple_path_find", to_string(params)); + Json::Value const resp { + env.rpc("json", "ripple_path_find", to_string(params))}; BEAST_EXPECT(resp[jss::result][jss::alternatives].size() == 0); env(pay(alice, carol, bob["USD"](50)), ter(tecPATH_DRY));