diba-io / bitmask-core

Core functionality for the BitMask wallet
https://bitmask.app
Other
95 stars 21 forks source link

Support CRDT to RgbWallet #353

Closed crisdut closed 1 year ago

crisdut commented 1 year ago

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

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

josediegorobles commented 1 year ago

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 commented 1 year ago
crisdut commented 1 year ago
  • 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 commented 1 year ago
crisdut commented 1 year ago
  • 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?

cryptoquick commented 1 year ago
josediegorobles commented 1 year ago

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)