DA compression for fuel-vm types #670
Conversation
There was a problem hiding this comment.
Thank you nice PR!=) It is cool that Compact can be derived almost for all types.
I feel like we put too many details about the implementation of the algorithm and how keys are created, and it seems we can simplify that part a lot to be agnostic. But the main with compaction looks good; I will review it one more time, more precisely when PR is in final shape.
BTW, compilation fails for me:
| let start: Key<T> = self.start_keys.value(); | ||
| let end: Key<T> = self.safe_keys_start.value(); | ||
| // Check if the value is in the possibly-overwritable range | ||
| if !key.is_between(start, end) { |
There was a problem hiding this comment.
What if the key is not in the range but at the beginning of the whole range, for example, zero? You will overflow the end and may also overwrite it.
| { | ||
| type Compact = ArrayWrapper<S, T::Compact>; | ||
|
|
||
| fn count(&self) -> CountPerTable { |
There was a problem hiding this comment.
Do we want to have a separate implementation for bytes later to be more performance?
There was a problem hiding this comment.
This fully monomorphizes. Zero-cost abstraction.
| if v == needle { | ||
| return Some(key); | ||
| } | ||
| key = key.next(); |
There was a problem hiding this comment.
How do we know that it will not overflow?
There was a problem hiding this comment.
It's allowed to overflow. Wrapping around is the expected behavior.
| } | ||
|
|
||
| tables!( | ||
| AssetId: [u8; 32], |
There was a problem hiding this comment.
Do we need to add ContractId as well?
| pub sender: Address, | ||
| /// The receiver on the `Fuel` chain. | ||
| #[cfg_attr(feature = "da-compression", da_compress(registry = "Address"))] | ||
| pub recipient: Address, | ||
| pub amount: Word, |
There was a problem hiding this comment.
I think we can skip amount since it is restorable informaiton
There was a problem hiding this comment.
Because we can find get from L1 message?
| pub recipient: Address, | ||
| pub amount: Word, | ||
| pub nonce: Nonce, | ||
| #[derivative(Debug(format_with = "fmt_as_field"))] | ||
| pub witness_index: Specification::Witness, | ||
| #[derivative(Debug(format_with = "fmt_as_field"))] | ||
| #[cfg_attr(feature = "da-compression", da_compress(skip))] |
There was a problem hiding this comment.
Hmmm, I think we need to keep the used gas price. It is true that we should be able to calculate it based on the other field of the transaction, but it requires VM initialization for fraud proofs wit right consensus parameters and maybe it is better to start without it for now.
There was a problem hiding this comment.
Ok sure. We can keep it for now.
| pub enum Output { | ||
| Coin { | ||
| #[cfg_attr(feature = "da-compression", da_compress(registry = "Address"))] |
There was a problem hiding this comment.
Hmm, I think it would be better to support the actual path to the table's type instead of using a string. Instead of registry = "Address" -> registry = ::fuel_compression::tables::Address or simply ::fuel_compression::tables::Address
There was a problem hiding this comment.
Proc macros had some limits around this, but maybe they have been lifted? I'll take a look.
| @@ -20,6 +20,7 @@ use rand::{ | |||
| #[derive(Debug, Default, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| #[cfg_attr(feature = "typescript", wasm_bindgen::prelude::wasm_bindgen)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
| #[cfg_attr(feature = "da-compression", derive(fuel_compression::Compact))] | |||
| #[derive(fuel_types::canonical::Deserialize, fuel_types::canonical::Serialize)] | |||
| pub struct UtxoId { | |||
There was a problem hiding this comment.
If I remember correctly, we agreed on replacing UtxoId with TxPointer. Do you plan to do that during this PR or not?
There was a problem hiding this comment.
I'm not planning to do that here, since that's separate from the BiggerType -> Key separation I'm truing to achieve in the PR. Maybe I'll add it here if I can refactor like this #670 (comment)
| { | ||
| /// Reverse lookup. | ||
| /// TODO: possibly add a lookup table for this, if deemed necessary | ||
| pub fn lookup_value(&self, needle: &T::Type) -> Option<Key<T>> { |
There was a problem hiding this comment.
I think we put too much business logic here with overflowing and counters. I feel like it should be part of the fuel-core while fuel-compression is responsible only for iterating over the fields and calling next_key::<Table>(value). The type that implements next_key may be decided in which way he wants to do that.
The count function also sounds like something related to the exact algorithm implementation.
There was a problem hiding this comment.
I agree; if I can get the logic separated it would be really nice. I'll look into it.
| for item in self.iter() { | ||
| count += item.count(); | ||
| } |
There was a problem hiding this comment.
| for item in self.iter() { | |
| count += item.count(); | |
| } | |
| count = self.iter().fold(count, |acc, &item| acc + item.count()); |
There was a problem hiding this comment.
I find the plain loop way easier to read.
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>
See correspoing fuel-core PR as well: FuelLabs/fuel-core#1609
This PR adds
Compactderive macro andfuel-compressionmachinery, that are used to convertTransactionand contained types intoCompactTransactionandCompact*. The compact types are equivalent to the original types, except that malleable fields are removed, and often-repeated types likeAssetIdandAddressare converted to three-byte keys to a separate database, called temporal storage. See the containedfuel-compression/README.mdfor an extended description.