Skip to content

Commit

Permalink
Merge pull request stellar#2740 from MonsieurNicolas/fixReplayError
Browse files Browse the repository at this point in the history 
 Give a sensible default value for operations error code

Reviewed-by: jonjove
  • Loading branch information
@latobarita
latobarita authored Oct 1, 2020
2 parents db6b220 + 13f4697 commit 5b2989d
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 44 deletions.
61 changes: 54 additions & 7 deletions src/test/TxTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,15 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
TransactionResult checkResult;
TransactionResultCode code;
AccountEntry srcAccountBefore;

auto checkedTx = TransactionFrameBase::makeTransactionFromWire(
app.getNetworkID(), tx->getEnvelope());
bool checkedTxApplyRes = false;
{
LedgerTxn ltxFeeProc(ltx);
check = tx->checkValid(ltxFeeProc, 0, 0, 0);
checkResult = tx->getResult();
// use checkedTx here for validity check as to keep tx untouched
check = checkedTx->checkValid(ltxFeeProc, 0, 0, 0);
checkResult = checkedTx->getResult();
REQUIRE((!check || checkResult.result.code() == txSUCCESS));

// now, check what happens when simulating what happens during a ledger
Expand All @@ -135,6 +140,28 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
// the same way)
// * a valid tx can fail later
code = checkResult.result.code();

// compute the same changes in parallel on checkedTx
{
LedgerTxn ltxCleanTx(ltxFeeProc);
auto baseFee = ltxCleanTx.loadHeader().current().baseFee;
if (code != txNO_ACCOUNT)
{
checkedTx->processFeeSeqNum(ltxCleanTx, baseFee);
}
// else, leave feeCharged as per checkValid
try
{
TransactionMeta cleanTm(2);
checkedTxApplyRes = checkedTx->apply(app, ltxCleanTx, cleanTm);
}
catch (...)
{
checkedTx->getResult().result.code(txINTERNAL_ERROR);
}
// do not commit this one
}

if (code != txNO_ACCOUNT)
{
srcAccountBefore = loadAccount(ltxFeeProc, tx->getSourceID(), true)
Expand All @@ -150,6 +177,8 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
{
REQUIRE(checkResult.feeCharged >= tx->getResult().feeCharged);
}
// this is to ignore potential changes in feeCharged
// as `checkValid` returns an estimate
checkResult.feeCharged = tx->getResult().feeCharged;

// verify that the fee got processed
Expand Down Expand Up @@ -178,6 +207,12 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
}
REQUIRE(currAcc == prevAcc);
}
else
{
// this basically makes it that we ignore that field when we
// don't have an account
tx->getResult().feeCharged = checkResult.feeCharged;
}
ltxFeeProc.commit();
}

Expand All @@ -193,6 +228,12 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
{
tx->getResult().result.code(txINTERNAL_ERROR);
}

// check that both tx and cleanTx behave the same
REQUIRE(res == checkedTxApplyRes);
REQUIRE(tx->getEnvelope() == checkedTx->getEnvelope());
REQUIRE(tx->getResult() == checkedTx->getResult());

REQUIRE((!res || tx->getResultCode() == txSUCCESS));

if (!res || tx->getResultCode() != txSUCCESS)
Expand Down Expand Up @@ -321,12 +362,15 @@ validateTxResults(TransactionFramePtr const& tx, Application& app,
TransactionResult const& applyResult)
{
auto shouldValidateOk = validationResult.code == txSUCCESS;

auto checkedTx = TransactionFrameBase::makeTransactionFromWire(
app.getNetworkID(), tx->getEnvelope());
{
LedgerTxn ltx(app.getLedgerTxnRoot());
REQUIRE(tx->checkValid(ltx, 0, 0, 0) == shouldValidateOk);
REQUIRE(checkedTx->checkValid(ltx, 0, 0, 0) == shouldValidateOk);
}
REQUIRE(tx->getResult().result.code() == validationResult.code);
REQUIRE(tx->getResult().feeCharged == validationResult.fee);
REQUIRE(checkedTx->getResult().result.code() == validationResult.code);
REQUIRE(checkedTx->getResult().feeCharged == validationResult.fee);

// do not try to apply if checkValid returned false
if (!shouldValidateOk)
Expand All @@ -352,7 +396,7 @@ validateTxResults(TransactionFramePtr const& tx, Application& app,

TxSetResultMeta
closeLedgerOn(Application& app, uint32 ledgerSeq, int day, int month, int year,
std::vector<TransactionFrameBasePtr> const& txs)
std::vector<TransactionFrameBasePtr> const& txs, bool skipValid)
{
auto txSet = std::make_shared<TxSetFrame>(
app.getLedgerManager().getLastClosedLedgerHeader().hash);
Expand All @@ -363,7 +407,10 @@ closeLedgerOn(Application& app, uint32 ledgerSeq, int day, int month, int year,
}

txSet->sortForHash();
REQUIRE(txSet->checkValid(app, 0, 0));
if (!skipValid)
{
REQUIRE(txSet->checkValid(app, 0, 0));
}

StellarValue sv(txSet->getContentsHash(), getTestDate(day, month, year),
emptyUpgradeSteps, STELLAR_VALUE_BASIC);
Expand Down
3 changes: 2 additions & 1 deletion src/test/TxTests.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ void validateTxResults(TransactionFramePtr const& tx, Application& app,

TxSetResultMeta
closeLedgerOn(Application& app, uint32 ledgerSeq, int day, int month, int year,
std::vector<TransactionFrameBasePtr> const& txs = {});
std::vector<TransactionFrameBasePtr> const& txs = {},
bool skipValid = false);

SecretKey getRoot(Hash const& networkID);

Expand Down
11 changes: 9 additions & 2 deletions src/transactions/OperationFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ OperationFrame::OperationFrame(Operation const& op, OperationResult& res,
TransactionFrame& parentTx)
: mOperation(op), mParentTx(parentTx), mResult(res)
{
resetResultSuccess();
}

bool
Expand Down Expand Up @@ -229,8 +230,7 @@ OperationFrame::checkValid(SignatureChecker& signatureChecker,
}
}

mResult.code(opINNER);
mResult.tr().type(mOperation.body.type());
resetResultSuccess();

return doCheckValid(ledgerVersion);
}
Expand All @@ -243,6 +243,13 @@ OperationFrame::loadSourceAccount(AbstractLedgerTxn& ltx,
return mParentTx.loadAccount(ltx, header, getSourceID());
}

void
OperationFrame::resetResultSuccess()
{
mResult.code(opINNER);
mResult.tr().type(mOperation.body.type());
}

void
OperationFrame::insertLedgerKeysToPrefetch(
std::unordered_set<LedgerKey>& keys) const
Expand Down
4 changes: 4 additions & 0 deletions src/transactions/OperationFrame.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ class OperationFrame
LedgerTxnEntry loadSourceAccount(AbstractLedgerTxn& ltx,
LedgerTxnHeader const& header);

// given an operation, gives a default value representing "success" for the
// result
void resetResultSuccess();

public:
static std::shared_ptr<OperationFrame>
makeHelper(Operation const& op, OperationResult& res,
Expand Down
121 changes: 87 additions & 34 deletions src/transactions/test/TxEnvelopeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ TEST_CASE("txenvelope", "[tx][envelope]")
{
std::string name;
bool autoRemove;
std::function<SignerKey(TransactionFrame const&)> createSigner;
std::function<SignerKey(TransactionFrame&)> createSigner;
std::function<void(TransactionFrame&)> sign;
};

Expand All @@ -419,17 +419,17 @@ TEST_CASE("txenvelope", "[tx][envelope]")
0, 0, 'G', 'H', 'I', 'J', 'K', 'L'};
auto alternatives = std::vector<AltSignature>{
AltSignature{"hash tx", true,
[](TransactionFrame const& tx) {
[](TransactionFrame& tx) {
tx.clearCached();
return SignerKeyUtils::preAuthTxKey(tx);
},
[](TransactionFrame&) {}},
AltSignature{"hash x", false,
[x](TransactionFrame const&) {
return SignerKeyUtils::hashXKey(x);
},
[x](TransactionFrame& tx) {
tx.addSignature(SignatureUtils::signHashX(x));
}}};
AltSignature{
"hash x", false,
[x](TransactionFrame&) { return SignerKeyUtils::hashXKey(x); },
[x](TransactionFrame& tx) {
tx.addSignature(SignatureUtils::signHashX(x));
}}};

for (auto const& alternative : alternatives)
{
Expand Down Expand Up @@ -990,10 +990,10 @@ TEST_CASE("txenvelope", "[tx][envelope]")
tx = a1.tx({root.op(payment(a1, 100))});
getSignatures(tx).clear();

setSeqNum(tx, tx->getSeqNum() + 1);
// add signer twice.
tx->addSignature(s1);
tx->addSignature(s1);
setSeqNum(tx, tx->getSeqNum() + 1);
a1.setSequenceNumber(a1.getLastSequenceNumber() -
1);

Expand Down Expand Up @@ -1143,8 +1143,8 @@ TEST_CASE("txenvelope", "[tx][envelope]")
auto setup = [&]() {
tx = a1.tx({payment(root, 1000)});
getSignatures(tx).clear();
tx->addSignature(s1);
setSeqNum(tx, tx->getSeqNum() + 1);
tx->addSignature(s1);
a1.setSequenceNumber(a1.getLastSequenceNumber() -
1);

Expand Down Expand Up @@ -1268,8 +1268,9 @@ TEST_CASE("txenvelope", "[tx][envelope]")
for_versions_from(3, *app, [&] {
auto tx = a1.tx({payment(root, 1000)});
getSignatures(tx).clear();
tx->addSignature(s1);

setSeqNum(tx, tx->getSeqNum() + 1);
tx->addSignature(s1);
a1.setSequenceNumber(a1.getLastSequenceNumber() - 1);

SignerKey sk = SignerKeyUtils::hashXKey(x);
Expand Down Expand Up @@ -1568,6 +1569,8 @@ TEST_CASE("txenvelope", "[tx][envelope]")
{payment(a1.getPublicKey(), paymentAmount)});
setMinTime(txFrame, 1000);
setMaxTime(txFrame, start + 300000);
getSignatures(txFrame).clear();
txFrame->addSignature(root);

closeLedgerOn(*app, 3, 2, 7, 2014);
applyCheck(txFrame, *app);
Expand Down Expand Up @@ -1606,6 +1609,8 @@ TEST_CASE("txenvelope", "[tx][envelope]")
TimePoint const lowerBound,
bool const expectSuccess) {
setMinTime(txFrame, minTime);
getSignatures(txFrame).clear();
txFrame->addSignature(root);
{
LedgerTxn ltx(app->getLedgerTxnRoot());
REQUIRE(txFrame->checkValid(
Expand Down Expand Up @@ -1660,6 +1665,8 @@ TEST_CASE("txenvelope", "[tx][envelope]")
{payment(a1.getPublicKey(), paymentAmount)});

setMinTime(txFrame, 1000);
getSignatures(txFrame).clear();
txFrame->addSignature(root);

closeLedgerOn(*app, 3, 3, 7, 2014);

Expand All @@ -1682,6 +1689,8 @@ TEST_CASE("txenvelope", "[tx][envelope]")
SECTION("success")
{
setMaxTime(txFrame, upperBoundCloseTime);
getSignatures(txFrame).clear();
txFrame->addSignature(root);

{
LedgerTxn ltx(app->getLedgerTxnRoot());
Expand Down Expand Up @@ -1933,32 +1942,76 @@ TEST_CASE("txenvelope", "[tx][envelope]")
});
}

SECTION("remove signer, do something")
SECTION("reduce auth, do something")
{
a.setOptions(setSigner(makeSigner(b, 1)) | setMasterWeight(1) |
setLowThreshold(2) | setMedThreshold(2) |
setHighThreshold(2));
a.setOptions(setSigner(makeSigner(b, 2)) | setMasterWeight(3) |
setLowThreshold(1) | setMedThreshold(3) |
setHighThreshold(5));

for_versions({1, 2, 3, 4, 5, 6, 8, 9}, *app, [&] {
auto tx = a.tx({setOptions(setSigner(makeSigner(b, 0))),
setOptions(setHomeDomain("stellar.org"))});
tx->addSignature(b);
SECTION("single tx")
{
SECTION("valid")
{
for_versions({1, 2, 3, 4, 5, 6, 8, 9}, *app, [&] {
auto tx =
a.tx({setOptions(setSigner(makeSigner(b, 1))),
setOptions(setSigner(makeSigner(b, 2)))});
tx->addSignature(b);

validateTxResults(
tx, *app, {baseFee * 2, txSUCCESS},
expectedResult(baseFee * 2, 2, txFAILED,
{SET_OPTIONS_SUCCESS, opBAD_AUTH}));
});
for_versions_from({7, 10}, *app, [&] {
auto tx =
a.tx({setOptions(setSigner(makeSigner(b, 1))),
setOptions(setSigner(makeSigner(b, 2)))});
tx->addSignature(b);

validateTxResults(
tx, *app, {baseFee * 2, txSUCCESS},
expectedResult(
baseFee * 2, 2, txSUCCESS,
{SET_OPTIONS_SUCCESS, SET_OPTIONS_SUCCESS}));
});
}
SECTION("missing signature")
{
for_versions_from(10, *app, [&] {
auto tx =
a.tx({payment(root, 1000),
setOptions(setSigner(makeSigner(b, 2)))});
// missing b signature
applyCheck(tx, *app);
REQUIRE(tx->getResultCode() == txFAILED);
REQUIRE(PaymentOpFrame::getInnerCode(
getFirstResult(*tx)) == PAYMENT_SUCCESS);
REQUIRE(tx->getOperations()[1]->getResultCode() ==
opBAD_AUTH);
});
}
}
SECTION("multiple tx")
{
for_versions_from(10, *app, [&] {
auto tx1 = a.tx({setOptions(setSigner(makeSigner(b, 1)))});
tx1->addSignature(b);
auto tx2 = a.tx({payment(root, 1000),
setOptions(setSigner(makeSigner(b, 2)))});
tx2->addSignature(b);

validateTxResults(
tx, *app, {baseFee * 2, txSUCCESS},
expectedResult(baseFee * 2, 2, txFAILED,
{SET_OPTIONS_SUCCESS, opBAD_AUTH}));
});
for_versions_from({7, 10}, *app, [&] {
auto tx = a.tx({setOptions(setSigner(makeSigner(b, 0))),
setOptions(setHomeDomain("stellar.org"))});
tx->addSignature(b);
auto r =
closeLedgerOn(*app, 2, 1, 2, 2016, {tx1, tx2}, true);

validateTxResults(
tx, *app, {baseFee * 2, txSUCCESS},
expectedResult(baseFee * 2, 2, txSUCCESS,
{SET_OPTIONS_SUCCESS, SET_OPTIONS_SUCCESS}));
});
REQUIRE(tx1->getResultCode() == txSUCCESS);
REQUIRE(tx2->getResultCode() == txFAILED);
REQUIRE(PaymentOpFrame::getInnerCode(
getFirstResult(*tx2)) == PAYMENT_SUCCESS);
REQUIRE(tx2->getOperations()[1]->getResultCode() ==
opBAD_AUTH);
});
}
}

SECTION("add signer, increase thresholds, do something")
Expand Down

0 comments on commit 5b2989d

Please sign in to comment.