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.

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

Deep freeze (#5187) #5187

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

Deep freeze (#5187) #5187

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e57d068
Implement Deep Freeze
HowardHinnant Aug 2, 2024
a0d48ae
[FOLD] Move feature to top of list
HowardHinnant Oct 31, 2024
4aac190
[FOLD] Make use of the deep freeze flags malformed prior
HowardHinnant Oct 31, 2024
306a112
Fixed creation of buy offer transaction to fail with tecFROZEN instantly
vvysokikh1 Nov 15, 2024
f5efa31
Fixed comments #1
vvysokikh1 Nov 18, 2024
8e1977f
Reworked freeze flag handling in SetTrust
vvysokikh1 Nov 20, 2024
a162663
Moved modified trustline flags extraction into separate function
vvysokikh1 Nov 20, 2024
c7c26ac
Added more check to test cases
vvysokikh1 Nov 21, 2024
805102a
Added payment tests for freeze and deep freeze
vvysokikh1 Nov 22, 2024
de58a26
Clarified behavior of individual freeze
vvysokikh1 Nov 22, 2024
91d291a
Added offer test for freeze and deep freeze
vvysokikh1 Nov 28, 2024
e0d59ef
More offer tests
vvysokikh1 Dec 2, 2024
ffa6ccd
Fixed typo
vvysokikh1 Dec 4, 2024
f9107a7
Fixed tx flags check in case trustline doesn't exist yet
vvysokikh1 Dec 4, 2024
1f9ecb8
Added tests for trustline creation freeze handling
vvysokikh1 Dec 4, 2024
010f36d
Partially fixed comments
vvysokikh1 Dec 6, 2024
6bae95d
Partially fixed comments
vvysokikh1 Dec 10, 2024
b5a10ec
Fixed buy offer crossing when deep frozen
vvysokikh1 Dec 10, 2024
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/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 80;
static constexpr std::size_t numFeatures = 81;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down
2 changes: 2 additions & 0 deletions include/xrpl/protocol/LedgerFormats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ enum LedgerSpecificFlags {
lsfHighFreeze = 0x00800000, // True, high side has set freeze flag
lsfAMMNode = 0x01000000, // True, trust line to AMM. Used by client
// apps to identify payments via AMM.
lsfLowDeepFreeze = 0x02000000,
vvysokikh1 marked this conversation as resolved.
Show resolved Hide resolved
lsfHighDeepFreeze = 0x04000000,

// ltSIGNER_LIST
lsfOneOwnerCount = 0x00010000, // True, uses only one OwnerCount
Expand Down
4 changes: 3 additions & 1 deletion include/xrpl/protocol/TxFlags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,11 @@ constexpr std::uint32_t tfSetNoRipple = 0x00020000;
constexpr std::uint32_t tfClearNoRipple = 0x00040000;
constexpr std::uint32_t tfSetFreeze = 0x00100000;
constexpr std::uint32_t tfClearFreeze = 0x00200000;
constexpr std::uint32_t tfSetDeepFreeze = 0x00400000;
constexpr std::uint32_t tfClearDeepFreeze = 0x00800000;
constexpr std::uint32_t tfTrustSetMask =
~(tfUniversal | tfSetfAuth | tfSetNoRipple | tfClearNoRipple | tfSetFreeze |
tfClearFreeze);
tfClearFreeze | tfSetDeepFreeze | tfClearDeepFreeze);

// EnableAmendment flags:
constexpr std::uint32_t tfGotMajority = 0x00010000;
Expand Down
1 change: 1 addition & 0 deletions include/xrpl/protocol/detail/features.macro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

// InvariantsV1_1 will be changes to Supported::yes when all the
vvysokikh1 marked this conversation as resolved.
Show resolved Hide resolved
// invariants expected to be included under it are complete.
XRPL_FEATURE(DeepFreeze, Supported::yes, VoteBehavior::DefaultNo)
XRPL_FEATURE(MPTokensV1, Supported::yes, VoteBehavior::DefaultNo)
XRPL_FEATURE(InvariantsV1_1, Supported::no, VoteBehavior::DefaultNo)
XRPL_FIX (NFTokenPageLinks, Supported::yes, VoteBehavior::DefaultNo)
Expand Down
208 changes: 208 additions & 0 deletions src/test/app/AMMExtended_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3615,6 +3615,208 @@ struct AMMExtended_test : public jtx::AMMTest
}
}

void
testRippleDeepState(FeatureBitset features)
vvysokikh1 marked this conversation as resolved.
Show resolved Hide resolved
{
testcase("RippleState Deep Freeze");

using namespace test::jtx;
Env env(*this, features);

Account const G1{"G1"};
Account const alice{"alice"};
Account const bob{"bob"};

env.fund(XRP(1'000), G1, alice, bob);
env.close();

env.trust(G1["USD"](100), bob);
env.trust(G1["USD"](205), alice);
env.close();

env(pay(G1, bob, G1["USD"](10)));
env(pay(G1, alice, G1["USD"](205)));
env.close();

AMM ammAlice(env, alice, XRP(500), G1["USD"](105));

{
auto lines = getAccountLines(env, bob);
if (!BEAST_EXPECT(checkArraySize(lines[jss::lines], 1u)))
return;
BEAST_EXPECT(lines[jss::lines][0u][jss::account] == G1.human());
BEAST_EXPECT(lines[jss::lines][0u][jss::limit] == "100");
BEAST_EXPECT(lines[jss::lines][0u][jss::balance] == "10");
}

{
auto lines = getAccountLines(env, alice, G1["USD"]);
if (!BEAST_EXPECT(checkArraySize(lines[jss::lines], 1u)))
return;
BEAST_EXPECT(lines[jss::lines][0u][jss::account] == G1.human());
BEAST_EXPECT(lines[jss::lines][0u][jss::limit] == "205");
// 105 transferred to AMM
BEAST_EXPECT(lines[jss::lines][0u][jss::balance] == "100");
}

{
// Account with line unfrozen (proving operations normally work)
// test: can make Payment on that line
env(pay(alice, bob, G1["USD"](1)));

// test: can receive Payment on that line
env(pay(bob, alice, G1["USD"](1)));
env.close();
}

{
// Is created via a TrustSet with SetFreeze flag
// test: sets LowFreeze | HighFreeze flags
env(trust(G1, bob["USD"](0), tfSetFreeze | tfSetDeepFreeze));
auto affected = env.meta()->getJson(
vvysokikh1 marked this conversation as resolved.
Show resolved Hide resolved
JsonOptions::none)[sfAffectedNodes.fieldName];
if (!BEAST_EXPECT(checkArraySize(affected, 2u)))
return;
auto ff =
affected[1u][sfModifiedNode.fieldName][sfFinalFields.fieldName];
BEAST_EXPECT(
ff[sfLowLimit.fieldName] ==
G1["USD"](0).value().getJson(JsonOptions::none));
if (features[featureDeepFreeze])
{
BEAST_EXPECT(ff[jss::Flags].asUInt() & lsfLowDeepFreeze);
BEAST_EXPECT(!(ff[jss::Flags].asUInt() & lsfHighDeepFreeze));
}
else
{
BEAST_EXPECT(!(ff[jss::Flags].asUInt() & lsfLowDeepFreeze));
BEAST_EXPECT(!(ff[jss::Flags].asUInt() & lsfHighDeepFreeze));
}
env.close();
}

{
// Account with line deep frozen by issuer
// test: can not buy more assets on that line
env(offer(bob, G1["USD"](5), XRP(25)));
auto affected = env.meta()->getJson(
JsonOptions::none)[sfAffectedNodes.fieldName];
if (!BEAST_EXPECT(checkArraySize(affected, 4u)))
return;
auto ff =
affected[1u][sfModifiedNode.fieldName][sfFinalFields.fieldName];
auto amt = STAmount{Issue{to_currency("USD"), noAccount()}, -15}
.value()
.getJson(JsonOptions::none);
if (features[featureDeepFreeze])
{
BEAST_EXPECT(ff[sfHighLimit.fieldName] == Json::nullValue);
BEAST_EXPECT(ff[sfBalance.fieldName] == Json::nullValue);
}
else
{
BEAST_EXPECT(
ff[sfHighLimit.fieldName] ==
bob["USD"](100).value().getJson(JsonOptions::none));
BEAST_EXPECT(ff[sfBalance.fieldName] == amt);
}
env.close();
if (features[featureDeepFreeze])
{
BEAST_EXPECT(ammAlice.expectBalances(
XRP(500), G1["USD"](105), ammAlice.tokens()));
}
else
{
BEAST_EXPECT(ammAlice.expectBalances(
XRP(525), G1["USD"](100), ammAlice.tokens()));
}
}

{
// test: can not sell assets from that line
env(offer(bob, XRP(1), G1["USD"](5)), ter(tecUNFUNDED_OFFER));

// test: can not make Payment from that line
env(pay(bob, alice, G1["USD"](1)), ter(tecPATH_DRY));

if (features[featureDeepFreeze])
{
// test: can not receive Payment on that line
env(pay(alice, bob, G1["USD"](1)), ter(tecPATH_DRY));
}
else
{
// test: can receive Payment on that line
env(pay(alice, bob, G1["USD"](1)));
}
}

{
// check G1 account lines
// test: shows deep freeze
auto lines = getAccountLines(env, G1);
Json::Value bobLine;
for (auto const& it : lines[jss::lines])
{
if (it[jss::account] == bob.human())
{
bobLine = it;
break;
}
}
if (!BEAST_EXPECT(bobLine))
return;
BEAST_EXPECT(bobLine[jss::freeze] == true);
if (features[featureDeepFreeze])
BEAST_EXPECT(bobLine[jss::balance] == "-10");
else
BEAST_EXPECT(bobLine[jss::balance] == "-16");
}

{
// test: shows deep freeze peer
auto lines = getAccountLines(env, bob);
Json::Value g1Line;
for (auto const& it : lines[jss::lines])
{
if (it[jss::account] == G1.human())
{
g1Line = it;
break;
}
}
if (!BEAST_EXPECT(g1Line))
return;
BEAST_EXPECT(g1Line[jss::freeze_peer] == true);
if (features[featureDeepFreeze])
BEAST_EXPECT(g1Line[jss::balance] == "10");
else
BEAST_EXPECT(g1Line[jss::balance] == "16");
}

{
// Is cleared via a TrustSet with ClearDeepFreeze flag
// test: sets LowDeepFreeze | HighDeepFreeze flags
env(trust(G1, bob["USD"](0), tfClearDeepFreeze));
auto affected = env.meta()->getJson(
JsonOptions::none)[sfAffectedNodes.fieldName];
if (!features[featureDeepFreeze] &&
BEAST_EXPECT(checkArraySize(affected, 1u)))
return;
if (!BEAST_EXPECT(checkArraySize(affected, 2u)))
return;
auto ff =
affected[1u][sfModifiedNode.fieldName][sfFinalFields.fieldName];
BEAST_EXPECT(
ff[sfLowLimit.fieldName] ==
G1["USD"](0).value().getJson(JsonOptions::none));
BEAST_EXPECT(!(ff[jss::Flags].asUInt() & lsfLowDeepFreeze));
BEAST_EXPECT(!(ff[jss::Flags].asUInt() & lsfHighDeepFreeze));
env.close();
}
}

void
testGlobalFreeze(FeatureBitset features)
{
Expand Down Expand Up @@ -4129,7 +4331,13 @@ struct AMMExtended_test : public jtx::AMMTest
{
using namespace test::jtx;
auto const sa = supported_amendments();
testRippleState(sa - featureDeepFreeze);
testRippleDeepState(sa - featureDeepFreeze);
testGlobalFreeze(sa - featureDeepFreeze);
testOffersWhenFrozen(sa - featureDeepFreeze);

testRippleState(sa);
testRippleDeepState(sa);
testGlobalFreeze(sa);
testOffersWhenFrozen(sa);
}
Expand Down
43 changes: 43 additions & 0 deletions src/test/app/AMM_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4397,6 +4397,48 @@ struct AMM_test : public jtx::AMMTest
0,
std::nullopt,
{features});

// Individually deep frozen account
if (features[featureDeepFreeze])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need to repeat the code. Just add a conditional error:

auto const err = features[featureDeepFreeze] ? ter(tecPATH_DRY) : ter(tesSUCCESS)

And then use it in pay:

env(pay(alice, carol, USD(1)),
            path(~USD),
            sendmax(XRP(10)),
            txflags(tfNoRippleDirect | tfPartialPayment),
            err);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, you shouldn't test here and other updated unit-tests if the feature is disabled because trust is going to fail. You need a separate test for disabled feature in SetTrust and also add to SetTrust tests for invalid combination of the flags. Also need to add OfferCreate and offer crossing, if not added yet, to Offer.

{
Copy link
Collaborator

Choose a reason for hiding this comment

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

AMM Create, Deposit, Withdraw are not impacted in any way by deep freeze, correct? Just want to check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

from design perspective it should not, deep freeze imposes a superset of restrictions of the existing regular freeze feature. so any restrictions in these transactions should have already been implemented through the freeze feature already.

testAMM(
[&](AMM& ammAlice, Env& env) {
env(trust(
gw, carol["USD"](0), tfSetFreeze | tfSetDeepFreeze));
env(trust(
gw, alice["USD"](0), tfSetFreeze | tfSetDeepFreeze));
env.close();
env(pay(alice, carol, USD(1)),
path(~USD),
sendmax(XRP(10)),
txflags(tfNoRippleDirect | tfPartialPayment),
ter(tecPATH_DRY));
},
std::nullopt,
0,
std::nullopt,
{features});
}
else
{
testAMM(
[&](AMM& ammAlice, Env& env) {
env(trust(
gw, carol["USD"](0), tfSetFreeze | tfSetDeepFreeze));
env(trust(
gw, alice["USD"](0), tfSetFreeze | tfSetDeepFreeze));
env.close();
env(pay(alice, carol, USD(1)),
path(~USD),
sendmax(XRP(10)),
txflags(tfNoRippleDirect | tfPartialPayment),
ter(tesSUCCESS));
},
std::nullopt,
0,
std::nullopt,
{features});
}
}

void
Expand Down Expand Up @@ -6882,6 +6924,7 @@ struct AMM_test : public jtx::AMMTest
testBasicPaymentEngine(all - fixAMMv1_1);
testBasicPaymentEngine(all - fixReducedOffersV2);
testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2);
testBasicPaymentEngine(all - featureDeepFreeze);
testAMMTokens();
testAmendment();
testFlags();
Expand Down
67 changes: 67 additions & 0 deletions src/test/app/Check_test.cpp
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems to missing some test when cashing a check. Example:

  1. alice is deep frozen on USD
  2. bob creates a USD check
  3. alice tries to cash the check (which should fail)

for these tests we should also add pre/post amendment behavior as Greg noted, would be easier to compare the difference in behavior

Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,72 @@ class Check_test : public beast::unit_test::suite
env(trust(alice, USD(0), tfClearFreeze));
env.close();
}
{
// Deep Frozen trust line. Check creation should be similar to
// payment behavior in the face of frozen trust lines.
env.trust(USD(1000), alice);
env.trust(USD(1000), bob);
env.close();
env(pay(gw1, alice, USD(25)));
env(pay(gw1, bob, USD(25)));
env.close();

// Setting trustline deep freeze in one direction prevents alice
// from creating a check for USD. And bob and gw1 should not be
// able to create a check for USD to alice.
env(trust(gw1, alice["USD"](0), tfSetFreeze | tfSetDeepFreeze));
env.close();
env(check::create(alice, bob, USD(50)), ter(tecFROZEN));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should add deep freeze check to CreateCheck transactor.

env.close();
env(pay(alice, bob, USD(1)), ter(tecPATH_DRY));
env.close();
env(check::create(bob, alice, USD(50)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this fail on check create? Alice is not allowed to receive anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

from my understanding we would always allow check creation, it would fail only when someone tries to cash it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any use-cases where CheckCreate needs to succeed for deep-frozen tokens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was an idea we discussed quite some time ago.

Basically, creation of the check is not a payment. It's only a promise to pay.

In that sense the freeze should not affect the creation of checks. It only should prevent cashing out of such checks when necessary.

Later on the sentiment has changed and we decided to address this behavior separately (if we feel like we need to). I'm still reworking Check_test and AMM_test so this is not yet final.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. The existing behavior returns tecFROZEN error even if the trust-line is regularly frozen. This file has three instances of tecFROZEN return code for GlobalFreeze, low-account-freeze and high-account-freeze of the trust-line.

Since a deep-frozen trust line implies that the trust-line is also regularly frozen, there should be continuity of behavior. Sorry, I didn't get around to reading the source code yesterday.

Any change in behavior will break backwards compatibility right?

env.close();
if (features[featureDeepFreeze])
env(pay(bob, alice, USD(1)), ter(tecPATH_DRY));
else
env(pay(bob, alice, USD(1)));
env.close();
env(check::create(gw1, alice, USD(50)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line appears to contradict the comment in line 560. gw1 has deep-frozen the trust-line with alice and USD Issued-Currency. Why is the CheckCreate operation successful here ?

env.close();
env(pay(gw1, alice, USD(1)));
env.close();

// Clear that freeze. Now check creation works.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could someone explain the difference in behavior when low-account deep-freezes a trust line versus high-account deep-freezes a trust line? Is there any impact on the behavior of Checks ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's technically no difference. High freeze and low freeze flags are simply used to figure out which counterparty enacted freeze. If we have 2 accounts, A and B, the hash of one of those must be higher (be it A > B, or B > A).

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, understood

env(trust(gw1, alice["USD"](0), tfClearFreeze | tfClearDeepFreeze));
Copy link
Collaborator

Choose a reason for hiding this comment

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

when the freeze is cleared, perhaps the trust-line limit should be higher? If you'd want send a payment through.

env.close();
env(check::create(alice, bob, USD(50)));
env.close();
env(check::create(bob, alice, USD(50)));
env.close();
env(check::create(gw1, alice, USD(50)));
env.close();

// Deep Freezing in the other direction does effect alice's USD
// check creation, and prevents bob and gw1 from writing a check
// for USD to alice.
env(trust(alice, USD(0), tfSetFreeze | tfSetDeepFreeze));
env.close();
env(check::create(alice, bob, USD(50)));
env.close();
if (features[featureDeepFreeze])
env(pay(alice, bob, USD(1)), ter(tecPATH_DRY));
else
env(pay(alice, bob, USD(1)));
env.close();
env(check::create(bob, alice, USD(50)), ter(tecFROZEN));
env.close();
env(pay(bob, alice, USD(1)), ter(tecPATH_DRY));
env.close();
env(check::create(gw1, alice, USD(50)), ter(tecFROZEN));
env.close();
env(pay(gw1, alice, USD(1)), ter(tecPATH_DRY));
env.close();

// Clear that deep freeze.
env(trust(alice, USD(0), tfClearFreeze | tfClearDeepFreeze));
env.close();
}

// Expired expiration.
env(check::create(alice, bob, USD(50)),
Expand Down Expand Up @@ -2719,6 +2785,7 @@ class Check_test : public beast::unit_test::suite
auto const sa = supported_amendments();
testWithFeats(sa - featureCheckCashMakesTrustLine);
testWithFeats(sa - disallowIncoming);
testWithFeats(sa - featureDeepFreeze);
testWithFeats(sa);

testTrustLineCreation(sa); // Test with featureCheckCashMakesTrustLine
Expand Down
Loading