Add Transaction Invariant Checks #2054

mellery451 · 2017-03-15T17:13:27Z

Add support for implementing invariant checks that are performed on the ApplyContext for transactions. Invariants are expected to never fail, thus they result in a tec if a failure is detected. If claiming a fee causes an invariant failure, then we tef instead.

Since there is some additional work for each transaction when this enabled, I compared execution time for our full unit-test suite. The times below are total time or running --unittest ten times on this branch and current develop. The conclusion is that this adds about 1 second to total --unittest execution time. I think that indicates fairly minimal impact, but certainly I'd be happy to do more testing.

INVARIANTS:
real    12m10.897s
user    8m58.236s
sys     0m17.008s

DEVELOP :
real    11m59.416s
user    8m49.016s
sys     0m16.576s

This change enables the amendment for all jtx::Env by default. This is a departure from our standard so far of keeping the Env free of all amendments by default - so this particular change needs some specific discussion.

codecov-io · 2017-03-15T17:49:54Z

Codecov Report

Merging #2054 into develop will increase coverage by 0.03%.
The diff coverage is 96.55%.

@@             Coverage Diff             @@
##           develop    #2054      +/-   ##
===========================================
+ Coverage    68.88%   68.91%   +0.03%     
===========================================
  Files          676      682       +6     
  Lines        49495    49696     +201     
===========================================
+ Hits         34095    34249     +154     
- Misses       15400    15447      +47

Impacted Files	Coverage Δ
src/ripple/app/tx/impl/ApplyContext.h	`100% <ø> (ø)`	⬆️
src/ripple/app/main/Amendments.cpp	`100% <ø> (ø)`	⬆️
src/ripple/app/tx/impl/Transactor.h	`100% <ø> (ø)`	⬆️
src/ripple/protocol/impl/TER.cpp	`93.75% <ø> (ø)`	⬆️
src/ripple/protocol/STLedgerEntry.h	`64.7% <ø> (ø)`	⬆️
src/ripple/protocol/TER.h	`100% <ø> (ø)`	⬆️
src/ripple/app/tx/impl/ApplyContext.cpp	`100% <100%> (+20%)`	⬆️
src/ripple/app/tx/impl/InvariantCheck.h	`100% <100%> (ø)`
src/ripple/protocol/impl/Feature.cpp	`100% <100%> (ø)`	⬆️
src/ripple/app/tx/impl/InvariantCheck.cpp	`100% <100%> (ø)`
... and 34 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 c6b6d82...c095edc. Read the comment docs.

nbougalis · 2017-03-16T00:57:56Z

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

+ApplyContext::checkInvariants(TER terResult)
+{
+    if (view_->rules().enabled(featureEnforceInvariants) &&
+        app.config().ENFORCE_INVARIANTS)


I understand that invariant checkers should never fail, and so setting ENFORCE_INVARIANTS to false with the amendment enabled should not result in a behavioral change. Still, I'm a bit concerned with having this option.

@JoelKatz can you comment on your thinking behind the config option. What's the use case for configurable on/off ?

nbougalis · 2017-03-16T01:00:20Z

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

+    {
+        if (accountDeleted_)
+        {
+            JLOG(j.error()) << "Invariant failed: an account-root was deleted" ;


I'd prefer JLOG(j.fatal()) for failing invariants; also nitpick: "account root".

nbougalis · 2017-03-16T01:03:43Z

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

+std::array <std::unique_ptr <InvariantCheckBase>, numChecks>
+getInvariantChecks()
+{
+    return std::array <std::unique_ptr <InvariantCheckBase>, numChecks> {{


My concern here is that we're doing a lot of memory allocation and deallocation. I suspect that of the performance overhead you've observed, a significant portion of that will trace back to memory management.

Is there a way we could avoid the dynamic allocations? Paging @HowardHinnant.

@nbougalis @mellery451 I made a patch that avoids dynamic allocations. See: seelabs@e5ef5be

I removed the base class, and moved the concrete classes into the header. I changed the invariants collection from an array to a tuple, and used the "Sean Parent" trick to iterate through the tuple.

That's the fancy way to do this. We could also just put the invariants in a struct and hard code calling each invariant as well. I am not against the simple way, but wanted to show my take of the "fancy" way.

Conceptually, I'm fine with either: the "Sean Parent" way is neat and very "modern C++". However, there's something to be said about simplicity. I'm curious how this actually looks like in assembly. I'll have to head over to godbolt.

Looks good to me. Sorry to be late to the party. I didn't get the page yesterday.

I like the approach by @seelabs and in my testing the performance is better. Would there be any reason to add a CRTP base just to force/document the interface for these classes (the two required methods). The compiler will enforce this without the base, but I didn't know if there is any advantage to having a templated base class. Either way, I will grab your commit and go from there.

A base class with pure virtuals should work fine for that - no need for CRTP. The concrete classes are already marked as final, and the we access them through the concrete type in the tuple. The compiler shouldn't need to go through the vtable (although I have not confirmed that).

Even if it did go through the vtable, I doubt we'd see a measurable overall performance difference.

Playing around with godbolt, gcc did skip the vtable when the concrete class was marked as final.

nbougalis · 2017-03-16T01:05:46Z

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

+ * @brief Invariant: corresponding modified ledger entries should match in type
+ *
+ */
+struct LedgerEntryTypesMatch :


Please mark these classes final. I know it's a small detail, but it could help the compiler devirtualize calls, which may have a performance impact.

nbougalis · 2017-03-16T01:19:37Z

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

+    auto const txnAcct = view().peek(
+        keylet::account(ctx_.tx.getAccountID(sfAccount)));
+
+    std::uint32_t t_seq = ctx_.tx.getSequence ();


Unnecessary local variable - just use the direct call in line 592?

nbougalis · 2017-03-16T01:28:45Z

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

+    };
+
+private:
+    int64_t drops_;


Should be std::int64_t and have #include <cstdint>

nbougalis · 2017-03-16T01:28:49Z

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

+struct XRPNotCreated :
+    public InvariantCheckBase
+{
+    XRPNotCreated() : drops_(0) {};


You could have std::int64_t drops_ = 0; and entirely ditch this constructor (no need to even =default it).

nbougalis · 2017-03-16T01:29:58Z

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

+        {
+            return true;
+        }
+        else


I'd ditch the else branch here:

if(-1*fee <= drops_ && drops_ <= 0) return true; JLOG(j.error()) << "Invariant failed: XRP net change was " ...

nbougalis · 2017-03-16T01:31:52Z

src/ripple/app/tx/impl/Transactor.h

@@ -168,6 +168,7 @@ class Transactor
 private:
    void setSeq ();
    TER payFee ();
+    void claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const& removedOffers = {});


Personal preference: I'd rather not have the default parameter here, and require it at the one single call site where it's missing.

nbougalis · 2017-03-16T03:01:22Z

src/test/jtx/Env.h

@@ -147,6 +149,11 @@ class Env
        memoize(Account::master);
        Pathfinder::initPathTable();
        construct(std::forward<Args>(args)...);


Why not simply:

// Enable the invariant enforcement amendment by default. construct(features(featureEnforceInvariants), std::forward<Args>(args)...);

As for disabling it, two questions:

Under what circumstances would we not want to have this? If the answer is "no reason" then we don't even need to mention this in a comment.

The technique you document on how to disable the invariant checking isn't what you, yourself, actually use: instead you use the ENFORCE_INVARIANTS configuration option. I think using that makes more sense.

I can't think of any reason not to have it on. Right now my one test that disables it was really just there to prove that disabling by config actually works.

good point - let's see what @JoelKatz says about the config use case and then I'll possibly change the comment and/or the test case.

seelabs · 2017-03-16T14:07:07Z

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

+ * an account root should never be the target of a delete
+ */
+struct AccountRootsNotDeleted :
+    public InvariantCheckBase


Note: structs default to public inheritance, so public is redundant here. No change needed.

seelabs · 2017-03-16T14:08:06Z

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

+        return true;
+    };
+
+private:


Nit: since this struct has a private member I'd probably make is a class.

sublimator · 2017-03-16T14:10:21Z

Wadda concept

seelabs · 2017-03-17T20:05:09Z

src/ripple/app/tx/impl/InvariantCheck.h

@@ -90,27 +89,26 @@ class XRPNotCreated final
 class AccountRootsNotDeleted final


Nit: remove final here as well

mellery451 · 2017-03-17T23:02:17Z

I pushed a change to the way these checks are documented, but I'm happy to roll that back if folks don't like it.

ximinez · 2017-03-17T22:54:34Z

src/ripple/app/tx/impl/InvariantCheck.h

+ * @return std:array of pointers to InvariantCheckBase subtypes.
+ */
+InvariantChecks
+getInvariantChecks()


Classic builds are failing to link because it's finding multiple definitions of this function. You need template<class = void> to tell the compiler that this is ok.

template<class = void> InvariantChecks getInvariantChecks()

PS. This is why the Travis build is failing.

ximinez · 2017-03-17T23:58:12Z

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

+ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence<Is...>)
+{
+    if (view_->rules().enabled(featureEnforceInvariants) &&
+        app.config().ENFORCE_INVARIANTS)


The more I look at it, the less I like ENFORCE_INVARIANTS, particularly in light of f5af8b0, which moved us a step toward removing Application/Config dependencies from a lot of calls.

If the ability to disable invariant checking at run-time is really necessary, how about defining a NoEnforceInvariants amendment which is NOT added to supportedAmendments, but can be added to [features] without needing any new config code?

Both @nbougalis and @ximinez have expressed concern over the configuration option (to enable/disable the checks). One suggestion from @JoelKatz is to make the checks mandatory for validators - so presumably we would just fail to start if you are configured as a validator and have it disabled. I think the use case for having the config option at all is to support development/debugging workflows - say, replaying txs with and without the checks.

Would this validator config prohibition alleviate the concerns here?

seelabs · 2017-03-20T12:49:05Z

src/ripple/app/tx/impl/InvariantCheck.h

@@ -168,6 +168,7 @@ using InvariantChecks = std::tuple<
 *
 * @see ripple::InvariantChecker_PROTOTYPE
 */
+template<class = void>


There are two ways I know to solve the duplicate symbol issue you encountered: 1) The template<class = void> as above; 2) inline.

We've been using the template<class = void> when we would prefer that a function not be inlined. For this function, it make sense to inline it. So I'd prefer inline in this case.

I think inline is more familiar to most C++ programmers, and we should prefer vanilla solutions unless we have a reason to reach for fancier idioms.

No change is needed for this case, but I'd rather not see template<class = void> replace inline in general.

mellery451 · 2017-03-21T15:42:06Z

current timing results for unittests (10 iterations)

INVARIANTS:
real    12m11.484s
user    9m6.316s
sys     0m16.468s

DEVELOP :
real    12m7.864s
user    8m59.568s
sys     0m16.320s

Also, doc preview: https://mellery451.github.io/pages/docs/rippled/index.html

ximinez

I like where this is going, but there are few significant problems below.

ximinez · 2017-03-20T21:33:01Z

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

+            drops_ -= (*before)[sfAmount].xrp().drops();
+            break;
+        default:
+            break;


Because the whole point here is to check for programming errors elsewhere, I'm thinking we want to explicitly check all the known LedgerEntryTypes here, and fail if we get an unknown type.

case ltDIR_NODE: case ltRIPPLE_STATE: [...etc...] break; default: // set failure flag, assert, or maybe even throw since this is a programming oversight error break;

If we want to add this check for known types of LE, I'd be inclined to create a separate check for this rather than wrapping it into this one. @JoelKatz do you think it's worth adding a LedgerEntryType-is-valid invariant check?

I'd combine this check into LedgerEntryTypesMatch - in addition to checking that types match, we can ensure that only known LETs are added.

This works for me. If catching it in LedgerEntryTypesMatch or similar isn't enough to think through the other invariant classes, then we've got bigger problems.

ximinez · 2017-03-20T21:35:39Z

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

+{
+    auto fee = tx.getFieldAmount(sfFee).xrp().drops();
+    if(-1*fee <= drops_ && drops_ <= 0)
+        return true;


I assume this is a range because a tx could claim less than the specified fee if the relevant account has a balance lower than that, and is thus zeroed out. Correct?

sounds right to me ;)

ximinez · 2017-03-21T22:29:29Z

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

+    txnAcct->setFieldAmount (sfBalance, balance - fee);
+    txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);
+
+    if (terResult == tecOVERSIZE && (! removedOffers.empty()))


Is the extra check for removedOffers really necessary? It doesn't save much overhead (calling the function, and starting the loop), and it requires knowledge of the implementation of removeUnfundedOffers (ie. knowing that it only loops over the vector).

that was existing code (refactored) - but I agree, I don't see much benefit in that empty check.

It can be safely removed.

ximinez · 2017-03-21T22:44:16Z

src/test/ledger/Invariants_test.cpp

+        BEAST_EXPECT(hasInvariants != env.app().config().features.end());
+        auto const& section = env.app().config().section("invariants");
+        BEAST_EXPECT(get<bool> (section, "enabled", true));
+    }


I would also like to see BEAST_EXPECT(env.current()->rules().enabled(featureEnforceInvariants));

ximinez · 2017-03-21T22:47:51Z

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

+            claimFee(fee, terResult, {});
+            //Check invariants *again* to ensure the fee claiming doesn't
+            //violate invariants.
+            terResult = ctx_.checkInvariants(terResult);


If checkInvariants fails the second time, it returns tefINVARIANT_FAILED. tef results are not supposed to do anything to the ledger, but this falls through to destroy XRP and apply the tx changes to the view. That means a tef result would get written to the ledger, which violates a different invariant. You could do:

if (didApply) { // Check invariants // if `tecINVARIANT_FAILED` not returned, we can proceed to apply the tx terResult = ctx_.checkInvariants(terResult); if (terResult == tecINVARIANT_FAILED) { // if invariants failed, claim a fee still claimFee(fee, terResult, {}); //Check invariants *again* to ensure the fee claiming doesn't //violate invariants. terResult = ctx_.checkInvariants(terResult); didApply = isTecClaim(terResult); } } if (didApply) { // Transaction succeeded fully or (retries are // not allowed and the transaction could claim a fee) if (!view().open()) { [...]

I had the mistaken impression that apply() would handle this - but I think you are correct about needing to guard against the tef - will fix.

Good catch.

Maybe apply() should check, too, perhaps with an assert, but all the checking has been done at this level, so we should keep doing that.

ximinez · 2017-03-21T22:50:15Z

src/test/jtx/Env.h

-        // amendment. If any Env needs to disable this amendment,
-        // suggest calling this after creating the Env:
-        //   env.app().config().features.erase(featureEnforceInvariants)
+        // amendment by default.


Nit: Repetition of the word "default"

ximinez · 2017-03-21T22:55:26Z

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

+        auto const& section = app.config().section("check_invariants");
+        if (get<bool> (section, "enabled", true))
+        {
+            auto checkers = getInvariantChecks();


Suggest flipping this if to improve readability and reduce indentation of the "real" body of the function.

if (!view_->rules().enabled(featureEnforceInvariants) || !get<bool>(app.config().section("check_invariants"), "enabled", true)) return terResult; [...the checks...]

I met you half-way and reduced the level indent by combining the conditionals - I think that helps.

This value isn't documented in the example config file. Should it be?

Also, if I'm reading this right, the syntax is:

[check_invariants] enabled=true

I can't think of any reason why [check_invariants] would get multiple entries, so making the syntax complex for complexity's sake doesn't sit well with me. At the very least, I'd suggest:

[check_invariants] true

But since we expect this option to only be tweaked for debugging purposes, why can't we add it as a command-line flag like --no-invariant-checking?

@JoelKatz would the command line option work for you or is the config option still preferred?

As for the existing implementation, it seems like having unlabeled values in a section is considered "legacy" behavior and that's why I avoided it. I don't have a strong opinion - I'll implement whatever our current config best practices are (if someone can clarify).

If we do stick with some form of config option, I'll add it to the example config.

nbougalis · 2017-03-20T18:55:55Z

src/test/jtx/Env.h

-        construct(std::forward<Args>(args)...);
+        // default enable the the invariant enforcement
+        // amendment. If any Env needs to disable this amendment,
+        // suggest calling this after creating the Env:


Perhaps update the comment to indicate that the "proper" way to unset this is to modify the Env config to set ENFORCE_INVARIANTS to false.

nbougalis · 2017-03-22T16:32:43Z

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

+            drops_ -= (*before)[sfAmount].xrp().drops();
+            break;
+        default:
+            break;


I'd combine this check into LedgerEntryTypesMatch - in addition to checking that types match, we can ensure that only known LETs are added.

nbougalis · 2017-03-22T16:34:29Z

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

+}
+
+bool
+AccountRootsNotDeleted::finalize(STTx const& /*tx*/, TER /*tec*/, beast::Journal const& j)


Not to nitpick, but I'd rather not have the commented-out names here (and other places) - they add very little, beyond what the types provide. Having (STTx const&, TER, beast::Journal const& j) should be fine. Unless others feel strongly about them staying in, I'd remove them.

will remove

nbougalis · 2017-03-22T16:38:06Z

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

+    txnAcct->setFieldAmount (sfBalance, balance - fee);
+    txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);
+
+    if (terResult == tecOVERSIZE && (! removedOffers.empty()))


It can be safely removed.

nbougalis · 2017-03-22T16:39:58Z

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

+            claimFee(fee, terResult, {});
+            //Check invariants *again* to ensure the fee claiming doesn't
+            //violate invariants.
+            terResult = ctx_.checkInvariants(terResult);


Good catch.

nbougalis · 2017-03-22T16:45:20Z

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

+        auto const& section = app.config().section("check_invariants");
+        if (get<bool> (section, "enabled", true))
+        {
+            auto checkers = getInvariantChecks();


This value isn't documented in the example config file. Should it be?

Also, if I'm reading this right, the syntax is:

[check_invariants] enabled=true

I can't think of any reason why [check_invariants] would get multiple entries, so making the syntax complex for complexity's sake doesn't sit well with me. At the very least, I'd suggest:

[check_invariants] true

But since we expect this option to only be tweaked for debugging purposes, why can't we add it as a command-line flag like --no-invariant-checking?

nbougalis · 2017-03-22T21:01:22Z

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

+        case ltLEDGER_HASHES:
+        case ltAMENDMENTS:
+        case ltFEE_SETTINGS:
+        case ltESCROW:


If we wanted to be extra nitpicky, we could check if the respective amendment is enabled here.

I don't believe we're passing rules or the feature set into the invariants right now, so we would have to add that if we wanted to do this. I'm happy to look into it more if there is sufficient value...

ximinez

Looks good now. What's going on with the Appveyor build failure?

mellery451 · 2017-03-23T20:26:02Z

@ximinez yep - I'm looking into it. Some (questionable) code that I added works fine on linux and silently fails at runtime on windows. The questionable code is f9ff3da#diff-a9a3e5216fb43a4420aca7e52f8a7167R32 which allows me to modify a private member under test. I might have to page @HowardHinnant on this one. It's dodgy anyhow, so might just remove it and leave that one check untested.

ximinez · 2017-03-23T20:26:51Z

@mellery451 I had a bad feeling about that one.

HowardHinnant · 2017-03-23T20:33:15Z

That's some pretty crazy code. If Windows doesn't like it, my vote is dump it. Another possibility (besides forgoing the test), might be to declare the test struct a friend (requiring a forward declaration of the test). That might be more trouble than it's worth.

ximinez · 2017-03-23T20:59:28Z

Testing the functionality would be worth it to me, and I wouldn't object to friending the test struct.

mellery451 · 2017-03-23T21:03:06Z

I'm game - I agree the test coverage is preferable. I'll submit that change.

JoelKatz · 2017-04-12T22:18:44Z

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

+                tefINVARIANT_FAILED :
+                tecINVARIANT_FAILED ;
+            JLOG(journal.error()) <<
+                "Transaction has failed one or more invariants";


This doesn't log enough information to let you do anything about the issue, particularly if you are only logging errors. We should log as much as we possibly can since this is only going to hit very rarely and only in cases we really need to investigate. Specifically, we should log as much information as we can about the ledger, the transaction, and the execution context.

would logging the JSON for the transaction be sufficient, or do we need more? Should I just append it to the current error log message or do we prefer to have a second message at a different log-level (say warn or info) that has the detailed info?

Add new functionality to enforce one or more sanity checks (invariants) on transactions. Add tests for each new invariant check. Allow for easily adding additional invariant checks in the future. Also Resolves ------------- - RIPD-1426 - RIPD-1427 - RIPD-1428 - RIPD-1429 - RIPD-1430 - RIPD-1431 - RIPD-1432 Release Notes ------------- Creates a new ammendment named "EnforceInvariants" which must be enabled in order for these new checks to run on each transaction.

nbougalis · 2017-04-19T20:31:04Z

Merged as 026a249

mellery451 requested review from JoelKatz, nbougalis and ximinez March 15, 2017 17:14

nbougalis reviewed Mar 16, 2017

View reviewed changes

seelabs reviewed Mar 16, 2017

View reviewed changes

seelabs reviewed Mar 17, 2017

View reviewed changes

ximinez requested changes Mar 17, 2017

View reviewed changes

ximinez reviewed Mar 17, 2017

View reviewed changes

seelabs reviewed Mar 20, 2017

View reviewed changes

ximinez requested changes Mar 21, 2017

View reviewed changes

nbougalis suggested changes Mar 22, 2017

View reviewed changes

nbougalis approved these changes Mar 22, 2017

View reviewed changes

ximinez approved these changes Mar 23, 2017

View reviewed changes

JoelKatz requested changes Apr 12, 2017

View reviewed changes

nbougalis closed this Apr 19, 2017

		@@ -90,27 +89,26 @@ class XRPNotCreated final
		class AccountRootsNotDeleted final

Add Transaction Invariant Checks #2054

Add Transaction Invariant Checks #2054

Conversation

mellery451 commented Mar 15, 2017

codecov-io commented Mar 15, 2017 • edited

Codecov Report

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

sublimator commented Mar 16, 2017 via email

Choose a reason for hiding this comment

mellery451 commented Mar 17, 2017

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

mellery451 commented Mar 21, 2017 • edited

ximinez 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

nbougalis Mar 22, 2017 • edited

Choose a reason for hiding this comment

mellery451 Mar 22, 2017 • edited

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

nbougalis Mar 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

mellery451 commented Mar 23, 2017

ximinez commented Mar 23, 2017

HowardHinnant commented Mar 23, 2017

ximinez commented Mar 23, 2017

mellery451 commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis commented Apr 19, 2017

codecov-io commented Mar 15, 2017 •

edited

mellery451 commented Mar 21, 2017 •

edited

nbougalis Mar 22, 2017 •

edited

mellery451 Mar 22, 2017 •

edited

nbougalis Mar 22, 2017 •

edited