XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.52k stars 1.47k forks source link

A trust line with only lsf*Auth flags set is removed from the ledger [rippled version 1.12.0-rc4] #4698

Open scottschurr opened 1 year ago

scottschurr commented 1 year ago

Issue Description

A trust line is removed from the ledger unexpectedly.

Steps to Reproduce

The following unit test member function reproduces the problem.

    void
    testTrustLineWithOnlySetfAuthIsRemoved()
    {
        testcase("Trust line with only lsf*Auth");
        using namespace jtx;

        Env env{*this};

        // auth1 and auth2 are both issuers that require authorization.  They
        // will create a (required) trust line so they can cross offers between
        // themselves.
        Account const auth1("auth1");
        Account const auth2("auth2");
        auto const USD1 = auth1["USD"];
        auto const USD2 = auth2["USD"];

        env.fund(XRP(100000), auth1, auth2);
        env.close();
        env.require(owners(auth1, 0), owners(auth2, 0));

        // Trust lines to auth issuers must be authorized.
        env(fset(auth1, asfRequireAuth));
        env(fset(auth2, asfRequireAuth));
        env.close();

        // Since auth1 and auth2 have the asfRequireAuth flag set in their
        // accounts, they must explicitly give permission to holders of
        // their currencies.  This call creates a trust line with one of the
        // lsf*Auth flags set.
        env(trust(auth1, USD2(0), tfSetfAuth));
        env.close();
        // After creating the trust line we find this ledger entry as expected.
        BEAST_EXPECT(env.le(
            keylet::line(auth1.id(), auth2.id(), USD1.currency)));
        env.require(owners(auth1, 1), owners(auth2, 0));

        // Now auth2 reciprocates by attempting to set the other lsf*Auth
        // flag in the trust line.  Surprisingly, the trust line is removed
        // instead.
        env(trust(auth2, USD1(0), tfSetfAuth));
        env.close();
        // There's no good reason why the immediately preceeding TrustSet
        // should cause the previously inserted trust line to be removed
        // from the ledger.  But that's what happens.  So the following
        // tests fail.
        BEAST_EXPECT(env.le(
            keylet::line(auth1.id(), auth2.id(), USD1.currency)));
        env.require(owners(auth1, 1), owners(auth2, 1));
    }

The workaround is easy. Setting a non-zero trust limit changes the behaviors. Setting non-zero trust limits on both sides of the trust line results in expected behavior.

Given that there's an easy work around, use non-zero trust limits, this is not a high priority bug.

Environment

macOS Monterey 12.5.1 Apple clang version 13.1.6 (clang-1316.0.21.2.5)

ckeshava commented 11 months ago

Hello @scottschurr , Thank you for writing the unit test, it is helpful to visualize the context.

Is this not the intended behavior? I'm of the understanding that Trust Lines in their default settings are candidates for removal, irrespective of their Authorized flag status. (Docs: https://xrpl.org/trust-lines-and-issuing.html#reserves-and-deletion) But I'm not sure when this removal is triggered.

If we modify this behavior, we will have to rethink the conditions for deleting a Trust Line.

image
scottschurr commented 11 months ago

It looks like the behavior I'm seeing is documented and expected. So it makes sense to close this issue. Thanks for researching this and sorry for the noise.

mvadari commented 11 months ago

Hello @scottschurr , Thank you for writing the unit test, it is helpful to visualize the context.

Is this not the intended behavior? I'm of the understanding that Trust Lines in their default settings are candidates for removal, irrespective of their Authorized flag status. (Docs: https://xrpl.org/trust-lines-and-issuing.html#reserves-and-deletion)

Having the "Authorized" flag means that the trustline isn't in default settings. I've tried deleting trustlines with lsfAuth before, it doesn't work normally.

mvadari commented 11 months ago

Here's the script I wrote to test out this authorized trustline functionality, if anyone wants to replicate:

const xrpl = require("xrpl");

async function test() {
  const client = new xrpl.Client("wss://s.altnet.rippletest.net:51233");
  await client.connect();
  console.log("connected");
  const { wallet: issuer } = await client.fundWallet();
  const { wallet: customer } = await client.fundWallet();

  const accountSet = {
    TransactionType: 'AccountSet',
    Account: issuer.address,
    SetFlag: xrpl.AccountSetAsfFlags.asfRequireAuth,
  }
  console.log(await client.submitAndWait(accountSet, {autofill: true, wallet: issuer}))

  const customerTrust = {
    TransactionType: 'TrustSet',
    Account: customer.address,
    LimitAmount: {currency: "USD", issuer: issuer.address, value: "2"}
  }
  console.log(await client.submitAndWait(customerTrust, {autofill: true, wallet: customer}))

  const issuerTrust = {
    TransactionType: 'TrustSet',
    Account: issuer.address,
    LimitAmount: {currency: "USD", issuer: customer.address, value: "0"},
    Flags: xrpl.TrustSetFlags.tfSetfAuth,
  }
  console.log(await client.submitAndWait(issuerTrust, {autofill: true, wallet: issuer}))

  console.log((await client.request({
    command: 'account_objects',
    account: issuer.address,
  })).result.account_objects)

  console.log((await client.request({
    command: 'account_objects',
    account: customer.address,
  })).result.account_objects)

  const issuerDelete = {
    TransactionType: 'TrustSet',
    Account: issuer.address,
    LimitAmount: {currency: "USD", issuer: customer.address, value: "0"},
    Flags: 0,
  }
  console.log(await client.submitAndWait(issuerDelete, {autofill: true, wallet: issuer}))

  console.log((await client.request({
    command: 'account_objects',
    account: issuer.address,
  })).result.account_objects)

  console.log((await client.request({
    command: 'account_objects',
    account: customer.address,
  })).result.account_objects)

  await client.disconnect()
}

test()
ckeshava commented 11 months ago

I'm not able to run this script, perhaps I'm missing a dependency

$ python3 trustlineDel.py # I have used this file-name for your script.
  File "/Users/ckeshavabs/personal-code/rippled/cmake-build-debug/trustlineDel.py", line 1
    const xrpl = require("xrpl");
          ^^^^
SyntaxError: invalid syntax

The expected behavior of your code is: The issuer's account should not have any objects whereas the customer's account should have one trust line. Is this correct?

Since the customer has not reset the trust line settings to default, the trust line will still take up one owner reserve.

mvadari commented 11 months ago

It's Javascript, not Python.

The expected behavior is that the trustline is created + authorized, but trying to delete the trustline by resetting everything else to default settings (since you can't unset lsfAuth) doesn't work - the trustline still exists.

ckeshava commented 11 months ago

@mvadari Haha thanks for the clarification, my bad. Do you execute this with node.js runtime?

The customer has not reset the settings to default from their end. So the trust line should not be deleted. Doesn't the highlighted portion of the docs address your concern?

image
mvadari commented 11 months ago

@mvadari Haha thanks for the clarification, my bad. Do you execute this with node.js runtime?

Yes.

The customer has not reset the settings to default from their end. So the trust line should not be deleted. Doesn't the highlighted portion of the docs address your concern?

See the paragraph under the highlighted part.

ckeshava commented 11 months ago

Here's a unit-test in C++ that seems to contradict your findings. It's a slight modification of @scottschurr 's code above. I've tried to mimic your javascript code in that unit test.

In your code, you haven't reset the customer's limit to zero, do you intend to keep it that way? It should be reset to 0, if you want to delete that trust line.

Nonetheless, I observed something strange with the javascript code. The persistence of the trust lines occurs even without the use of Authorized flag.

1. Create a trust line with non-zero limit, do not use Authorised flag
2. Reset the trust line's limit to zero. Send the TrustSet transaction.
3. Check the account_objects on both the accounts --> The trust line is not deleted

JS Code:

const xrpl = require("xrpl");

async function test() {
  const client = new xrpl.Client("wss://s.altnet.rippletest.net:51233");
  await client.connect();
  console.log("connected");
  const { wallet: issuer } = await client.fundWallet();
  const { wallet: customer } = await client.fundWallet();

  console.log("Creating trust line from customer's end\n")
  const customerTrust = {
    TransactionType: 'TrustSet',
    Account: customer.address,
    LimitAmount: {currency: "USD", issuer: issuer.address, value: "2"}
  }
  console.log(await client.submitAndWait(customerTrust, {autofill: true, wallet: customer}))

  console.log("Creating trust line from issuer's end\n")
  const issuerTrust = {
    TransactionType: 'TrustSet',
    Account: issuer.address,
    LimitAmount: {currency: "USD", issuer: customer.address, value: "0"},
  }
  console.log(await client.submitAndWait(issuerTrust, {autofill: true, wallet: issuer}))

  const custDelete = {
    TransactionType: 'TrustSet',
    Account: customer.address,
    LimitAmount: {currency: "USD", issuer: issuer.address, value: "0"},
  }
  console.log(await client.submitAndWait(custDelete, {autofill: true, wallet: customer}))

  console.log("Completed the reset of trustline from customer's" +
      " perspective. Account objects for issuer and customer resp\n")
  console.log((await client.request({
    command: 'account_objects',
    account: issuer.address,
  })).result.account_objects)

  console.log((await client.request({
    command: 'account_objects',
    account: customer.address,
  })).result.account_objects)

  await client.disconnect()
}

test()

Results from the JS code: (the trustline has not been deleted from either party)

➜  cmake-build-debug git:(AuthFlag) ✗ node trustLineDel.js | tee resetLimitTo0                   
connected
Creating trust line from customer's end

{
  id: 30,
  result: {
    Account: 'rBDHZdESQMrFjaJ9w1DrGyM6WbdA7s9Pp9',
    Fee: '12',
    Flags: 0,
    LastLedgerSequence: 43559904,
    LimitAmount: {
      currency: 'USD',
      issuer: 'rsUMS4sCLnNZLHdDwAcgjkUjxN3aHPU6ke',
      value: '2'
    },
    Sequence: 43559884,
    SigningPubKey: 'ED5F2E1A61A51EA56FAE827524DE20F661738C4EC3DB42875D4FA68278797D9555',
    TransactionType: 'TrustSet',
    TxnSignature: '190AB2CFBC174097D8661A33FF6C231BC03948132ED518D94C1E39644DB08F887EDF48DDFAB4D4BC2E3BE5A2DD60C31561A96CFD5FEEC00B15DF2D4CA1E73B03',
    ctid: 'C298ABCE00000001',
    date: 755389311,
    hash: '1F0C8650127C8D2C95414C2825C0773A1E08197CCDCDDFA13197B85BF1DEEB77',
    inLedger: 43559886,
    ledger_index: 43559886,
    meta: {
      AffectedNodes: [Array],
      TransactionIndex: 0,
      TransactionResult: 'tesSUCCESS'
    },
    validated: true
  },
  type: 'response'
}
Creating trust line from issuer's end

{
  id: 44,
  result: {
    Account: 'rsUMS4sCLnNZLHdDwAcgjkUjxN3aHPU6ke',
    Fee: '12',
    Flags: 0,
    LastLedgerSequence: 43559906,
    LimitAmount: {
      currency: 'USD',
      issuer: 'rBDHZdESQMrFjaJ9w1DrGyM6WbdA7s9Pp9',
      value: '0'
    },
    Sequence: 43559882,
    SigningPubKey: 'ED27292B71AA4297D2301501CE68DAB7DD0D69D6083CD7A404B22CF386E6068D9D',
    TransactionType: 'TrustSet',
    TxnSignature: 'CCECD8CB70152342970832FA1ACD787CEC562CD7F0079693E615B961B4213F14EF36C27D91D0F27E8CC194822697B7D4E9305A2C7C89CD463D9D5CCCE74AAE0B',
    ctid: 'C298ABD000000001',
    date: 755389320,
    hash: 'FEF1023598540602D2CF1F007EE648683906D5E3F2DC1A77112A15604D064AF9',
    inLedger: 43559888,
    ledger_index: 43559888,
    meta: {
      AffectedNodes: [Array],
      TransactionIndex: 0,
      TransactionResult: 'tesSUCCESS'
    },
    validated: true
  },
  type: 'response'
}
{
  id: 60,
  result: {
    Account: 'rBDHZdESQMrFjaJ9w1DrGyM6WbdA7s9Pp9',
    Fee: '12',
    Flags: 0,
    LastLedgerSequence: 43559908,
    LimitAmount: {
      currency: 'USD',
      issuer: 'rsUMS4sCLnNZLHdDwAcgjkUjxN3aHPU6ke',
      value: '0'
    },
    Sequence: 43559885,
    SigningPubKey: 'ED5F2E1A61A51EA56FAE827524DE20F661738C4EC3DB42875D4FA68278797D9555',
    TransactionType: 'TrustSet',
    TxnSignature: '27BAE05F2FDCBEC0824C8ECF6CDD74BB8DD417330578BB3835708F3723D2D54CC82E50AB9035F672516A153FC93BAF6943477CEB39E3ADF021CDA0A9CBD72D01',
    ctid: 'C298ABD200020001',
    date: 755389330,
    hash: '7ADC9F28F08B91625396EF1AC13C54307F113786A8B2514FEB5C1E7055EB83D4',
    inLedger: 43559890,
    ledger_index: 43559890,
    meta: {
      AffectedNodes: [Array],
      TransactionIndex: 2,
      TransactionResult: 'tesSUCCESS'
    },
    validated: true
  },
  type: 'response'
}
Completed the reset of trustline from customer's perspective. Account objects for issuer and customer resp

[
  {
    Balance: {
      currency: 'USD',
      issuer: 'rrrrrrrrrrrrrrrrrrrrBZbvji',
      value: '0'
    },
    Flags: 1179648,
    HighLimit: {
      currency: 'USD',
      issuer: 'rBDHZdESQMrFjaJ9w1DrGyM6WbdA7s9Pp9',
      value: '0'
    },
    HighNode: '0',
    LedgerEntryType: 'RippleState',
    LowLimit: {
      currency: 'USD',
      issuer: 'rsUMS4sCLnNZLHdDwAcgjkUjxN3aHPU6ke',
      value: '0'
    },
    LowNode: '0',
    PreviousTxnID: '7ADC9F28F08B91625396EF1AC13C54307F113786A8B2514FEB5C1E7055EB83D4',
    PreviousTxnLgrSeq: 43559890,
    index: '0BD2358A2AF0A23AB5743B9C9D7FC4B5F6E4C1CB65D39467D67F20E7A9763417'
  }
]
[
  {
    Balance: {
      currency: 'USD',
      issuer: 'rrrrrrrrrrrrrrrrrrrrBZbvji',
      value: '0'
    },
    Flags: 1179648,
    HighLimit: {
      currency: 'USD',
      issuer: 'rBDHZdESQMrFjaJ9w1DrGyM6WbdA7s9Pp9',
      value: '0'
    },
    HighNode: '0',
    LedgerEntryType: 'RippleState',
    LowLimit: {
      currency: 'USD',
      issuer: 'rsUMS4sCLnNZLHdDwAcgjkUjxN3aHPU6ke',
      value: '0'
    },
    LowNode: '0',
    PreviousTxnID: '7ADC9F28F08B91625396EF1AC13C54307F113786A8B2514FEB5C1E7055EB83D4',
    PreviousTxnLgrSeq: 43559890,
    index: '0BD2358A2AF0A23AB5743B9C9D7FC4B5F6E4C1CB65D39467D67F20E7A9763417'
  }
]

I have modified your code slightly to demonstrate this issue. I'm not sure why the js and C++ code differ in their behavior. Am I missing something?

mvadari commented 11 months ago

It looks like you're never actually authorizing the trustline. The issuer needs to authorize it, not the customer. In this case, auth1 is the issuer and auth2 is the customer.

Some things that make that testcase confusing to understand:

ckeshava commented 11 months ago

Okay, I've fixed the mistakes in the previous iteration. Here's the unit test : https://github.com/XRPLF/rippled/compare/develop...ckeshava:rippled:AuthFlag

If you reset the limits of the trust lines to the default, they are deleted from the accounts. This holds true for Authorised Trust lines too.

This is incoherent with a section of the documentation

image
ckeshava commented 9 months ago

@mvadari I have an update on this issue.

There is an explanation for the difference in the behavior of C++ Unit tests and the Typescript libraries. many thanks to @ckniffen for brain-storming with me and helping me debug this issue.

When new accounts are created in C++ unit tests, "defaultRipple" : true is set in the AccountRoot object. On the other hand, the Client class in Typescript sets defaultRipple: false.

The behavior of C++ unit tests is documented here: https://github.com/XRPLF/rippled/blob/901152bd930447720ca4e738a10ffd878f401a53/src/test/jtx/Env.h#L574 I don't know if developers have a similar understanding of the TypeScript library code.

Typescript's generateFundedWallet sets the defaultRipple to false somewhere in its internals, I don't know the exact location yet.

Due to the above reasons, trust lines created in the two languages have different properties.

{ lsfHighReserve: true } // Trust lines created from C++ unit tests { lsfHighReserve: true, lsfLowNoRipple: true } // Trust lines created from Typescript Client class

Since the trust lines generated from Typescript is not in the default state, they are not deleted even if developers update balance and limit trust line settings to 0. Note: This behavior is observed even without the use of lsfAuthFlag. I didn't use that flag in my experiments to keep it simple.

I haven't debugged the python library yet, I'm facing a tangential issue on that front.

If this is a well-known intended behavior, then I'd recommend highlighting it in the docs. Otherwise, I'll look for specific places in the Typescript code that's causing this discrepency.