Skip to content
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

Support CRDT to RgbWallet #353

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Support CRDT to RgbWallet #353

merged 1 commit into from
Aug 24, 2023

Conversation

crisdut
Copy link
Member

@crisdut crisdut commented Aug 23, 2023

Description

This PR allow us use CRDT (automerge) strategy in rgb_wallets to avoid loose contract state in concurrency operations.

Now, transfers operations pull "a fork" of the rgb_wallet from carbonado server to use in client-side (wasm32) and push a merged version of the rgb-wallet to carbonado server.

How it works
We have two new methods: retrieve_fork_wallets and store_fork_wallets. The first method make a copy of the wallet and the other method store the merged versoin of the wallet.

Limitations

  • To use automerge in your project we need use to derive macros: Hydrate and Reconcile. Unfortunately, we use a lot of structs based in other crates, and it would take a while to add automerge in all dependencies. One workaround used was to create the "raw structs". This struct is similar to "DTO (Data Transfer Object)" pattern, allow us use the derives to work with automerge.
  • Today the implementation is limited to RgbWallets. I intend to extend this to Stock as well, but creating a DTO for this type of structure can be quite painful, as it is not clear how strict-types can interfere with this.
  • Although the concurrency tests worked, I feel that we are not 100% sure about the solution, especially in high concurrency. To mitigate this, the rgb module was refactored to the "unit of work" pattern, to ensure that information persists only at the end of an operation. Another approach that we can use is to add cache in the files retrieval, of a few seconds, for the BME to get the "same" of the files.

By now, we use automerge and autosurgeon crates to handle the CRDT.

Misc
In the medium term, I intend to add this functionality directly to Stock, in rgb-wallet project, if I can combine with base58 (armored).

Closes #268

@crisdut crisdut requested review from cryptoquick and josediegorobles and removed request for cryptoquick August 23, 2023 03:04
@crisdut crisdut self-assigned this Aug 23, 2023
@crisdut crisdut requested a review from cryptoquick August 23, 2023 03:04
@crisdut crisdut added carbonado rgb enhancement New feature or request help wanted Extra attention is needed labels Aug 23, 2023
@crisdut crisdut added this to the 0.7.0 milestone Aug 23, 2023
@crisdut crisdut added bug Something isn't working and removed bug Something isn't working labels Aug 23, 2023
@crisdut crisdut modified the milestones: 0.7.0, 0.6.3 Aug 23, 2023
@crisdut crisdut changed the title Support to CRDT to RgbWallet Support CRDT to RgbWallet Aug 23, 2023
@crisdut crisdut force-pushed the CD/automerge branch 3 times, most recently from f12759b to 0c3fc74 Compare August 23, 2023 13:58
Copy link
Member

@josediegorobles josediegorobles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josediegorobles
Copy link
Member

I think the two errors I passed to you are related with previous errors but better be more or less sure that is that before merge.

@cryptoquick
Copy link
Member

  • This is a perfectly fine scope for this PR, but we definitely need Stock automerge for RGB Stock CRDTs #268 to be considered closed. It's possible we may need to convince Maxim it's okay to add Hydrate and Reconcile macros to strict_types. This of course must not affect consensus, and that can be verified through semantic ID.
  • I'm not too worried about high concurrency with individual wallet users. What might get tricky would be server wallets, but ideally if we have on-chain swap PSBTs, we won't ever need to take custody of RGB assets, and so we shouldn't need server wallets.

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic work and is a great start. Just got a couple minor changes requested.

@crisdut
Copy link
Member Author

crisdut commented Aug 23, 2023

  • This is a perfectly fine scope for this PR, but we definitely need Stock automerge for RGB Stock CRDTs #268 to be considered closed. It's possible we may need to convince Maxim it's okay to add Hydrate and Reconcile macros to strict_types. This of course must not affect consensus, and that can be verified through semantic ID.

Ok, sure.

I think CDRT works at the "plain text" level, so the merge should happen in a step before serialization. My only question is whether the reconcile process would work with these confined vectors. Today the main bug in transfers occurs in the RgbWallet wallet, as some taprets are being saved and as this part does not use strict-types, we end up using postcard encoding. I thought of creating a version of RgbWallet supporting strict-types, to test whether adding each new element would always make the file bigger and because of that, it would be saved in carbonado. But I guess that's not the real problem.

What do you think? Does this experiment make sense?

  • I'm not too worried about high concurrency with individual wallet users. What might get tricky would be server wallets, but ideally if we have on-chain swap PSBTs, we won't ever need to take custody of RGB assets, and so we shouldn't need server wallets.

I'm not so sure, I had done some simulations of how the BME behaves (concurrent accesses on a single wallet) and inevitably we can have serious cases where two versions of the same wallet writing concurrently can end up erasing previous information. One way to eliminate this was to use unit of work and now, automerge. But the way BME calls each operation individually and concurrently can end up impacting this. Do you agree?

@cryptoquick
Copy link
Member

  • I see, that's a good point. If this solves our immediate issue, that's good, we can consider RGB Stock CRDTs #268 closed by this PR. Automerge will result in bigger files as they're changed, but seeing as this is financial information, utmost paranoia is justifiable in technical design so funds are never lost. That is the primary engineering constraint, over a few kilobytes of storage space with every change.
  • Perhaps we can retrieve the c15 file a second time, further down in the file, to reconcile after changes have been made right before writing, to minimize the latency potential. Does that make sense?

@crisdut
Copy link
Member Author

crisdut commented Aug 23, 2023

  • I see, that's a good point. If this solves our immediate issue, that's good, we can consider RGB Stock CRDTs #268 closed by this PR. Automerge will result in bigger files as they're changed, but seeing as this is financial information, utmost paranoia is justifiable in technical design so funds are never lost. That is the primary engineering constraint, over a few kilobytes of storage space with every change.

Ok, I will make RgbWallet with strict-type in another branch to test, ok?

  • Perhaps we can retrieve the c15 file a second time, further down in the file, to reconcile after changes have been made right before writing, to minimize the latency potential. Does that make sense?

I implemented it in a similar way to this one, I'll detail it below:

  1. When we retrieve data from carbonado server, I store the "main" (Using git as an analogy) automerge information in another carbonado file (.c15) and return the "fork/branch" version of the RgbWallet
  2. Make all operations in web wallet (wasm32)
  3. Before send the data to carbonado server, I reconcile "fork" version with all changes occurred in the step 2
  4. Instead of sending the RgbWallet struct to the server, I only send the changes provided by the "fork/branch" version.
  5. In the carbonado server, I retrieve the data with "main" version of the RgbWallet and merge with changes from the fork version.
  6. I store the RgbWallet merged version in the file

This is the way I found that makes the most sense. Do you agree?

@crisdut crisdut requested a review from cryptoquick August 23, 2023 21:49
@cryptoquick
Copy link
Member

  • Sure, separate branch is good

  • Neat, so the merge happens serverside, too. I think that's good to support, I think at this point we might just pull bitmask-core into carbonado-node and other users can make use of automerge for their own applications.

@josediegorobles
Copy link
Member

It makes sense what you did @crisdut.
Apart from that I made a lot of testing and is all ok save, from the error that I passed you:
"Occurs an error in payment step. Consignmnet has not been completed. contract rgb:21QtLWSvB5SGV9c71hZyB83n7g5LyuKYMpMJXMAFnQYdm7KKF4 is unknown. Probably you haven't imported the contract yet."

It's only with one token that I created, but I created a lot more without any problems at all. And no problems after any transactions.

For me is ACK for merge (and for @cryptoquick to deploy a new npm rc)

Copy link
Member

@josediegorobles josediegorobles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK
Will merge this and build a new release. Any additional changes can come in on a new PR.

@cryptoquick cryptoquick merged commit 4ecc503 into CD/fix-storage Aug 24, 2023
@cryptoquick cryptoquick deleted the CD/automerge branch August 24, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carbonado enhancement New feature or request help wanted Extra attention is needed rgb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants