diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index b170f4995f..5394cc98f5 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -912,11 +912,23 @@ TxQ::apply(Application& app, OpenView& view, LastLedgerSeq and MaybeTx::retriesRemaining. */ auto const balance = (*sle)[sfBalance].xrp(); + /* Get the minimum possible reserve. If fees exceed + this amount, the transaction can't be queued. + Considering that typical fees are several orders + of magnitude smaller than any current or expected + future reserve, this calculation is simpler than + trying to figure out the potential changes to + the ownerCount that may occur to the account + as a result of these transactions, and removes + any need to account for other transactions that + may affect the owner count while these are queued. + */ + auto const reserve = view.fees().accountReserve(0); auto totalFee = multiTxn->fee; if (multiTxn->includeCurrentFee) totalFee += (*tx)[sfFee].xrp(); if (totalFee >= balance || - totalFee >= view.fees().accountReserve(0)) + totalFee >= reserve) { // Drop the current transaction JLOG(j_.trace()) << @@ -933,9 +945,12 @@ TxQ::apply(Application& app, OpenView& view, auto const sleBump = multiTxn->applyView->peek( keylet::account(account)); + auto const potentialTotalSpend = multiTxn->fee + + std::min(balance - std::min(balance, reserve), + multiTxn->potentialSpend); + assert(potentialTotalSpend > 0); sleBump->setFieldAmount(sfBalance, - balance - (multiTxn->fee + - multiTxn->potentialSpend)); + balance - potentialTotalSpend); sleBump->setFieldU32(sfSequence, tSeq); } } diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index c05b45ebed..4ad934ab82 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -268,7 +268,7 @@ EscrowCreate::doApply() // Deduct owner's balance, increment owner count (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount]; - (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] + 1; + adjustOwnerCount(ctx_.view(), sle, 1, ctx_.journal); ctx_.view().update(sle); return tesSUCCESS; @@ -502,7 +502,7 @@ EscrowFinish::doApply() // Adjust source owner count auto const sle = ctx_.view().peek( keylet::account(account)); - (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] - 1; + adjustOwnerCount(ctx_.view(), sle, -1, ctx_.journal); ctx_.view().update(sle); // Remove escrow from ledger @@ -585,7 +585,7 @@ EscrowCancel::doApply() auto const sle = ctx_.view().peek( keylet::account(account)); (*sle)[sfBalance] = (*sle)[sfBalance] + (*slep)[sfAmount]; - (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] - 1; + adjustOwnerCount(ctx_.view(), sle, -1, ctx_.journal); ctx_.view().update(sle); // Remove escrow from ledger diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index cffd2da728..5be551db34 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -144,7 +144,7 @@ closeChannel ( assert ((*slep)[sfAmount] >= (*slep)[sfBalance]); (*sle)[sfBalance] = (*sle)[sfBalance] + (*slep)[sfAmount] - (*slep)[sfBalance]; - (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] - 1; + adjustOwnerCount(view, sle, -1, j); view.update (sle); // Remove PayChan from ledger @@ -254,7 +254,7 @@ PayChanCreate::doApply() // Deduct owner's balance, increment owner count (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount]; - (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] + 1; + adjustOwnerCount(ctx_.view(), sle, 1, ctx_.journal); ctx_.view ().update (sle); return tesSUCCESS; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 3ba7f71a4c..2573036523 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -124,7 +124,7 @@ class TxQ_test : public beast::unit_test::suite } void - initFee(jtx::Env& env, std::size_t expectedInLedger, std::uint32_t base, + initFee(jtx::Env& env, std::size_t expectedPerLedger, std::uint32_t base, std::uint32_t units, std::uint32_t reserve, std::uint32_t increment) { // Run past the flag ledger so that a Fee change vote occurs and @@ -143,7 +143,7 @@ class TxQ_test : public beast::unit_test::suite env.close(env.now() + 5s, 10000ms); checkMetrics(env, 0, 2 * (ripple::detail::supportedAmendments().size() + 1), - 0, expectedInLedger, 256); + 0, expectedPerLedger, 256); auto const fees = env.current()->fees(); BEAST_EXPECT(fees.base == base); BEAST_EXPECT(fees.units == units); @@ -1315,9 +1315,11 @@ public: void testInFlightBalance() { using namespace jtx; + testcase("In-flight balance checks"); Env env(*this, - makeConfig({ { "minimum_txn_in_ledger_standalone", "3" } })); + makeConfig({ { "minimum_txn_in_ledger_standalone", "3" } }, + {{"account_reserve", "200"}, {"owner_reserve", "50"}})); auto alice = Account("alice"); auto charlie = Account("charlie"); @@ -1325,20 +1327,25 @@ public: auto queued = ter(terQUEUED); - BEAST_EXPECT(env.current()->fees().base == 10); - BEAST_EXPECT(env.current()->fees().reserve == 200 * 1000000); - BEAST_EXPECT(env.current()->fees().increment == 50 * 1000000); + // Set the fee reserves _really_ low so transactions with fees + // in the ballpark of the reserves can be queued. With default + // reserves, a couple hundred transactions would have to be + // queued before the open ledger fee approached the reserve, + // which would unnecessarily slow down this test. + initFee(env, 3, 10, 10, 200, 50); - checkMetrics(env, 0, boost::none, 0, 3, 256); + auto limit = 3; + + checkMetrics(env, 0, 60, 0, limit, 256); env.fund(XRP(50000), noripple(alice, charlie), gw); - checkMetrics(env, 0, boost::none, 4, 3, 256); + checkMetrics(env, 0, 60, limit + 1, limit, 256); auto USD = gw["USD"]; auto BUX = gw["BUX"]; ////////////////////////////////////////// - // Offer with high XRP out blocks later txs + // Offer with high XRP out and low fee doesn't block auto aliceSeq = env.seq(alice); auto aliceBal = env.balance(alice); @@ -1349,25 +1356,99 @@ public: // XRP will be taken (except the reserve). env(offer(alice, BUX(5000), XRP(50000)), queued); + checkMetrics(env, 1, 60, limit + 1, limit, 256); - // So even a noop will look like alice - // doesn't have the balance to pay the fee - env(noop(alice), seq(aliceSeq + 1), ter(terINSUF_FEE_B)); - checkMetrics(env, 1, boost::none, 4, 3, 256); + // But because the reserve is protected, another + // transaction will be allowed to queue + env(noop(alice), seq(aliceSeq + 1), queued); + checkMetrics(env, 2, 60, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 8, 2, 4, 256); + ++limit; + checkMetrics(env, 0, limit*2, 2, limit, 256); // But once we close the ledger, we find alice // has plenty of XRP, because the offer didn't // cross (of course). env.require(balance(alice, aliceBal - drops(20)), owners(alice, 1)); + // cancel the offer + env(offer_cancel(alice, aliceSeq)); + + ////////////////////////////////////////// + // Offer with high XRP out and high total fee blocks later txs + fillQueue(env, alice); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); + aliceSeq = env.seq(alice); + aliceBal = env.balance(alice); + + env.require(owners(alice, 0)); + + // Alice creates an offer with a fee of half the reserve + env(offer(alice, BUX(5000), XRP(50000)), fee(drops(100)), + queued); + checkMetrics(env, 1, limit * 2, limit + 1, limit, 256); + + // Alice creates another offer with a fee + // that brings the total to just shy of the reserve + env(noop(alice), fee(drops(99)), seq(aliceSeq + 1), queued); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); + + // So even a noop will look like alice + // doesn't have the balance to pay the fee + env(noop(alice), fee(drops(51)), seq(aliceSeq + 2), + ter(terINSUF_FEE_B)); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); + + env.close(); + ++limit; + checkMetrics(env, 0, limit * 2, 3, limit, 256); + + // But once we close the ledger, we find alice + // has plenty of XRP, because the offer didn't + // cross (of course). + env.require(balance(alice, aliceBal - drops(250)), + owners(alice, 1)); + // cancel the offer + env(offer_cancel(alice, aliceSeq)); + + ////////////////////////////////////////// + // Offer with high XRP out and super high fee blocks later txs + fillQueue(env, alice); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); + aliceSeq = env.seq(alice); + aliceBal = env.balance(alice); + + env.require(owners(alice, 0)); + + // Alice creates an offer with a fee larger than the reserve + // This one can queue because it's the first in the queue for alice + env(offer(alice, BUX(5000), XRP(50000)), fee(drops(300)), + queued); + checkMetrics(env, 1, limit * 2, limit + 1, limit, 256); + + // So even a noop will look like alice + // doesn't have the balance to pay the fee + env(noop(alice), fee(drops(51)), seq(aliceSeq + 1), + ter(telCAN_NOT_QUEUE_BALANCE)); + checkMetrics(env, 1, limit * 2, limit + 1, limit, 256); + + env.close(); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); + + // But once we close the ledger, we find alice + // has plenty of XRP, because the offer didn't + // cross (of course). + env.require(balance(alice, aliceBal - drops(351)), + owners(alice, 1)); + // cancel the offer + env(offer_cancel(alice, aliceSeq)); ////////////////////////////////////////// // Offer with low XRP out allows later txs fillQueue(env, alice); - checkMetrics(env, 0, 8, 5, 4, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1378,21 +1459,24 @@ public: // And later transactions are just fine env(noop(alice), seq(aliceSeq + 1), queued); - checkMetrics(env, 2, 8, 5, 4, 256); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 10, 2, 5, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // But once we close the ledger, we find alice // has plenty of XRP, because the offer didn't // cross (of course). env.require(balance(alice, aliceBal - drops(20)), - owners(alice, 2)); + owners(alice, 1)); + // cancel the offer + env(offer_cancel(alice, aliceSeq)); ////////////////////////////////////////// - // Large XRP payment blocks later txs + // Large XRP payment doesn't block later txs fillQueue(env, alice); - checkMetrics(env, 0, 10, 6, 5, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1403,24 +1487,25 @@ public: env(pay(alice, charlie, XRP(50000)), queued); - // So even a noop will look like alice - // doesn't have the balance to pay the fee - env(noop(alice), seq(aliceSeq + 1), ter(terINSUF_FEE_B)); - checkMetrics(env, 1, 10, 6, 5, 256); + // But because the reserve is protected, another + // transaction will be allowed to queue + env(noop(alice), seq(aliceSeq + 1), queued); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 12, 2, 6, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // But once we close the ledger, we find alice // still has most of her balance, because the // payment was unfunded! env.require(balance(alice, aliceBal - drops(20)), - owners(alice, 2)); + owners(alice, 0)); ////////////////////////////////////////// // Small XRP payment allows later txs fillQueue(env, alice); - checkMetrics(env, 0, 12, 7, 6, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1432,33 +1517,34 @@ public: // And later transactions are just fine env(noop(alice), seq(aliceSeq + 1), queued); - checkMetrics(env, 2, 12, 7, 6, 256); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 14, 2, 7, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // The payment succeeds env.require(balance(alice, aliceBal - XRP(500) - drops(20)), - owners(alice, 2)); + owners(alice, 0)); ////////////////////////////////////////// // Large IOU payment allows later txs auto const amount = USD(500000); env(trust(alice, USD(50000000))); env(trust(charlie, USD(50000000))); - checkMetrics(env, 0, 14, 4, 7, 256); + checkMetrics(env, 0, limit * 2, 4, limit, 256); // Close so we don't have to deal // with tx ordering in consensus. env.close(); env(pay(gw, alice, amount)); - checkMetrics(env, 0, 14, 1, 7, 256); + checkMetrics(env, 0, limit * 2, 1, limit, 256); // Close so we don't have to deal // with tx ordering in consensus. env.close(); fillQueue(env, alice); - checkMetrics(env, 0, 14, 8, 7, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1472,21 +1558,22 @@ public: // But that's fine, because it doesn't affect // alice's XRP balance (other than the fee, of course). env(noop(alice), seq(aliceSeq + 1), queued); - checkMetrics(env, 2, 14, 8, 7, 256); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 16, 2, 8, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // So once we close the ledger, alice has her // XRP balance, but her USD balance went to charlie. env.require(balance(alice, aliceBal - drops(20)), balance(alice, USD(0)), balance(charlie, aliceUSD), - owners(alice, 3), + owners(alice, 1), owners(charlie, 1)); ////////////////////////////////////////// - // Large XRP to IOU payment blocks later txs. + // Large XRP to IOU payment doesn't block later txs. env(offer(gw, XRP(500000), USD(50000))); // Close so we don't have to deal @@ -1494,7 +1581,7 @@ public: env.close(); fillQueue(env, charlie); - checkMetrics(env, 0, 16, 9, 8, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1508,27 +1595,28 @@ public: env(pay(alice, charlie, USD(1000)), sendmax(XRP(60000)), queued); - // So even a noop will look like alice - // doesn't have the balance to pay the fee - env(noop(alice), seq(aliceSeq + 1), ter(terINSUF_FEE_B)); - checkMetrics(env, 1, 16, 9, 8, 256); + // But because the reserve is protected, another + // transaction will be allowed to queue + env(noop(alice), seq(aliceSeq + 1), queued); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 18, 2, 9, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // So once we close the ledger, alice sent a payment // to charlie using only a portion of her XRP balance env.require(balance(alice, aliceBal - XRP(10000) - drops(20)), balance(alice, USD(0)), balance(charlie, charlieUSD + USD(1000)), - owners(alice, 3), + owners(alice, 1), owners(charlie, 1)); ////////////////////////////////////////// // Small XRP to IOU payment allows later txs. fillQueue(env, charlie); - checkMetrics(env, 0, 18, 10, 9, 256); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); aliceSeq = env.seq(alice); aliceBal = env.balance(alice); @@ -1544,18 +1632,43 @@ public: // And later transactions are just fine env(noop(alice), seq(aliceSeq + 1), queued); - checkMetrics(env, 2, 18, 10, 9, 256); + checkMetrics(env, 2, limit * 2, limit + 1, limit, 256); env.close(); - checkMetrics(env, 0, 20, 2, 10, 256); + ++limit; + checkMetrics(env, 0, limit * 2, 2, limit, 256); // So once we close the ledger, alice sent a payment // to charlie using only a portion of her XRP balance env.require(balance(alice, aliceBal - XRP(5000) - drops(20)), balance(alice, USD(0)), balance(charlie, charlieUSD + USD(500)), - owners(alice, 3), + owners(alice, 1), owners(charlie, 1)); + + ////////////////////////////////////////// + // Edge case: what happens if the balance is below the reserve? + env(noop(alice), fee(env.balance(alice) - drops(30))); + env.close(); + + fillQueue(env, charlie); + checkMetrics(env, 0, limit * 2, limit + 1, limit, 256); + + aliceSeq = env.seq(alice); + aliceBal = env.balance(alice); + BEAST_EXPECT(aliceBal == drops(30)); + + env(noop(alice), fee(drops(25)), queued); + env(noop(alice), seq(aliceSeq + 1), ter(terINSUF_FEE_B)); + BEAST_EXPECT(env.balance(alice) == drops(30)); + + checkMetrics(env, 1, limit * 2, limit + 1, limit, 256); + + env.close(); + ++limit; + checkMetrics(env, 0, limit * 2, 1, limit, 256); + BEAST_EXPECT(env.balance(alice) == drops(5)); + } void testConsequences()