`fixTrustLinesToSelf`: Introduce amendment to handle trustlines to self #4105

a-noni-mousse · 2022-02-20T07:33:49Z

Trust lines to self are impossible but because of the old bug there are two such trust lines which exist:
2F8F21EFCAFD7ACFB07D5BB04F0D2E18587820C7611305BB674A64EAB0FA71E1 and 326035D5C0560A9DA8636545DD5A1B0DFCFF63E68D491B5522B767BB00564B1A. The fixTrustLinesToSelf fix amendment will remove them when it activates. I do not know how to test this change.

High Level Overview of Change

Context of Change

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)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Trust lines must be between two different accounts but two trust lines exist where an account extends trust to itself. They were created in the early days, likely because of bugs that have been fixed. The fixTrustLinesToSelf amendment will remove those trust lines when it activates.

manojsdoshi · 2022-05-04T19:04:51Z

@cryptobrad can you please rebase your PR to 1.9 it is giving us merge conflicts while adding this to the next beta. Thanks 🙏

Trustlines must be between two different accounts but two trustlines exist where an account extends trust to itself. They were created in the early days, likely because of bugs that have been fixed. The new fixTrustLinesToSelf amendment will remove those trustlines when it activates.

a-noni-mousse · 2022-07-29T19:20:21Z

I have finished the rebase. Will this be merged?

drlongle · 2023-02-15T11:56:30Z

src/ripple/app/tx/impl/Change.cpp

+        removeTrustLineToSelf(
+            sb,
+            uint256{
+                "326035D5C0560A9DA8636545DD5A1B0DFCFF63E68D491B5522B767BB00564B1A"sv}))


If the first call to removeTrustLineToSelf fails, the second call to removeTrustLineToSelf will not be executed. This is due to short-circuit evaluation.

I am not sure if this is intended but just want to point it out.

I did on purpose so if deletion fails amendment activation fails and does a roll back, but deletion would not fail.

drlongle · 2023-02-15T12:07:03Z

src/ripple/app/tx/impl/SetTrust.cpp

+    // two on 19-02-2022. This code was here to allow those trust lines to be
+    // deleted. The fixTrustLinesToSelf fix amendment will remove them when it
+    // enables so this code will no longer be needed.
+    if (!view().rules().enabled(fixTrustLinesToSelf) &&


I would drop !view().rules().enabled(fixTrustLinesToSelf) because I don't know if all trust lines to self have been deleted. I feel that this is less defensive as it could be.

Can't drop that because there are currently two trustlines to self. Without the check, servers with and without this PR would behave differently.

The code says that if fixTrustLinesToSelf is disabled and account_ == uDstAccountID, we can delete a trustline. Once fixTrustLinesToSelf is enabled, no other self-issued trustlines will be deleted because the first operand of the && operator will be false.

This is fine as long as we know that there are only two self-issued trustlines on the ledger.

intelliot · 2023-02-16T05:46:17Z

Because this PR was originally merged with only 1 approval (and we generally require 2 from core ledger engineers), I asked @drlongle to review. @a-noni-mousse @nbougalis could you respond to the thoughts above?

We see that there are two trust lines hard-coded to be deleted. Can someone share how they verified that these trust lines are actually to self? Also, can you share how you confirmed that there are no other trust lines to self?

intelliot · 2023-02-23T00:53:57Z

@a-noni-mousse @nbougalis if either of you would like to make the changes suggested by drlongle above, please open a new PR with your changes. Thank you!

intelliot · 2023-02-27T22:12:11Z

Take current ledger; iterate all trustline (RippleState) objects, and list any trustlines where the two sides of the trustline are equal to each other, i.e. if A and B are the same.

We know of these:

Test Plan:

Bring up a network that starts from current network state. (Copy database files from existing Mainnet server. No need for [full] history.) Use --ledger <ledger hash> (and maybe --load?)
Accelerate amendment activation (change timeout from 2 weeks to a short period of time, e.g. 1 hour). [feature] won't do the right thing as it won't activate the amendment in the normal way. Maybe amendment_majority?
Verify that the amendment worked correctly.

a-noni-mousse · 2023-03-01T03:52:37Z

Take current ledger; iterate all trustline (RippleState) objects, and list any trustlines where the two sides of the trustline are equal to each other, i.e. if A and B are the same.

We know of these:

* [2F8F21EFCAFD7ACFB07D5BB04F0D2E18587820C7611305BB674A64EAB0FA71E1](https://xrpl.org/websocket-api-tool.html?server=wss%3A%2F%2Fs1.ripple.com%2F&req=%7B%22command%22%3A%22ledger_entry%22%2C%22index%22%3A%222F8F21EFCAFD7ACFB07D5BB04F0D2E18587820C7611305BB674A64EAB0FA71E1%22%2C%22ledger_index%22%3A%22validated%22%7D)

* [326035D5C0560A9DA8636545DD5A1B0DFCFF63E68D491B5522B767BB00564B1A](https://xrpl.org/websocket-api-tool.html?server=wss%3A%2F%2Fs1.ripple.com%2F&req=%7B%22command%22%3A%22ledger_entry%22%2C%22index%22%3A%22326035D5C0560A9DA8636545DD5A1B0DFCFF63E68D491B5522B767BB00564B1A%22%2C%22ledger_index%22%3A%22validated%22%7D)

Test Plan:

* Bring up a network that starts from current network state. (Copy database files from existing Mainnet server. No need for [full] history.) Use `--ledger <ledger hash>` (and maybe `--load`?)

* Accelerate amendment activation (change timeout from 2 weeks to a short period of time, e.g. 1 hour). `[feature]` won't do the right thing as it won't activate the amendment in the normal way. Maybe `amendment_majority`?

* Verify that the amendment worked correctly.

I wrote code to parse the SHA Map with objects and find all the trustline where high and low accounts are same and only two you show exist in the recent ledger and no more can be created, but did not put it into this pr because it's not appropriate.

let me know what u need

intelliot · 2023-03-01T17:57:30Z

I wrote code to parse the SHA Map with objects and find all the trustline where high and low accounts are same and only two you show exist in the recent ledger and no more can be created, but did not put it into this pr because it's not appropriate.

For transparency and reproducibility, we would appreciate if you could share that code. If it's not much code, you can paste it here. If it's larger, you can put it in a public GitHub gist or repository.

intelliot · 2023-03-04T06:42:50Z

@sgramkumar At your convenience, please write up the results of your testing here.

sgramkumar · 2023-03-07T17:46:37Z

@sgramkumar At your convenience, please write up the results of your testing here.

This was tested with mainnet db dump, and accelerated amendment activation (fixTrustLinesToSelf) on 1.10.0-rc3
After the amendment was activated, the following 2 trustlines were removed

   "result" : {
      "error" : "entryNotFound",
      "ledger_hash" : "A7C9310D455DF826C01B41659C646D17CA796556D7E4BF392AF6A7A7F4872AAD",
      "ledger_index" : 78112941,
      "request" : {
         "api_version" : 1,
         "command" : "ledger_entry",
         "index" : "2F8F21EFCAFD7ACFB07D5BB04F0D2E18587820C7611305BB674A64EAB0FA71E1",
         "ledger_index" : "validated",
         "method" : "ledger_entry"
      },
      "status" : "error",
      "validated" : true
   }

(and for the other hash too)

nbougalis approved these changes Mar 21, 2022

View reviewed changes

a-noni-mousse force-pushed the patch7 branch from fedcf11 to 71bbf5a Compare July 19, 2022 18:57

a-noni-mousse force-pushed the patch7 branch from 71bbf5a to 2b4b630 Compare July 29, 2022 19:19

This was referenced Aug 4, 2022

Proposed 1.10.0-b1 #4265

Closed

Proposed 1.10.0-b1 #4270

Merged

nbougalis closed this in #4270 Aug 26, 2022

intelliot added Tech Debt Non-urgent improvements Amendment Testable labels Feb 14, 2023

drlongle reviewed Feb 15, 2023

View reviewed changes

intelliot reopened this Feb 16, 2023

intelliot added this to the 1.10.0 milestone Feb 16, 2023

intelliot mentioned this pull request Mar 2, 2023

Proposal: Amendment to clean up the "LedgerHashes" object #3218

Open

intelliot assigned sgramkumar Mar 7, 2023

intelliot requested a review from sgramkumar March 7, 2023 06:22

intelliot closed this Mar 15, 2023

intelliot changed the title ~~Trustlines to self are impossible but because of the old bug there ar…~~ fixTrustLinesToSelf: Introduce amendment to handle trustlines to self May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fixTrustLinesToSelf`: Introduce amendment to handle trustlines to self #4105

`fixTrustLinesToSelf`: Introduce amendment to handle trustlines to self #4105

a-noni-mousse commented Feb 20, 2022 •

edited by intelliot

manojsdoshi commented May 4, 2022

a-noni-mousse commented Jul 29, 2022

drlongle Feb 15, 2023

a-noni-mousse Mar 1, 2023

drlongle Feb 15, 2023

intelliot Feb 27, 2023

drlongle Feb 27, 2023 •

edited

intelliot commented Feb 16, 2023

intelliot commented Feb 23, 2023

intelliot commented Feb 27, 2023 •

edited

a-noni-mousse commented Mar 1, 2023

intelliot commented Mar 1, 2023

intelliot commented Mar 4, 2023

sgramkumar commented Mar 7, 2023

fixTrustLinesToSelf: Introduce amendment to handle trustlines to self #4105

fixTrustLinesToSelf: Introduce amendment to handle trustlines to self #4105

Conversation

a-noni-mousse commented Feb 20, 2022 • edited by intelliot

High Level Overview of Change

Context of Change

Type of Change

manojsdoshi commented May 4, 2022

a-noni-mousse commented Jul 29, 2022

drlongle Feb 15, 2023

Choose a reason for hiding this comment

a-noni-mousse Mar 1, 2023

Choose a reason for hiding this comment

drlongle Feb 15, 2023

Choose a reason for hiding this comment

intelliot Feb 27, 2023

Choose a reason for hiding this comment

drlongle Feb 27, 2023 • edited

Choose a reason for hiding this comment

intelliot commented Feb 16, 2023

intelliot commented Feb 23, 2023

intelliot commented Feb 27, 2023 • edited

a-noni-mousse commented Mar 1, 2023

intelliot commented Mar 1, 2023

intelliot commented Mar 4, 2023

sgramkumar commented Mar 7, 2023

`fixTrustLinesToSelf`: Introduce amendment to handle trustlines to self #4105

`fixTrustLinesToSelf`: Introduce amendment to handle trustlines to self #4105

a-noni-mousse commented Feb 20, 2022 •

edited by intelliot

drlongle Feb 27, 2023 •

edited

intelliot commented Feb 27, 2023 •

edited