Unconditionalize 2017 amendments #3292

scottschurr · 2020-03-09T17:52:42Z

Amendments that were enabled in 2017 have now been on the books and active for over two years. Once an amendment is enabled it cannot be revoked. So the only situations where these amendments are disabled is when replaying ledgers (and their contained transactions) from before 2018.

It is uncommon, but not unheard of, to want to replay ledgers that are more than two years old. If these changes are accepted into the code base then a user wanting to replay a ledger from before 2018 will need to build an old version of the rippled code. This requirement is reinforced by a message that the rippled software generates if it is asked to replay a ledger from before 2018.

This pull request consists of a collection of small commits. Each commit unconditionalizes one specific amendment. Each commit is, to all intents and purposes, standalone. Reviewing the commits one at a time may make the review process easier. My intention is that these separate commits will not be squashed before merging. That way, in case one of the commits has an error, identifying the problem using a binary search will be easier.

nbougalis

Looks good. Left a couple of comments, most of which are at your discretion. Please remove the dead code I pointed out; once that's done, this is a 👍 from me.

nbougalis · 2020-03-12T21:47:46Z

src/ripple/app/consensus/RCLConsensus.cpp

@@ -119,8 +119,7 @@ RCLConsensus::Adaptor::acquireLedger(LedgerHash const& hash)
    // Notify inbound transactions of the new ledger sequence number
    inboundTransactions_.newRound(built->info().seq);

-    // Use the ledger timing rules of the acquired ledger
-    parms_.useRoundedCloseTime = built->rules().enabled(fix1528);
+    parms_.useRoundedCloseTime = true;


I think that with this change, useRoundedCloseTime will only ever be true, in which case this member (and the conditionals it control) can be removed. Please double-check if that's the case.

nbougalis · 2020-03-12T21:49:26Z

src/ripple/app/tx/impl/ApplyContext.cpp

@@ -90,51 +90,48 @@ ApplyContext::checkInvariantsHelper(
    XRPAmount const fee,
    std::index_sequence<Is...>)
 {
-    if (view_->rules().enabled(featureEnforceInvariants))
+    auto checkers = getInvariantChecks();


I think that this can be moved inside the try. I don't know that it matters much, but just pointing it out.

nbougalis · 2020-03-12T21:52:06Z

src/ripple/app/tx/impl/Escrow.cpp

@@ -488,10 +477,10 @@ EscrowFinish::doApply()
    }

    // Remove escrow from recipient's owner directory, if present.
-    if (ctx_.view ().rules().enabled(fix1523) && (*slep)[~sfDestinationNode])
+    if (auto const optPage = (*slep)[~sfDestinationNode]; optPage)


Out of curiosity, what do others think about code clarity? Are the two forms identical or am I missing something?

if (auto const optPage = (*slep)[~sfDestinationNode]; optPage))

vs.

if (auto const optPage = (*slep)[~sfDestinationNode])

The two forms have identical results. I'm happy to change. The second form is more in keeping with the rest of the code base.

nbougalis · 2020-03-12T21:57:05Z

src/ripple/ledger/impl/View.cpp

+    if (strictOrder)
+        return view.dirAppend(dir, uLedgerIndex, fDescriber);
+    else
+        return view.dirInsert(dir, uLedgerIndex, fDescriber);

    JLOG (j.trace()) << "dirAdd:" <<


Now that we unconditionalized this amendment, the code from here on down and until the end of the function—about 100 lines worth—is unreachable and can simply be eliminated.

Good catch! I was not paying close enough attention. Thanks.

nbougalis · 2020-03-12T22:01:03Z

src/ripple/protocol/Feature.h

@@ -366,24 +365,23 @@ foreachFeature(FeatureBitset bs, F&& f)

 extern uint256 const featureTickets;
 extern uint256 const featureOwnerPaysFee;
-extern uint256 const featureCompareFlowV1V2;
-extern uint256 const featurePayChan;
+extern uint256 const retiredPayChan;


I think it makes sense to group the unconditionalized amendments together at the bottom, with a comment explaining that they are amendments that have been activated and are live and the "pre-amendment" code has been removed.

scottschurr · 2020-03-13T18:53:27Z

@nbougalis, thanks for the review comments. I believe all your comments have been addressed. If I missed anything please let me know.

BTW, despite how Github displays them, the [FOLD] commits are actually placed immediately adjacent to their target in the commit stack. I dunno why Github shows them in time order not in commit order, but here we are...

pwang200

I just have a question, actually in previously merged code. Sorry I did not review that PR, nor the spec. About the warning in ApplicationImp::loadOldLedger(), when an really old ledger is replayed, the code logs a warning message and continues. What are the possible outcomes? I mean, will unrecognized data be ignored? Or will data be handled differently which could result in some different states, but no long term effect because later ledgers will be correctly replayed, or something else? Will the expected behavior be tested?

scottschurr · 2020-03-17T14:42:46Z

Hi @pwang200, thanks for the review.

About the warning in ApplicationImp::loadOldLedger(), when an really old ledger is replayed, the code logs a warning message and continues. What are the possible outcomes?

First, it's important to know that the warning is associated with replaying ledgers more than two years old. Replaying a ledger is a debugging technique that must be actively requested by someone using rippled. And ledger replay happens on a stand-alone rippled instance; it never happens on the live network.

With that background, let me get back to your original question. There are three possible outcomes. From most likely to least likely they are:

The ledger replays exactly like it did the first time it ran on the network. Most amendments affect bugs in corner cases or enable new features. Most transactions don't live in the corner cases and old (validated) transactions can't use features that were enabled after the transaction was validated.
All transactions in the replayed ledger are replayed, but the outcomes of some of the transactions are different from the outcomes when the ledger was originally produced. The difference in outcomes can be seen by comparing the metadata. This would happen because amendments that were not in force at the time the original transaction was validated are now unconditionally in force. So a transaction that was working in one of those amendment-affected corner cases would exhibit the new behavior.
The server could crash or show some other undesirable behavior. I'm only including this case to be complete. We don't currently know of any way to cause this, but it can't be ruled out. The server might crash because the old ledger contains some data configuration that is no longer allowed and is not present in current ledgers. The ledger is robust to most unexpected data configurations, but there is no way to guarantee that it can correctly handle all unexpected data configurations.

Even in the crashing case nothing bad happens to the network, since the replay is done on a stand-alone instance

If someone wants to understand why an old transaction originally played the way it did, and they run across case 2 or 3, then they would need to build an old version of the server from source and use it for the replay. That's also the advice that the warning provides.

pwang200

Thanks for the explanation of my question. It is very clear.

miguelportilla · 2020-03-25T15:54:42Z

src/ripple/protocol/Feature.h

@@ -369,7 +369,7 @@ extern uint256 const featurePayChan;
 extern uint256 const featureFlow;
 extern uint256 const featureCompareTakerFlowCross;
 extern uint256 const featureFlowCross;
-extern uint256 const featureCryptoConditions;
+extern uint256 const retiredCryptoConditions;


I noticed no rule enabled calls were removed. I assume the amendment was never proposed?

I'm not entirely sure about the history. Apparently the amendment was already unconditionalized before I got here. It looks like it had to do with SusPay, which was never enabled (https://xrpl.org/known-amendments.html#cryptoconditions). But the CryptoConditions amendment is live and on the ledger.

miguelportilla · 2020-03-25T15:56:30Z

src/ripple/protocol/impl/Feature.cpp

@@ -161,7 +161,7 @@ uint256 const featureFlow = *getRegisteredFeature("Flow");
 uint256 const featureCompareTakerFlowCross = *getRegisteredFeature("CompareTakerFlowCross");
 uint256 const featureFlowCross = *getRegisteredFeature("FlowCross");
 uint256 const retiredCryptoConditions = *getRegisteredFeature("CryptoConditions");
-uint256 const featureTickSize = *getRegisteredFeature("TickSize");
+uint256 const retiredTickSize = *getRegisteredFeature("TickSize");


I am guessing a retired tick size is full :)

miguelportilla · 2020-03-25T16:53:09Z

src/ripple/ledger/impl/PaymentSandbox.cpp

+    // 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).
+    auto adjustedAmt = std::min({amount, lastBal - delta, minBal});


Excellent comment!

miguelportilla · 2020-03-25T17:29:02Z

src/ripple/app/paths/impl/PaySteps.cpp

-
-std::pair<TER, Strand>
-toStrandV2 (
+toStrand (


Great reduction in unnecessary code!

miguelportilla · 2020-03-25T21:32:11Z

src/ripple/ledger/impl/View.cpp

+    if (strictOrder)
+        return view.dirAppend(dir, uLedgerIndex, fDescriber);
+    else
+        return view.dirInsert(dir, uLedgerIndex, fDescriber);


Consider eliminating the else statement

Ah never mind, I see you did that in a later commit.

miguelportilla · 2020-03-25T21:45:13Z

src/ripple/app/tx/impl/Escrow.cpp

        if (! ctx_.view().dirRemove(
-                keylet::ownerDir((*slep)[sfDestination]), page, k.key, true))
+                keylet::ownerDir((*slep)[sfDestination]), *optPage, k.key, true))


Nit: beyond 80 cols, consider removing one tab indentation.

miguelportilla · 2020-03-25T22:00:27Z

src/test/rpc/AccountSet_test.cpp

@@ -33,6 +33,8 @@ class AccountSet_test : public beast::unit_test::suite

    void testNullAccountSet()
    {
+        testcase ("No AccountSet");


Great job giving these names.

miguelportilla · 2020-03-25T22:08:13Z

src/test/rpc/AccountSet_test.cpp

        using namespace test::jtx;
-        auto runTest = [](Env&& env, double tr)
+
+        auto runTest = [this](double tr)


I think transferRate or rate might be an improvement over tr.

miguelportilla · 2020-03-25T22:13:06Z

src/test/rpc/AccountSet_test.cpp

+        // Test gateway with a variety of allowed transfer rates
+        for (double tr = 1.0; tr <= 2.0; tr += 0.03125)
+        {
+            runTest (tr);


Considering the runTest lambda is now only called from here, why not just move the code inside the loop?

miguelportilla · 2020-03-25T22:13:53Z

src/test/rpc/AccountSet_test.cpp

+        // operation of these legacy TransferRates.
+        //
+        // Two out-of-bound values are currently in the ledger (March 2020)
+        // They are 4.0 and 4.294967295.  So those are the values we test.


Excellent comments!

miguelportilla · 2020-03-25T22:15:49Z

src/test/rpc/AccountSet_test.cpp

+        //
+        // Two out-of-bound values are currently in the ledger (March 2020)
+        // They are 4.0 and 4.294967295.  So those are the values we test.
+        for (double tr : {4.0, 4.294967295})


Consider renaming tr to transferRate or rate.

miguelportilla · 2020-03-25T22:20:53Z

src/test/rpc/AccountSet_test.cpp

-            env.enableFeature(fix1201);
+            env.trust(USD(10), alice, bob);
+            env.close();
+            env(rate(gw, 2.0));


Should the rate be set here to tr?

We wish to use use transferRate, but transferRate is currently invalid. So we hack the invalid value into the ledger a few lines down. I'm adding the following comment to justify the 2.0:

// We'd like to use transferRate here, but the transactor // blocks transfer rates that large. So we use an acceptable // transfer rate here and later hack the ledger to replace // the acceptable value with an out-of-bounds value.

scottschurr

@miguelportilla, thanks for the great review. I think I've addressed all your comments. At your convenience let me know if I missed anything. Thanks.

scottschurr · 2020-03-31T22:59:39Z

src/test/rpc/AccountSet_test.cpp

-            env.enableFeature(fix1201);
+            env.trust(USD(10), alice, bob);
+            env.close();
+            env(rate(gw, 2.0));


We wish to use use transferRate, but transferRate is currently invalid. So we hack the invalid value into the ledger a few lines down. I'm adding the following comment to justify the 2.0:

// We'd like to use transferRate here, but the transactor // blocks transfer rates that large. So we use an acceptable // transfer rate here and later hack the ledger to replace // the acceptable value with an out-of-bounds value.

codecov-io · 2020-04-01T20:41:27Z

Codecov Report

Merging #3292 into develop will decrease coverage by 0.07%.
The diff coverage is 97.14%.

@@             Coverage Diff             @@
##           develop    #3292      +/-   ##
===========================================
- Coverage    70.61%   70.54%   -0.08%     
===========================================
  Files          676      676              
  Lines        54339    54112     -227     
===========================================
- Hits         38374    38173     -201     
+ Misses       15965    15939      -26

Impacted Files	Coverage Δ
src/ripple/app/consensus/RCLConsensus.cpp	`59.22% <ø> (+0.04%)`	⬆️
src/ripple/app/tx/impl/PayChan.cpp	`90.12% <ø> (+1.00%)`	⬆️
src/ripple/consensus/ConsensusParms.h	`100.00% <ø> (ø)`
src/ripple/protocol/Feature.h	`100.00% <ø> (ø)`
src/ripple/protocol/impl/Feature.cpp	`91.66% <ø> (ø)`
src/ripple/app/tx/impl/Escrow.cpp	`84.91% <90.90%> (-0.98%)`	⬇️
src/ripple/app/paths/impl/BookStep.cpp	`84.76% <100.00%> (-0.04%)`	⬇️
src/ripple/app/paths/impl/PaySteps.cpp	`80.49% <100.00%> (+5.61%)`	⬆️
src/ripple/app/tx/impl/ApplyContext.cpp	`100.00% <100.00%> (ø)`
src/ripple/app/tx/impl/SetAccount.cpp	`98.11% <100.00%> (-0.02%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f00f263...a90f5ee. Read the comment docs.

miguelportilla

👍

scottschurr · 2020-04-09T19:38:17Z

Squashed all fixes into their appropriate commits and rebased to 1.6.0-b1.

scottschurr requested review from pwang200 and miguelportilla March 9, 2020 17:52

scottschurr assigned pwang200 and miguelportilla Mar 9, 2020

nbougalis suggested changes Mar 12, 2020

View reviewed changes

scottschurr force-pushed the 2017amendments branch from 6bc0e72 to d7fc6f0 Compare March 13, 2020 18:46

nbougalis previously approved these changes Mar 13, 2020

View reviewed changes

pwang200 reviewed Mar 17, 2020

View reviewed changes

pwang200 previously approved these changes Mar 17, 2020

View reviewed changes

miguelportilla reviewed Mar 25, 2020

View reviewed changes

scottschurr dismissed stale reviews from pwang200 and nbougalis via a90f5ee April 1, 2020 18:06

scottschurr force-pushed the 2017amendments branch from d7fc6f0 to a90f5ee Compare April 1, 2020 18:06

scottschurr commented Apr 1, 2020

View reviewed changes

carlhua requested a review from miguelportilla April 6, 2020 14:10

carlhua requested review from pwang200 and nbougalis April 6, 2020 14:10

miguelportilla previously approved these changes Apr 6, 2020

View reviewed changes

scottschurr added 13 commits April 9, 2020 09:31

Remove unused featureCompareFlowV1V2

1661c84

Remove conditionals for featureCryptoConditions enabled 03Jan2017

6bb9dd2

Remove conditionals for featureTickSize enabled 21Feb2017

78d771a

Remove conditionals for featurePayChan enabled 31Mar2017

3e87086

Remove conditionals for fix1368 enabled 31Mar2017

e93a44f

Remove conditionals for featureEscrow enabled 31Mar2017

c828279

Remove conditionals for fix1373 enabled 07Jul2017

a6246b0

Remove conditionals for featureEnforceInvariants enabled 07Jul2017

46a76fb

Remove conditionals for featureSortedDirectories enabled 14Nov2017

323dbc7

Remove conditionals for fix1528 enabled 14Nov2017

8cf7c95

Remove conditionals for fix1523 enabled 14Nov2017

3d3b6d8

Remove conditionals for fix1512 enabled 14Nov2017

9f8d648

Remove conditionals for fix1201 enabled 14Nov2017

0d83223

scottschurr dismissed miguelportilla’s stale review via 0d83223 April 9, 2020 19:36

scottschurr force-pushed the 2017amendments branch from a90f5ee to 0d83223 Compare April 9, 2020 19:36

pwang200 approved these changes Apr 13, 2020

View reviewed changes

pwang200 added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 13, 2020

nbougalis mentioned this pull request Apr 15, 2020

Proposed 1.6.0-b2 #3357

Merged

nbougalis closed this in #3357 Apr 15, 2020

nbougalis merged commit 0d83223 into XRPLF:develop Apr 15, 2020

scottschurr deleted the 2017amendments branch April 16, 2020 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unconditionalize 2017 amendments #3292

Unconditionalize 2017 amendments #3292

scottschurr commented Mar 9, 2020

nbougalis left a comment

nbougalis Mar 12, 2020 •

edited

Loading

nbougalis Mar 12, 2020

nbougalis Mar 12, 2020 •

edited

Loading

scottschurr Mar 13, 2020

nbougalis Mar 12, 2020

scottschurr Mar 13, 2020

nbougalis Mar 12, 2020

scottschurr commented Mar 13, 2020

pwang200 left a comment •

edited

Loading

scottschurr commented Mar 17, 2020

pwang200 left a comment

miguelportilla Mar 25, 2020

scottschurr Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020 •

edited

Loading

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020 •

edited

Loading

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

miguelportilla Mar 25, 2020

scottschurr Mar 31, 2020

scottschurr left a comment

scottschurr Mar 31, 2020

codecov-io commented Apr 1, 2020

miguelportilla left a comment

scottschurr commented Apr 9, 2020

Unconditionalize 2017 amendments #3292

Unconditionalize 2017 amendments #3292

Conversation

scottschurr commented Mar 9, 2020

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented Mar 13, 2020

pwang200 left a comment • edited Loading

Choose a reason for hiding this comment

scottschurr commented Mar 17, 2020

pwang200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 1, 2020

Codecov Report

miguelportilla left a comment

Choose a reason for hiding this comment

scottschurr commented Apr 9, 2020

nbougalis Mar 12, 2020 •

edited

Loading

nbougalis Mar 12, 2020 •

edited

Loading

pwang200 left a comment •

edited

Loading

miguelportilla Mar 25, 2020 •

edited

Loading

miguelportilla Mar 25, 2020 •

edited

Loading