Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Transaction Invariant Checks #2054

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@mellery451
Copy link
Contributor

commented Mar 15, 2017

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.

@mellery451 mellery451 requested review from JoelKatz, nbougalis and ximinez Mar 15, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Mar 15, 2017

Codecov Report

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

Impacted file tree graph

@@             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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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.

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 16, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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.

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 16, 2017

Contributor

@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.

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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.

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Mar 16, 2017

Contributor

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

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 17, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 17, 2017

Contributor

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.

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 17, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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.

auto const txnAcct = view().peek(
keylet::account(ctx_.tx.getAccountID(sfAccount)));

std::uint32_t t_seq = ctx_.tx.getSequence ();

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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

};

private:
int64_t drops_;

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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

{
return true;
}
else

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

I'd ditch the else branch here:

if(-1*fee <= drops_ && drops_ <= 0)
    return true;
JLOG(j.error()) << "Invariant failed: XRP net change was "
    ...
@@ -168,6 +168,7 @@ class Transactor
private:
void setSeq ();
TER payFee ();
void claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const& removedOffers = {});

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 16, 2017

Member

Why not simply:

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

As for disabling it, two questions:

  1. 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.
  2. 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.

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 17, 2017

Author Contributor
  1. 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.
  2. 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.
* an account root should never be the target of a delete
*/
struct AccountRootsNotDeleted :
public InvariantCheckBase

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 16, 2017

Contributor

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

return true;
};

private:

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 16, 2017

Contributor

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

@sublimator

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

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

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 17, 2017

Contributor

Nit: remove final here as well

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2017

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.

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 17, 2017

Contributor

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.

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 17, 2017

Contributor

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?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 20, 2017

Author Contributor

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?

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

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 20, 2017

Contributor

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 mellery451 force-pushed the mellery451:invariants branch from 2468bd2 to b3faff1 Mar 20, 2017

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

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

@mellery451 mellery451 force-pushed the mellery451:invariants branch from b3faff1 to 808d155 Mar 21, 2017

@ximinez
Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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;

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 22, 2017

Contributor

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.

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

sounds right to me ;)

txnAcct->setFieldAmount (sfBalance, balance - fee);
txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);

if (terResult == tecOVERSIZE && (! removedOffers.empty()))

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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).

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

It can be safely removed.

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

added

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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())
        {
[...]

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

Good catch.

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 22, 2017

Contributor

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.

// 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.

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

Nit: Repetition of the word "default"

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

fixed

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

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 21, 2017

Contributor

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...]

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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?

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

@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.

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:

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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

}

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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.

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

will remove

txnAcct->setFieldAmount (sfBalance, balance - fee);
txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);

if (terResult == tecOVERSIZE && (! removedOffers.empty()))

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

It can be safely removed.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

Good catch.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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?

@mellery451 mellery451 force-pushed the mellery451:invariants branch from 3201749 to d5ec721 Mar 22, 2017

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Mar 22, 2017

Member

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

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 22, 2017

Author Contributor

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
Copy link
Contributor

left a comment

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

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

@mellery451 I had a bad feeling about that one.

@HowardHinnant

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

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

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2017

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

@mellery451 mellery451 force-pushed the mellery451:invariants branch 2 times, most recently from 48750ca to 5b9240c Mar 23, 2017

@mellery451 mellery451 force-pushed the mellery451:invariants branch from 5b9240c to bcf7b00 Apr 3, 2017

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

This comment has been minimized.

Copy link
@JoelKatz

JoelKatz Apr 12, 2017

Member

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.

This comment has been minimized.

Copy link
@mellery451

mellery451 Apr 12, 2017

Author Contributor

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?

Implement transaction invariant checks (RIPD-1425)
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.

@mellery451 mellery451 force-pushed the mellery451:invariants branch from 5d4e9f1 to c095edc Apr 17, 2017

@nbougalis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Merged as 026a249

@nbougalis nbougalis closed this Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.