Skip to content

Retire fix1571 Amendment #5925

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.

Sign up for GitHub

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

Merged
bthomee merged 1 commit into from
Oct 29, 2025
Merged

Retire fix1571 Amendment #5925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/xrpl/protocol/detail/features.macro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYe
XRPL_FIX (1578, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FIX (1571, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes)
Expand Down Expand Up @@ -154,3 +153,4 @@ XRPL_RETIRE(fix1513)
XRPL_RETIRE(fix1515)
XRPL_RETIRE(fix1543)
XRPL_RETIRE(fix1781)
XRPL_RETIRE(fix1571)
101 changes: 39 additions & 62 deletions src/test/app/Escrow_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,74 +294,51 @@ struct Escrow_test : public beast::unit_test::suite
}

void
test1571(FeatureBitset features)
testRequiresConditionOrFinishAfter(FeatureBitset features)
{
using namespace jtx;
using namespace std::chrono;

{
testcase("Implied Finish Time (without fix1571)");

Env env(*this, testable_amendments() - fix1571);
auto const baseFee = env.current()->fees().base;
env.fund(XRP(5000), "alice", "bob", "carol");
env.close();

// Creating an escrow without a finish time and finishing it
// is allowed without fix1571:
auto const seq1 = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 1s),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq1), fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));

env.close();

// Creating an escrow without a finish time and a condition is
// also allowed without fix1571:
auto const seq2 = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 1s),
escrow::condition(escrow::cb1),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq2),
escrow::condition(escrow::cb1),
escrow::fulfillment(escrow::fb1),
fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5200));
}
testcase("RequiresConditionOrFinishAfter");

{
testcase("Implied Finish Time (with fix1571)");

Env env(*this, features);
auto const baseFee = env.current()->fees().base;
env.fund(XRP(5000), "alice", "bob", "carol");
env.close();
Env env(*this, features);
auto const baseFee = env.current()->fees().base;
env.fund(XRP(5000), "alice", "bob", "carol");
env.close();

// Creating an escrow with only a cancel time is not allowed:
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
fee(baseFee * 150),
ter(temMALFORMED));
// Creating an escrow with only a cancel time is not allowed:
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
fee(baseFee * 150),
ter(temMALFORMED));

// Creating an escrow with only a cancel time and a condition is
// allowed:
auto const seq = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
escrow::condition(escrow::cb1),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq),
escrow::condition(escrow::cb1),
escrow::fulfillment(escrow::fb1),
fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));
}
// Creating an escrow with only a cancel time and a condition is
// allowed:
auto const seq = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
escrow::condition(escrow::cb1),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq),
escrow::condition(escrow::cb1),
escrow::fulfillment(escrow::fb1),
fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));

// Creating an escrow with only a cancel time and a finish time is
// allowed:
auto const seqFt = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::finish_time(env.now()), // Set finish time to now so that
// we can call finish immediately.
escrow::cancel_time(env.now() + 50s),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seqFt), fee(150 * baseFee));
BEAST_EXPECT(
env.balance("bob") ==
XRP(5200)); // 5100 (from last transaction) + 100
Copy link
Contributor Author

@pratikmankawde pratikmankawde Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ximinez Can you please confirm if this new unit test which checks presence of cancel time and finish time combination is correct?

Copy link
Contributor

@vvysokikh1 vvysokikh1 Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering there's a bunch of tests in this file that have almost same check (search for "Metadata to self" testcase for example), I'd say this seems correct

Copy link
Contributor Author

@pratikmankawde pratikmankawde Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming.

}

void
Expand Down Expand Up @@ -1708,7 +1685,7 @@ struct Escrow_test : public beast::unit_test::suite
testTiming(features);
testTags(features);
testDisallowXRP(features);
test1571(features);
testRequiresConditionOrFinishAfter(features);
testFails(features);
testLockup(features);
testEscrowConditions(features);
Expand Down
120 changes: 25 additions & 95 deletions src/xrpld/app/tx/detail/Escrow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,12 @@ EscrowCreate::preflight(PreflightContext const& ctx)
ctx.tx[sfCancelAfter] <= ctx.tx[sfFinishAfter])
return temBAD_EXPIRATION;

if (ctx.rules.enabled(fix1571))
{
// In the absence of a FinishAfter, the escrow can be finished
// immediately, which can be confusing. When creating an escrow,
// we want to ensure that either a FinishAfter time is explicitly
// specified or a completion condition is attached.
if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition])
return temMALFORMED;
}
// In the absence of a FinishAfter, the escrow can be finished
// immediately, which can be confusing. When creating an escrow,
// we want to ensure that either a FinishAfter time is explicitly
// specified or a completion condition is attached.
if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition])
return temMALFORMED;

if (auto const cb = ctx.tx[~sfCondition])
{
Expand Down Expand Up @@ -446,41 +443,11 @@ EscrowCreate::doApply()
{
auto const closeTime = ctx_.view().info().parentCloseTime;

// Prior to fix1571, the cancel and finish times could be greater
// than or equal to the parent ledgers' close time.
//
// With fix1571, we require that they both be strictly greater
// than the parent ledgers' close time.
if (ctx_.view().rules().enabled(fix1571))
{
if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
return tecNO_PERMISSION;

if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter]))
return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
if (ctx_.tx[~sfCancelAfter])
{
auto const cancelAfter = ctx_.tx[sfCancelAfter];

if (closeTime.time_since_epoch().count() >= cancelAfter)
return tecNO_PERMISSION;
}

if (ctx_.tx[~sfFinishAfter])
{
auto const finishAfter = ctx_.tx[sfFinishAfter];
if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
return tecNO_PERMISSION;

if (closeTime.time_since_epoch().count() >= finishAfter)
return tecNO_PERMISSION;
}
// LCOV_EXCL_STOP
}
if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter]))
return tecNO_PERMISSION;

auto const sle = ctx_.view().peek(keylet::account(account_));
if (!sle)
Expand Down Expand Up @@ -1027,38 +994,16 @@ EscrowFinish::doApply()
}

// If a cancel time is present, a finish operation should only succeed prior
// to that time. fix1571 corrects a logic error in the check that would make
// a finish only succeed strictly after the cancel time.
if (ctx_.view().rules().enabled(fix1571))
{
auto const now = ctx_.view().info().parentCloseTime;
// to that time.
auto const now = ctx_.view().info().parentCloseTime;

// Too soon: can't execute before the finish time
if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter]))
return tecNO_PERMISSION;
// Too soon: can't execute before the finish time
if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter]))
return tecNO_PERMISSION;

// Too late: can't execute after the cancel time
if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
// Too soon?
if ((*slep)[~sfFinishAfter] &&
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfFinishAfter])
return tecNO_PERMISSION;

// Too late?
if ((*slep)[~sfCancelAfter] &&
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfCancelAfter])
return tecNO_PERMISSION;
// LCOV_EXCL_STOP
}
// Too late: can't execute after the cancel time
if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION;

// Check cryptocondition fulfillment
{
Expand Down Expand Up @@ -1315,30 +1260,15 @@ EscrowCancel::doApply()
return tecNO_TARGET;
}

if (ctx_.view().rules().enabled(fix1571))
{
auto const now = ctx_.view().info().parentCloseTime;
auto const now = ctx_.view().info().parentCloseTime;

// No cancel time specified: can't execute at all.
if (!(*slep)[~sfCancelAfter])
return tecNO_PERMISSION;
// No cancel time specified: can't execute at all.
if (!(*slep)[~sfCancelAfter])
return tecNO_PERMISSION;

// Too soon: can't execute before the cancel time.
if (!after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
// Too soon?
if (!(*slep)[~sfCancelAfter] ||
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfCancelAfter])
return tecNO_PERMISSION;
// LCOV_EXCL_STOP
}
// Too soon: can't execute before the cancel time.
if (!after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION;

AccountID const account = (*slep)[sfAccount];

Expand Down