Ripd1373 #2015

seelabs · 2017-02-13T19:17:49Z

New rules for payment paths:

Sanity check on newly created strands
Better loop detection
Better tests (test every combination of path element pairs)
Disallow any root issuer (even for xrp)
Disallow compount element typs in path
Issue was not reset when currency was XRP
Add amendment

seelabs · 2017-02-13T19:18:26Z

The "Fix limiting step re-execute bug" does not need to be reviewed, it's a separate PR.

scottschurr

I found a very few nits, most of which you can take or leave. Looks very nice for the most part.

There is one change, however, that makes the embedded programmer lurking under my skin squirm. When you turn all of those unit tests into variadic templates just so you can pass in a variable number of features you're making the code generator work overtime. And all of the features are the same type! I hope we don't need to take up that much address space with almost identical unit tests.

Perhaps the same goal could be accomplished by passing in a vector<uint256> or something similar? Please consider that. Thanks.

scottschurr · 2017-02-14T16:44:44Z

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

+        !isConsistent(deliver) || (sendMaxIssue && !isConsistent(*sendMaxIssue)))
+        return {temBAD_PATH, Strand{}};
+
+    for (auto const& p : path)


The tiniest of nits: an STPath consists of ordered STPathElements? To me p is not evocative of STPathElement. Perhaps e or elem?

scottschurr · 2017-02-14T16:54:37Z

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

+    {
+        auto const t = p.getNodeType();
+
+        if (t > STPathElement::Type::typeAll || !t)


Just a thought, would you get a better check if you replace t > STPathElement::Type::typeAll with (t & ~STPathElement::Type::typeAll) != 0?

scottschurr · 2017-02-14T17:49:57Z

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

+        return pe.getNodeType () & STPathElement::typeCurrency;
+    };
+
+    std::vector<STPathElement> normPath;


I vaguely recall there's an upper limit on Path length (although I'm not seeing evidence of that here)? If there is an upper limit you could replace this std::vector with a boost::container::static_vector and avoid a trip to the heap. But you'd also have to add a check that path.size() was in range. Just a thought.

That limit lives in Payment.h and is checked in the payment tx preclaim. I could do this in two ways:

Make that limit public and add a dependency between PaySteps.cpp and Payment.h. I don't like dependencies between lower-level routines and higher level.

I could set a largish limit - say 32 - and a comment that if the number of path steps are increased, that should be increased as well. That seems fragile.

While I love getting rid of memory allocations, I don't think it's a huge deal in this case. It's done once at the start of the payment. I'd probably keep it as-is.

scottschurr · 2017-02-15T00:54:57Z

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

+
+    Issue curIssue = [&]
+    {
+        auto& currency =


auto const&?

scottschurr · 2017-02-15T00:55:37Z

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

+            normPath.emplace_back(sendMaxIssue->account, boost::none, boost::none);
+        }
+
+        for (auto& i : path)


auto const&?

scottschurr · 2017-02-15T00:58:35Z

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

+            if (accts.first != curAccount)
+                return false;
+
+            if (auto b = s->bookStepBook())


auto const?

scottschurr · 2017-02-15T02:05:23Z

src/test/ledger/BookDirs_test.cpp

@@ -94,6 +96,7 @@ struct BookDirs_test : public beast::unit_test::suite
    void run() override
    {
        test_bookdir();
+        test_bookdir(featureFlow, featureToStrandV2);


This one doesn't need test_bookdir(featureFlow)? If not it might be a good idea to include a comment why.

For certain simpler tests, I only wanted to test the pre-flow version and the latest version of flow. It wasn't necessary to test every combination of amendments and switches.

scottschurr · 2017-02-15T02:08:21Z

src/test/ledger/PaymentSandbox_test.cpp

+            testReserve(fs...);
+        };
+        testAll();
+        testAll(featureFlow, featureToStrandV2);


This one doesn't need test_bookdir(featureFlow)? If not it might be a good idea to include a comment why.

Same reason as above. I only needed to test the pre-flow and the latest flow.

scottschurr · 2017-02-15T02:10:30Z

src/test/rpc/GatewayBalances_test.cpp

@@ -153,6 +155,7 @@ class GatewayBalances_test : public beast::unit_test::suite
    run() override
    {
        testGWB();
+        testGWB(featureFlow, featureToStrandV2);


This one doesn't need test_bookdir(featureFlow)? If not it might be a good idea to include a comment why.

scottschurr · 2017-02-15T02:11:38Z

src/test/rpc/NoRipple_test.cpp

-        testDefaultRipple();
+
+        auto withFeatsTests = [this](auto&&... fs) {
+            testNegativeBalance();


I believe these three calls are missing the (fs...), so they perform exactly the same tests three times.

Yep, good eye. Fixed.

seelabs · 2017-02-15T16:35:48Z

@scottschurr I added two commits to address your comments. The first contains the misc fixes you suggested. The second replaces uses of variadic templates with std::initializer_list to address your concerns about link size.

scottschurr

Changes look good. I noticed one place where a bit test may have accidentally been omitted, but I'll leave that to your judgment. So I'm giving this a preemptive 👍

scottschurr · 2017-02-15T17:03:35Z

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


-        if (t > STPathElement::Type::typeAll || !t)
+        if (t & ~STPathElement::Type::typeAll)


Previously there was a check here for t having no bits set. That check seemed useful and is not covered by the new check. Was the no-bits check lost intentionally?

scottschurr · 2017-02-17T18:54:14Z

Visual Studio can't handle the C++14 constexpr expressions in PayStrand_test.cpp.

nbougalis · 2017-03-17T03:15:48Z

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

+    {
+        auto const t = pe.getNodeType();
+
+        if ((t & ~STPathElement::Type::typeAll) || !t)


No need for the Type:: since Type isn't an enum class. Feel free to change that to make it one, but then I'd ditch the type prefix.

nbougalis · 2017-03-17T03:17:35Z

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

+            ownerPaysTransferFee, seenDirectIssues, seenBookOuts, j};
+    };
+
+    for (int i = 0; i < normPath.size () - 1; ++i)


So, at first I was going to say use std::size_t to avoid type-conversion warnings. And I think you should do that, but then I got to thinking: what guarantees do we have about normPath.size()?

Certainly it can't be zero - it has to be at least one. This means that using std::size_t is safe. But is it guaranteed to be at least 2? It better be, because we access the i+1 path element inside this loop.

fixed size_t issue.

Note: If size were one, we'd still be OK (the loop would not run at all). If size were zero, we'd have a bug, as the size()-1 would wrap around.

nbougalis · 2017-03-17T04:50:00Z

src/test/app/CrossingLimits_test.cpp

@@ -103,12 +105,12 @@ class CrossingLimits_test : public beast::unit_test::suite
                balance("carol", USD(150)),
                balance("bob", USD(0)), owners ("bob", 1)));
    }
-
+    


Micronit: whitespace

seelabs · 2017-03-17T14:17:03Z

Pushed commit to address latest round of feedback

scottschurr · 2017-03-17T16:00:33Z

Latest changes look good to me. Still 👍 assuming conflicts are resolved.

nbougalis

👍 pending rebasing

* Sanity check on newly created strands * Better loop detection * Better tests (test every combination of path element pairs) * Disallow any root issuer (even for xrp) * Disallow compount element typs in path * Issue was not reset when currency was XRP * Add amendment

seelabs · 2017-03-20T18:29:11Z

rebased onto 0.60.0 Will squash after ci runs

scottschurr · 2017-03-20T19:38:39Z

src/test/app/PayStrand_test.cpp

+      last };
+
+    std::uint16_t state_ = 0;
+    static_assert(static_cast<size_t>(SB::last) <= sizeof(state_) * 8, "");


Appveyor suggests Visual Studio can't handle this static_assert. This might work for all our compilers...

static_assert (static_cast<size_t>(SB::last) <= sizeof(decltype (state_)) * 8, "");

Thanks. I pushed a commit with the suggested fix.

codecov-io · 2017-03-20T20:24:14Z

Codecov Report

Merging #2015 into develop will decrease coverage by 0.05%.
The diff coverage is 75.95%.

@@             Coverage Diff             @@
##           develop    #2015      +/-   ##
===========================================
- Coverage     67.7%   67.64%   -0.06%     
===========================================
  Files          680      680              
  Lines        49219    49399     +180     
===========================================
+ Hits         33323    33416      +93     
- Misses       15896    15983      +87

Impacted Files	Coverage Δ
src/ripple/app/main/Amendments.cpp	`100% <ø> (ø)`	⬆️
src/ripple/app/paths/impl/XRPEndpointStep.cpp	`71.87% <100%> (-4.22%)`	⬇️
src/ripple/app/paths/impl/Steps.h	`82.97% <100%> (+5.2%)`	⬆️
src/ripple/protocol/impl/Feature.cpp	`100% <100%> (ø)`	⬆️
src/ripple/protocol/STPathSet.h	`98.36% <100%> (ø)`	⬆️
src/ripple/app/paths/impl/DirectStep.cpp	`71.18% <100%> (-8.34%)`	⬇️
src/ripple/app/paths/impl/BookStep.cpp	`79.79% <50%> (-5.97%)`	⬇️
src/ripple/app/paths/impl/PaySteps.cpp	`71.84% <75%> (ø)`	⬆️
... 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 0df1b09...591eb70. Read the comment docs.

scottschurr · 2017-03-21T16:28:46Z

Squashed and merged to develop as 846723d

seelabs requested review from nbougalis and scottschurr February 13, 2017 19:20

seelabs assigned nbougalis and scottschurr Feb 13, 2017

seelabs added the Tx Change label Feb 13, 2017

scottschurr reviewed Feb 15, 2017

View reviewed changes

scottschurr approved these changes Feb 15, 2017

View reviewed changes

nbougalis reviewed Mar 17, 2017

View reviewed changes

nbougalis approved these changes Mar 17, 2017

View reviewed changes

nbougalis added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 17, 2017

scottschurr mentioned this pull request Mar 17, 2017

Use payment flow code for offer crossing (RIPD-1094): #1884

Closed

seelabs added 7 commits March 20, 2017 14:07

[fold] update old payment tests to use new features

a636dd1

[fold] Address PR feedback

ab39f61

[fold] Use initializer_list instead of variatic templates

7298e43

[fold] Fix check for no attributes

b140285

[fold] Address review feedback

1da4b76

[fold] Fixup after rebase

ee4fb02

seelabs force-pushed the ripd1373 branch from 4d67209 to ee4fb02 Compare March 20, 2017 18:28

scottschurr reviewed Mar 20, 2017

View reviewed changes

[fold] Workaround for VS static assert

591eb70

scottschurr mentioned this pull request Mar 21, 2017

Develop next #2059

Merged

scottschurr closed this Mar 21, 2017

seelabs deleted the ripd1373 branch July 13, 2017 21:12


		if (t > STPathElement::Type::typeAll \|\| !t)
		if (t & ~STPathElement::Type::typeAll)

Ripd1373 #2015

Ripd1373 #2015

Conversation

seelabs commented Feb 13, 2017

seelabs commented Feb 13, 2017

scottschurr 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Feb 15, 2017

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Mar 17, 2017

scottschurr commented Mar 17, 2017

nbougalis left a comment

Choose a reason for hiding this comment

seelabs commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 20, 2017

Codecov Report

scottschurr commented Mar 21, 2017

seelabs Mar 17, 2017 •

edited

Loading