diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 0fb9b02508..e5eaa0097b 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -82,7 +82,7 @@ namespace ripple { @param now the current time @param mark the cutoff point - @return true if \a now refers to a time strictly after \a mark, false otherwise. + @return true if \a now refers to a time strictly after \a mark, else false. */ static inline bool after (NetClock::time_point now, std::uint32_t mark) { @@ -251,18 +251,13 @@ EscrowCreate::doApply() } // If it's not a self-send, add escrow to recipient's owner directory. - if (ctx_.view ().rules().enabled(fix1523)) + if (auto const dest = ctx_.tx[sfDestination]; dest != ctx_.tx[sfAccount]) { - auto const dest = ctx_.tx[sfDestination]; - - if (dest != ctx_.tx[sfAccount]) - { - auto page = dirAdd(ctx_.view(), keylet::ownerDir(dest), slep->key(), - false, describeOwnerDir(dest), ctx_.app.journal ("View")); - if (!page) - return tecDIR_FULL; - (*slep)[sfDestinationNode] = *page; - } + auto page = dirAdd(ctx_.view(), keylet::ownerDir(dest), slep->key(), + false, describeOwnerDir(dest), ctx_.app.journal ("View")); + if (!page) + return tecDIR_FULL; + (*slep)[sfDestinationNode] = *page; } // Deduct owner's balance, increment owner count @@ -482,10 +477,10 @@ EscrowFinish::doApply() } // Remove escrow from recipient's owner directory, if present. - if (ctx_.view ().rules().enabled(fix1523) && (*slep)[~sfDestinationNode]) + if (auto const optPage = (*slep)[~sfDestinationNode]) { - auto const page = (*slep)[sfDestinationNode]; - if (! ctx_.view().dirRemove(keylet::ownerDir(destID), page, k.key, true)) + if (! ctx_.view().dirRemove( + keylet::ownerDir(destID), *optPage, k.key, true)) { return tefBAD_LEDGER; } @@ -564,11 +559,10 @@ EscrowCancel::doApply() } // Remove escrow from recipient's owner directory, if present. - if (ctx_.view ().rules().enabled(fix1523) && (*slep)[~sfDestinationNode]) + if (auto const optPage = (*slep)[~sfDestinationNode]; optPage) { - auto const page = (*slep)[sfDestinationNode]; if (! ctx_.view().dirRemove( - keylet::ownerDir((*slep)[sfDestination]), page, k.key, true)) + keylet::ownerDir((*slep)[sfDestination]), *optPage, k.key, true)) { return tefBAD_LEDGER; } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 6f6b6e94fb..99e7d2ff1b 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -380,7 +380,7 @@ extern uint256 const retiredSortedDirectories; extern uint256 const fix1201; extern uint256 const fix1512; extern uint256 const fix1513; -extern uint256 const fix1523; +extern uint256 const retiredFix1523; extern uint256 const retiredFix1528; extern uint256 const featureDepositAuth; extern uint256 const featureChecks; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index fd36ff254a..682c760ca9 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -171,7 +171,7 @@ uint256 const retiredSortedDirectories = *getRegisteredFeature("SortedDirectorie uint256 const fix1201 = *getRegisteredFeature("fix1201"); uint256 const fix1512 = *getRegisteredFeature("fix1512"); uint256 const fix1513 = *getRegisteredFeature("fix1513"); -uint256 const fix1523 = *getRegisteredFeature("fix1523"); +uint256 const retiredFix1523 = *getRegisteredFeature("fix1523"); uint256 const retiredFix1528 = *getRegisteredFeature("fix1528"); uint256 const featureDepositAuth = *getRegisteredFeature("DepositAuth"); uint256 const featureChecks = *getRegisteredFeature("Checks"); diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 84ec7f7eba..6d78986c54 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -1056,28 +1056,7 @@ struct Escrow_test : public beast::unit_test::suite auto const carol = Account("carol"); { - testcase ("Metadata & Ownership (without fix1523)"); - Env env(*this, supported_amendments() - fix1523); - env.fund(XRP(5000), alice, bruce, carol); - - auto const seq = env.seq(alice); - env(escrow(alice, carol, XRP(1000)), finish_time(env.now() + 1s)); - - BEAST_EXPECT((*env.meta())[sfTransactionResult] == - static_cast(tesSUCCESS)); - - auto const escrow = env.le(keylet::escrow(alice.id(), seq)); - BEAST_EXPECT(escrow); - - ripple::Dir aod (*env.current(), keylet::ownerDir(alice.id())); - BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 1); - BEAST_EXPECT(std::find(aod.begin(), aod.end(), escrow) != aod.end()); - - ripple::Dir cod (*env.current(), keylet::ownerDir(carol.id())); - BEAST_EXPECT(cod.begin() == cod.end()); - } - { - testcase ("Metadata (with fix1523, to self)"); + testcase ("Metadata to self"); Env env(*this); env.fund(XRP(5000), alice, bruce, carol); @@ -1141,7 +1120,7 @@ struct Escrow_test : public beast::unit_test::suite } } { - testcase ("Metadata (with fix1523, to other)"); + testcase ("Metadata to other"); Env env(*this); env.fund(XRP(5000), alice, bruce, carol); @@ -1266,139 +1245,6 @@ struct Escrow_test : public beast::unit_test::suite } } - void testDeletedDestination() - { - // Make sure an Escrow behaves as expected if its destination is - // deleted from the ledger. This can only happen if fix1523 is - // not enabled. fix1523 is what adds the Escrow to the Escrow's - // destination directory. - testcase ("Deleted destination"); - - using namespace jtx; - using namespace std::chrono; - - Account const alice {"alice"}; - Account const bruce {"bruce"}; - Account const carol {"carol"}; - Account const daria {"daria"}; - Account const evita {"evita"}; - Account const fiona {"fiona"}; - Account const grace {"grace"}; - - Env env(*this, supported_amendments() - fix1523); - - env.fund(XRP(5000), alice, bruce, carol, daria, evita, fiona, grace); - env.close(); - - // Because fix1523 is not in play, bruce does not have a back link - // from his directory to the escrow. So we should be able to delete - // bruce's account even though he is the destination of an escrow. - std::uint32_t const aliceEscrowSeq {env.seq(alice)}; - env(escrow(alice, bruce, XRP(1000)), finish_time(env.now() + 1s)); - - // Similar to bruce, we should be able to delete daria's account. - std::uint32_t const carolEscrowSeq {env.seq(carol)}; - env(escrow(carol, daria, XRP(1000)), - finish_time(env.now() + 1s), cancel_time(env.now() + 2s)); - env.close(); - - // Now engage fix1523 so any additional escrows we make will have - // back links from both the source and the destination. - env.enableFeature (fix1523); - env.close(); - - // Neither evita not fiona should be able to delete their accounts - // once this escrow is created. - std::uint32_t const evitaEscrowSeq {env.seq(evita)}; - env(escrow(evita, fiona, XRP(1000)), finish_time(env.now() + 1s)); - env.close(); - - // Close enough ledgers to be able to delete any of the accounts. - { - std::uint32_t const openLedgerSeq {env.current()->seq()}; - int const delta = [&]()->int { - if (env.seq(alice) + 255 > openLedgerSeq) - return env.seq(alice) - openLedgerSeq + 255; - return 0; - }(); - for (int i = 0; i < delta; ++i) - env.close(); - } - - // The presence of escrows should prevent all of the accounts from - // being deleted except for bruce and daria. - auto const acctDelFee {drops (env.current()->fees().increment)}; - env (acctdelete (alice, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env (acctdelete (bruce, grace), fee (acctDelFee)); - env (acctdelete (carol, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env (acctdelete (daria, grace), fee (acctDelFee)); - env (acctdelete (evita, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env (acctdelete (fiona, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env.close(); - - // At this point the destination of alice's escrow is missing. Make - // sure the escrow still behaves well. In particular, an EscrowFinish - // should fail but leave the Escrow object in the ledger. - env(finish(alice, alice, aliceEscrowSeq), ter(tecNO_DST)); - env.close(); - - // Verify that alice's escrow remains in the ledger. - Keylet const aliceEscrowKey { - keylet::escrow (alice.id(), aliceEscrowSeq)}; - BEAST_EXPECT (env.current()->exists (aliceEscrowKey)); - - // Since alice still has the escrow she should not be able to delete - // her account. - env (acctdelete (alice, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - - // The fact that the destination of carol's escrow has evaporated - // should have no impact on whether carol's escrow can be canceled. - env(cancel(carol, carol, carolEscrowSeq)); - env.close(); - - // Now that carol's escrow is gone carol should be able to delete - // her account. - env (acctdelete (carol, grace), fee (acctDelFee)); - env.close(); - - // We'll now resurrect bruce's account. alice should then be able to - // finish her escrow. - env(pay (grace, bruce, XRP (5000))); - env.close(); - - // bruce's account should have returned to the ledger. - BEAST_EXPECT (env.current()->exists (keylet::account (bruce.id()))); - BEAST_EXPECT (env.balance (bruce) == XRP (5000)); - - // Verify that alice's escrow is still in the ledger. - BEAST_EXPECT (env.current()->exists (aliceEscrowKey)); - - // alice finishes her escrow to bruce's resurrected account. - env(finish(alice, alice, aliceEscrowSeq)); - env.close(); - - // Verify that alice's escrow is gone from the ledger. - BEAST_EXPECT (! env.current()->exists (aliceEscrowKey)); - - // Now alice can delete her account. - env (acctdelete (alice, grace), fee (acctDelFee)); - env.close(); - - // eveta and fiona are still prevented from deletion by evita's escrow. - env (acctdelete (evita, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env (acctdelete (fiona, grace), fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); - env.close(); - - // Finish evita's escrow. Then both evita's and fiona's accounts can - // be deleted. - env(finish(fiona, evita, evitaEscrowSeq)); - env.close(); - - env (acctdelete (evita, grace), fee (acctDelFee)); - env (acctdelete (fiona, grace), fee (acctDelFee)); - env.close(); - } - void run() override { testEnablement(); @@ -1411,7 +1257,6 @@ struct Escrow_test : public beast::unit_test::suite testEscrowConditions(); testMetaAndOwnership(); testConsequences(); - testDeletedDestination(); } };