XChainBridge

[seelabs]

High Level Overview of Change

This pull request introduces the functionality needed to support bridging assets across ledgers. This implementation focus on connecting two XRPL-like ledgers. It is meant to be used in conjunction with a witness server. The witness server has not been released, but an in-development version can be found here: https://github.com/seelabs/xbridge_witness and may be used to test this functionality. A python library to help setup and experiment with this code can be found here: https://pypi.org/project/xrpl-sidechain-cli/

The document in https://github.com/seelabs/rippled/blob/xbridge/docs/bridge/spec.md gives an overview of the new ledger object and transactions. It's recommended reading before diving into the code.

There are some unit tests included in the PR, but they are not complete, and more unit tests will be added during the review. Code documentation is also not complete, and additional code documentation will be added during the review.

There are 12 expected unit test failures, all from the KnownFormatToGRPC tests. This is because the new transactions and ledger objects do not have gRPC support, as this functionality is being removed (ref: https://github.com/XRPLF/rippled/pull/4272). Once that PR is merged I will rebase this PR and the unit test failures will go away.

The first commit in this PR introduces structured logging. That does not need to be reviewed in this pull request. I'll make a separate PR for that.

Type of Change

• [X ] New feature (non-breaking change which adds functionality)

[cjcobb23]

Just wanted to confirm, this doesn't add any new RPCs, but simply expands the functionality of ledger_entry. Is that correct?

[greg7mdp]

Just wanted to confirm, this doesn't add any new RPCs, but simply expands the functionality of ledger_entry. Is that correct?

Yes, ledger_entry can return the new xchain ledger objects. It also has updates to the account_objects (you can request jss::xchain_claim_id) and the subscribe RPC commands (new boundary tag).

[scottschurr]

I tried signing attestations using accounts that are present in the ledger. Then I gave the in-ledger accounts regular keys and disabled the master keys. That works with ordinary multisigning, but is not currently working for XChain XRP transfers.

I have a unit test that reproduces the problem here: https://github.com/scottschurr/rippled/commit/5f15add691eeefde3c615f3c667148c44a755a9b#diff-b4e237ef0eaad4f10295147c159b78f2258452beb7f25b5a3256452af51a7916R800

I think this is an important test case because fixing it requires changes to the XChainClaimAttestationBatchElement object members. An XChainClaimAttestationBatchElement currently looks like this:

            "XChainClaimAttestationBatchElement" : {
               "Account" : "<AccountID of source account>",
               "Amount" : "300000000",
               "AttestationRewardAccount" : "rJ8cp18RtUVDjgge6PFKDU4m9DqUN6marQ",
               "Destination" : "rGieEqubRyAuN55e2cT13XCN93MBzA5w43",
               "PublicKey" : "<public key of signer>",
               "Signature" : "...",
               "WasLockingChainSend" : 1,
               "XChainClaimID" : "1"
            }

Note that the the PublicKey and the Account are not related. If the PublicKey isn a regular key, then you have no information for looking up the account associated with that regular key.

So I suggest that the Account field should be the AccountID of the signer. That's exactly how it works for multisigning. But we still need a field that identifies the source of the funds that are being attested to (I think that's the information we're moving out of the Account field). If I got that right, then consider adding an OtherChainSource field to the XChainClaimAttestationBatchElement. The end result would look like this:

            "XChainClaimAttestationBatchElement" : {
               "Account" : "<AccountID of signer>",
               "Amount" : "300000000",
               "AttestationRewardAccount" : "rJ8cp18RtUVDjgge6PFKDU4m9DqUN6marQ",
               "Destination" : "rGieEqubRyAuN55e2cT13XCN93MBzA5w43",
               "OtherChainSource" : "<AccountID of source account>",
               "PublicKey" : "<public key of signer>",
               "Signature" : "...",
               "WasLockingChainSend" : 1,
               "XChainClaimID" : "1"
            }

That may not be the best solution and I'm very open to other ideas. But since XChain is leveraging the existing MultiSign behavior, I think the described problem really deserves a solution. And I'd like the solution to be as similar to preexisting multisigning as possible.

[scottschurr]

I have a question about whether we want to disallow a bridge where LockingChainDoor is the same as the LockingChainIssue::issuer.

Here's a bridge with that configuration:

{
   "IssuingChainDoor" : "ra8zzeHjuHrwxAnDKHbXXoxXEe5mDxM8mq",
   "IssuingChainIssue" : {
      "currency" : "SAU",
      "issuer" : "ra8zzeHjuHrwxAnDKHbXXoxXEe5mDxM8mq"
   },
   "LockingChainDoor" : "rUR5mVQjdEH6HEvUKrcBMhZh9Rcsh45GT6",
   "LockingChainIssue" : {
      "currency" : "XAU",
      "issuer" : "rUR5mVQjdEH6HEvUKrcBMhZh9Rcsh45GT6"
   }
}

Note that the LockingChainDoor and LockingChainCurrency:issuer values are identical.

First, it works. I tried it. No problems.

But it feels funny. If these two roles are shared, then there's nothing actually being "locked" in a documented way on the locking chain.

As is always true with an IOU, the issuer can issue an arbitrary amount. If the issuer were only issuing on-chain, we can derive the issuer's debt by summing all trust lines associated with the currency and issuer. And we can do that on any ledger. So the issuer's debt is recorded (indirectly) in every ledger.

However in this case the issuer is issuing off-chain. There is no immediate record of the issuer's debt. Their debt could be derived by going through the metadata of all of the historical XChainCommit transactions with this particular Door and summing them. But that could be a tall order. And that does not even track funds that are returned from the sidechain.

In contrast, if the LockingChainDoor and the LockingChainIssue:issuer are not the same, then there is a clear record kept on chain regarding exactly how much currency has been sent off chain. That record is in the trust line between the door and the issuer. So it's in a single easily identified place. Nice.

I like having that record. And not having that record makes me nervous.

So I'm wondering if we want to forbid the LockingChainDoor from being the same as the LockingChainIssue:issuer. It would be trivial to forbid. And we could loosen the rule later with an amendment if there were a good reason.

Thoughts? Thanks.

[scottschurr]

On a related note, let's look at the IssuingChain side of a bridge.

Here's one example of a bridge:

{
   "IssuingChainDoor" : "ra8zzeHjuHrwxAnDKHbXXoxXEe5mDxM8mq",
   "IssuingChainIssue" : {
      "currency" : "SAU",
      "issuer" : "ra8zzeHjuHrwxAnDKHbXXoxXEe5mDxM8mq"
   },
   "LockingChainDoor" : "rUR5mVQjdEH6HEvUKrcBMhZh9Rcsh45GT6",
   "LockingChainIssue" : {
      "currency" : "XAU",
      "issuer" : "rHzYmgPBCipE63Mzd3tBvd1Au3Ev1EMt6N"
   }
}

Notice that the IssuingChainDoor and IssuingChainIssue:issuer are identical. It turns out that is mandatory. There's a bit of code that enforces that rule. If the creator of the bridge doesn't follow that rule they get temSIDECHAIN_BAD_ISSUES.

The bridge is laid out with symmetry in mind. We'd like to believe that a bridge behaves symmetrically. But, as a matter of fact, it doesn't. The locking chain and the issuing chain follow different rules.

What I'd like to suggest is that the structure of the bridge could actually reflect some of the constraints that it is required to follow. This should reduce error checking and also reduce the size of the bridge in ledger. For example, we could do something like this:

{
   "IssuingChainDoor" : "ra8zzeHjuHrwxAnDKHbXXoxXEe5mDxM8mq",
   "IssuingChainCurrency" : "SAU",
   "LockingChainDoor" : "rUR5mVQjdEH6HEvUKrcBMhZh9Rcsh45GT6",
   "LockingChainIssue" : {
      "currency" : "XAU",
      "issuer" : "rHzYmgPBCipE63Mzd3tBvd1Au3Ev1EMt6N"
   }
}

This looks less symmetrical, but it presents fewer invalid options and reduces the on-ledger size.

Just a thought. The current implementation works.

[RichardAH]

Do we really need 14k LOC to implement cross chain functionality?

[scottschurr]

Judging by some of the added commits I thought that maybe the problem with in-ledger attestors with disabled master keys had been addressed. But it looks like I was wrong on that. Here's a link to a commit that demonstrates the problem is still present: https://github.com/scottschurr/rippled/commits/scottd-xchain-in-ledger-multisigners

[seelabs]

@scottschurr I added a patch to address the multisig issue you brought up. The relevant patch is b67562614 [WIP] More complete multi sign scheme for door accounts (and I just noticed the [WIP] tag. That should me [FOLD] - oh well). Peng is writing tests for this and the tests will be merged shortly.

[scottschurr]

Since attestations are no longer batched I think it would be worthwhile to audit XChain related names that include "batch". Maybe you want to save some of these names intentionally, but I'd guess that most of these names should be fixed in some way.

Here are the suspect names I found using grep:

• STXChainAttestationBatch.cpp
• STXChainAttestationBatch.h
• STXChainAttestationBatch (in a comment)
• sfXChainClaimAttestationBatch
• sfXChainClaimAttestationBatchElement
• sfXChainCreateAccountAttestationBatch
• sfXChainCreateAccountAttestationBatchElement
• XChainClaimAttestation::TBatchAttestation
• XChainCreateAccountAttestation::TBatchAttestation
• TAttestation::TBatchAttestation
• namespace AttestationBatch

I may have tagged some of these names elsewhere. If so, sorry for the duplication.

[seelabs]

@scottschurr Note: the "Make build more mainstream like" Does not need to be reviewed. It's there because I don't want to rebase onto develop while the review is still in progress.

[seelabs]

Squashed and rebased onto the latest develop branch.

[seelabs]

Note: I noticed a couple of tasks that I need to resolve:

1. I need to add a ticket sequence to the bridge transactions.
2. When merging I changed some AMM ids. I need to change these back to the origionals.
3. Disallow sidechains with IOUs that can clawback funds.

[intelliot]

Can you give an update on the following?

The first commit in this PR introduces structured logging. That does not need to be reviewed in this pull request. I'll make a separate PR for that.

[intelliot]

• some integration tests are being expanded/improved.
  • therefore, recommend waiting a couple days before merging this.
• @seelabs plans to rebase on develop ~next week (after testing is done)

note: developers are not blocked, and can already experiment/develop on the existing sidechain devnet.

• planning for a sidechain devnet reset ~next week, after this PR is merged.

[intelliot]

notes: sidechains testing is expected to complete in a couple days; also verifying one potential bug. can tag the release on Wednesday/Thursday, and use the beta release on Devnet when it is reset on Sep 19th.

[seelabs]

Squashed and merged onto the latest develop

[intelliot]

note: to make this commit easier to find in the future, consider adding "XChainBridge" (name of amendment) in the commit title

[intelliot]

suggested commit message title line:

`XChainBridge`: Introduce sidechain support (XLS-38): (#4292)

(the pattern of putting the amendment name first has been used before)