Skip to content

Commit

Permalink
[FOLD] Fix handling of tefINVARIANT failure in tx:
Browse files Browse the repository at this point in the history 
Properly handle tef failure for invariants and do not apply the tx.
Add formatting and comment fixes. Add test assertion for feature
enabled test.
  • Loading branch information
@mellery451
mellery451 committed Mar 22, 2017
1 parent 808d155 commit 3201749
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 66 deletions.
67 changes: 33 additions & 34 deletions src/ripple/app/tx/impl/ApplyContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,42 +75,41 @@ template<std::size_t... Is>
TER
ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence<Is...>)
{
if (view_->rules().enabled(featureEnforceInvariants))
if (view_->rules().enabled(featureEnforceInvariants) &&
get<bool> (app.config().section("check_invariants"), "enabled", true))
{
auto const& section = app.config().section("check_invariants");
if (get<bool> (section, "enabled", true))
{
auto checkers = getInvariantChecks();

// call each check's per-entry method
visit (
[&checkers](
uint256 const& index,
bool isDelete,
std::shared_ptr <SLE const> const& before,
std::shared_ptr <SLE const> const& after)
{
// Sean Parent for_each_argument trick
(void)std::array<int, sizeof...(Is)>{
{((std::get<Is>(checkers).visitEntry(index, isDelete, before, after)), 0)...}
};
});

// Sean Parent for_each_argument trick
// (a fold expression with `&&` would be really nice here when we move to C++-17)
std::array<bool, sizeof...(Is)> finalizeCheck{{std::get<Is>(checkers).finalize(tx, terResult, journal)...}};
// then call each check's finalizer to see that it passes
if (!std::all_of(
finalizeCheck.cbegin(),
finalizeCheck.cend(),
[](auto const& b) { return b; }))
auto checkers = getInvariantChecks();

// call each check's per-entry method
visit (
[&checkers](
uint256 const& index,
bool isDelete,
std::shared_ptr <SLE const> const& before,
std::shared_ptr <SLE const> const& after)
{
terResult = (terResult == tecINVARIANT_FAILED) ?
tefINVARIANT_FAILED :
tecINVARIANT_FAILED ;
JLOG(journal.error()) <<
"Transaction has failed one or more invariants";
}
// Sean Parent for_each_argument trick
(void)std::array<int, sizeof...(Is)>{
{((std::get<Is>(checkers).
visitEntry(index, isDelete, before, after)), 0)...}
};
});

// Sean Parent for_each_argument trick
// (a fold expression with `&&` would be really nice here when we move
// to C++-17)
std::array<bool, sizeof...(Is)> finalizers {{
std::get<Is>(checkers).finalize(tx, terResult, journal)...}};

// call each check's finalizer to see that it passes
if (! std::all_of( finalizers.cbegin(), finalizers.cend(),
[](auto const& b) { return b; }))
{
terResult = (terResult == tecINVARIANT_FAILED) ?
tefINVARIANT_FAILED :
tecINVARIANT_FAILED ;
JLOG(journal.error()) <<
"Transaction has failed one or more invariants";
}
}

Expand Down
49 changes: 40 additions & 9 deletions src/ripple/app/tx/impl/InvariantCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace ripple {

void
XRPNotCreated::visitEntry(
uint256 const& /*index*/,
uint256 const&,
bool isDelete,
std::shared_ptr <SLE const> const& before,
std::shared_ptr <SLE const> const& after)
Expand Down Expand Up @@ -84,7 +84,7 @@ XRPNotCreated::finalize(STTx const& tx, TER /*tec*/, beast::Journal const& j)

void
AccountRootsNotDeleted::visitEntry(
uint256 const& /*index*/,
uint256 const&,
bool isDelete,
std::shared_ptr <SLE const> const& before,
std::shared_ptr <SLE const> const& /*after*/)
Expand All @@ -94,35 +94,66 @@ AccountRootsNotDeleted::visitEntry(
}

bool
AccountRootsNotDeleted::finalize(STTx const& /*tx*/, TER /*tec*/, beast::Journal const& j)
AccountRootsNotDeleted::finalize(STTx const&, TER, beast::Journal const& j)
{
if (! accountDeleted_)
return true;

JLOG(j.fatal()) << "Invariant failed: an account root was deleted";
return false;
};
}

//------------------------------------------------------------------------------

void
LedgerEntryTypesMatch::visitEntry(
uint256 const& /*index*/,
bool /*isDelete*/,
uint256 const&,
bool,
std::shared_ptr <SLE const> const& before,
std::shared_ptr <SLE const> const& after)
{
if (before && after && before->getType() != after->getType())
typeMismatch_ = true;

if (after)
{
switch (after->getType())
{
case ltACCOUNT_ROOT:
case ltDIR_NODE:
case ltRIPPLE_STATE:
case ltTICKET:
case ltSIGNER_LIST:
case ltOFFER:
case ltLEDGER_HASHES:
case ltAMENDMENTS:
case ltFEE_SETTINGS:
case ltESCROW:
case ltPAYCHAN:
break;
default:
invalidTypeAdded_ = true;
break;
}
}
}

bool
LedgerEntryTypesMatch::finalize(STTx const& /*tx*/, TER /*tec*/, beast::Journal const& j)
LedgerEntryTypesMatch::finalize(STTx const&, TER, beast::Journal const& j)
{
if (! typeMismatch_)
if ((! typeMismatch_) && (! invalidTypeAdded_))
return true;

JLOG(j.fatal()) << "Invariant failed: ledger entry type mismatch";
if (typeMismatch_)
{
JLOG(j.fatal()) << "Invariant failed: ledger entry type mismatch";
}

if (invalidTypeAdded_)
{
JLOG(j.fatal()) << "Invariant failed: invalid ledger entry type added";
}

return false;
}

Expand Down
34 changes: 18 additions & 16 deletions src/ripple/app/tx/impl/InvariantCheck.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ class XRPNotCreated

void
visitEntry(
uint256 const& /*index*/,
bool /*isDelete*/,
std::shared_ptr<SLE const> const& /*before*/,
std::shared_ptr<SLE const> const& /*after*/);
uint256 const&,
bool,
std::shared_ptr<SLE const> const&,
std::shared_ptr<SLE const> const&);

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

/**
Expand All @@ -122,34 +122,36 @@ class AccountRootsNotDeleted

void
visitEntry(
uint256 const& /*index*/,
bool /*isDelete*/,
std::shared_ptr<SLE const> const& /*before*/,
std::shared_ptr<SLE const> const& /*after*/);
uint256 const&,
bool,
std::shared_ptr<SLE const> const&,
std::shared_ptr<SLE const> const&);

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

/**
* @brief Invariant: corresponding modified ledger entries should match in type
* @brief Invariant: corresponding modified ledger entries should match in type and
* added entries should be a valid type.
*
*/
class LedgerEntryTypesMatch
{
bool typeMismatch_ = false;
bool invalidTypeAdded_ = false;

public:

void
visitEntry(
uint256 const& /*index*/,
bool /*isDelete*/,
std::shared_ptr<SLE const> const& /*before*/,
std::shared_ptr<SLE const> const& /*after*/);
uint256 const&,
bool,
std::shared_ptr<SLE const> const&,
std::shared_ptr<SLE const> const&);

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

// additional invariant checks can be declared above and then added to this
Expand Down
15 changes: 10 additions & 5 deletions src/ripple/app/tx/impl/Transactor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ Transactor::claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const&
txnAcct->setFieldAmount (sfBalance, balance - fee);
txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);

if (terResult == tecOVERSIZE && (! removedOffers.empty()))
if (terResult == tecOVERSIZE)
removeUnfundedOffers (view(), removedOffers, ctx_.app.journal ("View"));

view().update (txnAcct);
Expand Down Expand Up @@ -685,10 +685,6 @@ Transactor::operator()()
claimFee(fee, terResult, removedOffers);
didApply = true;
}
else if (!didApply)
{
JLOG(j_.debug()) << "Not applying transaction " << txID;
}

if (didApply)
{
Expand All @@ -702,8 +698,12 @@ Transactor::operator()()
//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)

Expand All @@ -727,6 +727,11 @@ Transactor::operator()()
// since we called apply(), it is not okay to look
// at view() past this point.
}
else
{
JLOG(j_.debug()) << "Not applying transaction " << txID;
}


JLOG(j_.trace()) <<
"apply: " << transToken(terResult) <<
Expand Down
3 changes: 1 addition & 2 deletions src/test/jtx/Env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@ class Env
{
memoize(Account::master);
Pathfinder::initPathTable();
// default enable the the invariant enforcement
// amendment by default.
// enable the the invariant enforcement amendment by default.
construct(
features(featureEnforceInvariants),
std::forward<Args>(args)...);
Expand Down
4 changes: 4 additions & 0 deletions src/test/ledger/Invariants_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,13 @@ class Invariants_test : public beast::unit_test::suite
using namespace test::jtx;
testcase ("feature enabled");
Env env {*this};

auto hasInvariants =
env.app().config().features.find (featureEnforceInvariants);
BEAST_EXPECT(hasInvariants != env.app().config().features.end());

BEAST_EXPECT(env.current()->rules().enabled(featureEnforceInvariants));

auto const& section = env.app().config().section("invariants");
BEAST_EXPECT(get<bool> (section, "enabled", true));
}
Expand Down

0 comments on commit 3201749

Please sign in to comment.