From a3a9dc26b4d4f049cf87fb7152f1fcef6697e505 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Thu, 1 Nov 2018 19:58:04 -0700 Subject: [PATCH] Introduce support for deletable accounts: The XRP Ledger utilizes an account model. Unlike systems based on a UTXO model, XRP Ledger accounts are first-class objects. This design choice allows the XRP Ledger to offer rich functionality, including the ability to own objects (offers, escrows, checks, signer lists) as well as other advanced features, such as key rotation and configurable multi-signing without needing to change a destination address. The trade-off is that accounts must be stored on ledger. The XRP Ledger applies reserve requirements, in XRP, to protect the shared global ledger from growing excessively large as the result of spam or malicious usage. Prior to this commit, accounts had been permanent objects; once created, they could never be deleted. This commit introduces a new amendment "DeletableAccounts" which, if enabled, will allow account objects to be deleted by executing the new "AccountDelete" transaction. Any funds remaining in the account will be transferred to an account specified in the deletion transaction. The amendment changes the mechanics of account creation; previously a new account would have an initial sequence number of 1. Accounts created after the amendment will have an initial sequence number that is equal to the ledger in which the account was created. Accounts can only be deleted if they are not associated with any obligations (like RippleStates, Escrows, or PayChannels) and if the current ledger sequence number exceeds the account's sequence number by at least 256 so that, if recreated, the account can be protected from transaction replay. --- Builds/CMake/RippledCore.cmake | 3 + src/ripple/app/misc/impl/TxQ.cpp | 6 +- src/ripple/app/paths/impl/BookStep.cpp | 10 + src/ripple/app/tx/impl/ApplyContext.cpp | 4 +- src/ripple/app/tx/impl/ApplyContext.h | 2 - src/ripple/app/tx/impl/CancelOffer.cpp | 10 +- src/ripple/app/tx/impl/CashCheck.cpp | 7 +- src/ripple/app/tx/impl/CreateCheck.cpp | 2 + src/ripple/app/tx/impl/CreateOffer.cpp | 5 + src/ripple/app/tx/impl/CreateTicket.cpp | 2 + src/ripple/app/tx/impl/DeleteAccount.cpp | 316 +++++++ src/ripple/app/tx/impl/DeleteAccount.h | 72 ++ src/ripple/app/tx/impl/DepositPreauth.cpp | 65 +- src/ripple/app/tx/impl/DepositPreauth.h | 6 + src/ripple/app/tx/impl/Escrow.cpp | 5 +- src/ripple/app/tx/impl/InvariantCheck.cpp | 129 ++- src/ripple/app/tx/impl/InvariantCheck.h | 106 ++- src/ripple/app/tx/impl/OfferStream.cpp | 42 +- src/ripple/app/tx/impl/OfferStream.h | 1 + src/ripple/app/tx/impl/PayChan.cpp | 19 +- src/ripple/app/tx/impl/Payment.cpp | 18 +- src/ripple/app/tx/impl/SetAccount.cpp | 7 +- src/ripple/app/tx/impl/SetRegularKey.cpp | 2 + src/ripple/app/tx/impl/SetSignerList.cpp | 159 ++-- src/ripple/app/tx/impl/SetSignerList.h | 14 +- src/ripple/app/tx/impl/SetTrust.cpp | 4 + src/ripple/app/tx/impl/Transactor.cpp | 23 +- src/ripple/app/tx/impl/applySteps.cpp | 9 +- src/ripple/ledger/ApplyView.h | 12 + src/ripple/ledger/View.h | 61 +- src/ripple/ledger/impl/ApplyStateTable.cpp | 26 +- src/ripple/ledger/impl/ApplyView.cpp | 64 ++ src/ripple/ledger/impl/View.cpp | 76 +- src/ripple/protocol/Feature.h | 2 + src/ripple/protocol/Indexes.h | 5 +- src/ripple/protocol/TER.h | 3 + src/ripple/protocol/TxFormats.h | 49 +- src/ripple/protocol/impl/Feature.cpp | 2 + src/ripple/protocol/impl/Indexes.cpp | 11 +- src/ripple/protocol/impl/TER.cpp | 3 + src/ripple/protocol/impl/TxFormats.cpp | 7 + src/ripple/protocol/jss.h | 1 + .../rpc/handlers/AccountCurrenciesHandler.cpp | 2 +- src/ripple/rpc/impl/DeliveredAmount.cpp | 3 +- src/ripple/unity/app_tx.cpp | 1 + src/test/app/AccountDelete_test.cpp | 789 ++++++++++++++++++ src/test/app/AccountTxPaging_test.cpp | 62 +- src/test/app/Escrow_test.cpp | 134 +++ src/test/app/Flow_test.cpp | 1 + src/test/app/Freeze_test.cpp | 2 +- src/test/app/Offer_test.cpp | 163 +++- src/test/app/PayChan_test.cpp | 264 +++++- src/test/app/Regression_test.cpp | 4 +- src/test/app/TxQ_test.cpp | 6 +- src/test/jtx.h | 1 + src/test/jtx/Env_test.cpp | 4 +- src/test/jtx/acctdelete.h | 39 + src/test/jtx/impl/acctdelete.cpp | 40 + src/test/ledger/Directory_test.cpp | 10 +- src/test/ledger/Invariants_test.cpp | 120 ++- src/test/ledger/PaymentSandbox_test.cpp | 4 +- src/test/rpc/AccountObjects_test.cpp | 61 +- src/test/rpc/AccountOffers_test.cpp | 28 +- src/test/rpc/AccountTx_test.cpp | 240 ++++-- src/test/rpc/Book_test.cpp | 4 +- src/test/rpc/LedgerRPC_test.cpp | 4 +- src/test/rpc/NoRipple_test.cpp | 1 + src/test/unity/app_test_unity1.cpp | 1 + src/test/unity/jtx_unity1.cpp | 1 + 69 files changed, 2859 insertions(+), 500 deletions(-) create mode 100644 src/ripple/app/tx/impl/DeleteAccount.cpp create mode 100644 src/ripple/app/tx/impl/DeleteAccount.h create mode 100644 src/test/app/AccountDelete_test.cpp create mode 100644 src/test/jtx/acctdelete.h create mode 100644 src/test/jtx/impl/acctdelete.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 888fcc7c3..e4d087255 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 c0789aa96..4e474c7b9 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 fa692ac95..365de2d74 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 8f3c16aad..33b69def0 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 cb7890d7d..a545a1c90 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 eaa76572d..70da5b5cf 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 acbb0b9f3..0d0590e90 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 5bddbbd46..b14b85bc4 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 672898103..14908d5ed 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 2f9fae474..1897c8fe6 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 000000000..d30738132 --- /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 000000000..24ec02842 --- /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 233530b0b..10f5c0ebf 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 d950d7eca..cdb03148b 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 b1b36e778..4d0d3d820 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 135b6e2e3..0ef4c9dfd 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 b1a2fbca3..af433221a 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 d4989337c..7c2f83c23 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 95e61aecf..a0d93d762 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 3f9027b05..6efe2aa9a 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 723d788af..2dcfbeef4 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 dfff835a5..0bf806060 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 ae46397b5..b6e3ad314 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 79d3b020e..190fd0c4f 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 ae9247464..1219d2bb2 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 fd7df0cbd..2c68badc0 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 e958169e8..e42557150 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 b23873338..131607178 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 7bc9e0fe3..9a084c75c 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 2f2c568bf..8e1afe577 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 7243897e8..467add5e4 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 ff1a297b2..e454c3330 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 c61bd6efa..5b3aa8455 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 c79db43e0..959ebf02d 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 4b9b902e6..70560b3b6 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 2fd3ca2fd..88e03a17b 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 cd366abee..90ef99a7e 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 2afd44e93..542f65ac7 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 b79e75726..526e58bba 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 0784b3e15..904e50134 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 aba971103..a61370270 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 d6f60e665..a9531c5f1 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 8be5ce3c3..f6261e0db 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 74cbd4ed4..dab57960e 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 000000000..2885807e8 --- /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 f1f57c69b..f58b4f4a5 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 5349b89b2..c323abf75 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 bcd533947..c95153719 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 36f73101a..637a4c67b 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 1557594b4..ddc0a91f8 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 5a7f7b36c..65dbfa264 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 e44a40cbb..927c82d0d 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 9ec809331..55056f62f 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 cc11e6cab..3944dd934 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 edde0c4dc..db2d75f78 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 000000000..ec0bca4b8 --- /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 000000000..c800ed411 --- /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 835829f3c..c3e2ab72d 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 6df45ef60..9e2fe1e56 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 7c8a0683e..0e303fe4b 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 a5c81eaa5..319cfae1a 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 b69621280..bc445f13f 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 3b20a67f2..e609b63fa 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 466383a41..3defd23d9 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 33e1dd7e4..a6f77ba82 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 c65abe170..c8491561b 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 9739a5a87..daf52b11c 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 61fcd9b8b..b97ded822 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