-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
XLS-38d: XChainBridge (side chains) #4292
Conversation
Just wanted to confirm, this doesn't add any new RPCs, but simply expands the functionality of |
Yes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
@@ -3660,6 +3671,11 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) | |||
*stTxn, meta->getResultTER(), true, curTxLedger); | |||
jvTx[jss::meta] = meta->getJson(JsonOptions::none); | |||
jvTx[jss::account_history_tx_index] = txHistoryIndex--; | |||
|
|||
if (i + 1 == num_txns || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed before, i + 1 == num_txns
is not always a "ledger_boundary". If that is fine, probably change the name of jss::account_history_ledger_boundary
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, as noted earlier, I don't understand how this change could be related to XChain. If it is not needed by XChain the change should go in a different pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for an optimization where attestations are submitted in batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for an optimization where attestations are submitted in batch.
I promise to not continue to beat the dead horse. But I have to give it one more thump.
- Attestations are no longer submitted in batch.
- Even if batch attestation submission is coming, there's no reason this change could not go into a different pull request.
I'm not saying this change is not needed. I'm just saying it doesn't belong in this pull request. Put it in a different pull request. The change is invisible in this pull request because XChain is so big.
No, this is not a hill in intend to die on. Do what you feel you need to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hear you, but I'd still like to keep this.
|
||
// Account create transactions must happen in order | ||
if (rewardAccounts && claimCount + 1 == attBegin->createCount) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question here because of above code comment:
"// Modify the object before it's potentially deleted, so the meta data
// will include the new attestations"
What if it has a quorum of signatures and no sleCID
? It seems the sleCID
will not be created. I think that is probably fine since the newly created sleCID
will be deleted immediately. What about metadata, is sleCID
needed for SDK libs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mostly comes up in testing, which is why I try to include as much metadata as I can. But we can't create metadata for the situation you describe (as you know). SDK libs should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few initial comments on the spec as I try to get oriented. I've not looked at any code yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intermediate review comments. Spotted a couple of weird looking things while trying to understand transactors...
CONSTRUCT_TYPED_SFIELD(sfIssuingChainIssue, "IssuingChainIssue", ISSUE, 2); | ||
|
||
// Bridge | ||
CONSTRUCT_TYPED_SFIELD(sfXChainBridge, "XChainBridge", XCHAIN_BRIDGE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be done with an InnerObjectFormat
. Is there a good reason not to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STObject can not contain STObjects - the serialization format doesn't support it. We can't have InnerObjectFormats that are part of a transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that you're seeing a problem. But it's not as simple as what you are describing.
An STObject
can absolutely contain a nested STObject
. It serializes and deserializes just fine. Here's a JSON representation of one case where it happens in metadata:
"CreatedNode" : {
"LedgerEntryType" : "DirectoryNode",
"LedgerIndex" : "3F42292CB6EBC9492B157DE0A9957A4F5A0A70C096E5C3E5F8F40700A79256E4",
"NewFields" : {
"Owner" : "rKutULRC2RumKvXiEoE7fDqHcHXEiUczf3",
"RootIndex" : "3F42292CB6EBC9492B157DE0A9957A4F5A0A70C096E5C3E5F8F40700A79256E4"
}
}
The CreatedNode
STObject
directly contains the NewFields
STObject
. I watched this take place while reconstituting the serialized metadata into an STObject
using the debugger. It really does work.
I wonder if maybe there are currently no incoming transactions where an STObject
directly contains another STObject
? If that's the case, then possibly there's a problem (bug?) with the parsing from JSON to STObject
?
That said, I'm pretty well convinced that what you're describing is not a limitation of the serialization format. I think you are seeing a different issue.
src/ripple/app/misc/NetworkOPs.cpp
Outdated
if (last) | ||
jvObj[jss::account_history_ledger_boundary] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jvObj[jss::account_history_ledger_boundary] = true;
should only be added if this is an account history subscribe. So these two lines should be moved into the for loop below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jvObj[jss::account_history_ledger_boundary] = true; should only be added if this is an account history subscribe
Hum, I don't think so, the boundary information is useful in both directions (going back for historical txs and forward for new tx). Maybe I should rename jss::account_history_ledger_boundary
to jss::ledger_boundary
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled how adding this field could have anything to do with XChain. The XChain branch is complex enough that I think side fixes (like this?) should go in another pull request.
Can someone justify that is change is needed for XChain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ledger boundary is needed for an optimization where we are submitting attestations in batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, since attestations are no longer submitted as a batch, the optimization is no longer required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not required, although I'd like to keep it. I do intend on re-add batch attestations, and this seems like a low-risk, minor addition to the rpc command. However, I also understand the desire to remove it. If you feel strongly, we can remove it, although I really would like to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about it. But XChain is a big change. I think it's smart to limit the scope where we can. It sounds like this change can be postponed until batch attestations are re-added? Is there anything critical about introducing this change now? That said, if you need the change then you need it.
Also, this is the first time I've heard that you intend to re-add attestation batching. Please think long and hard before re-adding batch attestations. Batching attestations created several complexities:
- The batched handling inside the server was messy,
- The handling of mixed successful and unsuccessful attestations and their error codes,
- Multiple payments in a single transaction if attestations to different destinations are in the same batch.
I really doubt the benefits of batching out weigh the added complexity and opportunity for error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I don't intend to add batch attestations before launch. They are an optimization. We can decide if we need them later.
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: scottschurr@5f15add#diff-b4e237ef0eaad4f10295147c159b78f2258452beb7f25b5a3256452af51a7916R800 I think this is an important test case because fixing it requires changes to the
Note that the the So I suggest that the
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few extra intermediate comments...
I have a question about whether we want to disallow a bridge where Here's a bridge with that configuration:
Note that the 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 In contrast, if the I like having that record. And not having that record makes me nervous. So I'm wondering if we want to forbid the Thoughts? Thanks. |
On a related note, let's look at the Here's one example of a bridge:
Notice that the 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:
This looks less symmetrical, but it presents fewer invalid options and reduces the on-ledger size. Just a thought. The current implementation works. |
Do we really need 14k LOC to implement cross chain functionality? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more intermediate comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished my review. Whew. Most comments are inline, but there are a few notes that don't fit the inline format. I'm leaving those here.
Naming
I disagree with a couple of the naming choices made here. But just because I disagree doesn't make me right. Here are naming changes I recommend:
-
I think Doors on Bridges make no intuitive sense. Physical bridges don't have doors. I think all three of these items (two doors and a bridge) make a Portal. There would probably be a Portal, and LockingPortal, and an IssuingPortal, but they are all really components of the same Portal.
-
I don't think you have Witnesses here, I think you have Attestors instead. Attestors provide Attestations.
Attestations Do Many Things
There's a problem with how many different things can happen in a single attestation batch. One attestation batch can:
- Perform multiple payments and/or create one or more accounts.
- Add multiple attestations. None, one, some, or all of the contained attestations may be invalid.
So, conceptually, a single attestation batch may have many successful operations as well as many failed operations. But the attestation batch only gets to return one result.
I think this conundrum deserves a special, and loud, call out in the documentation. And it's worth being really clear about the consequences of the chosen rules.
One part of this that's really important is that every single successful attestation from the batch must be added to the ClaimID
. Otherwise that ClaimID
may never be successfully claimed. If that means the every attestation batch that contains any attestations always returns tesSUCCESS`, then that's what it takes.
Timeouts
I think this whole thing could be made more error resilient by adding timeouts to the process of collecting attestations. At some point we just don't expect more attestations to come in. Once we get to that point it ought to be possible to submit a claim. I have to admit, I've not thought through the full implementation consequences of these timeouts. But I believe its worth figuring out.
Unit Tests
I didn't review the unit tests. I spent a lot of time struggling to understand them, but I didn't review them. For me, personally, the unit test code had factored many of the important details into hard to find corners. I was unable to understand what were, to me, important parts of the information flow given the way that the tests were written. So I gave up and wrote my own unit tests aimed at developing my comprehension of XChain rather than test coverage.
I don't think I could do a fair job of reviewing the unit tests given the way they are structured. I'd probably just rewrite them, which wouldn't help anybody.
src/ripple/app/misc/NetworkOPs.cpp
Outdated
if (last) | ||
jvObj[jss::account_history_ledger_boundary] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled how adding this field could have anything to do with XChain. The XChain branch is complex enough that I think side fixes (like this?) should go in another pull request.
Can someone justify that is change is needed for XChain?
@@ -3660,6 +3671,11 @@ NetworkOPsImp::addAccountHistoryJob(SubAccountHistoryInfoWeak subInfo) | |||
*stTxn, meta->getResultTER(), true, curTxLedger); | |||
jvTx[jss::meta] = meta->getJson(JsonOptions::none); | |||
jvTx[jss::account_history_tx_index] = txHistoryIndex--; | |||
|
|||
if (i + 1 == num_txns || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, as noted earlier, I don't understand how this change could be related to XChain. If it is not needed by XChain the change should go in a different pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commits look good as far as I can see. 👍
note: developers are not blocked, and can already experiment/develop on the existing sidechain devnet.
|
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. |
A bridge connects two blockchains: a locking chain and an issuing chain (also called a mainchain and a sidechain). Both are independent ledgers, with their own validators and potentially their own custom transactions. Importantly, there is a way to move assets from the locking chain to the issuing chain and a way to return those assets from the issuing chain back to the locking chain: the bridge. This key operation is called a cross-chain transfer. A cross-chain transfer is not a single transaction. It happens on two chains, requires multiple transactions, and involves an additional server type called a "witness". A bridge does not exchange assets between two ledgers. Instead, it locks assets on one ledger (the "locking chain") and represents those assets with wrapped assets on another chain (the "issuing chain"). A good model to keep in mind is a box with an infinite supply of wrapped assets. Putting an asset from the locking chain into the box will release a wrapped asset onto the issuing chain. Putting a wrapped asset from the issuing chain back into the box will release one of the existing locking chain assets back onto the locking chain. There is no other way to get assets into or out of the box. Note that there is no way for the box to "run out of" wrapped assets - it has an infinite supply. Co-authored-by: Gregory Popovitch <[email protected]>
Squashed and merged onto the latest develop |
note: to make this commit easier to find in the future, consider adding "XChainBridge" (name of amendment) in the commit title |
suggested commit message title line:
(the pattern of putting the amendment name first has been used before) |
This was likely put back when #4292 was rebased.
This was likely put back when #4292 was rebased.
A bridge connects two blockchains: a locking chain and an issuing chain (also called a mainchain and a sidechain). Both are independent ledgers, with their own validators and potentially their own custom transactions. Importantly, there is a way to move assets from the locking chain to the issuing chain and a way to return those assets from the issuing chain back to the locking chain: the bridge. This key operation is called a cross-chain transfer. A cross-chain transfer is not a single transaction. It happens on two chains, requires multiple transactions, and involves an additional server type called a "witness". A bridge does not exchange assets between two ledgers. Instead, it locks assets on one ledger (the "locking chain") and represents those assets with wrapped assets on another chain (the "issuing chain"). A good model to keep in mind is a box with an infinite supply of wrapped assets. Putting an asset from the locking chain into the box will release a wrapped asset onto the issuing chain. Putting a wrapped asset from the issuing chain back into the box will release one of the existing locking chain assets back onto the locking chain. There is no other way to get assets into or out of the box. Note that there is no way for the box to "run out of" wrapped assets - it has an infinite supply. Co-authored-by: Gregory Popovitch <[email protected]>
This was likely put back when XRPLF#4292 was rebased.
A bridge connects two blockchains: a locking chain and an issuing chain (also called a mainchain and a sidechain). Both are independent ledgers, with their own validators and potentially their own custom transactions. Importantly, there is a way to move assets from the locking chain to the issuing chain and a way to return those assets from the issuing chain back to the locking chain: the bridge. This key operation is called a cross-chain transfer. A cross-chain transfer is not a single transaction. It happens on two chains, requires multiple transactions, and involves an additional server type called a "witness". A bridge does not exchange assets between two ledgers. Instead, it locks assets on one ledger (the "locking chain") and represents those assets with wrapped assets on another chain (the "issuing chain"). A good model to keep in mind is a box with an infinite supply of wrapped assets. Putting an asset from the locking chain into the box will release a wrapped asset onto the issuing chain. Putting a wrapped asset from the issuing chain back into the box will release one of the existing locking chain assets back onto the locking chain. There is no other way to get assets into or out of the box. Note that there is no way for the box to "run out of" wrapped assets - it has an infinite supply. Co-authored-by: Gregory Popovitch <[email protected]>
This was likely put back when XRPLF#4292 was rebased.
A bridge connects two blockchains: a locking chain and an issuing chain (also called a mainchain and a sidechain). Both are independent ledgers, with their own validators and potentially their own custom transactions. Importantly, there is a way to move assets from the locking chain to the issuing chain and a way to return those assets from the issuing chain back to the locking chain: the bridge. This key operation is called a cross-chain transfer. A cross-chain transfer is not a single transaction. It happens on two chains, requires multiple transactions, and involves an additional server type called a "witness". A bridge does not exchange assets between two ledgers. Instead, it locks assets on one ledger (the "locking chain") and represents those assets with wrapped assets on another chain (the "issuing chain"). A good model to keep in mind is a box with an infinite supply of wrapped assets. Putting an asset from the locking chain into the box will release a wrapped asset onto the issuing chain. Putting a wrapped asset from the issuing chain back into the box will release one of the existing locking chain assets back onto the locking chain. There is no other way to get assets into or out of the box. Note that there is no way for the box to "run out of" wrapped assets - it has an infinite supply. Co-authored-by: Gregory Popovitch <[email protected]>
This was likely put back when XRPLF#4292 was rebased.
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 XLS-38d specification gives an overview of the new ledger object and transactions. It's recommended reading before diving into the code.
For high-level discussions relating to side chains, you can start a new discussion here.
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: #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