Deep freeze (#5187) #5187

vvysokikh1 · 2024-11-11T20:27:42Z

High Level Overview of Change

This PR implements Deep Freeze feature described in this specification: XLS-77d

Context of Change

Added new flags and functionality allowing trust lines to be deep-frozen.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Specification: XRPLF/XRPL-Standards#220

to accepting the amendment.

bthomee · 2024-11-14T13:39:49Z

src/xrpld/app/paths/detail/StepChecks.h

        {
            return terNO_LINE;
        }
+        if (view.rules().enabled(featureDeepFreeze))
+        {
+            if (sle->isFlag(lsfHighDeepFreeze) || sle->isFlag(lsfLowDeepFreeze))
+            {
+                return terNO_LINE;
+            }
+        }


In the original code, the if (sle->isFlag((dst > src) ? lsfHighFreeze : lsfLowFreeze)) is used to check whether the high or low freeze flag is set based on whether the dst Account ID is numerically larger or smaller than the src Account ID.

In the newly added code, since there are differences in behavior between which party (account) sets the deep freeze flag, shouldn't the same logic be used here as above, i.e. if (sle->isFlag((dst > src) ? lsfHighDeepFreeze : lsfLowDeepFreeze))?

Or, since the spec states:

When an issuer enacts Deep-freeze, the following rules apply to the tokens in that trust line:

...

The counterparty can neither send nor receive from others on the deep-frozen trustline

...

and:

An individual address can deep-freeze its trust line to an issuer or financial institution ...

It prevents other addresses from sending that financial institution's tokens to the individual address.

It also prevents the individual address from sending the token to other non-issuer addresses.

then if either party deep freezes the trust line then terNO_LINE should be returned, in which case the current code is fine as-is?

applying the same logic could work. But i don't believe that's necessary because, if either low or high deep freeze flag is enabled, the trustline balance cannot be changed regardless which party enacted the deepfreeze (other than the scenario where there is a direct payment between issuer and holder)

I'll share a comment I wrote elsewhere on this topic:

I'm actually having a hard time wrapping my head around the idea that if either deep freeze flag is set, then the line is deep frozen.
The way I'm trying to reconcile it is that the only reason for the two flags is to be able to keep track of which side of the trust line set it, and thus can clear it.

It would probably be worth adding an explicit comment here and anywhere else relevant that, unlike regular freeze (or really any of the other flags), a deep frozen trustline acts the same regardless of which side froze it. The only reason for two flags is to track which side(s) froze the line.

bthomee · 2024-11-14T14:03:54Z

src/xrpld/app/tx/detail/SetTrust.cpp

+        bool willSetFreeze = false;
        if (bSetFreeze && !bClearFreeze && !sle->isFlag(lsfNoFreeze))
        {
            uFlagsOut |= (bHigh ? lsfHighFreeze : lsfLowFreeze);
+            willSetFreeze = true;
        }
        else if (bClearFreeze && !bSetFreeze)
        {
            uFlagsOut &= ~(bHigh ? lsfHighFreeze : lsfLowFreeze);
        }

+        bool const alreadyFrozen =
+            bHigh ? sle->isFlag(lsfHighFreeze) : sle->isFlag(lsfLowFreeze);
+        if (bSetDeepFreeze && !bClearDeepFreeze &&
+            (willSetFreeze || alreadyFrozen))
+        {
+            if (ctx_.view().rules().enabled(featureDeepFreeze))
+                uFlagsOut |= (bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);
+            else
+                return temMALFORMED;
+        }
+        else if (bClearDeepFreeze && !bSetDeepFreeze)
+        {
+            if (ctx_.view().rules().enabled(featureDeepFreeze))
+                uFlagsOut &= ~(bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);
+            else
+                return temMALFORMED;
+        }


Wouldn't the following be easier to follow?

Suggested change

bool willSetFreeze = false;

if (bSetFreeze && !bClearFreeze && !sle->isFlag(lsfNoFreeze))

{

uFlagsOut |= (bHigh ? lsfHighFreeze : lsfLowFreeze);

willSetFreeze = true;

}

else if (bClearFreeze && !bSetFreeze)

{

uFlagsOut &= ~(bHigh ? lsfHighFreeze : lsfLowFreeze);

}

bool const alreadyFrozen =

bHigh ? sle->isFlag(lsfHighFreeze) : sle->isFlag(lsfLowFreeze);

if (bSetDeepFreeze && !bClearDeepFreeze &&

(willSetFreeze || alreadyFrozen))

{

if (ctx_.view().rules().enabled(featureDeepFreeze))

uFlagsOut |= (bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);

else

return temMALFORMED;

}

else if (bClearDeepFreeze && !bSetDeepFreeze)

{

if (ctx_.view().rules().enabled(featureDeepFreeze))

uFlagsOut &= ~(bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);

else

return temMALFORMED;

}

if (bSetFreeze && !bClearFreeze && !sle->isFlag(lsfNoFreeze))

{

uFlagsOut |= (bHigh ? lsfHighFreeze : lsfLowFreeze);

}

else if (bClearFreeze && !bSetFreeze)

{

uFlagsOut &= ~(bHigh ? lsfHighFreeze : lsfLowFreeze);

}

// First (un)set the deep freeze flag independently of the regular freeze flag.

if (bSetDeepFreeze && !bClearDeepFreeze && !sle->isFlag(lsfNoFreeze))

{

uFlagsOut |= (bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);

}

else if (bClearDeepFreeze && !bSetDeepFreeze)

{

uFlagsOut &= ~(bHigh ? lsfHighDeepFreeze : lsfLowDeepFreeze);

}

// Next ensure the deep freeze flag can only be set when the regular freeze flag is set.

if ((!(uFlagsOut & lsfLowFreeze) && (uFlagsOut & lsfLowDeepFreeze)) ||

(!(uFlagsOut & lsfHighFreeze) && (uFlagsOut & lsfHighDeepFreeze)))

{

return temMALFORMED;

}

If you wish you can gate the deep freeze section by if (bDeepFreezeEnabled), but since the set and clear flags will be zero when deep freeze is disabled, this wouldn't be absolutely necessary. There'd be a tiny performance hit as long as deep freeze is disabled (namely, extra conditional statements that would always be no-ops), while gaining performance when it is enabled (namely, not performing the deep freeze enabled check).

I was looking into this and I have a strong feeling that we miss the correct check on ClearFreeze. It should also include checking if the trust line is not deep frozen.

bthomee · 2024-11-14T14:45:29Z

src/xrpld/ledger/detail/View.cpp

-        isFrozen(view, account, currency, issuer))
+        ((zeroIfFrozen == fhZERO_IF_FROZEN) &&
+         isFrozen(view, account, currency, issuer)) ||
+        isDeepFrozen(view, account, currency, issuer))


This code change, by adding an additional isDeepFrozen check, should be unnecessary. Namely, deep freezing a trust line is only possible when it is already regularly frozen, while having deep freeze without regular freeze is not allowed; hence, the regular freeze check is sufficient to capture both cases.

After reverting this change, the newly introduced isDeepFreeze function can then also be removed as it's not used anywhere else.

I think the isDeepFrozen check is necessary because either side can deep freeze an account. It's possible that the account set the deep freeze flag. isFrozen would return false in that case, because "normal" freeze only stops the other side of the line from sending.

Good point - I stand corrected.

ximinez

Partial review. I didn't go through all the tests.

src/xrpld/app/tx/detail/CreateOffer.cpp

src/xrpld/app/tx/detail/SetTrust.cpp

src/xrpld/ledger/detail/View.cpp

ximinez · 2024-11-15T23:09:09Z

src/xrpld/ledger/detail/View.cpp

-        isFrozen(view, account, currency, issuer))
+        ((zeroIfFrozen == fhZERO_IF_FROZEN) &&
+         isFrozen(view, account, currency, issuer)) ||
+        isDeepFrozen(view, account, currency, issuer))


I think the isDeepFrozen check is necessary because either side can deep freeze an account. It's possible that the account set the deep freeze flag. isFrozen would return false in that case, because "normal" freeze only stops the other side of the line from sending.

ximinez · 2024-11-15T23:10:56Z

src/xrpld/ledger/detail/View.cpp

+        ((zeroIfFrozen == fhZERO_IF_FROZEN) &&
+         isFrozen(view, account, currency, issuer)) ||
+        isDeepFrozen(view, account, currency, issuer))


However, I do think there is a more subtle mistake here, which is that the check for isDeepFrozen ignores the zeroIfFrozen flag. My understanding of fhIGNORE_FREEZE is that it should ignore both types of freezes because the caller needs the true balance of the trust line at that point.

Suggested change

((zeroIfFrozen == fhZERO_IF_FROZEN) &&

isFrozen(view, account, currency, issuer)) ||

isDeepFrozen(view, account, currency, issuer))

(zeroIfFrozen == fhZERO_IF_FROZEN) &&

(isFrozen(view, account, currency, issuer) ||

isDeepFrozen(view, account, currency, issuer)))

I wonder if that is actually a mistake. This function is used to check if this account is allowed to spend. So this code:

zeroIfFrozen == fhZERO_IF_FROZEN) && (isFrozen(view, account, currency, issuer)

verifies that this trust line is frozen by issuer? If fhIGNORE_FREEZE set that means that it was frozen by currency holder and he can still spend if he wants?

In this case the deep freeze should not look at the flag and not allow spending in any case.

Please advise if you think this is not the correct understanding

EDITED: changed to handle both freeze and deep freeze only if fhZERO_IF_FROZEN is set
I suggest this refactoring to make it a bit more readable:

auto const sle = view.read(keylet::line(account, issuer, currency)); auto const allowBalance = [&]() { if (!sle) { return false; } if (zeroIfFrozen == fhZERO_IF_FROZEN) { if (isFrozen(view, account, currency, issuer) || isDeepFrozen(view, account, currency, issuer)) { return false; } } return true; }(); if (allowBalance) { amount = sle->getFieldAmount(sfBalance); if (account > issuer) { // Put balance in account terms. amount.negate(); } amount.setIssuer(issuer); } else { amount.clear(Issue{currency, issuer}); }

src/xrpld/app/tx/detail/SetTrust.cpp

ximinez

Partial review

src/xrpld/app/paths/detail/StepChecks.h

src/xrpld/app/tx/detail/SetTrust.cpp

Co-authored-by: Ed Hennis <ed@ripple.com>

include/xrpl/protocol/LedgerFormats.h

src/xrpld/app/tx/detail/SetTrust.cpp

include/xrpl/protocol/detail/features.macro

src/xrpld/ledger/detail/View.cpp

src/test/app/AMMExtended_test.cpp

gregtatcam · 2024-12-05T14:26:41Z

src/test/app/AMM_test.cpp

@@ -4397,6 +4397,48 @@ struct AMM_test : public jtx::AMMTest
            0,
            std::nullopt,
            {features});
+
+        // Individually deep frozen account
+        if (features[featureDeepFreeze])


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);

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.

src/test/app/AMMExtended_test.cpp

src/xrpld/app/tx/detail/SetTrust.cpp

src/xrpld/app/paths/detail/StepChecks.h

gregtatcam

I left a few comments.
Check unit-tests are failing most likely because there is no deep freeze check in CreateCheck transactor.
I still have to review the unit-tests.

gregtatcam · 2024-12-16T21:17:16Z

src/xrpld/app/tx/detail/SetTrust.cpp

+        bool const bSetFreeze = (uTxFlags & tfSetFreeze);
+        bool const bSetDeepFreeze = (uTxFlags & tfSetDeepFreeze);
+
+        if (bNoFreeze)


Don't need nested if's.

gregtatcam · 2024-12-16T21:30:31Z

src/xrpld/app/tx/detail/OfferStream.cpp

+        {
+            permRmOffer(entry->key());
+            JLOG(j_.trace())
+                << "Removing deep frozen unfunded offer " << entry->key();


Should add offer_ = TOffer<TIn, TOut>{};

gregtatcam · 2024-12-16T21:46:44Z

src/test/app/Check_test.cpp

+            // 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));


Should add deep freeze check to CreateCheck transactor.

src/xrpld/app/tx/detail/SetTrust.cpp

src/xrpld/app/tx/detail/OfferStream.cpp

shawnxie999 · 2024-12-17T16:14:15Z

src/xrpld/ledger/detail/View.cpp

+    auto const sle = view.read(keylet::line(account, issuer, currency));
+    return sle &&
+        (sle->isFlag(lsfHighDeepFreeze) || sle->isFlag(lsfLowDeepFreeze));


personally i find it easier to read as:

auto const sle = view.read(keylet::line(account, issuer, currency)); if (!sle) return false; return sle->isFlag(lsfHighDeepFreeze) || sle->isFlag(lsfLowDeepFreeze);

shawnxie999 · 2024-12-17T16:17:17Z

src/xrpld/ledger/detail/View.cpp

@@ -857,6 +893,7 @@ trustCreate(
    const bool bAuth,           // --> authorize account.
    const bool bNoRipple,       // --> others cannot ripple through
    const bool bFreeze,         // --> funds cannot leave
+    bool bDeepFreeze,           // --> can neither receive nor send funds


should this be const bool?

not really, unless we have to follow the same approach which is kind of pointless.

There's no real benefit of making bool const here and some people consider that bad practice altogether.

gregtatcam

I'd like to see:

more tests where account deep freezes the trustline as opposed to the issuer and update all unit-tests for this scenario.
side by-side tests (if feature {} else {}), where applicable, to show a difference between regular freeze and deep freeze
more payment tests with AMM and general payment tests with more steps in the path, where the frozen path is in the middle.
should test the rippling via an account; i.e. an account is in the path.

gregtatcam · 2024-12-17T13:53:51Z

src/test/app/Freeze_test.cpp

@@ -186,6 +186,181 @@ class Freeze_test : public beast::unit_test::suite
        }
    }

+    void
+    testDeepFreeze(FeatureBitset features)


Should add tests where an account freezes a trustline as opposed to an issuer. All tests - freeze, offer, payment should have these tests. I think check has these tests.

gregtatcam · 2024-12-17T13:57:04Z

src/test/app/Check_test.cpp

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


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

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

gregtatcam · 2024-12-17T14:03:11Z

src/test/app/AMM_test.cpp

+
+        // Individually deep frozen account
+        if (features[featureDeepFreeze])
+        {


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

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.

gregtatcam · 2024-12-17T14:24:21Z

src/test/app/Freeze_test.cpp

-        auto let =
-            affected[0u][sfModifiedNode.fieldName][sfLedgerEntryType.fieldName];
-        BEAST_EXPECT(let == jss::AccountRoot);
+            // test: cannot deep freeze already frozen line


Wrong comment? It's not already frozen, it can't be frozen.

It's in the context of no freeze, when no freeze is enacted we can't deep freeze already frozen line

Edit: global freeze -> no freeze

gregtatcam · 2024-12-17T14:32:41Z

src/test/app/Freeze_test.cpp

+        env.fund(XRP(10000), G1, A1, A2, A3);
+        env.close();
+
+        env.trust(G1["USD"](1000), A1, A2, A3);


Consider using IOU type instead of operator[]. IMHO it's a bit easier to read:

auto const USD = G1["USD"]; env.trust(USD(1000), A1, A2, A3);

gregtatcam · 2024-12-17T15:00:10Z

src/test/app/Freeze_test.cpp

+                balance(A2, G1["USD"](998)),
+                offers(A1, 0));
+
+            // test: cannot create passive sell offer


should tfPassive be set?

gregtatcam · 2024-12-17T15:03:44Z

src/test/app/Freeze_test.cpp

+            env(trust(G1, A1["USD"](0), tfClearFreeze | tfClearDeepFreeze));
+            env.close();
+            env.require(balance(A1, G1["USD"](1001)), offers(A1, 0));
+        }


Should the same tests be implemented without the DeepFreeze to show the difference if any?

gregtatcam · 2024-12-17T15:04:13Z

src/test/app/Freeze_test.cpp

+            env(trust(G1, A1["USD"](0), tfClearFreeze | tfClearDeepFreeze));
+            env.close();
+        }
+    }


Should the same tests be implemented without the DeepFreeze to show the difference if any?

shawnxie999 · 2024-12-17T18:23:33Z

src/test/app/Check_test.cpp

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

alice is deep frozen on USD

bob creates a USD check

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

shawnxie999 · 2025-01-06T17:24:55Z

src/test/app/AMMExtended_test.cpp

+        testRippleState(sa - featureDeepFreeze);
+        testGlobalFreeze(sa - featureDeepFreeze);
+        testOffersWhenFrozen(sa - featureDeepFreeze);
+
        testRippleState(sa);
        testGlobalFreeze(sa);
        testOffersWhenFrozen(sa);


Can be refactored

for (auto const features : {sa, sa - featureDeepFreeze}) { testRippleState(features); testGlobalFreeze(features); testOffersWhenFrozen(features); }

HowardHinnant added 3 commits October 30, 2024 12:36

Implement Deep Freeze

e57d068

Specification: XRPLF/XRPL-Standards#220

[FOLD] Move feature to top of list

a0d48ae

[FOLD] Make use of the deep freeze flags malformed prior

4aac190

to accepting the amendment.

vvysokikh1 requested review from bthomee, shawnxie999 and ximinez November 11, 2024 20:27

bthomee reviewed Nov 14, 2024

View reviewed changes

Fixed creation of buy offer transaction to fail with tecFROZEN instantly

306a112

ximinez requested changes Nov 16, 2024

View reviewed changes

vvysokikh1 added 4 commits November 18, 2024 18:59

Fixed comments #1

f5efa31

Reworked freeze flag handling in SetTrust

8e1977f

Moved modified trustline flags extraction into separate function

a162663

Added more check to test cases

c7c26ac

vvysokikh1 force-pushed the deep_freeze branch from 9421c37 to c7c26ac Compare November 21, 2024 11:43

vvysokikh1 added 2 commits November 22, 2024 14:19

Added payment tests for freeze and deep freeze

805102a

Clarified behavior of individual freeze

de58a26

vvysokikh1 force-pushed the deep_freeze branch from ba87028 to de58a26 Compare November 22, 2024 17:58

vvysokikh1 added 2 commits November 28, 2024 16:46

Added offer test for freeze and deep freeze

91d291a

More offer tests

e0d59ef

ximinez requested changes Dec 4, 2024

View reviewed changes

src/xrpld/app/paths/detail/StepChecks.h Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/SetTrust.cpp Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/SetTrust.cpp Outdated Show resolved Hide resolved

vvysokikh1 and others added 3 commits December 4, 2024 10:21

Fixed typo

ffa6ccd

Co-authored-by: Ed Hennis <ed@ripple.com>

Fixed tx flags check in case trustline doesn't exist yet

f9107a7

Added tests for trustline creation freeze handling

1f9ecb8

gregtatcam reviewed Dec 5, 2024

View reviewed changes

vvysokikh1 added 3 commits December 6, 2024 15:10

Partially fixed comments

010f36d

Partially fixed comments

6bae95d

Fixed buy offer crossing when deep frozen

b5a10ec

vvysokikh1 changed the title ~~[WIP] Deep freeze~~ Deep freeze (#5187) Dec 10, 2024

vvysokikh1 marked this pull request as ready for review December 10, 2024 15:41

gregtatcam reviewed Dec 16, 2024

View reviewed changes

shawnxie999 reviewed Dec 17, 2024

View reviewed changes

src/xrpld/app/tx/detail/SetTrust.cpp Show resolved Hide resolved

shawnxie999 reviewed Dec 17, 2024

View reviewed changes

src/xrpld/app/tx/detail/OfferStream.cpp Show resolved Hide resolved

shawnxie999 reviewed Dec 17, 2024

View reviewed changes

gregtatcam reviewed Dec 17, 2024

View reviewed changes

shawnxie999 reviewed Dec 17, 2024

View reviewed changes

shawnxie999 reviewed Jan 6, 2025

View reviewed changes

Deep freeze (#5187) #5187

Are you sure you want to change the base?

Deep freeze (#5187) #5187

Conversation

vvysokikh1 commented Nov 11, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

Choose a reason for hiding this comment

shawnxie999 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysokikh1 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysokikh1 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysokikh1 commented Nov 11, 2024 •

edited

Loading

shawnxie999 Nov 14, 2024 •

edited

Loading

vvysokikh1 Nov 18, 2024 •

edited

Loading

gregtatcam left a comment •

edited

Loading

vvysokikh1 Jan 6, 2025 •

edited

Loading