Skip to content

Commit

Permalink
make AccountMerge fail when the current sequence number is too far
Browse files Browse the repository at this point in the history 
This makes it compliant with the proposal dated 02-Feb-2018
stellar/stellar-protocol#53
  • Loading branch information
@MonsieurNicolas
MonsieurNicolas committed Mar 20, 2018
1 parent 4fe0945 commit 6257cb2
Show file tree
Hide file tree
Showing 10 changed files with 121 additions and 24 deletions.
2 changes: 2 additions & 0 deletions src/test/TestExceptions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ throwIf(AccountMergeResult const& result)
throw ex_ACCOUNT_MERGE_IMMUTABLE_SET{};
case ACCOUNT_MERGE_HAS_SUB_ENTRIES:
throw ex_ACCOUNT_MERGE_HAS_SUB_ENTRIES{};
case ACCOUNT_MERGE_SEQNUM_TOO_FAR:
throw ex_ACCOUNT_MERGE_SEQNUM_TOO_FAR{};
case ACCOUNT_MERGE_SUCCESS:
break;
default:
Expand Down
1 change: 1 addition & 0 deletions src/test/TestExceptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ TEST_EXCEPTION(ex_ACCOUNT_MERGE_MALFORMED)
TEST_EXCEPTION(ex_ACCOUNT_MERGE_NO_ACCOUNT)
TEST_EXCEPTION(ex_ACCOUNT_MERGE_IMMUTABLE_SET)
TEST_EXCEPTION(ex_ACCOUNT_MERGE_HAS_SUB_ENTRIES)
TEST_EXCEPTION(ex_ACCOUNT_MERGE_SEQNUM_TOO_FAR)

TEST_EXCEPTION(ex_ALLOW_TRUST_MALFORMED)
TEST_EXCEPTION(ex_ALLOW_TRUST_NO_TRUST_LINE)
Expand Down
1 change: 1 addition & 0 deletions src/transactions/ChangeTrustTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ TEST_CASE("change trust", "[tx][changetrust]")
}
SECTION("edit existing")
{
closeLedgerOn(*app, 2, 1, 1, 2016);
for_all_versions(*app, [&] {
root.changeTrust(idr, 100);
// Merge gateway back into root (the trustline still exists)
Expand Down
17 changes: 17 additions & 0 deletions src/transactions/MergeOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,23 @@ MergeOpFrame::doApply(Application& app, LedgerDelta& delta,
return false;
}

if (ledgerManager.getCurrentLedgerVersion() >= 10)
{
SequenceNumber maxSeqNum =
ledgerManager.getCurrentLedgerHeader().ledgerSeq;
maxSeqNum = (maxSeqNum << 32) - 1;

if (mSourceAccount->getSeqNum() > maxSeqNum)
{
app.getMetrics()
.NewMeter({"op-merge", "failure", "too-far"}, "operation")
.Mark();
innerResult().code(ACCOUNT_MERGE_SEQNUM_TOO_FAR);
return false;
}
}

// "success" path starts
if (!otherAccount->addBalance(sourceBalance))
{
throw std::runtime_error("merge overflowed destination balance");
Expand Down
66 changes: 58 additions & 8 deletions src/transactions/MergeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ TEST_CASE("merge", "[tx][merge]")
app->getLedgerManager().getMinBalance(5) + 20 * txfee;

auto a1 = root.create("A", 2 * minBalance);
auto b1 = root.create("B", minBalance);
auto gateway = root.create("gate", minBalance);

// close ledger to allow a1, b1 and gateway to be merged in the next ledger
closeLedgerOn(*app, 2, 1, 1, 2016);

SECTION("merge into self")
{
Expand All @@ -57,14 +62,11 @@ TEST_CASE("merge", "[tx][merge]")
SECTION("merge into non existent account")
{
for_all_versions(*app, [&] {
REQUIRE_THROWS_AS(a1.merge(getAccount("B").getPublicKey()),
REQUIRE_THROWS_AS(a1.merge(getAccount("C").getPublicKey()),
ex_ACCOUNT_MERGE_NO_ACCOUNT);
});
}

auto b1 = root.create("B", minBalance);
auto gateway = root.create("gate", minBalance);

SECTION("with create")
{
auto a1Balance = a1.getBalance();
Expand All @@ -89,7 +91,7 @@ TEST_CASE("merge", "[tx][merge]")
REQUIRE(!loadAccount(a1, *app, false));
});

for_versions_from(6, *app, [&] {
for_versions(6, 9, *app, [&] {
applyCheck(txFrame, *app);

auto result = MergeOpFrame::getInnerCode(
Expand All @@ -100,13 +102,32 @@ TEST_CASE("merge", "[tx][merge]")
a1Balance + b1Balance - txFrame->getFee());
REQUIRE(!loadAccount(a1, *app, false));
});

for_versions_from(10, *app, [&]() {
// can't merge an account that just got created
REQUIRE(!applyCheck(txFrame, *app));
REQUIRE(txFrame->getResult()
.result.results()[0]
.tr()
.accountMergeResult()
.code() == ACCOUNT_MERGE_SUCCESS);
REQUIRE(txFrame->getResult()
.result.results()[1]
.tr()
.createAccountResult()
.code() == CREATE_ACCOUNT_SUCCESS);
REQUIRE(txFrame->getResult()
.result.results()[2]
.tr()
.accountMergeResult()
.code() == ACCOUNT_MERGE_SEQNUM_TOO_FAR);
});
}

SECTION("merge, create, merge back")
{
auto a1Balance = a1.getBalance();
auto b1Balance = b1.getBalance();
auto a1SeqNum = a1.loadSequenceNumber();
auto createBalance = app->getLedgerManager().getMinBalance(1);
auto txFrame =
a1.tx({a1.op(accountMerge(b1)),
Expand All @@ -128,7 +149,9 @@ TEST_CASE("merge", "[tx][merge]")
REQUIRE(a1.getBalance() ==
a1Balance + b1Balance - txFrame->getFee());
REQUIRE(!loadAccount(b1, *app, false));
REQUIRE(a1SeqNum == a1.loadSequenceNumber());
// a1 gets recreated with a sequence number based on the current
// ledger
REQUIRE(a1.loadSequenceNumber() == 0x300000000ull);
});
}

Expand Down Expand Up @@ -500,7 +523,7 @@ TEST_CASE("merge", "[tx][merge]")
auto tx2 = a1.tx({payment(root, 100)});
auto a1Balance = a1.getBalance();
auto b1Balance = b1.getBalance();
auto r = closeLedgerOn(*app, 2, 1, 1, 2015, {tx1, tx2});
auto r = closeLedgerOn(*app, 3, 1, 1, 2017, {tx1, tx2});
checkTx(0, r, txSUCCESS);
checkTx(1, r, txNO_ACCOUNT);

Expand Down Expand Up @@ -546,6 +569,7 @@ TEST_CASE("merge", "[tx][merge]")
{
auto mergeFrom = root.create(
"merge-from", app->getLedgerManager().getMinBalance(0) + txfee);
closeLedgerOn(*app, 3, 1, 1, 2017);
for_versions_to(8, *app, [&] {
REQUIRE_THROWS_AS(mergeFrom.merge(root), ex_txINSUFFICIENT_BALANCE);
});
Expand All @@ -557,6 +581,7 @@ TEST_CASE("merge", "[tx][merge]")
{
auto mergeFrom = root.create(
"merge-from", app->getLedgerManager().getMinBalance(0) + txfee + 1);
closeLedgerOn(*app, 3, 1, 1, 2017);
for_versions_to(8, *app, [&] {
REQUIRE_THROWS_AS(mergeFrom.merge(root), ex_txINSUFFICIENT_BALANCE);
});
Expand All @@ -569,6 +594,7 @@ TEST_CASE("merge", "[tx][merge]")
auto mergeFrom =
root.create("merge-from", app->getLedgerManager().getMinBalance(0) +
2 * txfee - 1);
closeLedgerOn(*app, 3, 1, 1, 2017);
for_versions_to(8, *app, [&] {
REQUIRE_THROWS_AS(mergeFrom.merge(root), ex_txINSUFFICIENT_BALANCE);
});
Expand All @@ -580,6 +606,30 @@ TEST_CASE("merge", "[tx][merge]")
{
auto mergeFrom = root.create(
"merge-from", app->getLedgerManager().getMinBalance(0) + 2 * txfee);
closeLedgerOn(*app, 3, 1, 1, 2017);
for_all_versions(*app, [&] { mergeFrom.merge(root); });
}

SECTION("merge too far")
{
for_versions_from(10, *app, [&]() {
SequenceNumber maxSeqNum = app->getLedgerManager().getLedgerNum();
maxSeqNum = (maxSeqNum << 32) - 1;
maxSeqNum--; // a1.merge will increment at the transaction level
SECTION("at max = success")
{
a1.bumpSequence(maxSeqNum);
REQUIRE(a1.loadSequenceNumber() == maxSeqNum);
a1.merge(b1);
}
SECTION("passed max = failure")
{
maxSeqNum++;
a1.bumpSequence(maxSeqNum);
REQUIRE(a1.loadSequenceNumber() == maxSeqNum);
REQUIRE_THROWS_AS(a1.merge(b1),
ex_ACCOUNT_MERGE_SEQNUM_TOO_FAR);
}
});
}
}
2 changes: 2 additions & 0 deletions src/transactions/PathPaymentTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ TEST_CASE("pathpayment", "[tx][pathpayment]")
auto cur3 = makeAsset(gateway2, "CUR3");
auto cur4 = makeAsset(gateway2, "CUR4");

closeLedgerOn(*app, 2, 1, 1, 2016);

SECTION("path payment destination amount 0")
{
auto market = TestMarket{*app};
Expand Down
42 changes: 31 additions & 11 deletions src/transactions/PaymentTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(rootAccount->getBalance() == (1000000000000000000 - paymentAmount -
gatewayPayment * 2 - txfee * 3));

closeLedgerOn(*app, 2, 1, 1, 2016);

SECTION("Create account")
{
SECTION("Success")
Expand Down Expand Up @@ -133,6 +135,7 @@ TEST_CASE("payment", "[tx][payment]")

int64 a1Balance = a1.getBalance();
int64 b1Balance = b1.getBalance();
closeLedgerOn(*app, 3, 1, 2, 2016);

auto txFrame = a1.tx({payment(b1, 200), accountMerge(b1)});

Expand All @@ -159,6 +162,8 @@ TEST_CASE("payment", "[tx][payment]")
int64 a1Balance = a1.getBalance();
int64 b1Balance = b1.getBalance();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto txFrame = a1.tx({payment(b1, 200), b1.op(accountMerge(a1))});
txFrame->addSignature(b1);

Expand All @@ -182,6 +187,8 @@ TEST_CASE("payment", "[tx][payment]")
int64 a1Balance = a1.getBalance();
int64 b1Balance = b1.getBalance();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto txFrame = a1.tx({accountMerge(b1), payment(b1, 200)});

for_versions_to(7, *app, [&] {
Expand Down Expand Up @@ -278,7 +285,7 @@ TEST_CASE("payment", "[tx][payment]")
auto rootBalance = root.getBalance();

for_versions_to(8, *app, [&] {
auto r = closeLedgerOn(*app, 2, 1, 1, 2015, {tx1, tx2});
auto r = closeLedgerOn(*app, 3, 1, 2, 2016, {tx1, tx2});
checkTx(0, r, txSUCCESS);
checkTx(1, r, txINSUFFICIENT_BALANCE);

Expand All @@ -290,7 +297,7 @@ TEST_CASE("payment", "[tx][payment]")
});

for_versions_from(9, *app, [&] {
auto r = closeLedgerOn(*app, 2, 1, 1, 2015, {tx1, tx2});
auto r = closeLedgerOn(*app, 3, 1, 2, 2016, {tx1, tx2});
checkTx(0, r, txSUCCESS);
checkTx(1, r, txFAILED);
REQUIRE(r[1].first.result.result.results()[0]
Expand All @@ -315,6 +322,8 @@ TEST_CASE("payment", "[tx][payment]")
auto createSourceAccount = TestAccount{*app, getAccount("create")};
auto sourceSeqNum = sourceAccount.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx =
sourceAccount.tx({createAccount(createSourceAccount, createAmount),
accountMerge(createSourceAccount),
Expand Down Expand Up @@ -418,7 +427,7 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(tx->getResult().result.code() == txINTERNAL_ERROR);
});

for_versions_from(8, *app, [&] {
for_versions(8, 9, *app, [&] {
REQUIRE(!applyCheck(tx, *app));
REQUIRE(loadAccount(sourceAccount, *app));
REQUIRE(!loadAccount(createSourceAccount, *app, false));
Expand Down Expand Up @@ -452,6 +461,7 @@ TEST_CASE("payment", "[tx][payment]")
amount - createAmount - tx->getFee());
REQUIRE(tx->getResult().result.results()[2].code() == opNO_ACCOUNT);
});
// 10 and above, operation #2 fails (already covered in merge tests)
}

SECTION("pay, merge, create, pay, self")
Expand All @@ -462,10 +472,11 @@ TEST_CASE("payment", "[tx][payment]")
auto pay2Amount = 200000000;
auto sourceAccount = root.create("source", amount);
auto payAndMergeDestination = root.create("payAndMerge", amount);
auto sourceSeqNum = sourceAccount.getLastSequenceNumber();
auto payAndMergeDestinationSeqNum =
payAndMergeDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx =
sourceAccount.tx({payAndMergeDestination.op(
payment(payAndMergeDestination, pay1Amount)),
Expand All @@ -483,7 +494,7 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(sourceAccount.getBalance() == createAmount);
REQUIRE(payAndMergeDestination.getBalance() ==
amount + amount - createAmount - tx->getFee());
REQUIRE(sourceAccount.loadSequenceNumber() == sourceSeqNum);
REQUIRE(sourceAccount.loadSequenceNumber() == 0x400000000ull);
REQUIRE(payAndMergeDestination.loadSequenceNumber() ==
payAndMergeDestinationSeqNum);

Expand Down Expand Up @@ -534,7 +545,7 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(sourceAccount.getBalance() == createAmount);
REQUIRE(payAndMergeDestination.getBalance() ==
amount + amount - createAmount - tx->getFee());
REQUIRE(sourceAccount.loadSequenceNumber() == sourceSeqNum);
REQUIRE(sourceAccount.loadSequenceNumber() == 0x400000000ull);
REQUIRE(payAndMergeDestination.loadSequenceNumber() ==
payAndMergeDestinationSeqNum);

Expand Down Expand Up @@ -589,6 +600,8 @@ TEST_CASE("payment", "[tx][payment]")
auto payAndMergeDestinationSeqNum =
payAndMergeDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx =
sourceAccount.tx({payment(payAndMergeDestination, pay1Amount),
accountMerge(payAndMergeDestination),
Expand Down Expand Up @@ -657,7 +670,7 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(sourceAccount.getBalance() == createAmount - pay2Amount);
REQUIRE(payAndMergeDestination.getBalance() ==
amount + amount + pay2Amount - tx->getFee() - createAmount);
REQUIRE(sourceAccount.loadSequenceNumber() == sourceSeqNum);
REQUIRE(sourceAccount.loadSequenceNumber() == 0x400000000ull);
REQUIRE(payAndMergeDestination.loadSequenceNumber() ==
payAndMergeDestinationSeqNum);

Expand Down Expand Up @@ -715,6 +728,8 @@ TEST_CASE("payment", "[tx][payment]")
auto payAndMergeDestinationSeqNum =
payAndMergeDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx = sourceAccount.tx(
{payment(payAndMergeDestination, pay1Amount),
accountMerge(payAndMergeDestination),
Expand Down Expand Up @@ -788,7 +803,7 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(secondSourceAccount.getBalance() == amount - createAmount);
REQUIRE(payAndMergeDestination.getBalance() ==
amount + amount + pay2Amount - tx->getFee());
REQUIRE(sourceAccount.loadSequenceNumber() == sourceSeqNum);
REQUIRE(sourceAccount.loadSequenceNumber() == 0x400000000ull);
REQUIRE(secondSourceAccount.loadSequenceNumber() ==
secondSourceSeqNum);
REQUIRE(payAndMergeDestination.loadSequenceNumber() ==
Expand Down Expand Up @@ -847,7 +862,8 @@ TEST_CASE("payment", "[tx][payment]")
auto payDestination = root.create("pay", amount);
auto sourceSeqNum = sourceAccount.getLastSequenceNumber();
auto createSourceSeqNum = createSource.getLastSequenceNumber();
auto payDestinationSeqNum = payDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx = sourceAccount.tx({
createSource.op(createAccount(createDestination, create1Amount)),
Expand Down Expand Up @@ -875,8 +891,8 @@ TEST_CASE("payment", "[tx][payment]")
REQUIRE(payDestination.getBalance() == create2Amount);
REQUIRE(sourceAccount.loadSequenceNumber() == sourceSeqNum + 1);
REQUIRE(createSource.loadSequenceNumber() == createSourceSeqNum);
REQUIRE(payDestination.loadSequenceNumber() ==
payDestinationSeqNum);
REQUIRE(createDestination.loadSequenceNumber() == 0x400000000ull);
REQUIRE(payDestination.loadSequenceNumber() == 0x400000000ull);

REQUIRE(tx->getResult().result.code() == txSUCCESS);
REQUIRE(tx->getResult().result.results()[0].code() == opINNER);
Expand Down Expand Up @@ -929,6 +945,8 @@ TEST_CASE("payment", "[tx][payment]")
auto sourceSeqNum = sourceAccount.getLastSequenceNumber();
auto mergeDestinationSeqNum = mergeDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx = sourceAccount.tx({payment(sourceAccount, pay1Amount),
accountMerge(mergeDestination),
payment(sourceAccount, pay2Amount),
Expand Down Expand Up @@ -1070,6 +1088,8 @@ TEST_CASE("payment", "[tx][payment]")
auto sourceSeqNum = sourceAccount.getLastSequenceNumber();
auto mergeDestinationSeqNum = mergeDestination.getLastSequenceNumber();

closeLedgerOn(*app, 3, 1, 2, 2016);

auto tx = sourceAccount.tx({payment(sourceAccount, pay1Amount),
payment(sourceAccount, pay1Amount),
payment(sourceAccount, pay1Amount),
Expand Down
Loading

0 comments on commit 6257cb2

Please sign in to comment.