From 2c6b0f319320dff6e7dd9b39c9c480455b217b8c Mon Sep 17 00:00:00 2001 From: seelabs Date: Mon, 19 Dec 2016 13:55:28 -0500 Subject: [PATCH] Fix limiting step re-execute bug (RIPD-1368): The deferred credits table can compute a balance that's different from the ledger balance. Syntax: A number written with no decimal means that number exactly. I.e. "12". A number written with a decimal means that number has a non-zero digit at the lowest order digit. I.e. "12.XX" means a number like "12.00000000000005" Consider the following payment: alice (USD) -> USD/XRP -> (XRP) Bob Alice initially has 12.XX USD in her account. The strand is used to debit alice the following amounts: 1) Debit alice 5 2) Debit alice 0.XX 3) Debit alice 3.XX The next time the strand is explored, alice has a USD/XRP offer on the books, and her account is credited: 1) Credit alice 20 When the beginning of the strand is reached, consider what happens when alice is a limiting step. Calculate how much we can get out the step. According to the deferred credit table this is: 12.XX - (5 + 0.XX + 3.XX) This is also limited by alice's balance, which is large thanks to the credit she received in the book step. Now that the step has calculated how much we can get out, throw out the sandbox (the one with the credit), and re-execute. However, the following error occurs. We asked for 12.XX - (5 + 0.XX + 3.XX). However, the ledger has calculated that alice has: ((12.XX - 5) - 0.XX) - 3.XX That's a problem, because that number is smaller. Notice that there are two precision losing operations in the deferred credits table: 1) The 5 + 0.XX step 2) The 12.XX - (total of debits). (Notice total of debits is < 10) However, there is only one precision losing operation in the ledger calculation: 1) (Subtotal of 12.XX-5) - 0.XX That means the calculation for the ledger results in a number that's smaller than the deferred credits. Flow detects this as a re-execution error. --- src/ripple/app/main/Amendments.cpp | 3 +- src/ripple/ledger/View.h | 1 - src/ripple/ledger/impl/PaymentSandbox.cpp | 12 +++++ src/ripple/protocol/Feature.h | 1 + src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/Flow_test.cpp | 56 +++++++++++++++++++++++ 6 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/main/Amendments.cpp b/src/ripple/app/main/Amendments.cpp index fb0f2b071..1399a2de5 100644 --- a/src/ripple/app/main/Amendments.cpp +++ b/src/ripple/app/main/Amendments.cpp @@ -49,7 +49,8 @@ supportedAmendments () { "08DE7D96082187F6E6578530258C77FAABABE4C20474BDB82F04B021F1A68647 PayChan" }, { "740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D628A06927F11 Flow" }, { "1562511F573A19AE9BD103B5D6B9E01B3B46805AEC5D3C4805C902B514399146 CryptoConditions" }, - { "532651B4FD58DF8922A49BA101AB3E996E5BFBF95A913B3E392504863E63B164 TickSize" } + { "532651B4FD58DF8922A49BA101AB3E996E5BFBF95A913B3E392504863E63B164 TickSize" }, + { "67431BE4A5F355C45E36EED94B38DFACC1F111F547360F444642512CBC0E18E1 RIPD1368" } }; } diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 246e3b4a5..d78ad86b0 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -344,7 +344,6 @@ bool amendmentRIPD1274 (NetClock::time_point const closeTime); NetClock::time_point const& amendmentRIPD1298SoTime (); bool amendmentRIPD1298 (NetClock::time_point const closeTime); - } // ripple #endif diff --git a/src/ripple/ledger/impl/PaymentSandbox.cpp b/src/ripple/ledger/impl/PaymentSandbox.cpp index d686f0af4..569e0d7f8 100644 --- a/src/ripple/ledger/impl/PaymentSandbox.cpp +++ b/src/ripple/ledger/impl/PaymentSandbox.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -186,15 +187,26 @@ PaymentSandbox::balanceHook (AccountID const& account, { auto delta = amount.zeroed (); auto lastBal = amount; + auto minBal = amount; for (auto curSB = this; curSB; curSB = curSB->ps_) { if (auto adj = curSB->tab_.adjustments (account, issuer, currency)) { delta += adj->debits; lastBal = adj->origBalance; + if (lastBal < minBal) + minBal = lastBal; } } adjustedAmt = std::min(amount, lastBal - delta); + if (rules().enabled(featureRIPD1368)) + { + // The adjusted amount should never be larger than the balance. In + // some circumstances, it is possible for the deferred credits table + // to compute usable balance just slightly above what the ledger + // calculates (but always less than the actual balance). + adjustedAmt = std::min(adjustedAmt, minBal); + } if (amendmentRIPD1274 (info ().parentCloseTime)) adjustedAmt.setIssuer(amount.getIssuer()); } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 06109f39f..69d2d0d52 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -46,6 +46,7 @@ extern uint256 const featurePayChan; extern uint256 const featureFlow; extern uint256 const featureCryptoConditions; extern uint256 const featureTickSize; +extern uint256 const featureRIPD1368; } // ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index cb78ff5a3..8430c28ba 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -57,5 +57,6 @@ uint256 const featurePayChan = feature("PayChan"); uint256 const featureFlow = feature("Flow"); uint256 const featureCryptoConditions = feature("CryptoConditions"); uint256 const featureTickSize = feature("TickSize"); +uint256 const featureRIPD1368 = feature("RIPD1368"); } // ripple diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 3b573524a..90c6f298e 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -1379,6 +1379,61 @@ struct Flow_test : public beast::unit_test::suite } } + template + void + testReexecuteDirectStep(Features&&... fs) + { + testcase("ReexecuteDirectStep"); + + using namespace jtx; + Env env(*this, features(fs)...); + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + auto const USD = gw["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + env(trust(alice, USD(100))); + + env(pay( + gw, alice, + // 12.55.... + STAmount{USD.issue(), std::uint64_t(1255555555555555ull), -14, false})); + + env(offer(gw, + // 5.0... + STAmount{ + USD.issue(), std::uint64_t(5000000000000000ull), -15, false}, + XRP(1000))); + + env(offer(gw, + // .555... + STAmount{ + USD.issue(), std::uint64_t(5555555555555555ull), -16, false}, + XRP(10))); + + env(offer(gw, + // 4.44.... + STAmount{ + USD.issue(), std::uint64_t(4444444444444444ull), -15, false}, + XRP(.1))); + + env(offer(alice, + // 17 + STAmount{ + USD.issue(), std::uint64_t(1700000000000000ull), -14, false}, + XRP(.001))); + + // Need to be past this time to see the bug + env.close(amendmentRIPD1274SoTime() + + 100 * env.closed()->info().closeTimeResolution); + + env(pay(alice, bob, XRP(10000)), path(~XRP), sendmax(USD(100)), + txflags(tfPartialPayment | tfNoRippleDirect)); + } + + void run() override { testDirectStep (); @@ -1394,6 +1449,7 @@ struct Flow_test : public beast::unit_test::suite testSelfFundedXRPEndpoint(true); testUnfundedOffer(true); testUnfundedOffer(false); + testReexecuteDirectStep(featureFlow, featureRIPD1368); } };