STR-726: Rework on Inscription Structure #518
Conversation
There was a problem hiding this comment.
Another more general issue here that's being continued which we should rectify is that we're being very loose with our terminology. I've tried to be consistent with the code that's written in the review comments here to make it easy to understand, but we should rename several of the types to better reflect this.
See @john-light 's comment on jira ticket: https://alpenlabs.atlassian.net/browse/STR-649?focusedCommentId=10918
The term "blob" in the context of rollups generally refers specifically to DA. We're using the term this way in the chainstate types, but in this inscription code we're being loose with it and also using it to refer to what we put checkpoints into. I suggest we refer to the term "payload" to refer specifically to the data that gets put inside an envelope. There was some fields named to this effect before but we should be more explicit about it.
Secondly, since we're trying to optimize more for efficiency rather than following the inscription format, we should move away from the term "inscription" internally. It's following the same envelope format, but the internal structure is completely different, for good reason. So I suggest that we use the term "bundle" (or "data bundle" or "envelope bundle", whichever is more appropriate in a context) here instead. A bundle explicitly contains one or more envelopes with a tag and a payload. The envelopes within a bundle are independent and could contain messages from multiple protocols, if we wanted to.
If we were adding support for Celestia DA, we'd publish the payloads as separate messages and reason about them separately. As we're doing internally with the BlobId types.
So with this we'd rename types like this:
InscriptionBlob->BundlePayloadBlobType->PayloadTypeTagBlobCommitment->PayloadCommitmentBlobSpec->PayloadSpec- also maybe introducing a new type
BundleSpecto describe multiple envelopes in a single tx BlobDest->DataBundleDestBlobIntent->PayloadIntent- possibly some others that I'm missing
And of course (unfortunately) we have to update the comments on these types to refer to these changes.
Make sure to read all the points in the ticket. There's some requirements around code organization to help keep things clean that need to be addressed.
john-light
changed the title
voidash
force-pushed
the
STR-649-FIX-INSCRIPTION-STRUCTURE
branch
from
51a71ea to
df4c511
Compare
voidash
force-pushed
the
STR-649-FIX-INSCRIPTION-STRUCTURE
branch
from
df4c511 to
f90a764
Compare
There was a problem hiding this comment.
The type hierarchy for the bundle/envelope/payload types still feels a bit strange. Not sure the best way to fix that without just rewriting all the structs.
I also noticed there's the waiting_da_blobs and related state fields. Don't change these now since they're far away from the writer logic. I'm working on a PR touching related code and these will probably be refactored then.
voidash
force-pushed
the
STR-649-FIX-INSCRIPTION-STRUCTURE
branch
from
34a37c9 to
04f59bf
Compare
btcio,tx-parser: change parsing logic for inscription, add new protocol operation tx-parser,rpc: add two submit blob rpc endpoint variant, fix envelope entering bug tx-parser: taplo format state,tx-parser: add comments remove inscription from codebase and make it envelope, resolve comments EnvelopeDB to CommitRevealDb create envelope-tx, split btcio rpc types to different crate store whole Vec<u8> instead of BundledCommitment on DA ProtocolOperation formatting remove descriptor.rs rename tx-parser to tx-filter and envelope-tx to reveal-tx datatool: better description for newly added cmds add _dd to codespellrc, remove smart-fee arg from datatool, use writerDB instead of sequencerDB add DAFilterMode, change da submit rpc fix formatting issues resolve comments
voidash
force-pushed
the
STR-649-FIX-INSCRIPTION-STRUCTURE
branch
from
04f59bf to
98e875e
Compare
There was a problem hiding this comment.
What we discussed in DMs was a sensible approach. This is very different. What was the reasoning for doing this? There's a few issues with it. It duplicates the high level envelope parsing logic and making 2 passes for checkpoints and DA. If we were to add another type of payload (like a forced inclusion) this would mean another instance of it.
We always want to take and parse the full contents of checkpoint payloads, so using the visitor-based approach doesn't help us much, it just complicates the code. The goal of using the visitors for DA was to ideally avoid copies depending on the context.
Description
Following are the main changes made for Inscription Transaction
Type of Change
Checklist
Related Issues
STR-726