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

fix todo: rollkit adapter multiple blocks submission #62

Open
pepyakin opened this issue Nov 27, 2023 · 6 comments
Open

fix todo: rollkit adapter multiple blocks submission #62

pepyakin opened this issue Nov 27, 2023 · 6 comments

Comments

@pepyakin
Copy link
Contributor

pepyakin commented Nov 27, 2023

right now, rollkit adapter API can submit several blobs at the same time and expects that a single value "da layer height" is returned. It looks like it's implied that the blobs are going to be submitted all at the same time.

I am not sure why they make such an assumption, but it looks like we could:

  1. leverage the existing batch functionality from the utility pallet. It looks like batch_all variety should work better, so that all blobs are either included or not included.
  2. roll our own implementation of submit_blob_batch and then reimplement submit_blob as a degenerate case of submit_blob_batch with one blob.
  3. fuse the blobs together in the rollkit adapter or dock.

The (2) approach seems to be (1) better on the first sight, at least because it's more efficient. Specifically, it performs a single check, instead of performing the checks for each blob. The (2) will require additional benchmarking on our side, whereas (1) won't?

Note

There is a weird thing, that if the blob validation fails, the blob will not get into the final NMT, but it will still occupy space in the block.

See https://github.com/thrumdev/sugondat/issues/74

The (3) maybe a good approach for now. It doesn't require any changes on the runtime side and should be pretty simple.

@gabriele-0201
Copy link
Contributor

Probably to implement the solution 3 is enough to accept all the blobs by the rollkit adapter (Vec<u8>) and then encode them as a scale encoded Vec<Vec<u8>>, then the shim will always decode before the blob in this way and then send all the extracted blobs back to the adapter. The implementation seems to be very straight forward but it creates some sort of a additional requirement to being able to reads blob uploaded into sugondat, other rollups that don't want to use the shim will need to adapt of the encoding used by the shim.

This logic is off chain though so we can easily change it in the future if we don't like it anymore, thus seems to be a good starting point so to have a demo compliant to what rollkit expects .

@pepyakin
Copy link
Contributor Author

While squashing the blobs on the shim side is fine, I think it would be better placed in the adapter. Why?

This operation is not an implementation detail. It seems to be actually important and we cannot sweep it under the rug. We could document that shim's Rollkit_submitBlob endpoint would perform squashing for the user, but that feels awkward. Also, the rollkit adapter is reachable for the Go developers. In contrast, it's less likely that they will go check the shim code. OTOH, there is not much we win of placing this code in Rust side, because I expect the amount of code should be really neglegible.

@rphmeier
Copy link
Contributor

Out of curiosity, how do the rollkit adapters for other DA layers implement this?

@pepyakin
Copy link
Contributor Author

pepyakin commented Dec 1, 2023

It seems like there are two adapters besides the celestia one. bitcoin-da doesn't have much logic and I am not sure how exactly it implements the DA layer API. Avail targets the new API (as described in https://github.com/thrumdev/sugondat/issues/8#issuecomment-1812832555 ), as can be seen here. The new API doesn't suffer from this issue, because it returns a vector of IDs instead of a single height. To recap, an ID could be a serialized height+extrinsic_index so that would work just fine.

@gabriele-0201
Copy link
Contributor

This operation is not an implementation detail

That's totally true, I tried to implement it just out of curiosity to understand what it should look like but I agree that it needs to be something really clear to rollkit developers!

@pepyakin
Copy link
Contributor Author

May be solved in #106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants