diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 888fcc7c35..e4d087255b 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -492,6 +492,7 @@ else () src/ripple/app/tx/impl/CreateCheck.cpp src/ripple/app/tx/impl/CreateOffer.cpp src/ripple/app/tx/impl/CreateTicket.cpp + src/ripple/app/tx/impl/DeleteAccount.cpp src/ripple/app/tx/impl/DepositPreauth.cpp src/ripple/app/tx/impl/Escrow.cpp src/ripple/app/tx/impl/InvariantCheck.cpp @@ -721,6 +722,7 @@ else () nounity, test sources: subdir: app #]===============================] + src/test/app/AccountDelete_test.cpp src/test/app/AccountTxPaging_test.cpp src/test/app/AmendmentTable_test.cpp src/test/app/Check_test.cpp @@ -860,6 +862,7 @@ else () src/test/jtx/impl/JSONRPCClient.cpp src/test/jtx/impl/ManualTimeKeeper.cpp src/test/jtx/impl/WSClient.cpp + src/test/jtx/impl/acctdelete.cpp src/test/jtx/impl/amount.cpp src/test/jtx/impl/balance.cpp src/test/jtx/impl/check.cpp diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index c0789aa963..4e474c7b96 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -784,9 +784,7 @@ TxQ::apply(Application& app, OpenView& view, // so we don't get a false terPRE_SEQ. if (accountExists) { - auto const sle = view.read(keylet::account(account)); - - if (sle) + if (auto const sle = view.read(keylet::account(account)); sle) { auto& txQAcct = accountIter->second; auto const aSeq = (*sle)[sfSequence]; @@ -951,6 +949,8 @@ TxQ::apply(Application& app, OpenView& view, auto const sleBump = multiTxn->applyView->peek( keylet::account(account)); + if (!sleBump) + return {tefINTERNAL, false}; auto const potentialTotalSpend = multiTxn->fee + std::min(balance - std::min(balance, reserve), diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index fa692ac952..365de2d74d 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -1010,6 +1010,16 @@ BookStep::check(StrandContext const& ctx) const return temBAD_PATH_LOOP; } + auto issuerExists = [](ReadView const& view, Issue const& iss) -> bool { + return isXRP(iss.account) || view.read(keylet::account(iss.account)); + }; + + if (!issuerExists(ctx.view, book_.in) || !issuerExists(ctx.view, book_.out)) + { + JLOG(j_.debug()) << "BookStep: deleted issuer detected: " << *this; + return tecNO_ISSUER; + } + if (fix1443(ctx.view.info().parentCloseTime)) { if (ctx.prevStep) diff --git a/src/ripple/app/tx/impl/ApplyContext.cpp b/src/ripple/app/tx/impl/ApplyContext.cpp index 8f3c16aad3..33b69def06 100644 --- a/src/ripple/app/tx/impl/ApplyContext.cpp +++ b/src/ripple/app/tx/impl/ApplyContext.cpp @@ -103,7 +103,7 @@ ApplyContext::checkInvariantsHelper( std::shared_ptr const& before, std::shared_ptr const& after) { - (..., std::get(checkers).visitEntry(index, isDelete, before, after)); + (..., std::get(checkers).visitEntry(isDelete, before, after)); }); // Note: do not replace this logic with a `...&&` fold expression. @@ -112,7 +112,7 @@ ApplyContext::checkInvariantsHelper( // message won't be. Every failed invariant should write to the log, // not just the first one. std::array finalizers{ - {std::get(checkers).finalize(tx, result, fee, journal)...}}; + {std::get(checkers).finalize(tx, result, fee, *view_, journal)...}}; // call each check's finalizer to see that it passes if (! std::all_of( finalizers.cbegin(), finalizers.cend(), diff --git a/src/ripple/app/tx/impl/ApplyContext.h b/src/ripple/app/tx/impl/ApplyContext.h index cb7890d7d1..a545a1c901 100644 --- a/src/ripple/app/tx/impl/ApplyContext.h +++ b/src/ripple/app/tx/impl/ApplyContext.h @@ -31,8 +31,6 @@ namespace ripple { -// tx_enable_test - /** State information when applying a tx. */ class ApplyContext { diff --git a/src/ripple/app/tx/impl/CancelOffer.cpp b/src/ripple/app/tx/impl/CancelOffer.cpp index eaa76572d3..70da5b5cf5 100644 --- a/src/ripple/app/tx/impl/CancelOffer.cpp +++ b/src/ripple/app/tx/impl/CancelOffer.cpp @@ -59,8 +59,9 @@ CancelOffer::preclaim(PreclaimContext const& ctx) auto const id = ctx.tx[sfAccount]; auto const offerSequence = ctx.tx[sfOfferSequence]; - auto const sle = ctx.view.read( - keylet::account(id)); + auto const sle = ctx.view.read(keylet::account(id)); + if (! sle) + return terNO_ACCOUNT; if ((*sle)[sfSequence] <= offerSequence) { @@ -79,8 +80,9 @@ CancelOffer::doApply () { auto const offerSequence = ctx_.tx[sfOfferSequence]; - auto const sle = view().read( - keylet::account(account_)); + auto const sle = view().read(keylet::account(account_)); + if (! sle) + return tefINTERNAL; uint256 const offerIndex (getOfferIndex (account_, offerSequence)); diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index acbb0b9f3c..0d0590e908 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -321,7 +321,12 @@ CashCheck::doApply () ctx_.deliver (xrpDeliver); // The source account has enough XRP so make the ledger change. - transferXRP (psb, srcId, account_, xrpDeliver, viewJ); + if (TER const ter {transferXRP ( + psb, srcId, account_, xrpDeliver, viewJ)}; ter != tesSUCCESS) + { + // The transfer failed. Return the error code. + return ter; + } } else { diff --git a/src/ripple/app/tx/impl/CreateCheck.cpp b/src/ripple/app/tx/impl/CreateCheck.cpp index 5bddbbd46a..b14b85bc47 100644 --- a/src/ripple/app/tx/impl/CreateCheck.cpp +++ b/src/ripple/app/tx/impl/CreateCheck.cpp @@ -169,6 +169,8 @@ TER CreateCheck::doApply () { auto const sle = view().peek (keylet::account (account_)); + if (! sle) + return tefINTERNAL; // A check counts against the reserve of the issuing account, but we // check the starting balance because we want to allow dipping into the diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 672898103e..14908d5edf 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -151,6 +151,8 @@ CreateOffer::preclaim(PreclaimContext const& ctx) auto const cancelSequence = ctx.tx[~sfOfferSequence]; auto const sleCreator = ctx.view.read(keylet::account(id)); + if (! sleCreator) + return terNO_ACCOUNT; std::uint32_t const uAccountSequence = sleCreator->getFieldU32(sfSequence); @@ -1310,6 +1312,9 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel) } auto const sleCreator = sb.peek (keylet::account(account_)); + if (! sleCreator) + return { tefINTERNAL, false }; + { XRPAmount reserve = ctx_.view().fees().accountReserve( sleCreator->getFieldU32 (sfOwnerCount) + 1); diff --git a/src/ripple/app/tx/impl/CreateTicket.cpp b/src/ripple/app/tx/impl/CreateTicket.cpp index 2f9fae474e..1897c8fe69 100644 --- a/src/ripple/app/tx/impl/CreateTicket.cpp +++ b/src/ripple/app/tx/impl/CreateTicket.cpp @@ -56,6 +56,8 @@ TER CreateTicket::doApply () { auto const sle = view().peek(keylet::account(account_)); + if (! sle) + return tefINTERNAL; // A ticket counts against the reserve of the issuing account, but we // check the starting balance because we want to allow dipping into the diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp new file mode 100644 index 0000000000..d30738132a --- /dev/null +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -0,0 +1,316 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2019 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace ripple { + +NotTEC +DeleteAccount::preflight (PreflightContext const& ctx) +{ + if (! ctx.rules.enabled(featureDeletableAccounts)) + return temDISABLED; + + if (ctx.tx.getFlags() & tfUniversalMask) + return temINVALID_FLAG; + + auto const ret = preflight1 (ctx); + + if (!isTesSuccess (ret)) + return ret; + + if (ctx.tx[sfAccount] == ctx.tx[sfDestination]) + // An account cannot be deleted and give itself the resulting XRP. + return temDST_IS_SRC; + + return preflight2 (ctx); +} + +std::uint64_t +DeleteAccount::calculateBaseFee ( + ReadView const& view, + STTx const& tx) +{ + // The fee required for AccountDelete is one owner reserve. But the + // owner reserve is stored in drops. We need to convert it to fee units. + Fees const& fees {view.fees()}; + std::pair const mulDivResult { + mulDiv (fees.increment, fees.units, fees.base)}; + if (mulDivResult.first) + return mulDivResult.second; + + // If mulDiv returns false then overflow happened. Punt by using the + // standard calculation. + return Transactor::calculateBaseFee (view, tx); +} + +namespace +{ +// Define a function pointer type that can be used to delete ledger node types. +using DeleterFuncPtr = TER(*)(Application& app, ApplyView& view, + AccountID const& account, uint256 const& delIndex, + std::shared_ptr const& sleDel, beast::Journal j); + +// Local function definitions that provides signature compatibility. +TER +offerDelete (Application& app, ApplyView& view, + AccountID const& account, uint256 const& delIndex, + std::shared_ptr const& sleDel, beast::Journal j) +{ + return offerDelete (view, sleDel, j); +} + +TER +removeSignersFromLedger (Application& app, ApplyView& view, + AccountID const& account, uint256 const& delIndex, + std::shared_ptr const& sleDel, beast::Journal j) +{ + return SetSignerList::removeFromLedger (app, view, account); +} + +TER +removeDepositPreauthFromLedger (Application& app, ApplyView& view, + AccountID const& account, uint256 const& delIndex, + std::shared_ptr const& sleDel, beast::Journal j) +{ + return DepositPreauth::removeFromLedger ( + app, view, delIndex, j); +} + +// Return nullptr if the LedgerEntryType represents an obligation that can't be deleted +// Otherwise return the pointer to the function that can delete the non-obligation +DeleterFuncPtr nonObligationDeleter(LedgerEntryType t) +{ + switch (t){ + case ltOFFER: return offerDelete; + case ltSIGNER_LIST: return removeSignersFromLedger; + // case ltTICKET: return ???; + case ltDEPOSIT_PREAUTH: return removeDepositPreauthFromLedger; + default: + return nullptr; + } +} + +} // namespace + +TER +DeleteAccount::preclaim (PreclaimContext const& ctx) +{ + AccountID const account {ctx.tx[sfAccount]}; + AccountID const dst {ctx.tx[sfDestination]}; + + auto sleDst = ctx.view.read(keylet::account(dst)); + + if (!sleDst) + return tecNO_DST; + + if ((*sleDst)[sfFlags] & lsfRequireDestTag && !ctx.tx[~sfDestinationTag]) + return tecDST_TAG_NEEDED; + + // Check whether the destination account requires deposit authorization. + if (ctx.view.rules().enabled(featureDepositAuth) && + (sleDst->getFlags() & lsfDepositAuth)) + { + if (! ctx.view.exists (keylet::depositPreauth (dst, account))) + return tecNO_PERMISSION; + } + + auto sleAccount = ctx.view.read (keylet::account(account)); + assert(sleAccount); + if (! sleAccount) + return terNO_ACCOUNT; + + // We don't allow an account to be deleted if its sequence number + // is within 256 of the current ledger. This prevents replay of old + // transactions if this account is resurrected after it is deleted. + // + // We look at the account's Sequence rather than the transaction's + // Sequence in preparation for Tickets. + constexpr std::uint32_t seqDelta {255}; + if ((*sleAccount)[sfSequence] + seqDelta > ctx.view.seq()) + return tecTOO_SOON; + + // Verify that the account does not own any objects that would prevent + // the account from being deleted. + Keylet const ownerDirKeylet {keylet::ownerDir (account)}; + if (dirIsEmpty(ctx.view, ownerDirKeylet)) + return tesSUCCESS; + + std::shared_ptr sleDirNode {}; + unsigned int uDirEntry {0}; + uint256 dirEntry {beast::zero}; + + if (! cdirFirst ( + ctx.view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, ctx.j)) + // Account has no directory at all. Looks good. + return tesSUCCESS; + + std::int32_t deletableDirEntryCount {0}; + do + { + // Make sure any directory node types that we find are the kind + // we can delete. + Keylet const itemKeylet {ltCHILD, dirEntry}; + auto sleItem = ctx.view.read (itemKeylet); + if (! sleItem) + { + // Directory node has an invalid index. Bail out. + JLOG (ctx.j.fatal()) << "DeleteAccount: directory node in ledger " + << ctx.view.seq() << " has index to object that is missing: " + << to_string (dirEntry); + return tefBAD_LEDGER; + } + + LedgerEntryType const nodeType { + safe_cast((*sleItem)[sfLedgerEntryType])}; + + if (!nonObligationDeleter(nodeType)) + return tecHAS_OBLIGATIONS; + + // We found a deletable directory entry. Count it. If we find too + // many deletable directory entries then bail out. + if (++deletableDirEntryCount > maxDeletableDirEntries) + return tefTOO_BIG; + + } while (cdirNext ( + ctx.view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, ctx.j)); + + return tesSUCCESS; +} + +TER +DeleteAccount::doApply () +{ + auto src = view().peek(keylet::account(account_)); + assert(src); + + auto dst = view().peek(keylet::account(ctx_.tx[sfDestination])); + assert(dst); + + if (!src || !dst) + return tefBAD_LEDGER; + + // Delete all of the entries in the account directory. + Keylet const ownerDirKeylet {keylet::ownerDir (account_)}; + std::shared_ptr sleDirNode {}; + unsigned int uDirEntry {0}; + uint256 dirEntry {beast::zero}; + + if (view().exists(ownerDirKeylet) && dirFirst ( + view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, j_)) + { + do + { + // Choose the right way to delete each directory node. + Keylet const itemKeylet {ltCHILD, dirEntry}; + auto sleItem = view().peek (itemKeylet); + if (! sleItem) + { + // Directory node has an invalid index. Bail out. + JLOG (j_.fatal()) << "DeleteAccount: Directory node in ledger " + << view().seq() << " has index to object that is missing: " + << to_string (dirEntry); + return tefBAD_LEDGER; + } + + LedgerEntryType const nodeType { + safe_cast( + sleItem->getFieldU16(sfLedgerEntryType))}; + + if (auto deleter = nonObligationDeleter(nodeType)) + { + TER const result { + deleter(ctx_.app, view(), account_, dirEntry, sleItem, j_)}; + + if (! isTesSuccess (result)) + return result; + } + else + { + assert (! "Undeletable entry should be found in preclaim."); + JLOG (j_.error()) + << "DeleteAccount undeletable item not found in preclaim."; + return tecHAS_OBLIGATIONS; + } + + // dirFirst() and dirNext() are like iterators with exposed + // internal state. We'll take advantage of that exposed state + // to solve a common C++ problem: iterator invalidation while + // deleting elements from a container. + // + // We have just deleted one directory entry, which means our + // "iterator state" is invalid. + // + // 1. During the process of getting an entry from the + // directory uDirEntry was incremented from 0 to 1. + // + // 2. We then deleted the entry at index 0, which means the + // entry that was at 1 has now moved to 0. + // + // 3. So we verify that uDirEntry is indeed 1. Then we jam it + // back to zero to "un-invalidate" the iterator. + assert (uDirEntry == 1); + if (uDirEntry != 1) + { + JLOG (j_.error()) + << "DeleteAccount iterator re-validation failed."; + return tefBAD_LEDGER; + } + uDirEntry = 0; + + } while (dirNext ( + view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, j_)); + } + + // Transfer any XRP remaining after the fee is paid to the destination: + (*dst)[sfBalance] = (*dst)[sfBalance] + mSourceBalance; + (*src)[sfBalance] = (*src)[sfBalance] - mSourceBalance; + ctx_.deliver (mSourceBalance); + + assert ((*src)[sfBalance] == XRPAmount(0)); + + // If there's still an owner directory associated with the source account + // delete it. + if (view().exists(ownerDirKeylet) && !view().emptyDirDelete(ownerDirKeylet)) + { + JLOG(j_.error()) << "DeleteAccount cannot delete root dir node of " + << toBase58 (account_); + return tecHAS_OBLIGATIONS; + } + + // Re-arm the password change fee if we can and need to. + if (mSourceBalance > XRPAmount(0) && dst->isFlag(lsfPasswordSpent)) + dst->clearFlag(lsfPasswordSpent); + + view().update(dst); + view().erase(src); + + return tesSUCCESS; +} + +} diff --git a/src/ripple/app/tx/impl/DeleteAccount.h b/src/ripple/app/tx/impl/DeleteAccount.h new file mode 100644 index 0000000000..24ec02842d --- /dev/null +++ b/src/ripple/app/tx/impl/DeleteAccount.h @@ -0,0 +1,72 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2019 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TX_DELETEACCOUNT_H_INCLUDED +#define RIPPLE_TX_DELETEACCOUNT_H_INCLUDED + +#include +#include +#include + +namespace ripple { + +class DeleteAccount + : public Transactor +{ +public: + // Set a reasonable upper limit on the number of deletable directory + // entries an account may have before we decide the account can't be + // deleted. + // + // A limit is useful because if we go much past this limit the + // transaction will fail anyway due to too much metadata (tecOVERSIZE). + static constexpr std::int32_t maxDeletableDirEntries {1000}; + + explicit DeleteAccount (ApplyContext& ctx) + : Transactor(ctx) + { + } + + static + bool + affectsSubsequentTransactionAuth(STTx const& tx) + { + return true; + } + + static + NotTEC + preflight (PreflightContext const& ctx); + + static + std::uint64_t + calculateBaseFee ( + ReadView const& view, + STTx const& tx); + + static + TER + preclaim (PreclaimContext const& ctx); + + TER doApply () override; +}; + +} + +#endif diff --git a/src/ripple/app/tx/impl/DepositPreauth.cpp b/src/ripple/app/tx/impl/DepositPreauth.cpp index 233530b0b3..10f5c0ebfe 100644 --- a/src/ripple/app/tx/impl/DepositPreauth.cpp +++ b/src/ripple/app/tx/impl/DepositPreauth.cpp @@ -113,6 +113,8 @@ DepositPreauth::doApply () if (ctx_.tx.isFieldPresent (sfAuthorize)) { auto const sleOwner = view().peek (keylet::account (account_)); + if (! sleOwner) + return {tefINTERNAL}; // A preauth counts against the reserve of the issuing account, but we // check the starting balance because we want to allow dipping into the @@ -154,36 +156,49 @@ DepositPreauth::doApply () } else { - // Verify that the Preauth entry they asked to remove is - // in the ledger. AccountID const unauth {ctx_.tx[sfUnauthorize]}; uint256 const preauthIndex {getDepositPreauthIndex (account_, unauth)}; - auto slePreauth = view().peek (keylet::depositPreauth (preauthIndex)); - if (! slePreauth) - { - // Error should have been caught in preclaim. - JLOG(j_.warn()) << "Selected DepositPreauth does not exist."; - return tecNO_ENTRY; - } - - auto viewJ = ctx_.app.journal ("View"); - std::uint64_t const page {(*slePreauth)[sfOwnerNode]}; - if (! view().dirRemove ( - keylet::ownerDir (account_), page, preauthIndex, true)) - { - JLOG(j_.warn()) << "Unable to delete DepositPreauth from owner."; - return tefBAD_LEDGER; - } - - // If we succeeded, update the DepositPreauth owner's reserve. - auto const sleOwner = view().peek (keylet::account (account_)); - adjustOwnerCount (view(), sleOwner, -1, viewJ); - - // Remove DepositPreauth from ledger. - view().erase (slePreauth); + return DepositPreauth::removeFromLedger ( + ctx_.app, view(), preauthIndex, j_); } return tesSUCCESS; } +TER +DepositPreauth::removeFromLedger (Application& app, + ApplyView& view, uint256 const& preauthIndex, beast::Journal j) +{ + // Verify that the Preauth entry they asked to remove is + // in the ledger. + std::shared_ptr const slePreauth { + view.peek (keylet::depositPreauth (preauthIndex))}; + if (! slePreauth) + { + JLOG(j.warn()) << "Selected DepositPreauth does not exist."; + return tecNO_ENTRY; + } + + AccountID const account {(*slePreauth)[sfAccount]}; + std::uint64_t const page {(*slePreauth)[sfOwnerNode]}; + if (! view.dirRemove ( + keylet::ownerDir (account), page, preauthIndex, false)) + { + JLOG(j.fatal()) << "Unable to delete DepositPreauth from owner."; + return tefBAD_LEDGER; + } + + // If we succeeded, update the DepositPreauth owner's reserve. + auto const sleOwner = view.peek (keylet::account (account)); + if (! sleOwner) + return tefINTERNAL; + + adjustOwnerCount (view, sleOwner, -1, app.journal ("View")); + + // Remove DepositPreauth from ledger. + view.erase (slePreauth); + + return tesSUCCESS; +} + } // namespace ripple diff --git a/src/ripple/app/tx/impl/DepositPreauth.h b/src/ripple/app/tx/impl/DepositPreauth.h index d950d7eca8..cdb03148ba 100644 --- a/src/ripple/app/tx/impl/DepositPreauth.h +++ b/src/ripple/app/tx/impl/DepositPreauth.h @@ -42,6 +42,12 @@ public: preclaim(PreclaimContext const& ctx); TER doApply () override; + + // Interface used by DeleteAccount + static + TER + removeFromLedger (Application& app, ApplyView& view, + uint256 const& delIndex, beast::Journal j); }; } // ripple diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index b1b36e7787..4d0d3d8207 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -196,8 +196,9 @@ EscrowCreate::doApply() } auto const account = ctx_.tx[sfAccount]; - auto const sle = ctx_.view().peek( - keylet::account(account)); + auto const sle = ctx_.view().peek(keylet::account(account)); + if (! sle) + return tefINTERNAL; // Check reserve and funds availability { diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 135b6e2e32..0ef4c9dfd1 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -19,12 +19,13 @@ #include #include +#include +#include namespace ripple { void TransactionFeeCheck::visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&) @@ -35,8 +36,9 @@ TransactionFeeCheck::visitEntry( bool TransactionFeeCheck::finalize( STTx const& tx, - TER const result, + TER const, XRPAmount const fee, + ReadView const&, beast::Journal const& j) { // We should never charge a negative fee @@ -70,7 +72,6 @@ TransactionFeeCheck::finalize( void XRPNotCreated::visitEntry( - uint256 const&, bool isDelete, std::shared_ptr const& before, std::shared_ptr const& after) @@ -123,9 +124,10 @@ XRPNotCreated::visitEntry( bool XRPNotCreated::finalize( - STTx const& tx, + STTx const&, TER const, XRPAmount const fee, + ReadView const&, beast::Journal const& j) { // The net change should never be positive, as this would mean that the @@ -153,7 +155,6 @@ XRPNotCreated::finalize( void XRPBalanceChecks::visitEntry( - uint256 const&, bool, std::shared_ptr const& before, std::shared_ptr const& after) @@ -185,7 +186,12 @@ XRPBalanceChecks::visitEntry( } bool -XRPBalanceChecks::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j) +XRPBalanceChecks::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { if (bad_) { @@ -200,7 +206,6 @@ XRPBalanceChecks::finalize(STTx const&, TER const, XRPAmount const, beast::Journ void NoBadOffers::visitEntry( - uint256 const&, bool isDelete, std::shared_ptr const& before, std::shared_ptr const& after) @@ -226,7 +231,12 @@ NoBadOffers::visitEntry( } bool -NoBadOffers::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journal const& j) +NoBadOffers::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { if (bad_) { @@ -241,7 +251,6 @@ NoBadOffers::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journal void NoZeroEscrow::visitEntry( - uint256 const&, bool isDelete, std::shared_ptr const& before, std::shared_ptr const& after) @@ -268,7 +277,12 @@ NoZeroEscrow::visitEntry( } bool -NoZeroEscrow::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journal const& j) +NoZeroEscrow::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { if (bad_) { @@ -283,19 +297,37 @@ NoZeroEscrow::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journa void AccountRootsNotDeleted::visitEntry( - uint256 const&, bool isDelete, std::shared_ptr const& before, std::shared_ptr const&) { if (isDelete && before && before->getType() == ltACCOUNT_ROOT) - accountDeleted_ = true; + accountsDeleted_++; } bool -AccountRootsNotDeleted::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j) +AccountRootsNotDeleted::finalize( + STTx const& tx, + TER const result, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { - if (! accountDeleted_) + if (tx.getTxnType() == ttACCOUNT_DELETE && result == tesSUCCESS) + { + if (accountsDeleted_ == 1) + return true; + + if (accountsDeleted_ == 0) + JLOG(j.fatal()) << "Invariant failed: account deletion " + "succeeded without deleting an account"; + else + JLOG(j.fatal()) << "Invariant failed: account deletion " + "succeeded but deleted multiple accounts!"; + return false; + } + + if (accountsDeleted_ == 0) return true; JLOG(j.fatal()) << "Invariant failed: an account root was deleted"; @@ -306,7 +338,6 @@ AccountRootsNotDeleted::finalize(STTx const&, TER const, XRPAmount const, beast: void LedgerEntryTypesMatch::visitEntry( - uint256 const&, bool, std::shared_ptr const& before, std::shared_ptr const& after) @@ -340,7 +371,12 @@ LedgerEntryTypesMatch::visitEntry( } bool -LedgerEntryTypesMatch::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j) +LedgerEntryTypesMatch::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { if ((! typeMismatch_) && (! invalidTypeAdded_)) return true; @@ -362,7 +398,6 @@ LedgerEntryTypesMatch::finalize(STTx const&, TER const, XRPAmount const, beast:: void NoXRPTrustLines::visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const& after) @@ -379,7 +414,12 @@ NoXRPTrustLines::visitEntry( } bool -NoXRPTrustLines::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j) +NoXRPTrustLines::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const& j) { if (! xrpTrustLine_) return true; @@ -388,5 +428,58 @@ NoXRPTrustLines::finalize(STTx const&, TER const, XRPAmount const, beast::Journa return false; } +//------------------------------------------------------------------------------ + +void +ValidNewAccountRoot::visitEntry( + bool, + std::shared_ptr const& before, + std::shared_ptr const& after) +{ + if (!before && after->getType() == ltACCOUNT_ROOT) + { + accountsCreated_++; + accountSeq_ = (*after)[sfSequence]; + } +} + +bool +ValidNewAccountRoot::finalize( + STTx const& tx, + TER const result, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) +{ + if (accountsCreated_ == 0) + return true; + + if (accountsCreated_ > 1) + { + JLOG(j.fatal()) << "Invariant failed: multiple accounts " + "created in a single transaction"; + return false; + } + + // From this point on we know exactly one account was created. + if (tx.getTxnType() == ttPAYMENT && result == tesSUCCESS) + { + std::uint32_t const startingSeq { + view.rules().enabled(featureDeletableAccounts) ? view.seq() : 1}; + + if (accountSeq_ != startingSeq) + { + JLOG(j.fatal()) << "Invariant failed: account created with " + "wrong starting sequence number"; + return false; + } + return true; + } + + JLOG(j.fatal()) << "Invariant failed: account root created " + "by a non-Payment or by an unsuccessful transaction"; + return false; +} + } // ripple diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index b1a2fbca32..af433221a5 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -32,6 +32,8 @@ namespace ripple { +class ReadView; + #if GENERATING_DOCS /** * @brief Prototype for invariant check implementations. @@ -49,14 +51,12 @@ public: /** * @brief called for each ledger entry in the current transaction. * - * @param index the key (identifier) for the ledger entry * @param isDelete true if the SLE is being deleted * @param before ledger entry before modification by the transaction * @param after ledger entry after modification by the transaction */ void visitEntry( - uint256 const& index, bool isDelete, std::shared_ptr const& before, std::shared_ptr const& after); @@ -68,6 +68,7 @@ public: * @param tx the transaction being applied * @param tec the current TER result of the transaction * @param fee the fee actually charged for this transaction + * @param view a ReadView of the ledger being modified * @param j journal for logging * * @return true if check passes, false if it fails @@ -77,6 +78,7 @@ public: STTx const& tx, TER const tec, XRPAmount const fee, + ReadView const& view, beast::Journal const& j); }; #endif @@ -92,13 +94,17 @@ class TransactionFeeCheck public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -116,36 +122,45 @@ class XRPNotCreated public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** * @brief Invariant: we cannot remove an account ledger entry * - * We iterate all accounts roots that were modified, and ensure that any that + * We iterate all account roots that were modified, and ensure that any that * were present before the transaction was applied continue to be present - * afterwards. + * afterwards unless they were explicitly deleted by a successful + * AccountDelete transaction. */ class AccountRootsNotDeleted { - bool accountDeleted_ = false; + std::uint32_t accountsDeleted_ = 0; public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -162,13 +177,17 @@ class XRPBalanceChecks public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -183,13 +202,17 @@ class LedgerEntryTypesMatch public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -205,13 +228,17 @@ class NoXRPTrustLines public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -228,14 +255,17 @@ class NoBadOffers public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); - + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; /** @@ -249,14 +279,43 @@ class NoZeroEscrow public: void visitEntry( - uint256 const&, bool, std::shared_ptr const&, std::shared_ptr const&); bool - finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&); + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); +}; +/** + * @brief Invariant: a new account root must be the consequence of a payment, + * must have the right starting sequence, and the payment + * may not create more than one new account root. + */ +class ValidNewAccountRoot +{ + std::uint32_t accountsCreated_ = 0; + std::uint32_t accountSeq_ = 0; // Only meaningful if accountsCreated_ > 0 + +public: + void + visitEntry( + bool, + std::shared_ptr const&, + std::shared_ptr const&); + + bool + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); }; // additional invariant checks can be declared above and then added to this @@ -269,7 +328,8 @@ using InvariantChecks = std::tuple< XRPNotCreated, NoXRPTrustLines, NoBadOffers, - NoZeroEscrow + NoZeroEscrow, + ValidNewAccountRoot >; /** diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index d4989337ce..7c2f83c239 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -22,18 +22,35 @@ namespace ripple { -template -TOfferStreamBase::TOfferStreamBase (ApplyView& view, ApplyView& cancelView, - Book const& book, NetClock::time_point when, - StepCounter& counter, beast::Journal journal) - : j_ (journal) - , view_ (view) - , cancelView_ (cancelView) - , book_ (book) - , expire_ (when) - , tip_ (view, book_) - , counter_ (counter) +namespace { +bool +checkIssuers(ReadView const& view, Book const& book) { + auto issuerExists = [](ReadView const& view, Issue const& iss) -> bool { + return isXRP(iss.account) || view.read(keylet::account(iss.account)); + }; + return issuerExists(view, book.in) && issuerExists(view, book.out); +} +} // namespace + +template +TOfferStreamBase::TOfferStreamBase( + ApplyView& view, + ApplyView& cancelView, + Book const& book, + NetClock::time_point when, + StepCounter& counter, + beast::Journal journal) + : j_(journal) + , view_(view) + , cancelView_(cancelView) + , book_(book) + , validBook_(checkIssuers(view, book)) + , expire_(when) + , tip_(view, book_) + , counter_(counter) +{ + assert(validBook_); } // Handle the case where a directory item with no corresponding ledger entry @@ -122,6 +139,9 @@ TOfferStreamBase::step () // Modifying the order or logic of these // operations causes a protocol breaking change. + if (!validBook_) + return false; + for(;;) { ownerFunds_ = boost::none; diff --git a/src/ripple/app/tx/impl/OfferStream.h b/src/ripple/app/tx/impl/OfferStream.h index 95e61aecff..a0d93d762a 100644 --- a/src/ripple/app/tx/impl/OfferStream.h +++ b/src/ripple/app/tx/impl/OfferStream.h @@ -73,6 +73,7 @@ protected: ApplyView& view_; ApplyView& cancelView_; Book book_; + bool validBook_; NetClock::time_point const expire_; BookTip tip_; TOffer offer_; diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index 3f9027b05c..6efe2aa9a5 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -154,6 +154,9 @@ closeChannel ( // Transfer amount back to owner, decrement owner count auto const sle = view.peek (keylet::account (src)); + if (! sle) + return tefINTERNAL; + assert ((*slep)[sfAmount] >= (*slep)[sfBalance]); (*sle)[sfBalance] = (*sle)[sfBalance] + (*slep)[sfAmount] - (*slep)[sfBalance]; @@ -197,6 +200,8 @@ PayChanCreate::preclaim(PreclaimContext const &ctx) { auto const account = ctx.tx[sfAccount]; auto const sle = ctx.view.read (keylet::account (account)); + if (! sle) + return terNO_ACCOUNT; // Check reserve and funds availability { @@ -237,6 +242,9 @@ PayChanCreate::doApply() { auto const account = ctx_.tx[sfAccount]; auto const sle = ctx_.view ().peek (keylet::account (account)); + if (!sle) + return tefINTERNAL; + auto const dst = ctx_.tx[sfDestination]; // Create PayChan in ledger @@ -345,6 +353,8 @@ PayChanFund::doApply() } auto const sle = ctx_.view ().peek (keylet::account (txAccount)); + if (! sle) + return tefINTERNAL; { // Check reserve and funds availability @@ -359,6 +369,13 @@ PayChanFund::doApply() return tecUNFUNDED; } + // do not allow adding funds if dst does not exist + if (AccountID const dst = (*slep)[sfDestination]; + !ctx_.view().read(keylet::account(dst))) + { + return tecNO_DST; + } + (*slep)[sfAmount] = (*slep)[sfAmount] + ctx_.tx[sfAmount]; ctx_.view ().update (slep); @@ -486,7 +503,7 @@ PayChanClaim::doApply() auto const sled = ctx_.view ().peek (keylet::account (dst)); if (!sled) - return terNO_ACCOUNT; + return tecNO_DST; // Obeying the lsfDisallowXRP flag was a bug. Piggyback on // featureDepositAuth to remove the bug. diff --git a/src/ripple/app/tx/impl/Payment.cpp b/src/ripple/app/tx/impl/Payment.cpp index 723d788af6..2dcfbeef4a 100644 --- a/src/ripple/app/tx/impl/Payment.cpp +++ b/src/ripple/app/tx/impl/Payment.cpp @@ -332,10 +332,15 @@ Payment::doApply () if (!sleDst) { + std::uint32_t const seqno { + view().rules().enabled(featureDeletableAccounts) ? + view().seq() : 1}; + // Create the account. sleDst = std::make_shared(k); sleDst->setAccountID(sfAccount, uDstAccountID); - sleDst->setFieldU32(sfSequence, 1); + sleDst->setFieldU32(sfSequence, seqno); + view().insert(sleDst); } else @@ -435,10 +440,13 @@ Payment::doApply () // Direct XRP payment. + auto const sleSrc = view().peek(keylet::account(account_)); + if (! sleSrc) + return tefINTERNAL; + // uOwnerCount is the number of entries in this ledger for this // account that require a reserve. - auto const uOwnerCount = view().read( - keylet::account(account_))->getFieldU32 (sfOwnerCount); + auto const uOwnerCount = sleSrc->getFieldU32 (sfOwnerCount); // This is the total reserve in drops. auto const reserve = view().fees().accountReserve(uOwnerCount); @@ -499,9 +507,7 @@ Payment::doApply () } // Do the arithmetic for the transfer and make the ledger change. - view() - .peek(keylet::account(account_)) - ->setFieldAmount(sfBalance, mSourceBalance - saDstAmount); + sleSrc->setFieldAmount(sfBalance, mSourceBalance - saDstAmount); sleDst->setFieldAmount( sfBalance, sleDst->getFieldAmount(sfBalance) + saDstAmount); diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index dfff835a50..0bf8060604 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -171,8 +171,9 @@ SetAccount::preclaim(PreclaimContext const& ctx) std::uint32_t const uTxFlags = ctx.tx.getFlags(); - auto const sle = ctx.view.read( - keylet::account(id)); + auto const sle = ctx.view.read(keylet::account(id)); + if (! sle) + return terNO_ACCOUNT; std::uint32_t const uFlagsIn = sle->getFieldU32(sfFlags); @@ -201,6 +202,8 @@ TER SetAccount::doApply () { auto const sle = view().peek(keylet::account(account_)); + if (! sle) + return tefINTERNAL; std::uint32_t const uFlagsIn = sle->getFieldU32 (sfFlags); std::uint32_t uFlagsOut = uFlagsIn; diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index ae46397b52..b6e3ad3142 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -82,6 +82,8 @@ SetRegularKey::doApply () { auto const sle = view ().peek ( keylet::account (account_)); + if (! sle) + return tefINTERNAL; if (!minimumFee (ctx_.app, ctx_.baseFee, view ().fees (), view ().flags ())) sle->setFlag (lsfPasswordSpent); diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index 79d3b020ee..190fd0c4f1 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -143,6 +143,87 @@ SetSignerList::preCompute() return Transactor::preCompute(); } +// The return type is signed so it is compatible with the 3rd argument +// of adjustOwnerCount() (which must be signed). +// +// NOTE: This way of computing the OwnerCount associated with a SignerList +// is valid until the featureMultiSignReserve amendment passes. Once it +// passes then just 1 OwnerCount is associated with a SignerList. +static int +signerCountBasedOwnerCountDelta (std::size_t entryCount) +{ + // We always compute the full change in OwnerCount, taking into account: + // o The fact that we're adding/removing a SignerList and + // o Accounting for the number of entries in the list. + // We can get away with that because lists are not adjusted incrementally; + // we add or remove an entire list. + // + // The rule is: + // o Simply having a SignerList costs 2 OwnerCount units. + // o And each signer in the list costs 1 more OwnerCount unit. + // So, at a minimum, adding a SignerList with 1 entry costs 3 OwnerCount + // units. A SignerList with 8 entries would cost 10 OwnerCount units. + // + // The static_cast should always be safe since entryCount should always + // be in the range from 1 to 8. We've got a lot of room to grow. + assert (entryCount >= STTx::minMultiSigners); + assert (entryCount <= STTx::maxMultiSigners); + return 2 + static_cast(entryCount); +} + +static TER +removeSignersFromLedger ( + Application& app, ApplyView& view, Keylet const& accountKeylet, + Keylet const& ownerDirKeylet, Keylet const& signerListKeylet) +{ + // We have to examine the current SignerList so we know how much to + // reduce the OwnerCount. + SLE::pointer signers = view.peek (signerListKeylet); + + // If the signer list doesn't exist we've already succeeded in deleting it. + if (!signers) + return tesSUCCESS; + + // There are two different ways that the OwnerCount could be managed. + // If the lsfOneOwnerCount bit is set then remove just one owner count. + // Otherwise use the pre-MultiSignReserve amendment calculation. + int removeFromOwnerCount = -1; + if ((signers->getFlags() & lsfOneOwnerCount) == 0) + { + STArray const& actualList = signers->getFieldArray (sfSignerEntries); + removeFromOwnerCount = + signerCountBasedOwnerCountDelta (actualList.size()) * -1; + } + + // Remove the node from the account directory. + auto const hint = (*signers)[sfOwnerNode]; + + if (! view.dirRemove( + ownerDirKeylet, hint, signerListKeylet.key, false)) + { + return tefBAD_LEDGER; + } + + adjustOwnerCount(view, + view.peek(accountKeylet), removeFromOwnerCount, app.journal("View")); + + view.erase (signers); + + return tesSUCCESS; +} + +TER +SetSignerList::removeFromLedger ( + Application& app, ApplyView& view, AccountID const& account) +{ + auto const accountKeylet = keylet::account (account); + auto const ownerDirKeylet = keylet::ownerDir (account); + auto const signerListKeylet = keylet::signers (account); + + return removeSignersFromLedger ( + app, view, accountKeylet, ownerDirKeylet, signerListKeylet); +} + NotTEC SetSignerList::validateQuorumAndSignerEntries ( std::uint32_t quorum, @@ -212,10 +293,12 @@ SetSignerList::replaceSignerList () // old signer list. May reduce the reserve, so this is done before // checking the reserve. if (TER const ter = removeSignersFromLedger ( - accountKeylet, ownerDirKeylet, signerListKeylet)) + ctx_.app, view(), accountKeylet, ownerDirKeylet, signerListKeylet)) return ter; auto const sle = view().peek(accountKeylet); + if (!sle) + return tefINTERNAL; // Compute new reserve. Verify the account has funds to meet the reserve. std::uint32_t const oldOwnerCount {(*sle)[sfOwnerCount]}; @@ -269,6 +352,9 @@ SetSignerList::destroySignerList () // Destroying the signer list is only allowed if either the master key // is enabled or there is a regular key. SLE::pointer ledgerEntry = view().peek (accountKeylet); + if (! ledgerEntry) + return tefINTERNAL; + if ((ledgerEntry->isFlag (lsfDisableMaster)) && (!ledgerEntry->isFieldPresent (sfRegularKey))) return tecNO_ALTERNATIVE_KEY; @@ -276,48 +362,7 @@ SetSignerList::destroySignerList () auto const ownerDirKeylet = keylet::ownerDir (account_); auto const signerListKeylet = keylet::signers (account_); return removeSignersFromLedger( - accountKeylet, ownerDirKeylet, signerListKeylet); -} - -TER -SetSignerList::removeSignersFromLedger (Keylet const& accountKeylet, - Keylet const& ownerDirKeylet, Keylet const& signerListKeylet) -{ - // We have to examine the current SignerList so we know how much to - // reduce the OwnerCount. - SLE::pointer signers = view().peek (signerListKeylet); - - // If the signer list doesn't exist we've already succeeded in deleting it. - if (!signers) - return tesSUCCESS; - - // There are two different ways that the OwnerCount could be managed. - // If the lsfOneOwnerCount bit is set then remove just one owner count. - // Otherwise use the pre-MultiSignReserve amendment calculation. - int removeFromOwnerCount = -1; - if ((signers->getFlags() & lsfOneOwnerCount) == 0) - { - STArray const& actualList = signers->getFieldArray (sfSignerEntries); - removeFromOwnerCount = - signerCountBasedOwnerCountDelta (actualList.size()) * -1; - } - - // Remove the node from the account directory. - auto const hint = (*signers)[sfOwnerNode]; - - if (! ctx_.view().dirRemove( - ownerDirKeylet, hint, signerListKeylet.key, false)) - { - return tefBAD_LEDGER; - } - - auto viewJ = ctx_.app.journal("View"); - adjustOwnerCount( - view(), view().peek(accountKeylet), removeFromOwnerCount, viewJ); - - ctx_.view().erase (signers); - - return tesSUCCESS; + ctx_.app, view(), accountKeylet, ownerDirKeylet, signerListKeylet); } void @@ -345,32 +390,4 @@ SetSignerList::writeSignersToSLE ( ledgerEntry->setFieldArray (sfSignerEntries, toLedger); } -// The return type is signed so it is compatible with the 3rd argument -// of adjustOwnerCount() (which must be signed). -// -// NOTE: This way of computing the OwnerCount associated with a SignerList -// is valid until the featureMultiSignReserve amendment passes. Once it -// passes then just 1 OwnerCount is associated with a SignerList. -int -SetSignerList::signerCountBasedOwnerCountDelta (std::size_t entryCount) -{ - // We always compute the full change in OwnerCount, taking into account: - // o The fact that we're adding/removing a SignerList and - // o Accounting for the number of entries in the list. - // We can get away with that because lists are not adjusted incrementally; - // we add or remove an entire list. - // - // The rule is: - // o Simply having a SignerList costs 2 OwnerCount units. - // o And each signer in the list costs 1 more OwnerCount unit. - // So, at a minimum, adding a SignerList with 1 entry costs 3 OwnerCount - // units. A SignerList with 8 entries would cost 10 OwnerCount units. - // - // The static_cast should always be safe since entryCount should always - // be in the range from 1 to 8. We've got a lot of room to grow. - assert (entryCount >= STTx::minMultiSigners); - assert (entryCount <= STTx::maxMultiSigners); - return 2 + static_cast(entryCount); -} - } // namespace ripple diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h index ae9247464e..1219d2bb25 100644 --- a/src/ripple/app/tx/impl/SetSignerList.h +++ b/src/ripple/app/tx/impl/SetSignerList.h @@ -67,6 +67,12 @@ public: TER doApply () override; void preCompute() override; + // Interface used by DeleteAccount + static + TER + removeFromLedger ( + Application& app, ApplyView& view, AccountID const& account); + private: static std::tuplegetFieldU32 (sfOwnerCount); diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index fd7df0cbd4..2c68badc0c 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -185,8 +185,10 @@ Transactor::checkFee (PreclaimContext const& ctx, return tesSUCCESS; auto const id = ctx.tx.getAccountID(sfAccount); - auto const sle = ctx.view.read( - keylet::account(id)); + auto const sle = ctx.view.read(keylet::account(id)); + if (! sle) + return terNO_ACCOUNT; + auto const balance = (*sle)[sfBalance].xrp(); if (balance < feePaid) @@ -211,8 +213,9 @@ TER Transactor::payFee () { auto const feePaid = calculateFeePaid(ctx_.tx); - auto const sle = view().peek( - keylet::account(account_)); + auto const sle = view().peek(keylet::account(account_)); + if (! sle) + return tefINTERNAL; // Deduct the fee, so it's not available during the transaction. // Will only write the account back if the transaction succeeds. @@ -276,8 +279,9 @@ Transactor::checkSeq (PreclaimContext const& ctx) void Transactor::setSeq () { - auto const sle = view().peek( - keylet::account(account_)); + auto const sle = view().peek(keylet::account(account_)); + if (! sle) + return; std::uint32_t const t_seq = ctx_.tx.getSequence (); @@ -354,6 +358,9 @@ Transactor::checkSingleSign (PreclaimContext const& ctx) auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); auto const idAccount = ctx.tx.getAccountID(sfAccount); auto const sleAccount = ctx.view.read(keylet::account(idAccount)); + if (! sleAccount) + return terNO_ACCOUNT; + bool const isMasterDisabled = sleAccount->isFlag(lsfDisableMaster); if (ctx.view.rules().enabled(fixMasterKeyAsRegularKey)) @@ -597,6 +604,10 @@ Transactor::reset(XRPAmount fee) auto const txnAcct = view().peek( keylet::account(ctx_.tx.getAccountID(sfAccount))); + if (! txnAcct) + // The account should never be missing from the ledger. But if it + // is missing then we can't very well charge it a fee, can we? + return beast::zero; auto const balance = txnAcct->getFieldAmount (sfBalance).xrp (); diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index e958169e83..e42557150c 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/src/ripple/app/tx/impl/applySteps.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -63,6 +64,7 @@ invoke_preflight (PreflightContext const& ctx) case ttTICKET_CANCEL: return CancelTicket ::preflight(ctx); case ttTICKET_CREATE: return CreateTicket ::preflight(ctx); case ttTRUST_SET: return SetTrust ::preflight(ctx); + case ttACCOUNT_DELETE: return DeleteAccount ::preflight(ctx); case ttAMENDMENT: case ttFEE: return Change ::preflight(ctx); default: @@ -132,6 +134,7 @@ invoke_preclaim (PreclaimContext const& ctx) case ttTICKET_CANCEL: return invoke_preclaim(ctx); case ttTICKET_CREATE: return invoke_preclaim(ctx); case ttTRUST_SET: return invoke_preclaim(ctx); + case ttACCOUNT_DELETE: return invoke_preclaim(ctx); case ttAMENDMENT: case ttFEE: return invoke_preclaim(ctx); default: @@ -167,6 +170,7 @@ invoke_calculateBaseFee( case ttTICKET_CANCEL: return CancelTicket::calculateBaseFee(view, tx); case ttTICKET_CREATE: return CreateTicket::calculateBaseFee(view, tx); case ttTRUST_SET: return SetTrust::calculateBaseFee(view, tx); + case ttACCOUNT_DELETE: return DeleteAccount::calculateBaseFee(view, tx); case ttAMENDMENT: case ttFEE: return Change::calculateBaseFee(view, tx); default: @@ -213,6 +217,7 @@ invoke_calculateConsequences(STTx const& tx) case ttTICKET_CANCEL: return invoke_calculateConsequences(tx); case ttTICKET_CREATE: return invoke_calculateConsequences(tx); case ttTRUST_SET: return invoke_calculateConsequences(tx); + case ttACCOUNT_DELETE: return invoke_calculateConsequences(tx); case ttAMENDMENT: case ttFEE: [[fallthrough]]; @@ -248,8 +253,9 @@ invoke_apply (ApplyContext& ctx) case ttTICKET_CANCEL: { CancelTicket p(ctx); return p(); } case ttTICKET_CREATE: { CreateTicket p(ctx); return p(); } case ttTRUST_SET: { SetTrust p(ctx); return p(); } + case ttACCOUNT_DELETE: { DeleteAccount p(ctx); return p(); } case ttAMENDMENT: - case ttFEE: { Change p(ctx); return p(); } + case ttFEE: { Change p(ctx); return p(); } default: assert(false); return { temUNKNOWN, false }; @@ -355,3 +361,4 @@ doApply(PreclaimResult const& preclaimResult, } } // ripple + diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index b238733389..1316071783 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -371,6 +371,18 @@ public: return dirRemove (directory, page, key.key, keepRoot); } /** @} */ + + /** Remove the specified directory, if it is empty. + + @param directory the identifier of the directory node to be deleted + @return \c true if the directory was found and was successfully deleted + \c false otherwise. + + @note The function should only be called with the root entry (i.e. with + the first page) of a directory. + */ + bool + emptyDirDelete(Keylet const& directory); }; } // ripple diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 7bc9e0fe35..9a084c75cd 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -56,24 +56,24 @@ enum FreezeHandling fhZERO_IF_FROZEN }; -bool +[[nodiscard]] bool isGlobalFrozen (ReadView const& view, AccountID const& issuer); -bool +[[nodiscard]] bool isFrozen (ReadView const& view, AccountID const& account, Currency const& currency, AccountID const& issuer); // Returns the amount an account can spend without going into debt. // // <-- saAmount: amount of currency held by account. May be negative. -STAmount +[[nodiscard]] STAmount accountHolds (ReadView const& view, AccountID const& account, Currency const& currency, AccountID const& issuer, FreezeHandling zeroIfFrozen, beast::Journal j); -STAmount +[[nodiscard]] STAmount accountFunds (ReadView const& view, AccountID const& id, STAmount const& saDefault, FreezeHandling freezeHandling, beast::Journal j); @@ -84,7 +84,7 @@ accountFunds (ReadView const& view, AccountID const& id, // necessary. // // @param ownerCountAdj positive to add to count, negative to reduce count. -XRPAmount +[[nodiscard]] XRPAmount xrpLiquid (ReadView const& view, AccountID const& id, std::int32_t ownerCountAdj, beast::Journal j); @@ -105,14 +105,14 @@ forEachItemAfter (ReadView const& view, AccountID const& id, unsigned int limit, std::function< bool (std::shared_ptr const&)> f); -Rate +[[nodiscard]] Rate transferRate (ReadView const& view, AccountID const& issuer); /** Returns `true` if the directory is empty @param key The key of the directory */ -bool +[[nodiscard]] bool dirIsEmpty (ReadView const& view, Keylet const& k); @@ -139,12 +139,12 @@ cdirNext (ReadView const& view, beast::Journal j); // Return the list of enabled amendments -std::set +[[nodiscard]] std::set getEnabledAmendments (ReadView const& view); // Return a map of amendments that have achieved majority using majorityAmendments_t = std::map ; -majorityAmendments_t +[[nodiscard]] majorityAmendments_t getMajorityAmendments (ReadView const& view); /** Return the hash of a ledger by sequence. @@ -156,7 +156,7 @@ getMajorityAmendments (ReadView const& view); @return The hash of the ledger with the given sequence number or boost::none. */ -boost::optional +[[nodiscard]] boost::optional hashOfSeq (ReadView const& ledger, LedgerIndex seq, beast::Journal journal); @@ -184,10 +184,12 @@ getCandidateLedger (LedgerIndex requested) both be valid. Use the first form if you have both ledgers, use the second form if you have not acquired the valid ledger yet */ -bool areCompatible (ReadView const& validLedger, ReadView const& testLedger, +[[nodiscard]] bool +areCompatible (ReadView const& validLedger, ReadView const& testLedger, beast::Journal::Stream& s, const char* reason); -bool areCompatible (uint256 const& validHash, LedgerIndex validIndex, +[[nodiscard]] bool +areCompatible (uint256 const& validHash, LedgerIndex validIndex, ReadView const& testLedger, beast::Journal::Stream& s, const char* reason); //------------------------------------------------------------------------------ @@ -224,7 +226,7 @@ dirNext (ApplyView& view, uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. beast::Journal j); -std::function +[[nodiscard]] std::function describeOwnerDir(AccountID const& account); // deprecated @@ -243,7 +245,7 @@ dirAdd (ApplyView& view, This can set an initial balance. */ -TER +[[nodiscard]] TER trustCreate (ApplyView& view, const bool bSrcHigh, AccountID const& uSrcAccountID, @@ -261,7 +263,7 @@ trustCreate (ApplyView& view, std::uint32_t uSrcQualityOut, beast::Journal j); -TER +[[nodiscard]] TER trustDelete (ApplyView& view, std::shared_ptr const& sleRippleState, AccountID const& uLowAccountID, @@ -274,6 +276,7 @@ trustDelete (ApplyView& view, The passed `sle` be obtained from a prior call to view.peek() */ +// [[nodiscard]] // nodiscard commented out so Flow, BookTip and others compile. TER offerDelete (ApplyView& view, std::shared_ptr const& sle, @@ -289,12 +292,14 @@ offerDelete (ApplyView& view, // - Redeeming IOUs and/or sending sender's own IOUs. // - Create trust line of needed. // --> bCheckIssuer : normally require issuer to be involved. +// [[nodiscard]] // nodiscard commented out so DirectStep.cpp compiles. TER rippleCredit (ApplyView& view, AccountID const& uSenderID, AccountID const& uReceiverID, const STAmount & saAmount, bool bCheckIssuer, beast::Journal j); +// [[nodiscard]] // nodiscard commented out so DeliverNodeForward.cpp compiles. TER accountSend (ApplyView& view, AccountID const& from, @@ -302,41 +307,41 @@ accountSend (ApplyView& view, const STAmount & saAmount, beast::Journal j); -TER +[[nodiscard]] TER issueIOU (ApplyView& view, AccountID const& account, STAmount const& amount, Issue const& issue, beast::Journal j); -TER +[[nodiscard]] TER redeemIOU (ApplyView& view, AccountID const& account, STAmount const& amount, Issue const& issue, beast::Journal j); -TER +[[nodiscard]] TER transferXRP (ApplyView& view, AccountID const& from, AccountID const& to, STAmount const& amount, beast::Journal j); -NetClock::time_point const& fix1141Time (); -bool fix1141 (NetClock::time_point const closeTime); +[[nodiscard]] NetClock::time_point const& fix1141Time (); +[[nodiscard]] bool fix1141 (NetClock::time_point const closeTime); -NetClock::time_point const& fix1274Time (); -bool fix1274 (NetClock::time_point const closeTime); +[[nodiscard]] NetClock::time_point const& fix1274Time (); +[[nodiscard]] bool fix1274 (NetClock::time_point const closeTime); -NetClock::time_point const& fix1298Time (); -bool fix1298 (NetClock::time_point const closeTime); +[[nodiscard]] NetClock::time_point const& fix1298Time (); +[[nodiscard]] bool fix1298 (NetClock::time_point const closeTime); -NetClock::time_point const& fix1443Time (); -bool fix1443 (NetClock::time_point const closeTime); +[[nodiscard]] NetClock::time_point const& fix1443Time (); +[[nodiscard]] bool fix1443 (NetClock::time_point const closeTime); -NetClock::time_point const& fix1449Time (); -bool fix1449 (NetClock::time_point const closeTime); +[[nodiscard]] NetClock::time_point const& fix1449Time (); +[[nodiscard]] bool fix1449 (NetClock::time_point const closeTime); } // ripple diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 2f2c568bf7..8e1afe577d 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -562,10 +562,10 @@ ApplyStateTable::getForMod (ReadView const& base, auto const& item = iter->second; if (item.first == Action::erase) { - // VFALCO We need to think about throwing - // an exception or calling LogicError - JLOG(j.fatal()) << - "Trying to thread to deleted node"; + // The Destination of an Escrow or a PayChannel may have been + // deleted. In that case the account we're threading to will + // not be found and it is appropriate to return a nullptr. + JLOG(j.warn()) << "Trying to thread to deleted node"; return nullptr; } if (item.first != Action::cache) @@ -578,10 +578,10 @@ ApplyStateTable::getForMod (ReadView const& base, auto c = base.read (keylet::unchecked (key)); if (! c) { - // VFALCO We need to think about throwing - // an exception or calling LogicError - JLOG(j.fatal()) << - "ApplyStateTable::getForMod: key not found"; + // The Destination of an Escrow or a PayChannel may have been + // deleted. In that case the account we're threading to will + // not be found and it is appropriate to return a nullptr. + JLOG(j.warn()) << "ApplyStateTable::getForMod: key not found"; return nullptr; } auto sle = std::make_shared (*c); @@ -596,14 +596,12 @@ ApplyStateTable::threadTx (ReadView const& base, { auto const sle = getForMod(base, keylet::account(to).key, mods, j); - assert(sle); if (! sle) { - // VFALCO We need to think about throwing - // an exception or calling LogicError - JLOG(j.fatal()) << - "Threading to non-existent account: " << - toBase58(to); + // The Destination of an Escrow or PayChannel may have been deleted. + // In that case the account we are threading to will not be found. + // So this logging is just a warning. + JLOG(j.warn()) << "Threading to non-existent account: " << toBase58(to); return; } threadItem (meta, sle); diff --git a/src/ripple/ledger/impl/ApplyView.cpp b/src/ripple/ledger/impl/ApplyView.cpp index 7243897e8a..467add5e46 100644 --- a/src/ripple/ledger/impl/ApplyView.cpp +++ b/src/ripple/ledger/impl/ApplyView.cpp @@ -121,6 +121,70 @@ ApplyView::dirAdd ( return page; } +bool +ApplyView::emptyDirDelete(Keylet const& directory) +{ + auto node = peek(directory); + + if (!node) + return false; + + // Verify that the passed directory node is the directory root. + if (directory.type != ltDIR_NODE || + node->getFieldH256(sfRootIndex) != directory.key) + { + assert (!"emptyDirDelete() called with wrong node type"); + return false; + } + + // The directory still contains entries and so it cannot be removed + if (!node->getFieldV256(sfIndexes).empty()) + return false; + + std::uint64_t constexpr rootPage = 0; + auto prevPage = node->getFieldU64(sfIndexPrevious); + auto nextPage = node->getFieldU64(sfIndexNext); + + if (nextPage == rootPage && prevPage != rootPage) + LogicError ("Directory chain: fwd link broken"); + + if (prevPage == rootPage && nextPage != rootPage) + LogicError ("Directory chain: rev link broken"); + + // Older versions of the code would, in some cases, allow the last + // page to be empty. Remove such pages: + if (nextPage == prevPage && nextPage != rootPage) + { + auto last = peek(keylet::page(directory, nextPage)); + + if (!last) + LogicError ("Directory chain: fwd link broken."); + + if (!last->getFieldV256 (sfIndexes).empty()) + return false; + + // Update the first page's linked list and + // mark it as updated. + node->setFieldU64 (sfIndexNext, rootPage); + node->setFieldU64 (sfIndexPrevious, rootPage); + update(node); + + // And erase the empty last page: + erase(last); + + // Make sure our local values reflect the + // updated information: + nextPage = rootPage; + prevPage = rootPage; + } + + // If there are no other pages, erase the root: + if (nextPage == rootPage && prevPage == rootPage) + erase(node); + + return true; +} + bool ApplyView::dirRemove ( Keylet const& directory, diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index ff1a297b2f..e454c33305 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -122,16 +122,12 @@ void addRaw (LedgerInfo const& info, Serializer& s) } bool -isGlobalFrozen (ReadView const& view, - AccountID const& issuer) +isGlobalFrozen (ReadView const& view, AccountID const& issuer) { - // VFALCO Perhaps this should assert if (isXRP (issuer)) return false; - auto const sle = - view.read(keylet::account(issuer)); - if (sle && sle->isFlag (lsfGlobalFreeze)) - return true; + if (auto const sle = view.read(keylet::account(issuer))) + return sle->isFlag (lsfGlobalFreeze); return false; } @@ -548,7 +544,9 @@ dirIsEmpty (ReadView const& view, return true; if (! sleNode->getFieldV256 (sfIndexes).empty ()) return false; - // If there's another page, it must be non-empty + // The first page of a directory may legitimately be empty even if there + // are other pages (the first page is the anchor page) so check to see if + // there is another page. If there is, the directory isn't empty. return sleNode->getFieldU64 (sfIndexNext) == 0; } @@ -730,6 +728,8 @@ adjustOwnerCount (ApplyView& view, std::shared_ptr const& sle, std::int32_t amount, beast::Journal j) { + if (! sle) + return; assert(amount != 0); std::uint32_t const current {sle->getFieldU32 (sfOwnerCount)}; AccountID const id = (*sle)[sfAccount]; @@ -955,11 +955,16 @@ trustCreate (ApplyView& view, const bool bSetDst = saLimit.getIssuer () == uDstAccountID; const bool bSetHigh = bSrcHigh ^ bSetDst; + assert (sleAccount); + if (! sleAccount) + return tefINTERNAL; + assert (sleAccount->getAccountID (sfAccount) == (bSetHigh ? uHighAccountID : uLowAccountID)); - auto slePeer = view.peek (keylet::account( + auto const slePeer = view.peek (keylet::account( bSetHigh ? uLowAccountID : uHighAccountID)); - assert (slePeer); + if (! slePeer) + return tecNO_TARGET; // Remember deletion hints. sleRippleState->setFieldU64 (sfLowNode, *lowNode); @@ -1103,8 +1108,8 @@ rippleCredit (ApplyView& view, STAmount const& saAmount, bool bCheckIssuer, beast::Journal j) { - auto issuer = saAmount.getIssuer (); - auto currency = saAmount.getCurrency (); + AccountID const& issuer = saAmount.getIssuer (); + Currency const& currency = saAmount.getCurrency (); // Make sure issuer is involved. assert ( @@ -1114,10 +1119,10 @@ rippleCredit (ApplyView& view, // Disallow sending to self. assert (uSenderID != uReceiverID); - bool bSenderHigh = uSenderID > uReceiverID; - uint256 uIndex = getRippleStateIndex ( - uSenderID, uReceiverID, saAmount.getCurrency ()); - auto sleRippleState = view.peek (keylet::line(uIndex)); + bool const bSenderHigh = uSenderID > uReceiverID; + uint256 const uIndex = getRippleStateIndex ( + uSenderID, uReceiverID, currency); + auto const sleRippleState = view.peek (keylet::line(uIndex)); TER terResult; @@ -1126,8 +1131,8 @@ rippleCredit (ApplyView& view, if (!sleRippleState) { - STAmount saReceiverLimit({currency, uReceiverID}); - STAmount saBalance = saAmount; + STAmount const saReceiverLimit ({currency, uReceiverID}); + STAmount saBalance {saAmount}; saBalance.setIssuer (noAccount()); @@ -1138,8 +1143,10 @@ rippleCredit (ApplyView& view, auto const sleAccount = view.peek(keylet::account(uReceiverID)); + if (! sleAccount) + return tefINTERNAL; - bool noRipple = (sleAccount->getFlags() & lsfDefaultRipple) == 0; + bool const noRipple = (sleAccount->getFlags() & lsfDefaultRipple) == 0; terResult = trustCreate (view, bSenderHigh, @@ -1158,17 +1165,16 @@ rippleCredit (ApplyView& view, } else { - STAmount saBalance = sleRippleState->getFieldAmount (sfBalance); + STAmount saBalance = sleRippleState->getFieldAmount (sfBalance); if (bSenderHigh) saBalance.negate (); // Put balance in sender terms. - view.creditHook (uSenderID, - uReceiverID, saAmount, saBalance); + view.creditHook (uSenderID, uReceiverID, saAmount, saBalance); - STAmount saBefore = saBalance; + STAmount const saBefore = saBalance; - saBalance -= saAmount; + saBalance -= saAmount; JLOG (j.trace()) << "rippleCredit: " << to_string (uSenderID) << @@ -1285,7 +1291,9 @@ rippleSend (ApplyView& view, if (uSenderID == issuer || uReceiverID == issuer || issuer == noAccount()) { // Direct send: redeeming IOUs and/or sending own IOUs. - rippleCredit (view, uSenderID, uReceiverID, saAmount, false, j); + auto const ter = rippleCredit (view, uSenderID, uReceiverID, saAmount, false, j); + if (view.rules().enabled(featureDeletableAccounts) && ter != tesSUCCESS) + return ter; saActual = saAmount; return tesSUCCESS; } @@ -1450,10 +1458,13 @@ updateTrustLine ( STAmount const& after, beast::Journal j) { + if (!state) + return false; std::uint32_t const flags (state->getFieldU32 (sfFlags)); auto sle = view.peek (keylet::account(sender)); - assert (sle); + if (! sle) + return false; // YYY Could skip this if rippling in reverse. if (before > beast::zero @@ -1488,7 +1499,6 @@ updateTrustLine ( && !(flags & (bSenderHigh ? lsfLowReserve : lsfHighReserve))) return true; } - return false; } @@ -1519,12 +1529,14 @@ issueIOU (ApplyView& view, // NIKB TODO: The limit uses the receiver's account as the issuer and // this is unnecessarily inefficient as copying which could be avoided // is now required. Consider available options. - STAmount limit({issue.currency, account}); + STAmount const limit({issue.currency, account}); STAmount final_balance = amount; final_balance.setIssuer (noAccount()); - auto receiverAccount = view.peek (keylet::account(account)); + auto const receiverAccount = view.peek (keylet::account(account)); + if (! receiverAccount) + return tefINTERNAL; bool noRipple = (receiverAccount->getFlags() & lsfDefaultRipple) == 0; @@ -1645,8 +1657,10 @@ transferXRP (ApplyView& view, assert (from != to); assert (amount.native ()); - SLE::pointer sender = view.peek (keylet::account(from)); - SLE::pointer receiver = view.peek (keylet::account(to)); + SLE::pointer const sender = view.peek (keylet::account(from)); + SLE::pointer const receiver = view.peek (keylet::account(to)); + if (!sender || !receiver) + return tefINTERNAL; JLOG (j.trace()) << "transferXRP: " << to_string (from) << " -> " << to_string (to) << diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index c61bd6efa5..5b3aa84559 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -86,6 +86,7 @@ class FeatureCollections "fixMasterKeyAsRegularKey", "fixCheckThreading", "fixPayChanRecipientOwnerDir", + "DeletableAccounts", }; std::vector features; @@ -373,6 +374,7 @@ extern uint256 const fixTakerDryOfferRemoval; extern uint256 const fixMasterKeyAsRegularKey; extern uint256 const fixCheckThreading; extern uint256 const fixPayChanRecipientOwnerDir; +extern uint256 const featureDeletableAccounts; } // ripple diff --git a/src/ripple/protocol/Indexes.h b/src/ripple/protocol/Indexes.h index c79db43e0e..959ebf02d8 100644 --- a/src/ripple/protocol/Indexes.h +++ b/src/ripple/protocol/Indexes.h @@ -50,9 +50,6 @@ getLedgerAmendmentIndex (); uint256 getLedgerFeeIndex (); -uint256 -getAccountRootIndex (AccountID const& account); - uint256 getGeneratorIndex (AccountID const& uGeneratorID); @@ -203,7 +200,7 @@ struct quality_t }; static quality_t const quality {}; -/** The directry for the next lower quality */ +/** The directory for the next lower quality */ struct next_t { explicit next_t() = default; diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 4b9b902e69..70560b3b6e 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -152,6 +152,7 @@ enum TEFcodes : TERUnderlyingType tefNOT_MULTI_SIGNING, tefBAD_AUTH_MASTER, tefINVARIANT_FAILED, + tefTOO_BIG, }; //------------------------------------------------------------------------------ @@ -266,6 +267,8 @@ enum TECcodes : TERUnderlyingType tecEXPIRED = 148, tecDUPLICATE = 149, tecKILLED = 150, + tecHAS_OBLIGATIONS = 151, + tecTOO_SOON = 152, }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/TxFormats.h b/src/ripple/protocol/TxFormats.h index 2fd3ca2fd5..88e03a17b6 100644 --- a/src/ripple/protocol/TxFormats.h +++ b/src/ripple/protocol/TxFormats.h @@ -32,32 +32,33 @@ namespace ripple { */ enum TxType { - ttINVALID = -1, + ttINVALID = -1, - ttPAYMENT = 0, - ttESCROW_CREATE = 1, - ttESCROW_FINISH = 2, - ttACCOUNT_SET = 3, - ttESCROW_CANCEL = 4, - ttREGULAR_KEY_SET = 5, - ttNICKNAME_SET = 6, // open - ttOFFER_CREATE = 7, - ttOFFER_CANCEL = 8, - no_longer_used = 9, - ttTICKET_CREATE = 10, - ttTICKET_CANCEL = 11, - ttSIGNER_LIST_SET = 12, - ttPAYCHAN_CREATE = 13, - ttPAYCHAN_FUND = 14, - ttPAYCHAN_CLAIM = 15, - ttCHECK_CREATE = 16, - ttCHECK_CASH = 17, - ttCHECK_CANCEL = 18, - ttDEPOSIT_PREAUTH = 19, - ttTRUST_SET = 20, + ttPAYMENT = 0, + ttESCROW_CREATE = 1, + ttESCROW_FINISH = 2, + ttACCOUNT_SET = 3, + ttESCROW_CANCEL = 4, + ttREGULAR_KEY_SET = 5, + ttNICKNAME_SET = 6, // open + ttOFFER_CREATE = 7, + ttOFFER_CANCEL = 8, + no_longer_used = 9, + ttTICKET_CREATE = 10, + ttTICKET_CANCEL = 11, + ttSIGNER_LIST_SET = 12, + ttPAYCHAN_CREATE = 13, + ttPAYCHAN_FUND = 14, + ttPAYCHAN_CLAIM = 15, + ttCHECK_CREATE = 16, + ttCHECK_CASH = 17, + ttCHECK_CANCEL = 18, + ttDEPOSIT_PREAUTH = 19, + ttTRUST_SET = 20, + ttACCOUNT_DELETE = 21, - ttAMENDMENT = 100, - ttFEE = 101, + ttAMENDMENT = 100, + ttFEE = 101, }; /** Manages the list of known transaction formats. diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index cd366abee4..90ef99a7ef 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -119,6 +119,7 @@ detail::supportedAmendments () "fixMasterKeyAsRegularKey", "fixCheckThreading", "fixPayChanRecipientOwnerDir", + "DeletableAccounts", }; return supported; } @@ -177,5 +178,6 @@ uint256 const fixTakerDryOfferRemoval = *getRegisteredFeature("fixTakerDryOfferR uint256 const fixMasterKeyAsRegularKey = *getRegisteredFeature("fixMasterKeyAsRegularKey"); uint256 const fixCheckThreading = *getRegisteredFeature("fixCheckThreading"); uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir"); +uint256 const featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts"); } // ripple diff --git a/src/ripple/protocol/impl/Indexes.cpp b/src/ripple/protocol/impl/Indexes.cpp index 2afd44e93f..542f65ac7c 100644 --- a/src/ripple/protocol/impl/Indexes.cpp +++ b/src/ripple/protocol/impl/Indexes.cpp @@ -55,14 +55,6 @@ getLedgerFeeIndex () return sha512Half(std::uint16_t(spaceFee)); } -uint256 -getAccountRootIndex (AccountID const& account) -{ - return sha512Half( - std::uint16_t(spaceAccount), - account); -} - uint256 getGeneratorIndex (AccountID const& uGeneratorID) { @@ -215,8 +207,7 @@ namespace keylet { Keylet account_t::operator()( AccountID const& id) const { - return { ltACCOUNT_ROOT, - getAccountRootIndex(id) }; + return { ltACCOUNT_ROOT, sha512Half(std::uint16_t(spaceAccount), id) }; } Keylet child (uint256 const& key) diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index b79e757264..526e58bbae 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -74,6 +74,8 @@ transResults() { tecEXPIRED, { "tecEXPIRED", "Expiration time is passed." } }, { tecDUPLICATE, { "tecDUPLICATE", "Ledger object already exists." } }, { tecKILLED, { "tecKILLED", "FillOrKill offer killed." } }, + { tecHAS_OBLIGATIONS, { "tecHAS_OBLIGATIONS", "The account cannot be deleted since it has obligations." } }, + { tecTOO_SOON, { "tecTOO_SOON", "It is too early to attempt the requested operation. Please wait." } }, { tefALREADY, { "tefALREADY", "The exact transaction was already in this ledger." } }, { tefBAD_ADD_AUTH, { "tefBAD_ADD_AUTH", "Not authorized to add account." } }, @@ -93,6 +95,7 @@ transResults() { tefWRONG_PRIOR, { "tefWRONG_PRIOR", "This previous transaction does not match." } }, { tefBAD_AUTH_MASTER, { "tefBAD_AUTH_MASTER", "Auth for unclaimed account needs correct master key." } }, { tefINVARIANT_FAILED, { "tefINVARIANT_FAILED", "Fee claim violated invariants for the transaction." } }, + { tefTOO_BIG, { "tefTOO_BIG", "Transaction affects too many items." } }, { telLOCAL_ERROR, { "telLOCAL_ERROR", "Local failure." } }, { telBAD_DOMAIN, { "telBAD_DOMAIN", "Domain too long." } }, diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index 0784b3e151..904e501349 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -217,6 +217,13 @@ TxFormats::TxFormats () }, commonFields); + add (jss::AccountDelete, ttACCOUNT_DELETE, + { + { sfDestination, soeREQUIRED }, + { sfDestinationTag, soeOPTIONAL }, + }, + commonFields); + add (jss::DepositPreauth, ttDEPOSIT_PREAUTH, { { sfAuthorize, soeOPTIONAL }, diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index aba9711037..a613702705 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -43,6 +43,7 @@ namespace jss { JSS ( AL_hit_rate ); // out: GetCounts JSS ( Account ); // in: TransactionSign; field. +JSS ( AccountDelete ); // transaction type. JSS ( AccountRoot ); // ledger type. JSS ( AccountSet ); // transaction type. JSS ( Amendments ); // ledger type. diff --git a/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp b/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp index d6f60e665d..a9531c5f12 100644 --- a/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp +++ b/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp @@ -53,7 +53,7 @@ Json::Value doAccountCurrencies (RPC::Context& context) if (auto jvAccepted = RPC::accountFromString (accountID, strIdent, bStrict)) return jvAccepted; - if (! ledger->read(keylet::account(accountID))) + if (! ledger->exists(keylet::account(accountID))) return rpcError (rpcACT_NOT_FOUND); std::set send, receive; diff --git a/src/ripple/rpc/impl/DeliveredAmount.cpp b/src/ripple/rpc/impl/DeliveredAmount.cpp index 8be5ce3c36..f6261e0db1 100644 --- a/src/ripple/rpc/impl/DeliveredAmount.cpp +++ b/src/ripple/rpc/impl/DeliveredAmount.cpp @@ -57,7 +57,8 @@ insertDeliveredAmount( { TxType const tt{serializedTx->getTxnType()}; if (tt != ttPAYMENT && - tt != ttCHECK_CASH) + tt != ttCHECK_CASH && + tt != ttACCOUNT_DELETE) return; if (tt == ttCHECK_CASH && diff --git a/src/ripple/unity/app_tx.cpp b/src/ripple/unity/app_tx.cpp index 74cbd4ed4c..dab57960ef 100644 --- a/src/ripple/unity/app_tx.cpp +++ b/src/ripple/unity/app_tx.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp new file mode 100644 index 0000000000..2885807e87 --- /dev/null +++ b/src/test/app/AccountDelete_test.cpp @@ -0,0 +1,789 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2019 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include + +namespace ripple { +namespace test { + +class AccountDelete_test : public beast::unit_test::suite +{ +private: + std::uint32_t openLedgerSeq (jtx::Env& env) + { + return env.current()->seq(); + } + + // Helper function that verifies the expected DeliveredAmount is present. + // + // NOTE: the function _infers_ the transaction to operate on by calling + // env.tx(), which returns the result from the most recent transaction. + void verifyDeliveredAmount (jtx::Env& env, STAmount const& amount) + { + // Get the hash for the most recent transaction. + std::string const txHash { + env.tx()->getJson (JsonOptions::none)[jss::hash].asString()}; + + // Verify DeliveredAmount and delivered_amount metadata are correct. + // We can't use env.meta() here, because meta() doesn't include + // delivered_amount. + env.close(); + Json::Value const meta = env.rpc ("tx", txHash)[jss::result][jss::meta]; + + // Expect there to be a DeliveredAmount field. + if (! BEAST_EXPECT(meta.isMember (sfDeliveredAmount.jsonName))) + return; + + // DeliveredAmount and delivered_amount should both be present and + // equal amount. + Json::Value const jsonExpect {amount.getJson (JsonOptions::none)}; + BEAST_EXPECT (meta[sfDeliveredAmount.jsonName] == jsonExpect); + BEAST_EXPECT (meta[jss::delivered_amount] == jsonExpect); + } + + // Helper function to create a payment channel. + static Json::Value payChanCreate ( + jtx::Account const& account, + jtx::Account const& to, + STAmount const& amount, + NetClock::duration const& settleDelay, + NetClock::time_point const& cancelAfter, + PublicKey const& pk) + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelCreate; + jv[jss::Account] = account.human(); + jv[jss::Destination] = to.human(); + jv[jss::Amount] = amount.getJson (JsonOptions::none); + jv[sfSettleDelay.jsonName] = settleDelay.count(); + jv[sfCancelAfter.jsonName] = + cancelAfter.time_since_epoch().count() + 2; + jv[sfPublicKey.jsonName] = strHex (pk.slice()); + return jv; + }; + + // Close the ledger until the ledger sequence is large enough to close + // the account. If margin is specified, close the ledger so `margin` + // more closes are needed + void + incLgrSeqForAccDel( + jtx::Env& env, + jtx::Account const& acc, + std::uint32_t margin = 0) + { + int const delta = [&]()->int{ + if (env.seq(acc) + 255 > openLedgerSeq(env)) + return env.seq(acc) - openLedgerSeq(env) + 255 - margin; + return 0; + }(); + BEAST_EXPECT(margin==0 || delta >= 0); + for (int i = 0; i < delta; ++i) + env.close(); + BEAST_EXPECT(openLedgerSeq(env) == env.seq(acc) + 255 - margin); + } + +public: + void testBasics() + { + using namespace jtx; + + testcase("Basics"); + + Env env {*this}; + Account const alice("alice"); + Account const becky("becky"); + Account const carol("carol"); + Account const gw("gw"); + + env.fund(XRP(10000), alice, becky, carol, gw); + env.close(); + + // Alice can't delete her account and then give herself the XRP. + env (acctdelete (alice, alice), ter (temDST_IS_SRC)); + + // Invalid flags. + env (acctdelete (alice, becky), + txflags (tfImmediateOrCancel), ter (temINVALID_FLAG)); + + // Account deletion has a high fee. Make sure the fee requirement + // behaves as we expect. + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (alice, becky), ter (telINSUF_FEE_P)); + + // Try a fee one drop less than the required amount. + env (acctdelete (alice, becky), + fee (acctDelFee - drops(1)), ter (telINSUF_FEE_P)); + + // alice's account is created too recently to be deleted. + env (acctdelete (alice, becky), fee (acctDelFee), ter (tecTOO_SOON)); + + // Give becky a trustline. She is no longer deletable. + env (trust (becky, gw["USD"](1000))); + env.close(); + + // Give carol a deposit preauthorization, an offer, and a signer list. + // Even with all that she's still deletable. + env (deposit::auth (carol, becky)); + std::uint32_t const carolOfferSeq {env.seq (carol)}; + env (offer (carol, gw["USD"](51), XRP (51))); + env (signers (carol, 1, {{alice, 1}, {becky, 1}})); + + // Deleting should fail with TOO_SOON, which is a relatively + // cheap check compared to validating the contents of her directory. + env (acctdelete (alice, becky), fee (acctDelFee), ter (tecTOO_SOON)); + + // Close enough ledgers to almost be able to delete alice's account. + incLgrSeqForAccDel(env, alice, 1); + + // alice's account is still created too recently to be deleted. + env (acctdelete (alice, becky), fee (acctDelFee), ter (tecTOO_SOON)); + + // The most recent delete attempt advanced alice's sequence. So + // close two ledgers and her account should be deletable. + env.close(); + env.close(); + + { + auto const aliceOldBalance {env.balance (alice)}; + auto const beckyOldBalance {env.balance (becky)}; + + // Verify that alice's account exists but she has no directory. + BEAST_EXPECT (env.closed()->exists(keylet::account (alice.id()))); + BEAST_EXPECT (! env.closed()->exists(keylet::ownerDir(alice.id()))); + + env (acctdelete (alice, becky), fee (acctDelFee)); + verifyDeliveredAmount (env, aliceOldBalance - acctDelFee); + env.close(); + + // Verify that alice's account and directory are actually gone. + BEAST_EXPECT (! env.closed()->exists(keylet::account (alice.id()))); + BEAST_EXPECT (! env.closed()->exists(keylet::ownerDir(alice.id()))); + + // Verify that alice's XRP, minus the fee, was transferred to becky. + BEAST_EXPECT (env.balance (becky) == + aliceOldBalance + beckyOldBalance - acctDelFee); + } + + // Attempt to delete becky's account but get stopped by the trust line. + env (acctdelete (becky, carol), + fee (acctDelFee), ter (tecHAS_OBLIGATIONS)); + env.close(); + + // Verify that becky's account is still there. + env (noop (becky)); + + { + auto const beckyOldBalance {env.balance (becky)}; + auto const carolOldBalance {env.balance (carol)}; + + // Verify that Carol's account, directory, deposit + // preauthorization, offer, and signer list exist. + BEAST_EXPECT (env.closed()->exists (keylet::account (carol.id()))); + BEAST_EXPECT (env.closed()->exists (keylet::ownerDir(carol.id()))); + BEAST_EXPECT (env.closed()->exists (keylet::depositPreauth( + carol.id(), becky.id()))); + BEAST_EXPECT (env.closed()->exists (keylet::offer( + carol.id(), carolOfferSeq))); + BEAST_EXPECT (env.closed()->exists (keylet::signers (carol.id()))); + + // Delete carol's account even with stuff in her directory. Show + // that multisigning for the delete does not increase carol's fee. + env (acctdelete (carol, becky), fee (acctDelFee), msig (alice)); + verifyDeliveredAmount (env, carolOldBalance - acctDelFee); + env.close(); + + // Verify that Carol's account, directory, and other stuff are gone. + BEAST_EXPECT (! env.closed()->exists(keylet::account (carol.id()))); + BEAST_EXPECT (! env.closed()->exists(keylet::ownerDir(carol.id()))); + BEAST_EXPECT (! env.closed()->exists(keylet::depositPreauth ( + carol.id(), becky.id()))); + BEAST_EXPECT (! env.closed()->exists(keylet::offer ( + carol.id(), carolOfferSeq))); + BEAST_EXPECT (! env.closed()->exists(keylet::signers (carol.id()))); + + // Verify that Carol's XRP, minus the fee, was transferred to becky. + BEAST_EXPECT (env.balance (becky) == + carolOldBalance + beckyOldBalance - acctDelFee); + } + } + + void testDirectories() + { + // The code that deletes consecutive directory entries uses a + // peculiarity of the implementation. Make sure that peculiarity + // behaves as expected across owner directory pages. + using namespace jtx; + + testcase("Directories"); + + Env env {*this}; + Account const alice("alice"); + Account const gw("gw"); + + env.fund(XRP(10000), alice, gw); + env.close(); + + // Alice creates enough offers to require two owner directories. + for (int i {0}; i < 45; ++i) + { + env (offer (alice, gw["USD"](1), XRP (1))); + env.close(); + } + env.require (offers (alice, 45)); + + // Close enough ledgers to be able to delete alice's account. + incLgrSeqForAccDel(env, alice); + + // Verify that both directory nodes exist. + Keylet const aliceRootKey {keylet::ownerDir (alice.id())}; + Keylet const alicePageKey {keylet::page (aliceRootKey, 1)}; + BEAST_EXPECT (env.closed()->exists (aliceRootKey)); + BEAST_EXPECT (env.closed()->exists (alicePageKey)); + + // Delete alice's account. + auto const acctDelFee {drops (env.current()->fees().increment)}; + auto const aliceBalance {env.balance (alice)}; + env (acctdelete (alice, gw), fee (acctDelFee)); + verifyDeliveredAmount (env, aliceBalance - acctDelFee); + env.close(); + + // Both of alice's directory nodes should be gone. + BEAST_EXPECT (! env.closed()->exists (aliceRootKey)); + BEAST_EXPECT (! env.closed()->exists (alicePageKey)); + } + + void testOwnedTypes() + { + using namespace jtx; + + testcase("Owned types"); + + // We want to test both... + // o Old-style PayChannels without a recipient backlink as well as + // o New-styled PayChannels with the backlink. + // So we start the test using old-style PayChannels. Then we pass + // the amendment to get new-style PayChannels. + Env env {*this, supported_amendments() - fixPayChanRecipientOwnerDir}; + Account const alice("alice"); + Account const becky("becky"); + Account const gw("gw"); + + env.fund(XRP(100000), alice, becky, gw); + env.close(); + + // Give alice and becky a bunch of offers that we have to search + // through before we figure out that there's a non-deletable + // entry in their directory. + for (int i {0}; i < 200; ++i) + { + env (offer (alice, gw["USD"](1), XRP (1))); + env (offer (becky, gw["USD"](1), XRP (1))); + env.close(); + } + env.require (offers (alice, 200)); + env.require (offers (becky, 200)); + + // Close enough ledgers to be able to delete alice's and becky's + // accounts. + incLgrSeqForAccDel(env, alice); + incLgrSeqForAccDel(env, becky); + + // alice writes a check to becky. Until that check is cashed or + // canceled it will prevent alice's and becky's accounts from being + // deleted. + uint256 const checkId {getCheckIndex (alice, env.seq (alice))}; + env (check::create (alice, becky, XRP (1))); + env.close(); + + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (alice, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); + env (acctdelete (becky, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); + env.close(); + + // Cancel the check, but add an escrow. Again, with the escrow + // on board, alice and becky should not be able to delete their + // accounts. + env (check::cancel (becky, checkId)); + env.close(); + + // Lambda to create an escrow. + auto escrowCreate = [] ( + jtx::Account const& account, jtx::Account const& to, + STAmount const& amount, NetClock::time_point const& cancelAfter) + { + Json::Value jv; + jv[jss::TransactionType] = jss::EscrowCreate; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = account.human(); + jv[jss::Destination] = to.human(); + jv[jss::Amount] = amount.getJson(JsonOptions::none); + jv[sfFinishAfter.jsonName] = + cancelAfter.time_since_epoch().count() + 1; + jv[sfCancelAfter.jsonName] = + cancelAfter.time_since_epoch().count() + 2; + return jv; + }; + + using namespace std::chrono_literals; + std::uint32_t const escrowSeq {env.seq (alice)}; + env (escrowCreate (alice, becky, XRP(333), env.now() + 2s)); + env.close(); + + // alice and becky should be unable to delete their accounts because + // of the escrow. + env (acctdelete (alice, gw), fee(acctDelFee), ter (tecHAS_OBLIGATIONS)); + env (acctdelete (becky, gw), fee(acctDelFee), ter (tecHAS_OBLIGATIONS)); + env.close(); + + // Now cancel the escrow, but create a payment channel between + // alice and becky. + + // Lambda to cancel an escrow. + auto escrowCancel = [] (Account const& account, + Account const& from, std::uint32_t seq) + { + Json::Value jv; + jv[jss::TransactionType] = jss::EscrowCancel; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = account.human(); + jv[sfOwner.jsonName] = from.human(); + jv[sfOfferSequence.jsonName] = seq; + return jv; + }; + env (escrowCancel (becky, alice, escrowSeq)); + env.close(); + + Keylet const alicePayChanKey { + keylet::payChan (alice, becky, env.seq (alice))}; + + env (payChanCreate ( + alice, becky, XRP(57), 4s, env.now() + 2s, alice.pk())); + env.close(); + + // An old-style PayChannel does not add a back link from the + // destination. So with the PayChannel in place becky should be + // able to delete her account, but alice should not. + auto const beckyBalance {env.balance (becky)}; + env (acctdelete (alice, gw), fee(acctDelFee), ter (tecHAS_OBLIGATIONS)); + env (acctdelete (becky, gw), fee(acctDelFee)); + verifyDeliveredAmount (env, beckyBalance - acctDelFee); + env.close(); + + // Alice cancels her PayChannel which will leave her with only offers + // in her directory. + + // Lambda to close a PayChannel. + auto payChanClose = [] ( + jtx::Account const& account, + Keylet const& payChanKeylet, + PublicKey const& pk) + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelClaim; + jv[jss::Flags] = tfClose; + jv[jss::Account] = account.human(); + jv[sfPayChannel.jsonName] = to_string (payChanKeylet.key); + jv[sfPublicKey.jsonName] = strHex(pk.slice()); + return jv; + }; + env (payChanClose (alice, alicePayChanKey, alice.pk())); + env.close(); + + // Now enable the amendment so PayChannels add a backlink from the + // destination. + env.enableFeature (fixPayChanRecipientOwnerDir); + env.close(); + + // gw creates a PayChannel with alice as the destination. With the + // amendment passed this should prevent alice from deleting her + // account. + Keylet const gwPayChanKey {keylet::payChan (gw, alice, env.seq (gw))}; + + env (payChanCreate ( + gw, alice, XRP(68), 4s, env.now() + 2s, alice.pk())); + env.close(); + + // alice can't delete her account because of the PayChannel. + env (acctdelete (alice, gw), fee(acctDelFee), ter (tecHAS_OBLIGATIONS)); + env.close(); + + // alice closes the PayChannel which should (finally) allow her to + // delete her account. + env (payChanClose (alice, gwPayChanKey, alice.pk())); + env.close(); + + // Now alice can successfully delete her account. + auto const aliceBalance {env.balance (alice)}; + env (acctdelete (alice, gw), fee(acctDelFee)); + verifyDeliveredAmount (env, aliceBalance - acctDelFee); + env.close(); + } + + void testResurrection() + { + // Create an account with an old-style PayChannel. Delete the + // destination of the PayChannel then resurrect the destination. + // The PayChannel should still work. + using namespace jtx; + + testcase("Resurrection"); + + // We need an old-style PayChannel that doesn't provide a backlink + // from the destination. So don't enable the amendment with that fix. + Env env {*this, supported_amendments() - fixPayChanRecipientOwnerDir}; + Account const alice("alice"); + Account const becky("becky"); + + env.fund(XRP(10000), alice, becky); + env.close(); + + // Verify that becky's account root is present. + Keylet const beckyAcctKey {keylet::account (becky.id())}; + BEAST_EXPECT (env.closed()->exists (beckyAcctKey)); + + using namespace std::chrono_literals; + Keylet const payChanKey { + keylet::payChan (alice, becky, env.seq (alice))}; + auto const payChanXRP = XRP (37); + + env (payChanCreate ( + alice, becky, payChanXRP, 4s, env.now() + 1h, alice.pk())); + env.close(); + BEAST_EXPECT (env.closed()->exists (payChanKey)); + + // Close enough ledgers to be able to delete becky's account. + incLgrSeqForAccDel(env, becky); + + auto const beckyPreDelBalance {env.balance (becky)}; + + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (becky, alice), fee(acctDelFee)); + verifyDeliveredAmount ( + env, beckyPreDelBalance - acctDelFee); + env.close(); + + // Verify that becky's account root is gone. + BEAST_EXPECT (! env.closed()->exists (beckyAcctKey)); + + // All it takes is a large enough XRP payment to resurrect + // becky's account. Try too small a payment. + env (pay (alice, becky, XRP (19)), ter (tecNO_DST_INSUF_XRP)); + env.close(); + + // Actually resurrect becky's account. + env (pay (alice, becky, XRP (20))); + env.close(); + + // becky's account root should be back. + BEAST_EXPECT (env.closed()->exists (beckyAcctKey)); + BEAST_EXPECT (env.balance (becky) == XRP (20)); + + // becky's resurrected account can be the destination of alice's + // PayChannel. + auto payChanClaim = [&] () + { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelClaim; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = alice.human (); + jv[sfPayChannel.jsonName] = to_string (payChanKey.key); + jv[sfBalance.jsonName] = + payChanXRP.value().getJson (JsonOptions::none); + return jv; + }; + env (payChanClaim()); + env.close(); + + BEAST_EXPECT (env.balance (becky) == XRP (20) + payChanXRP); + } + + void testAmendmentEnable() + { + // Start with the featureDeletableAccounts amendment disabled. + // Then enable the amendment and delete an account. + using namespace jtx; + + testcase("Amendment enable"); + + Env env {*this, supported_amendments() - featureDeletableAccounts}; + Account const alice("alice"); + Account const becky("becky"); + + env.fund(XRP(10000), alice, becky); + env.close(); + + // Close enough ledgers to be able to delete alice's account. + incLgrSeqForAccDel(env, alice); + + // Verify that alice's account root is present. + Keylet const aliceAcctKey {keylet::account (alice.id())}; + BEAST_EXPECT (env.closed()->exists (aliceAcctKey)); + + auto const alicePreDelBal {env.balance (alice)}; + auto const beckyPreDelBal {env.balance (becky)}; + + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (alice, becky), fee(acctDelFee), ter (temDISABLED)); + env.close(); + + // Verify that alice's account root is still present and alice and + // becky both have their XRP. + BEAST_EXPECT (env.current()->exists (aliceAcctKey)); + BEAST_EXPECT (env.balance (alice) == alicePreDelBal); + BEAST_EXPECT (env.balance (becky) == beckyPreDelBal); + + // When the amendment is enabled the previous transaction is + // retried into the new open ledger and succeeds. + env.enableFeature (featureDeletableAccounts); + env.close(); + + // alice's account is still in the most recently closed ledger. + BEAST_EXPECT (env.closed()->exists (aliceAcctKey)); + + // Verify that alice's account root is gone from the current ledger + // and becky has alice's XRP. + BEAST_EXPECT (! env.current()->exists (aliceAcctKey)); + BEAST_EXPECT (env.balance (becky) == + alicePreDelBal + beckyPreDelBal - acctDelFee); + + env.close(); + BEAST_EXPECT (! env.closed()->exists (aliceAcctKey)); + } + + void testTooManyOffers() + { + // Put enough offers in an account that we refuse to delete the account. + using namespace jtx; + + testcase("Too many offers"); + + Env env {*this}; + Account const alice("alice"); + Account const gw("gw"); + + // Fund alice well so she can afford the reserve on the offers. + env.fund(XRP(10000000), alice, gw); + env.close(); + + // To increase the number of Books affected, change the currency of + // each offer. + std::string currency {"AAA"}; + + // Alice creates 1001 offers. This is one greater than the number of + // directory entries an AccountDelete will remove. + std::uint32_t const offerSeq0 {env.seq (alice)}; + constexpr int offerCount {1001}; + for (int i {0}; i < offerCount; ++i) + { + env (offer (alice, gw[currency](1), XRP (1))); + env.close(); + + // Increment to next currency. + ++currency[0]; + if (currency[0] > 'Z') + { + currency[0] = 'A'; + ++currency[1]; + } + if (currency[1] > 'Z') + { + currency[1] = 'A'; + ++currency[2]; + } + if (currency[2] > 'Z') + { + currency[0] = 'A'; + currency[1] = 'A'; + currency[2] = 'A'; + } + } + + // Close enough ledgers to be able to delete alice's account. + incLgrSeqForAccDel(env, alice); + + // Verify the existence of the expected ledger entries. + Keylet const aliceOwnerDirKey {keylet::ownerDir (alice.id())}; + { + std::shared_ptr closed {env.closed()}; + BEAST_EXPECT (closed->exists (keylet::account (alice.id()))); + BEAST_EXPECT (closed->exists (aliceOwnerDirKey)); + + // alice's directory nodes. + for (std::uint32_t i {0}; i < ((offerCount / 32) + 1); ++i) + BEAST_EXPECT (closed->exists ( + keylet::page (aliceOwnerDirKey, i))); + + // alice's offers. + for (std::uint32_t i {0}; i < offerCount; ++i) + BEAST_EXPECT (closed->exists ( + keylet::offer (alice.id(), offerSeq0 + i))); + } + + // Delete alice's account. Should fail because she has too many + // offers in her directory. + auto const acctDelFee {drops (env.current()->fees().increment)}; + + env (acctdelete (alice, gw), fee (acctDelFee), ter (tefTOO_BIG)); + + // Cancel one of alice's offers. Then the account delete can succeed. + env.require (offers (alice, offerCount)); + env(offer_cancel(alice, offerSeq0)); + env.close(); + env.require (offers (alice, offerCount - 1)); + + // alice successfully deletes her account. + auto const alicePreDelBal {env.balance (alice)}; + env (acctdelete (alice, gw), fee (acctDelFee)); + verifyDeliveredAmount (env, alicePreDelBal - acctDelFee); + env.close(); + + // Verify that alice's account root is gone as well as her directory + // nodes and all of her offers. + { + std::shared_ptr closed {env.closed()}; + BEAST_EXPECT (! closed->exists (keylet::account (alice.id()))); + BEAST_EXPECT (! closed->exists (aliceOwnerDirKey)); + + // alice's former directory nodes. + for (std::uint32_t i {0}; i < ((offerCount / 32) + 1); ++i) + BEAST_EXPECT (! closed->exists ( + keylet::page (aliceOwnerDirKey, i))); + + // alice's former offers. + for (std::uint32_t i {0}; i < offerCount; ++i) + BEAST_EXPECT (! closed->exists ( + keylet::offer (alice.id(), offerSeq0 + i))); + } + } + + void testImplicitlyCreatedTrustline() + { + // Show that a trust line that is implicitly created by offer crossing + // prevents an account from being deleted. + using namespace jtx; + + testcase("Implicitly created trust line"); + + Env env {*this}; + Account const alice{"alice"}; + Account const gw{"gw"}; + auto const BUX {gw["BUX"]}; + + env.fund(XRP(10000), alice, gw); + env.close(); + + // alice creates an offer that, if crossed, will implicitly create + // a trust line. + env (offer (alice, BUX(30), XRP(30))); + env.close(); + + // gw crosses alice's offer. alice should end up with BUX(30). + env (offer (gw, XRP(30), BUX(30))); + env.close(); + env.require (balance (alice, BUX(30))); + + // Close enough ledgers to be able to delete alice's account. + incLgrSeqForAccDel(env, alice); + + // alice and gw can't delete their accounts because of the implicitly + // created trust line. + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (alice, gw), fee (acctDelFee), ter(tecHAS_OBLIGATIONS)); + env.close(); + + env (acctdelete (gw, alice), fee (acctDelFee), ter(tecHAS_OBLIGATIONS)); + env.close(); + { + std::shared_ptr closed {env.closed()}; + BEAST_EXPECT (closed->exists (keylet::account (alice.id()))); + BEAST_EXPECT (closed->exists (keylet::account (gw.id()))); + } + } + + void testBalanceTooSmallForFee() + { + // See what happens when an account with a balance less than the + // incremental reserve tries to delete itself. + using namespace jtx; + + testcase("Balance too small for fee"); + + Env env {*this}; + Account const alice("alice"); + + // Note that the fee structure for unit tests does not match the fees + // on the production network (October 2019). Unit tests have a base + // reserve of 200 XRP. + env.fund (env.current()->fees().accountReserve(0), noripple (alice)); + env.close(); + + // Burn a chunk of alice's funds so she only has 1 XRP remaining in + // her account. + env (noop (alice), fee (env.balance(alice) - XRP(1))); + env.close(); + + auto const acctDelFee {drops (env.current()->fees().increment)}; + BEAST_EXPECT (acctDelFee > env.balance (alice)); + + // alice attempts to delete her account even though she can't pay + // the full fee. She specifies a fee that is larger than her balance. + // + // The balance of env.master should not change. + auto const masterBalance {env.balance (env.master)}; + env (acctdelete (alice, env.master), fee(acctDelFee), + ter (terINSUF_FEE_B)); + env.close(); + { + std::shared_ptr const closed {env.closed()}; + BEAST_EXPECT (closed->exists (keylet::account (alice.id()))); + BEAST_EXPECT (env.balance (env.master) == masterBalance); + } + + // alice again attempts to delete her account. This time she specifies + // her current balance in XRP. Again the transaction fails. + BEAST_EXPECT (env.balance (alice) == XRP (1)); + env (acctdelete (alice, env.master), fee(XRP (1)), + ter (telINSUF_FEE_P)); + env.close(); + { + std::shared_ptr closed {env.closed()}; + BEAST_EXPECT (closed->exists (keylet::account (alice.id()))); + BEAST_EXPECT (env.balance (env.master) == masterBalance); + } + } + + void run() override + { + testBasics(); + testDirectories(); + testOwnedTypes(); + testResurrection(); + testAmendmentEnable(); + testTooManyOffers(); + testImplicitlyCreatedTrustline(); + testBalanceTooSmallForFee(); + } +}; + +BEAST_DEFINE_TESTSUITE(AccountDelete,app,ripple); + +} // test +} // ripple diff --git a/src/test/app/AccountTxPaging_test.cpp b/src/test/app/AccountTxPaging_test.cpp index f1f57c69b1..f58b4f4a5d 100644 --- a/src/test/app/AccountTxPaging_test.cpp +++ b/src/test/app/AccountTxPaging_test.cpp @@ -110,7 +110,7 @@ class AccountTxPaging_test : public beast::unit_test::suite if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; BEAST_EXPECT(checkTransaction (txs[0u], 3, 3)); - BEAST_EXPECT(checkTransaction (txs[1u], 1, 3)); + BEAST_EXPECT(checkTransaction (txs[1u], 3, 3)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -118,8 +118,8 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 2, 4)); - BEAST_EXPECT(checkTransaction (txs[1u], 2, 4)); + BEAST_EXPECT(checkTransaction (txs[0u], 4, 4)); + BEAST_EXPECT(checkTransaction (txs[1u], 4, 4)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -127,8 +127,8 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 2, 5)); - BEAST_EXPECT(checkTransaction (txs[1u], 3, 5)); + BEAST_EXPECT(checkTransaction (txs[0u], 4, 5)); + BEAST_EXPECT(checkTransaction (txs[1u], 5, 5)); BEAST_EXPECT(! jrr[jss::marker]); } @@ -146,7 +146,7 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 1)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 1, 3)); + BEAST_EXPECT(checkTransaction (txs[0u], 3, 3)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -154,7 +154,7 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 1)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 2, 4)); + BEAST_EXPECT(checkTransaction (txs[0u], 4, 4)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -163,9 +163,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 2, 4)); - BEAST_EXPECT(checkTransaction (txs[1u], 2, 5)); - BEAST_EXPECT(checkTransaction (txs[2u], 3, 5)); + BEAST_EXPECT(checkTransaction (txs[0u], 4, 4)); + BEAST_EXPECT(checkTransaction (txs[1u], 4, 5)); + BEAST_EXPECT(checkTransaction (txs[2u], 5, 5)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -173,9 +173,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 4, 6)); - BEAST_EXPECT(checkTransaction (txs[1u], 5, 6)); - BEAST_EXPECT(checkTransaction (txs[2u], 6, 7)); + BEAST_EXPECT(checkTransaction (txs[0u], 6, 6)); + BEAST_EXPECT(checkTransaction (txs[1u], 7, 6)); + BEAST_EXPECT(checkTransaction (txs[2u], 8, 7)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -183,9 +183,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 7, 7)); - BEAST_EXPECT(checkTransaction (txs[1u], 8, 8)); - BEAST_EXPECT(checkTransaction (txs[2u], 9, 8)); + BEAST_EXPECT(checkTransaction (txs[0u], 9, 7)); + BEAST_EXPECT(checkTransaction (txs[1u], 10, 8)); + BEAST_EXPECT(checkTransaction (txs[2u], 11, 8)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -193,8 +193,8 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 10, 9)); - BEAST_EXPECT(checkTransaction (txs[1u], 11, 9)); + BEAST_EXPECT(checkTransaction (txs[0u], 12, 9)); + BEAST_EXPECT(checkTransaction (txs[1u], 13, 9)); BEAST_EXPECT(! jrr[jss::marker]); } @@ -204,8 +204,8 @@ class AccountTxPaging_test : public beast::unit_test::suite auto txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 11, 9)); - BEAST_EXPECT(checkTransaction (txs[1u], 10, 9)); + BEAST_EXPECT(checkTransaction (txs[0u], 13, 9)); + BEAST_EXPECT(checkTransaction (txs[1u], 12, 9)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -213,8 +213,8 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 2)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 9, 8)); - BEAST_EXPECT(checkTransaction (txs[1u], 8, 8)); + BEAST_EXPECT(checkTransaction (txs[0u], 11, 8)); + BEAST_EXPECT(checkTransaction (txs[1u], 10, 8)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -223,9 +223,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 7, 7)); - BEAST_EXPECT(checkTransaction (txs[1u], 6, 7)); - BEAST_EXPECT(checkTransaction (txs[2u], 5, 6)); + BEAST_EXPECT(checkTransaction (txs[0u], 9, 7)); + BEAST_EXPECT(checkTransaction (txs[1u], 8, 7)); + BEAST_EXPECT(checkTransaction (txs[2u], 7, 6)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -233,9 +233,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 4, 6)); - BEAST_EXPECT(checkTransaction (txs[1u], 3, 5)); - BEAST_EXPECT(checkTransaction (txs[2u], 2, 5)); + BEAST_EXPECT(checkTransaction (txs[0u], 6, 6)); + BEAST_EXPECT(checkTransaction (txs[1u], 5, 5)); + BEAST_EXPECT(checkTransaction (txs[2u], 4, 5)); if(! BEAST_EXPECT(jrr[jss::marker])) return; @@ -243,9 +243,9 @@ class AccountTxPaging_test : public beast::unit_test::suite txs = jrr[jss::transactions]; if (! BEAST_EXPECT(txs.isArray() && txs.size() == 3)) return; - BEAST_EXPECT(checkTransaction (txs[0u], 2, 4)); - BEAST_EXPECT(checkTransaction (txs[1u], 2, 4)); - BEAST_EXPECT(checkTransaction (txs[2u], 1, 3)); + BEAST_EXPECT(checkTransaction (txs[0u], 4, 4)); + BEAST_EXPECT(checkTransaction (txs[1u], 4, 4)); + BEAST_EXPECT(checkTransaction (txs[2u], 3, 3)); if(! BEAST_EXPECT(jrr[jss::marker])) return; diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 5349b89b21..c323abf755 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -1277,6 +1277,139 @@ 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(); @@ -1289,6 +1422,7 @@ struct Escrow_test : public beast::unit_test::suite testEscrowConditions(); testMetaAndOwnership(); testConsequences(); + testDeletedDestination(); } }; diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index bcd5339471..c951537199 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -1175,6 +1175,7 @@ struct Flow_test : public beast::unit_test::suite auto const USD = gw["USD"]; env.fund(XRP(100000000), alice, bob, carol, gw); + env.close(); env.trust(USD(10000), alice, carol); env(trust(bob, USD(10000), tfSetNoRipple)); env.trust(USD(10000), bob); diff --git a/src/test/app/Freeze_test.cpp b/src/test/app/Freeze_test.cpp index 36f73101af..637a4c67b0 100644 --- a/src/test/app/Freeze_test.cpp +++ b/src/test/app/Freeze_test.cpp @@ -522,7 +522,7 @@ class Freeze_test : public beast::unit_test::suite env.meta()->getJson(JsonOptions::none)[sfAffectedNodes.fieldName]; if(! BEAST_EXPECT(checkArraySize(affected, 8u))) return; - auto created = affected[5u][sfCreatedNode.fieldName]; + auto created = affected[0u][sfCreatedNode.fieldName]; BEAST_EXPECT(created[sfNewFields.fieldName][jss::Account] == A2.human()); env.close(); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 1557594b47..ddc0a91f84 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -2178,8 +2178,12 @@ public: BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-101"); } - // Helper function that validates a *defaulted* trustline. If the - // trustline is not defaulted then the tests will not pass. + // Helper function that validates a *defaulted* trustline: one that has + // no unusual flags set and doesn't have high or low limits set. Such a + // trustline may have an actual balance (it can be created automatically + // if a user places an offer to acquire an IOU for which they don't have + // a trust line defined). If the trustline is not defaulted then the tests + // will not pass. void verifyDefaultTrustline (jtx::Env& env, jtx::Account const& account, jtx::PrettyAmount const& expectBalance) @@ -2487,6 +2491,7 @@ public: env.fund (reserve (env, 3) + (fee * 3), alice); env.fund (reserve (env, 3) + (fee * 2), bob); env.close(); + env (trust(alice, usdOffer)); env (trust(bob, eurOffer)); env.close(); @@ -2494,6 +2499,7 @@ public: env (pay(gw, alice, usdOffer)); env (pay(gw, bob, eurOffer)); env.close(); + env.require ( balance (alice, usdOffer), balance (bob, eurOffer)); @@ -2510,34 +2516,44 @@ public: balance (bob, usdOffer), offers (alice, 0), offers (bob, 0)); + + // Alice's offer crossing created a default EUR trustline and + // Bob's offer crossing created a default USD trustline: verifyDefaultTrustline (env, alice, eurOffer); verifyDefaultTrustline (env, bob, usdOffer); // Make two more offers that leave one of the offers non-dry. - env (offer (alice, USD(999), eurOffer)); + // Guarantee the order of application by putting a close() + // between them. env (offer (bob, eurOffer, usdOffer)); - env.close(); + + env (offer (alice, USD(999), eurOffer)); + env.close(); + + env.require (offers (alice, 0)); + env.require (offers (bob, 1)); + env.require (balance (alice, USD(999))); env.require (balance (alice, EUR(1))); env.require (balance (bob, USD(1))); env.require (balance (bob, EUR(999))); - env.require (offers (alice, 0)); - verifyDefaultTrustline (env, alice, EUR(1)); - verifyDefaultTrustline (env, bob, USD(1)); + { auto bobsOffers = offersOnAccount (env, bob); - BEAST_EXPECT (bobsOffers.size() == 1); - auto const& bobsOffer = *(bobsOffers.front()); + if (BEAST_EXPECT(bobsOffers.size() == 1)) + { + auto const& bobsOffer = *(bobsOffers.front()); - BEAST_EXPECT (bobsOffer[sfTakerGets] == USD (1)); - BEAST_EXPECT (bobsOffer[sfTakerPays] == EUR (1)); + BEAST_EXPECT (bobsOffer[sfTakerGets] == USD(1)); + BEAST_EXPECT (bobsOffer[sfTakerPays] == EUR(1)); + } } // alice makes one more offer that cleans out bob's offer. env (offer (alice, USD(1), EUR(1))); - env.close(); + env.require (balance (alice, USD(1000))); env.require (balance (alice, EUR(none))); env.require (balance (bob, USD(none))); @@ -2548,6 +2564,22 @@ public: // The two trustlines that were generated by offers should be gone. BEAST_EXPECT (! env.le (keylet::line (alice.id(), EUR.issue()))); BEAST_EXPECT (! env.le (keylet::line (bob.id(), USD.issue()))); + + // Make two more offers that leave one of the offers non-dry. We + // need to properly sequence the transactions: + env (offer (alice, EUR(999), usdOffer)); + env.close(); + + env (offer (bob, usdOffer, eurOffer)); + env.close(); + + env.require (offers (alice, 0)); + env.require (offers (bob, 0)); + + env.require (balance (alice, USD(0))); + env.require (balance (alice, EUR(999))); + env.require (balance (bob, USD(1000))); + env.require (balance (bob, EUR(1))); } void @@ -4542,6 +4574,112 @@ public: env.require (offers (gw, 0)); } + void testDeletedOfferIssuer (FeatureBitset features) + { + // Show that an offer who's issuer has been deleted cannot be crossed. + using namespace jtx; + + testcase("Deleted offer issuer"); + + auto trustLineExists = [](jtx::Env const& env, + jtx::Account const& src, + jtx::Account const& dst, + Currency const& cur) -> bool { + return bool(env.le(keylet::line(src, dst, cur))); + }; + + Account const alice("alice"); + Account const becky("becky"); + Account const carol("carol"); + Account const gw("gateway"); + auto const USD = gw["USD"]; + auto const BUX = alice["BUX"]; + + Env env{*this, features}; + + env.fund (XRP(10000), alice, becky, carol, noripple (gw)); + env.trust (USD(1000), becky); + env (pay (gw, becky, USD(5))); + env.close(); + BEAST_EXPECT(trustLineExists(env, gw, becky, USD.currency)); + + // Make offers that produce USD and can be crossed two ways: + // direct XRP -> USD + // direct BUX -> USD + env (offer (becky, XRP(2), USD(2)), txflags(tfPassive)); + std::uint32_t const beckyBuxUsdSeq {env.seq (becky)}; + env (offer (becky, BUX(3), USD(3)), txflags(tfPassive)); + env.close(); + + // becky keeps the offers, but removes the trustline. + env(pay(becky, gw, USD(5))); + env.trust(USD(0), becky); + env.close(); + BEAST_EXPECT(!trustLineExists(env, gw, becky, USD.currency)); + BEAST_EXPECT(isOffer(env, becky, XRP(2), USD(2))); + BEAST_EXPECT(isOffer(env, becky, BUX(3), USD(3))); + + // Delete gw's account. + { + // The ledger sequence needs to far enough ahead of the account + // sequence before the account can be deleted. + int const delta = + [&env, &gw, openLedgerSeq = env.current()->seq()]() -> int + { + std::uint32_t const gwSeq {env.seq(gw)}; + if (gwSeq + 255 > openLedgerSeq) + return gwSeq - openLedgerSeq + 255; + return 0; + }(); + + for (int i = 0; i < delta; ++i) + env.close(); + + // Account deletion has a high fee. Account for that. + env(acctdelete(gw, alice), + fee(drops(env.current()->fees().increment))); + env.close(); + + // Verify that gw's account root is gone from the ledger. + BEAST_EXPECT(!env.closed()->exists(keylet::account(gw.id()))); + } + + // alice crosses becky's first offer. The offer create fails because + // the USD issuer is not in the ledger. + env (offer (alice, USD(2), XRP(2)), ter (tecNO_ISSUER)); + env.close(); + env.require (offers (alice, 0)); + BEAST_EXPECT(isOffer(env, becky, XRP(2), USD(2))); + BEAST_EXPECT(isOffer(env, becky, BUX(3), USD(3))); + + // alice crosses becky's second offer. Again, the offer create fails + // because the USD issuer is not in the ledger. + env (offer (alice, USD(3), BUX(3)), ter (tecNO_ISSUER)); + env.require (offers (alice, 0)); + BEAST_EXPECT(isOffer(env, becky, XRP(2), USD(2))); + BEAST_EXPECT(isOffer(env, becky, BUX(3), USD(3))); + + // Cancel becky's BUX -> USD offer so we can try auto-bridging. + env (offer_cancel (becky, beckyBuxUsdSeq)); + env.close(); + BEAST_EXPECT(!isOffer(env, becky, BUX(3), USD(3))); + + // alice creates an offer that can be auto-bridged with becky's + // remaining offer. + env.trust (BUX(1000), carol); + env (pay (alice, carol, BUX(2))); + + env (offer (alice, BUX(2), XRP(2))); + env.close(); + + // carol attempts the auto-bridge. Again, the offer create fails + // because the USD issuer is not in the ledger. + env (offer (carol, USD(2), BUX(2)), ter (tecNO_ISSUER)); + env.close(); + BEAST_EXPECT(isOffer(env, alice, BUX(2), XRP(2))); + BEAST_EXPECT(isOffer(env, becky, XRP(2), USD(2))); + } + void testTickSize (FeatureBitset features) { testcase ("Tick Size"); @@ -4719,6 +4857,7 @@ public: testMissingAuth (features); testRCSmoketest (features); testSelfAuth (features); + testDeletedOfferIssuer (features); testTickSize (features | featureTickSize); } diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 5a7f7b36c5..65dbfa2642 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1257,22 +1257,256 @@ struct PayChan_test : public beast::unit_test::suite } void - run () override + testAccountDelete() { - testSimple (); - testCancelAfter (); - testSettleDelay (); - testExpiration (); - testCloseDry (); - testDefaultAmount (); - testDisallowXRP (); - testDstTag (); - testDepositAuth (); - testMultiple (); - testRPC (); - testOptionalFields (); - testMalformedPK (); - testMetaAndOwnership (); + testcase("Account Delete"); + using namespace test::jtx; + using namespace std::literals::chrono_literals; + auto rmAccount = [this]( + Env& env, + Account const& toRm, + Account const& dst, + TER expectedTer = tesSUCCESS) { + // only allow an account to be deleted if the account's sequence + // number is at least 256 less than the current ledger sequence + for (auto minRmSeq = env.seq(toRm) + 257; + env.current()->seq() < minRmSeq; + env.close()) + { } + + env(acctdelete(toRm, dst), + fee(drops(env.current()->fees().increment)), + ter(expectedTer)); + env.close(); + this->BEAST_EXPECT( + isTesSuccess(expectedTer) == + !env.closed()->exists(keylet::account(toRm.id()))); + }; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + + for (bool const withOwnerDirFix : {false, true}) + { + auto const amd = withOwnerDirFix + ? supported_amendments() + : supported_amendments() - fixPayChanRecipientOwnerDir; + Env env{*this, amd}; + env.fund(XRP(10000), alice, bob, carol); + env.close(); + auto const feeDrops = env.current()->fees().base; + + // Create a channel from alice to bob + auto const pk = alice.pk(); + auto const settleDelay = 100s; + env(create(alice, bob, XRP(1000), settleDelay, pk)); + env.close(); + auto const chan = channel(*env.current(), alice, bob); + BEAST_EXPECT(channelBalance(*env.current(), chan) == XRP(0)); + BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); + + rmAccount(env, alice, carol, tecHAS_OBLIGATIONS); + // can only remove bob if the channel isn't in their owner direcotry + rmAccount( + env, + bob, + carol, + withOwnerDirFix ? TER(tecHAS_OBLIGATIONS) : TER(tesSUCCESS)); + + auto chanBal = channelBalance(*env.current(), chan); + auto chanAmt = channelAmount(*env.current(), chan); + BEAST_EXPECT(chanBal == XRP(0)); + BEAST_EXPECT(chanAmt == XRP(1000)); + + auto preBob = env.balance(bob); + auto const delta = XRP(50); + auto reqBal = chanBal + delta; + auto authAmt = reqBal + XRP(100); + assert(reqBal <= chanAmt); + + // claim should fail if the dst was removed + if (withOwnerDirFix) + { + env(claim(alice, chan, reqBal, authAmt)); + env.close(); + BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob + delta); + chanBal = reqBal; + } + else + { + auto const preAlice = env.balance(alice); + env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); + env.close(); + BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob); + BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); + } + + // fund should fail if the dst was removed + if (withOwnerDirFix) + { + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000))); + env.close(); + BEAST_EXPECT( + env.balance(alice) == preAlice - XRP(1000) - feeDrops); + BEAST_EXPECT( + channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); + chanAmt = chanAmt + XRP(1000); + } + else + { + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); + env.close(); + BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + } + + { + // Owner closes, will close after settleDelay + env(claim(alice, chan), txflags(tfClose)); + env.close(); + // settle delay hasn't ellapsed. Channels should exist. + BEAST_EXPECT(channelExists(*env.current(), chan)); + auto const closeTime = env.current()->info().parentCloseTime; + auto const minExpiration = closeTime + settleDelay; + env.close(minExpiration); + env(claim(alice, chan), txflags(tfClose)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + } + } + + { + // test resurrected account + Env env{*this, + supported_amendments() - fixPayChanRecipientOwnerDir}; + env.fund(XRP(10000), alice, bob, carol); + env.close(); + auto const feeDrops = env.current()->fees().base; + + // Create a channel from alice to bob + auto const pk = alice.pk(); + auto const settleDelay = 100s; + env(create(alice, bob, XRP(1000), settleDelay, pk)); + env.close(); + auto const chan = channel(*env.current(), alice, bob); + BEAST_EXPECT(channelBalance(*env.current(), chan) == XRP(0)); + BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); + + // Since `fixPayChanRecipientOwnerDir` is not active, can remove bob + rmAccount(env, bob, carol); + BEAST_EXPECT(!env.closed()->exists(keylet::account(bob.id()))); + + auto chanBal = channelBalance(*env.current(), chan); + auto chanAmt = channelAmount(*env.current(), chan); + BEAST_EXPECT(chanBal == XRP(0)); + BEAST_EXPECT(chanAmt == XRP(1000)); + auto preBob = env.balance(bob); + auto const delta = XRP(50); + auto reqBal = chanBal + delta; + auto authAmt = reqBal + XRP(100); + assert(reqBal <= chanAmt); + + { + // claim should fail, since bob doesn't exist + auto const preAlice = env.balance(alice); + env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); + env.close(); + BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob); + BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); + } + + { + // fund should fail, sincebob doesn't exist + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); + env.close(); + BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + } + + // resurrect bob + env(pay(alice, bob, XRP(20))); + env.close(); + BEAST_EXPECT(env.closed()->exists(keylet::account(bob.id()))); + + { + // alice should be able to claim + preBob = env.balance(bob); + reqBal = chanBal + delta; + authAmt = reqBal + XRP(100); + env(claim(alice, chan, reqBal, authAmt)); + BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob + delta); + chanBal = reqBal; + } + + { + // bob should be able to claim + preBob = env.balance(bob); + reqBal = chanBal + delta; + authAmt = reqBal + XRP(100); + auto const sig = + signClaimAuth(alice.pk(), alice.sk(), chan, authAmt); + env(claim(bob, chan, reqBal, authAmt, Slice(sig), alice.pk())); + BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob + delta - feeDrops); + chanBal = reqBal; + } + + { + // alice should be able to fund + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000))); + BEAST_EXPECT( + env.balance(alice) == preAlice - XRP(1000) - feeDrops); + BEAST_EXPECT( + channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); + chanAmt = chanAmt + XRP(1000); + } + + { + // Owner closes, will close after settleDelay + env(claim(alice, chan), txflags(tfClose)); + env.close(); + // settle delay hasn't ellapsed. Channels should exist. + BEAST_EXPECT(channelExists(*env.current(), chan)); + auto const closeTime = env.current()->info().parentCloseTime; + auto const minExpiration = closeTime + settleDelay; + env.close(minExpiration); + env(claim(alice, chan), txflags(tfClose)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + } + } + } + + void + run() override + { + testSimple(); + testCancelAfter(); + testSettleDelay(); + testExpiration(); + testCloseDry(); + testDefaultAmount(); + testDisallowXRP(); + testDstTag(); + testDepositAuth(); + testMultiple(); + testRPC(); + testOptionalFields(); + testMalformedPK(); + testMetaAndOwnership(); + testAccountDelete(); } }; diff --git a/src/test/app/Regression_test.cpp b/src/test/app/Regression_test.cpp index e44a40cbb3..927c82d0d0 100644 --- a/src/test/app/Regression_test.cpp +++ b/src/test/app/Regression_test.cpp @@ -38,7 +38,7 @@ struct Regression_test : public beast::unit_test::suite env.fund(XRP(10000), "alice", gw); env(offer("alice", USD(10), XRP(10)), require(owners("alice", 1))); env(offer("alice", USD(20), XRP(10)), json(R"raw( - { "OfferSequence" : 2 } + { "OfferSequence" : 4 } )raw"), require(owners("alice", 1))); } @@ -91,7 +91,7 @@ struct Regression_test : public beast::unit_test::suite // Specify the seq manually since the env's open ledger // doesn't know about this account. auto const jt = env.jt(noop("alice"), fee(expectedDrops), - seq(1)); + seq(2)); OpenView accum(&*next); diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 9ec809331d..55056f62f0 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -524,7 +524,7 @@ public: env.close(); // alice's transaction is still hanging around checkMetrics(env, 1, 8, 5, 4, 256, 700 * 256); - BEAST_EXPECT(env.seq(alice) == 1); + BEAST_EXPECT(env.seq(alice) == 3); // Keep alice's transaction waiting. env(noop(bob), fee(8000), queued); @@ -543,12 +543,12 @@ public: // into the ledger, so her transaction is gone, // though one of felicia's is still in the queue. checkMetrics(env, 1, 10, 6, 5, 256, 700 * 256); - BEAST_EXPECT(env.seq(alice) == 1); + BEAST_EXPECT(env.seq(alice) == 3); env.close(); // And now the queue is empty checkMetrics(env, 0, 12, 1, 6, 256, 800 * 256); - BEAST_EXPECT(env.seq(alice) == 1); + BEAST_EXPECT(env.seq(alice) == 3); } void testZeroFeeTxn() diff --git a/src/test/jtx.h b/src/test/jtx.h index cc11e6cabd..3944dd9347 100644 --- a/src/test/jtx.h +++ b/src/test/jtx.h @@ -24,6 +24,7 @@ #include #include +#include #include #include #include diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index edde0c4dc0..db2d75f783 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -202,8 +202,8 @@ public: { Env env(*this); env.fund(n, noripple("alice", gw)); - BEAST_EXPECT(env.seq("alice") == 1); - BEAST_EXPECT(env.seq(gw) == 1); + BEAST_EXPECT(env.seq("alice") == 3); + BEAST_EXPECT(env.seq(gw) == 3); } // autofill diff --git a/src/test/jtx/acctdelete.h b/src/test/jtx/acctdelete.h new file mode 100644 index 0000000000..ec0bca4b82 --- /dev/null +++ b/src/test/jtx/acctdelete.h @@ -0,0 +1,39 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2019 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TEST_JTX_ACCTDELETE_H_INCLUDED +#define RIPPLE_TEST_JTX_ACCTDELETE_H_INCLUDED + +#include +#include + +namespace ripple { +namespace test { +namespace jtx { + +/** Delete account. If successful transfer remaining XRP to dest. */ +Json::Value +acctdelete (Account const& account, Account const& dest); + +} // jtx + +} // test +} // ripple + +#endif diff --git a/src/test/jtx/impl/acctdelete.cpp b/src/test/jtx/impl/acctdelete.cpp new file mode 100644 index 0000000000..c800ed411b --- /dev/null +++ b/src/test/jtx/impl/acctdelete.cpp @@ -0,0 +1,40 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2019 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include + +namespace ripple { +namespace test { +namespace jtx { + +// Delete account. If successful transfer remaining XRP to dest. +Json::Value +acctdelete (jtx::Account const& account, jtx::Account const& dest) +{ + Json::Value jv; + jv[sfAccount.jsonName] = account.human(); + jv[sfDestination.jsonName] = dest.human(); + jv[sfTransactionType.jsonName] = jss::AccountDelete; + return jv; +} + +} // jtx +} // test +} // ripple diff --git a/src/test/ledger/Directory_test.cpp b/src/test/ledger/Directory_test.cpp index 835829f3cc..c3e2ab72d9 100644 --- a/src/test/ledger/Directory_test.cpp +++ b/src/test/ledger/Directory_test.cpp @@ -94,6 +94,7 @@ struct Directory_test : public beast::unit_test::suite env.fund(XRP(10000000), alice, bob, gw); // Insert 400 offers from Alice, then one from Bob: + std::uint32_t const firstOfferSeq {env.seq (alice)}; for (std::size_t i = 1; i <= 400; ++i) env(offer(alice, USD(10), XRP(10))); @@ -104,7 +105,7 @@ struct Directory_test : public beast::unit_test::suite auto dir = Dir(*env.current(), keylet::ownerDir(alice)); - std::uint32_t lastSeq = 1; + std::uint32_t lastSeq = firstOfferSeq - 1; // Check that the orders are sequential by checking // that their sequence numbers are: @@ -121,6 +122,7 @@ struct Directory_test : public beast::unit_test::suite Env env(*this); env.fund(XRP(10000000), alice, gw); + std::uint32_t const firstOfferSeq {env.seq (alice)}; for (std::size_t i = 1; i <= 400; ++i) env(offer(alice, USD(i), XRP(i))); env.close(); @@ -143,8 +145,9 @@ struct Directory_test : public beast::unit_test::suite // Ensure that the page contains the correct orders by // calculating which sequence numbers belong here. - std::uint32_t minSeq = 2 + (page * dirNodeMaxEntries); - std::uint32_t maxSeq = minSeq + dirNodeMaxEntries; + std::uint32_t const minSeq = + firstOfferSeq + (page * dirNodeMaxEntries); + std::uint32_t const maxSeq = minSeq + dirNodeMaxEntries; for (auto const& e : v) { @@ -295,6 +298,7 @@ struct Directory_test : public beast::unit_test::suite auto const USD = gw["USD"]; env.fund(XRP(10000), alice, gw); + env.close(); env.trust(USD(1000), alice); env(pay(gw, alice, USD(1000))); diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 6df45ef60f..9e2fe1e56e 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -39,15 +39,14 @@ class Invariants_test : public beast::unit_test::suite test::jtx::Account const& b, ApplyContext& ac)>; - using TXMod = std::function < - void (STTx& tx)>; - void doInvariantCheck( bool enabled, std::vector const& expect_logs, Precheck const& precheck, XRPAmount fee = XRPAmount{}, - TXMod txmod = [](STTx&){}) + STTx tx = STTx {ttACCOUNT_SET, [](STObject&){ }}, + std::initializer_list ters = + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}) { using namespace test::jtx; Env env {*this}; @@ -64,9 +63,6 @@ class Invariants_test : public beast::unit_test::suite env.fund (XRP (1000), A1, A2); env.close(); - // dummy/empty tx to setup the AccountContext - auto tx = STTx {ttACCOUNT_SET, [](STObject&){ } }; - txmod(tx); OpenView ov {*env.current()}; test::StreamSink sink {beast::severities::kWarning}; beast::Journal jlog {sink}; @@ -82,16 +78,17 @@ class Invariants_test : public beast::unit_test::suite BEAST_EXPECT(precheck(A1, A2, ac)); - TER tr = tesSUCCESS; // invoke check twice to cover tec and tef cases - for (auto i : {0,1}) + if (! BEAST_EXPECT(ters.size() == 2)) + return; + + TER terActual = tesSUCCESS; + for (TER const terExpect : ters) { - tr = ac.checkInvariants(tr, fee); + terActual = ac.checkInvariants(terActual, fee); if (enabled) { - BEAST_EXPECT(tr == (i == 0 - ? TER {tecINVARIANT_FAILED} - : TER {tefINVARIANT_FAILED})); + BEAST_EXPECT(terExpect == terActual); BEAST_EXPECT( boost::starts_with(sink.messages().str(), "Invariant failed:") || boost::starts_with(sink.messages().str(), @@ -105,7 +102,7 @@ class Invariants_test : public beast::unit_test::suite } else { - BEAST_EXPECT(tr == tesSUCCESS); + BEAST_EXPECT(terActual == tesSUCCESS); BEAST_EXPECT(sink.messages().str().empty()); } } @@ -147,11 +144,13 @@ class Invariants_test : public beast::unit_test::suite } void - testAccountsNotRemoved(bool enabled) + testAccountRootsNotRemoved(bool enabled) { using namespace test::jtx; testcase << "checks " << (enabled ? "enabled" : "disabled") << " - account root removed"; + + // An account was deleted, but not by an AccountDelete transaction. doInvariantCheck (enabled, {{ "an account root was deleted" }}, [](Account const& A1, Account const&, ApplyContext& ac) @@ -163,6 +162,35 @@ class Invariants_test : public beast::unit_test::suite ac.view().erase (sle); return true; }); + + // Successful AccountDelete transaction that didn't delete an account. + // + // Note that this is a case where a second invocation of the invariant + // checker returns a tecINVARIANT_FAILED, not a tefINVARIANT_FAILED. + // After a discussion with the team, we believe that's okay. + doInvariantCheck (enabled, + {{ "account deletion succeeded without deleting an account" }}, + [](Account const&, Account const&, ApplyContext& ac){return true;}, + XRPAmount{}, + STTx {ttACCOUNT_DELETE, [](STObject& tx){ }}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + + // Successful AccountDelete that deleted more than one account. + doInvariantCheck (enabled, + {{ "account deletion succeeded but deleted multiple accounts" }}, + [](Account const& A1, Account const& A2, ApplyContext& ac) + { + // remove two accounts from the view + auto const sleA1 = ac.view().peek (keylet::account(A1.id())); + auto const sleA2 = ac.view().peek (keylet::account(A2.id())); + if(!sleA1 || !sleA2) + return false; + ac.view().erase (sleA1); + ac.view().erase (sleA2); + return true; + }, + XRPAmount{}, + STTx {ttACCOUNT_DELETE, [](STObject& tx){ }}); } void @@ -300,7 +328,8 @@ class Invariants_test : public beast::unit_test::suite { "XRP net change of 0 doesn't match fee 20" }}, [](Account const&, Account const&, ApplyContext&) { return true; }, XRPAmount{20}, - [](STTx& tx) { tx.setFieldAmount(sfFee, XRPAmount{10}); } ); + STTx { ttACCOUNT_SET, + [](STObject& tx){tx.setFieldAmount(sfFee, XRPAmount{10});} }); } @@ -424,6 +453,62 @@ class Invariants_test : public beast::unit_test::suite }); } + void + testValidNewAccountRoot(bool enabled) + { + using namespace test::jtx; + testcase << "checks " << (enabled ? "enabled" : "disabled") << + " - valid new account root"; + + doInvariantCheck (enabled, + {{ "account root created by a non-Payment" }}, + [](Account const&, Account const&, ApplyContext& ac) + { + // Insert a new account root created by a non-payment into + // the view. + const Account A3 {"A3"}; + Keylet const acctKeylet = keylet::account (A3); + auto const sleNew = std::make_shared(acctKeylet); + ac.view().insert (sleNew); + return true; + }); + + doInvariantCheck (enabled, + {{ "multiple accounts created in a single transaction" }}, + [](Account const&, Account const&, ApplyContext& ac) + { + // Insert two new account roots into the view. + { + const Account A3 {"A3"}; + Keylet const acctKeylet = keylet::account (A3); + auto const sleA3 = std::make_shared(acctKeylet); + ac.view().insert (sleA3); + } + { + const Account A4 {"A4"}; + Keylet const acctKeylet = keylet::account (A4); + auto const sleA4 = std::make_shared(acctKeylet); + ac.view().insert (sleA4); + } + return true; + }); + + doInvariantCheck (enabled, + {{ "account created with wrong starting sequence number" }}, + [](Account const&, Account const&, ApplyContext& ac) + { + // Insert a new account root with the wrong starting sequence. + const Account A3 {"A3"}; + Keylet const acctKeylet = keylet::account (A3); + auto const sleNew = std::make_shared(acctKeylet); + sleNew->setFieldU32 (sfSequence, ac.view().seq() + 1); + ac.view().insert (sleNew); + return true; + }, + XRPAmount{}, + STTx {ttPAYMENT, [](STObject& tx){ }}); + } + public: void run () override { @@ -434,13 +519,14 @@ public: for(auto const& b : {false, true}) { testXRPNotCreated (b); - testAccountsNotRemoved (b); + testAccountRootsNotRemoved (b); testTypesMatch (b); testNoXRPTrustLine (b); testXRPBalanceCheck (b); testTransactionFeeCheck(b); testNoBadOffers (b); testNoZeroEscrow (b); + testValidNewAccountRoot (b); } } }; diff --git a/src/test/ledger/PaymentSandbox_test.cpp b/src/test/ledger/PaymentSandbox_test.cpp index 7c8a0683e0..0e303fe4b7 100644 --- a/src/test/ledger/PaymentSandbox_test.cpp +++ b/src/test/ledger/PaymentSandbox_test.cpp @@ -202,7 +202,7 @@ class PaymentSandbox_test : public beast::unit_test::suite auto const startingAmount = accountHolds ( pv, alice, iss.currency, iss.account, fhIGNORE_FREEZE, j); - redeemIOU (pv, alice, toDebit, iss, j); + BEAST_EXPECT(redeemIOU (pv, alice, toDebit, iss, j) == tesSUCCESS); BEAST_EXPECT(accountHolds (pv, alice, iss.currency, iss.account, fhIGNORE_FREEZE, j) == startingAmount - toDebit); @@ -217,7 +217,7 @@ class PaymentSandbox_test : public beast::unit_test::suite auto const startingAmount = accountHolds ( pv, alice, iss.currency, iss.account, fhIGNORE_FREEZE, j); - issueIOU (pv, alice, toCredit, iss, j); + BEAST_EXPECT(issueIOU (pv, alice, toCredit, iss, j) == tesSUCCESS); BEAST_EXPECT(accountHolds (pv, alice, iss.currency, iss.account, fhIGNORE_FREEZE, j) == startingAmount); diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index a5c81eaa5c..319cfae1ac 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -25,6 +25,8 @@ #include +#include + namespace ripple { namespace test { @@ -36,31 +38,14 @@ R"json({ "Flags" : 65536, "LedgerEntryType" : "Offer", "OwnerNode" : "0000000000000000", - "Sequence" : 4, + "Sequence" : 6, "TakerGets" : { "currency" : "USD", "issuer" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK", "value" : "1" }, "TakerPays" : "100000000", - "index" : "A984D036A0E562433A8377CA57D1A1E056E58C0D04818F8DFD3A1AA3F217DD82" -})json" -, -R"json({ - "Account" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK", - "BookDirectory" : "B025997A323F5C3E03DDF1334471F5984ABDE31C59D463525D038D7EA4C68000", - "BookNode" : "0000000000000000", - "Flags" : 65536, - "LedgerEntryType" : "Offer", - "OwnerNode" : "0000000000000000", - "Sequence" : 5, - "TakerGets" : { - "currency" : "USD", - "issuer" : "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW", - "value" : "1" - }, - "TakerPays" : "100000000", - "index" : "CAFE32332D752387B01083B60CC63069BA4A969C9730836929F841450F6A718E" + "index" : "29665262716C19830E26AEEC0916E476FC7D8EF195FF3B4F06829E64F82A3B3E" })json" , R"json({ @@ -108,6 +93,23 @@ R"json({ "LowNode" : "0000000000000000", "index" : "D89BC239086183EB9458C396E643795C1134963E6550E682A190A5F021766D43" })json" +, +R"json({ + "Account" : "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK", + "BookDirectory" : "B025997A323F5C3E03DDF1334471F5984ABDE31C59D463525D038D7EA4C68000", + "BookNode" : "0000000000000000", + "Flags" : 65536, + "LedgerEntryType" : "Offer", + "OwnerNode" : "0000000000000000", + "Sequence" : 7, + "TakerGets" : { + "currency" : "USD", + "issuer" : "r32rQHyesiTtdWFU7UJVtff4nCR5SHCbJW", + "value" : "1" + }, + "TakerPays" : "100000000", + "index" : "F03ABE26CB8C5F4AFB31A86590BD25C64C5756FCE5CE9704C27AFE291A4A29A1" +})json" }; class AccountObjects_test : public beast::unit_test::suite @@ -288,8 +290,7 @@ public: auto& aobj = resp[jss::result][jss::account_objects][i]; aobj.removeMember("PreviousTxnID"); aobj.removeMember("PreviousTxnLgrSeq"); - - BEAST_EXPECT( aobj == bobj[i+2]); + BEAST_EXPECT( aobj == bobj[i+1]); } } // test stepped one-at-a-time with limit=1, resume from prev marker @@ -492,7 +493,7 @@ public: auto const& ticket = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT (ticket[sfAccount.jsonName] == gw.human()); BEAST_EXPECT (ticket[sfLedgerEntryType.jsonName] == jss::Ticket); - BEAST_EXPECT (ticket[sfSequence.jsonName].asUInt() == 9); + BEAST_EXPECT (ticket[sfSequence.jsonName].asUInt() == 11); } { // See how "deletion_blockers_only" handles gw's directory. @@ -501,21 +502,25 @@ public: params[jss::deletion_blockers_only] = true; auto resp = env.rpc("json", "account_objects", to_string(params)); - constexpr Json::StaticString const expectedLedgerTypes[] = { - jss::Escrow, jss::Check, jss::RippleState, jss::PayChannel - }; - constexpr auto expectedAccountObjects{ + std::set const expectedLedgerTypes{ + {jss::Escrow.c_str(), + jss::Check.c_str(), + jss::RippleState.c_str(), + jss::PayChannel.c_str()}}; + + std::uint32_t const expectedAccountObjects{ static_cast(std::size(expectedLedgerTypes)) }; if (BEAST_EXPECT(acct_objs_is_size(resp, expectedAccountObjects))) { auto const& aobjs = resp[jss::result][jss::account_objects]; + std::set gotLedgerTypes; for (std::uint32_t i = 0; i < expectedAccountObjects; ++i) { - BEAST_EXPECT( - aobjs[i]["LedgerEntryType"] == expectedLedgerTypes[i]); + gotLedgerTypes.insert(aobjs[i]["LedgerEntryType"].asString()); } + BEAST_EXPECT(gotLedgerTypes == expectedLedgerTypes); } } { diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index b69621280f..bc445f13f8 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -99,30 +99,34 @@ public: env (pay (gw, bob, USD_gw (10))); env(offer(bob, XRP(100), USD_bob(1))); - env(offer(bob, XRP(100), USD_gw(1))); - env(offer(bob, XRP(10), USD_gw(2))); + env(offer(bob, XRP(200), USD_gw(2))); + env(offer(bob, XRP(30), USD_gw(6))); // make the RPC call auto const jroOuter = env.rpc("account_offers", bob.human())[jss::result][jss::offers]; if(BEAST_EXPECT(checkArraySize(jroOuter, 3u))) { + // Note that the returned offers are sorted by index, not by + // order of insertion or by sequence number. There is no + // guarantee that their order will not change in the future + // if the sequence numbers or the account IDs change. BEAST_EXPECT(jroOuter[0u][jss::quality] == "100000000"); BEAST_EXPECT(jroOuter[0u][jss::taker_gets][jss::currency] == "USD"); BEAST_EXPECT(jroOuter[0u][jss::taker_gets][jss::issuer] == gw.human()); - BEAST_EXPECT(jroOuter[0u][jss::taker_gets][jss::value] == "1"); - BEAST_EXPECT(jroOuter[0u][jss::taker_pays] == "100000000"); + BEAST_EXPECT(jroOuter[0u][jss::taker_gets][jss::value] == "2"); + BEAST_EXPECT(jroOuter[0u][jss::taker_pays] == "200000000"); - BEAST_EXPECT(jroOuter[1u][jss::quality] == "5000000"); + BEAST_EXPECT(jroOuter[1u][jss::quality] == "100000000"); BEAST_EXPECT(jroOuter[1u][jss::taker_gets][jss::currency] == "USD"); - BEAST_EXPECT(jroOuter[1u][jss::taker_gets][jss::issuer] == gw.human()); - BEAST_EXPECT(jroOuter[1u][jss::taker_gets][jss::value] == "2"); - BEAST_EXPECT(jroOuter[1u][jss::taker_pays] == "10000000"); + BEAST_EXPECT(jroOuter[1u][jss::taker_gets][jss::issuer] == bob.human()); + BEAST_EXPECT(jroOuter[1u][jss::taker_gets][jss::value] == "1"); + BEAST_EXPECT(jroOuter[1u][jss::taker_pays] == "100000000"); - BEAST_EXPECT(jroOuter[2u][jss::quality] == "100000000"); + BEAST_EXPECT(jroOuter[2u][jss::quality] == "5000000"); BEAST_EXPECT(jroOuter[2u][jss::taker_gets][jss::currency] == "USD"); - BEAST_EXPECT(jroOuter[2u][jss::taker_gets][jss::issuer] == bob.human()); - BEAST_EXPECT(jroOuter[2u][jss::taker_gets][jss::value] == "1"); - BEAST_EXPECT(jroOuter[2u][jss::taker_pays] == "100000000"); + BEAST_EXPECT(jroOuter[2u][jss::taker_gets][jss::issuer] == gw.human()); + BEAST_EXPECT(jroOuter[2u][jss::taker_gets][jss::value] == "6"); + BEAST_EXPECT(jroOuter[2u][jss::taker_pays] == "30000000"); } { diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 3b20a67f2d..e609b63fa8 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -22,12 +22,88 @@ #include #include +#include + namespace ripple { namespace test { class AccountTx_test : public beast::unit_test::suite { + // A data structure used to describe the basic structure of a + // transactions array node as returned by the account_tx RPC command. + struct NodeSanity + { + int const index; + Json::StaticString const& txType; + boost::container::flat_set created; + boost::container::flat_set deleted; + boost::container::flat_set modified; + + NodeSanity( + int idx, + Json::StaticString const& t, + std::initializer_list c, + std::initializer_list d, + std::initializer_list m) + : index(idx), txType(t) + { + auto buildSet = [](auto&& init){ + boost::container::flat_set r; + r.reserve(init.size()); + for(auto&& s : init) + r.insert(s); + return r; + }; + + created = buildSet(c); + deleted = buildSet(d); + modified = buildSet(m); + } + }; + + // A helper method tests can use to validate returned JSON vs NodeSanity. + void + checkSanity (Json::Value const& txNode, NodeSanity const& sane) + { + BEAST_EXPECT(txNode[jss::validated].asBool() == true); + BEAST_EXPECT( + txNode[jss::tx][sfTransactionType.jsonName].asString() == + sane.txType); + + // Make sure all of the expected node types are present. + boost::container::flat_set createdNodes; + boost::container::flat_set deletedNodes; + boost::container::flat_set modifiedNodes; + + for (Json::Value const& metaNode : + txNode[jss::meta][sfAffectedNodes.jsonName]) + { + if (metaNode.isMember (sfCreatedNode.jsonName)) + createdNodes.insert ( + metaNode[sfCreatedNode.jsonName] + [sfLedgerEntryType.jsonName].asString()); + + else if (metaNode.isMember (sfDeletedNode.jsonName)) + deletedNodes.insert ( + metaNode[sfDeletedNode.jsonName] + [sfLedgerEntryType.jsonName].asString()); + + else if (metaNode.isMember (sfModifiedNode.jsonName)) + modifiedNodes.insert ( + metaNode[sfModifiedNode.jsonName] + [sfLedgerEntryType.jsonName].asString()); + + else + fail ("Unexpected or unlabeled node type in metadata.", + __FILE__, __LINE__); + } + + BEAST_EXPECT(createdNodes == sane.created); + BEAST_EXPECT(deletedNodes == sane.deleted); + BEAST_EXPECT(modifiedNodes == sane.modified); + }; + void testParameters() { @@ -342,75 +418,6 @@ class AccountTx_test : public beast::unit_test::suite // Do a sanity check on each returned transaction. They should // be returned in the reverse order of application to the ledger. - struct NodeSanity - { - int const index; - Json::StaticString const& txType; - std::initializer_list created; - std::initializer_list deleted; - std::initializer_list modified; - }; - - auto checkSanity = [this] ( - Json::Value const& txNode, NodeSanity const& sane) - { - BEAST_EXPECT(txNode[jss::validated].asBool() == true); - BEAST_EXPECT( - txNode[jss::tx][sfTransactionType.jsonName].asString() == - sane.txType); - - // Make sure all of the expected node types are present. - std::vector createdNodes {}; - std::vector deletedNodes {}; - std::vector modifiedNodes{}; - - for (Json::Value const& metaNode : - txNode[jss::meta][sfAffectedNodes.jsonName]) - { - if (metaNode.isMember (sfCreatedNode.jsonName)) - createdNodes.push_back ( - metaNode[sfCreatedNode.jsonName] - [sfLedgerEntryType.jsonName].asString()); - - else if (metaNode.isMember (sfDeletedNode.jsonName)) - deletedNodes.push_back ( - metaNode[sfDeletedNode.jsonName] - [sfLedgerEntryType.jsonName].asString()); - - else if (metaNode.isMember (sfModifiedNode.jsonName)) - modifiedNodes.push_back ( - metaNode[sfModifiedNode.jsonName] - [sfLedgerEntryType.jsonName].asString()); - - else - fail ("Unexpected or unlabeled node type in metadata.", - __FILE__, __LINE__); - } - - auto cmpNodeTypes = [this] ( - char const* const errMsg, - std::vector& got, - std::initializer_list expList) - { - std::sort (got.begin(), got.end()); - - std::vector exp; - exp.reserve (expList.size()); - for (char const* nodeType : expList) - exp.push_back (nodeType); - std::sort (exp.begin(), exp.end()); - - if (got != exp) - { - fail (errMsg, __FILE__, __LINE__); - } - }; - - cmpNodeTypes ("Created mismatch", createdNodes, sane.created); - cmpNodeTypes ("Deleted mismatch", deletedNodes, sane.deleted); - cmpNodeTypes ("Modified mismatch", modifiedNodes, sane.modified); - }; - static const NodeSanity sanity[] { // txType, created, deleted, modified @@ -447,12 +454,107 @@ class AccountTx_test : public beast::unit_test::suite } } + void + testAccountDelete() + { + // Verify that if an account is resurrected then the account_tx RPC + // command still recovers all transactions on that account before + // and after resurrection. + using namespace test::jtx; + using namespace std::chrono_literals; + + Env env(*this); + Account const alice {"alice"}; + Account const becky {"becky"}; + + env.fund(XRP(10000), alice, becky); + env.close(); + + // Verify that becky's account root is present. + Keylet const beckyAcctKey {keylet::account (becky.id())}; + BEAST_EXPECT (env.closed()->exists (beckyAcctKey)); + + // becky does an AccountSet . + env (noop (becky)); + + // Close enough ledgers to be able to delete becky's account. + std::uint32_t const ledgerCount { + env.current()->seq() + 257 - env.seq (becky)}; + + for (std::uint32_t i = 0; i < ledgerCount; ++i) + env.close(); + + auto const beckyPreDelBalance {env.balance (becky)}; + + auto const acctDelFee {drops (env.current()->fees().increment)}; + env (acctdelete (becky, alice), fee (acctDelFee)); + env.close(); + + // Verify that becky's account root is gone. + BEAST_EXPECT (! env.closed()->exists (beckyAcctKey)); + + // All it takes is a large enough XRP payment to resurrect + // becky's account. Try too small a payment. + env (pay (alice, becky, XRP (19)), ter (tecNO_DST_INSUF_XRP)); + env.close(); + + // Actually resurrect becky's account. + env (pay (alice, becky, XRP (45))); + env.close(); + + // becky's account root should be back. + BEAST_EXPECT (env.closed()->exists (beckyAcctKey)); + BEAST_EXPECT (env.balance (becky) == XRP (45)); + + // becky pays alice. + env (pay (becky, alice, XRP(20))); + env.close(); + + // Setup is done. Look at the transactions returned by account_tx. + // Verify that account_tx locates all of becky's transactions. + Json::Value params; + params[jss::account] = becky.human(); + params[jss::ledger_index_min] = -1; + params[jss::ledger_index_max] = -1; + + Json::Value const result { + env.rpc("json", "account_tx", to_string(params))}; + + BEAST_EXPECT (result[jss::result][jss::status] == "success"); + BEAST_EXPECT (result[jss::result][jss::transactions].isArray()); + + Json::Value const& txs {result[jss::result][jss::transactions]}; + + // Do a sanity check on each returned transaction. They should + // be returned in the reverse order of application to the ledger. + static const NodeSanity sanity[] + { + // txType, created, deleted, modified +/* becky pays alice */ { 0, jss::Payment, {}, {}, {jss::AccountRoot, jss::AccountRoot}}, +/* alice resurrects becky's acct */ { 1, jss::Payment, {jss::AccountRoot}, {}, {jss::AccountRoot}}, +/* becky deletes her account */ { 2, jss::AccountDelete, {}, {jss::AccountRoot}, {jss::AccountRoot}}, +/* becky's noop */ { 3, jss::AccountSet, {}, {}, {jss::AccountRoot}}, +/* "fund" sets flags */ { 4, jss::AccountSet, {}, {}, {jss::AccountRoot}}, +/* "fund" creates becky's acct */ { 5, jss::Payment, {jss::AccountRoot}, {}, {jss::AccountRoot}} + }; + + BEAST_EXPECT (std::extent::value == + result[jss::result][jss::transactions].size()); + + for (unsigned int index {0}; + index < std::extent::value; ++index) + { + checkSanity (txs[index], sanity[index]); + } + } + public: void run() override { testParameters(); testContents(); + testAccountDelete(); } }; BEAST_DEFINE_TESTSUITE(AccountTx, app, ripple); diff --git a/src/test/rpc/Book_test.cpp b/src/test/rpc/Book_test.cpp index 466383a415..3defd23d9b 100644 --- a/src/test/rpc/Book_test.cpp +++ b/src/test/rpc/Book_test.cpp @@ -1043,7 +1043,7 @@ public: BEAST_EXPECT(jrOffer[jss::Flags] == 0); BEAST_EXPECT(jrOffer[sfLedgerEntryType.fieldName] == jss::Offer); BEAST_EXPECT(jrOffer[sfOwnerNode.fieldName] == "0000000000000000"); - BEAST_EXPECT(jrOffer[sfSequence.fieldName] == 3); + BEAST_EXPECT(jrOffer[sfSequence.fieldName] == 5); BEAST_EXPECT(jrOffer[jss::TakerGets] == USD(10).value().getJson(JsonOptions::none)); BEAST_EXPECT(jrOffer[jss::TakerPays] == @@ -1098,7 +1098,7 @@ public: BEAST_EXPECT(jrNextOffer[jss::Flags] == 0); BEAST_EXPECT(jrNextOffer[sfLedgerEntryType.fieldName] == jss::Offer); BEAST_EXPECT(jrNextOffer[sfOwnerNode.fieldName] == "0000000000000000"); - BEAST_EXPECT(jrNextOffer[sfSequence.fieldName] == 3); + BEAST_EXPECT(jrNextOffer[sfSequence.fieldName] == 5); BEAST_EXPECT(jrNextOffer[jss::TakerGets] == USD(5).value().getJson(JsonOptions::none)); BEAST_EXPECT(jrNextOffer[jss::TakerPays] == diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 33e1dd7e43..a6f77ba82d 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -265,8 +265,8 @@ class LedgerRPC_test : public beast::unit_test::suite } { constexpr char alicesAcctRootBinary[] { - "1100612200800000240000000225000000032D00000000554294BEBE5B569" - "A18C0A2702387C9B1E7146DC3A5850C1E87204951C6FDAA4C426240000002" + "1100612200800000240000000425000000032D00000000559CE54C3B934E4" + "73A995B477E92EC229F99CED5B62BF4D2ACE4DC42719103AE2F6240000002" "540BE4008114AE123A8556F3CF91154711376AFB0F894F832B3D" }; diff --git a/src/test/rpc/NoRipple_test.cpp b/src/test/rpc/NoRipple_test.cpp index c65abe1703..c8491561b5 100644 --- a/src/test/rpc/NoRipple_test.cpp +++ b/src/test/rpc/NoRipple_test.cpp @@ -86,6 +86,7 @@ public: Env env(*this, tweakedFeatures); env.fund(XRP(10000), gw, alice, bob, carol); + env.close(); env.trust(alice["USD"](100), bob); env.trust(bob["USD"](100), carol); diff --git a/src/test/unity/app_test_unity1.cpp b/src/test/unity/app_test_unity1.cpp index 9739a5a879..daf52b11c2 100644 --- a/src/test/unity/app_test_unity1.cpp +++ b/src/test/unity/app_test_unity1.cpp @@ -18,6 +18,7 @@ */ //============================================================================== +#include #include #include #include diff --git a/src/test/unity/jtx_unity1.cpp b/src/test/unity/jtx_unity1.cpp index 61fcd9b8b6..b97ded822d 100644 --- a/src/test/unity/jtx_unity1.cpp +++ b/src/test/unity/jtx_unity1.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include