feat(cannon): Binary serialization for snapshots #7559
Conversation
clabby
force-pushed
the
cl/cannon-ser
branch
2 times, most recently
from
883c57e to
37ee20f
Compare
|
Alternatively, if we don't care about compatibility with other VMs, it'll be much easier to use Go's built-in codec. gob alleviates the need to write and maintain the serde routines when new fields are added. It uses reflection to figure out the encoding. |
One of the primary reasons for this change is to define an easy-to-implement codec for the state snapshots so that other VM implementations, namely |
VMs that represent program memory differently, including cannon-rs, will have to massage the data quite a bit to integrate the snapshot. It follows that the snapshots should be in a standard format so it's easy to parse. We sorta had this with the JSON serializer, but an unstructured binary representation will be cumbersome and error-prone to parse. I suggest using something like protobuf, flatbuffers, or even rlp (ok maybe not), if the goal is to allow other VMs use the same snapshot format. |
|
Also, I dunno if it's a realistic usecase to support loading and dumping snapshots using different VM implementations. It's probably good to have something like this as a test vector, but not worth it as a user-facing feature imo. |
|
I'd potentially be open to changing op-challenger to not read cannon snapshots as well. We could run a cannon subcommand to convert a snapshot to a claim hash instead. We'd have to review the use cases to know what that would take but it would be good to keep them more decoupled if possible. |
| @@ -70,6 +72,214 @@ func (s *State) EncodeWitness() StateWitness { | |||
| return out | |||
| } | |||
|
|
|||
| func (s *State) Serialize(out io.Writer) error { | |||
There was a problem hiding this comment.
The state is so small (< 500 bytes) that it may just be worth serializing/deserializing the state witness, and writing/reading it as one blob into the writer, to avoid many different write/read calls.
|
See above suggestion regarding state serialization. Another benefit would also be that if we include the memory-hash as part of the state witness in the snapshot, then you can determine the claim-hash by just parsing and hashing the witness part of the full snapshot, which can be very fast, as you don't have to load the full state. |
|
If we prefix the state snapshot with the claim hash of the snapshot, maybe we don't have to invoke any binary at all, and are still able to generalize it, since the challenger would just always read the first 32 bytes as claim hash, and ignore the rest of the snapshot? |
This would definitely be nice if we stick with this format - good thinking. Stepping back a bit, this change can be slowrolled as there's no immediate need for it - What if we thought about making a common FFI schema for all of the VM implementations? The abstraction problem seems to be the binary + the op-challenger using go cannon-specific types for serialization etc. If not for that, the VM impls wouldn’t have to care about the serialization format at all. With a nice FFI schema for the VMs to implement, we could interact with VM implementations as a library in the op-challenger and galadriel (in the future) + a binary that supports it, and the user of the interface could decide if they’d like to even make snapshots, etc., taking that responsibility off of a specific VM implementation's types. Each one could decide how it wants to do this, and the interface could just have a standardized binary format similar to this one, but with a bit more rigidity. This would make it easier to use several different VM implementations in a drop-in way for services that utilize the interfaces (which can be implemented in multiple languages) and then leave everything else to the consumer, which would be nice. |
|
I like the idea of prefixing the snapshot with the key data as a simple but very effective step forward. I'm unclear on how much effort an FFI type schema would take to setup. The challenger has a few requirements that may not be obvious here though:
So we'd need to be able to easily get both the claim hash and the preimage (the state witness). We don't really want to just get the state witness and hash it ourselves as the hashing process requires setting the status byte which would be good to keep inside cannon if possible. Could be It would be fine for the VM to be just given a directory for its own use and it can manage snapshots etc itself, though the challenger assumes fetching a previously generated proof is fast currently. It is useful for the challenger to have a flag to control how often snapshots are stored to let the user trade off execution time vs disk space. Beyond that I'm a fan of simplifying the |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7559 +/- ##
===========================================
- Coverage 53.48% 52.41% -1.08%
===========================================
Files 162 163 +1
Lines 6048 6250 +202
Branches 970 970
===========================================
+ Hits 3235 3276 +41
- Misses 2691 2798 +107
- Partials 122 176 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -70,6 +72,214 @@ func (s *State) EncodeWitness() StateWitness { | |||
| return out | |||
| } | |||
|
|
|||
| func (s *State) Serialize(out io.Writer) error { | |||
There was a problem hiding this comment.
I'd like a quick spec on the format before we merge
|
Consensus seems to be this is a worthwhile change and should be fine to merge, but it's worth documenting the binary format first. The aim is just to make it easier to understand the format, not to set it as an official standard format - we may well make further changes to this file format and beak compatibility prior to fault proofs shipping to mainnet. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| serMemBuf := new(bytes.Buffer) | ||
| err := s.Memory.Serialize(serMemBuf) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| serMemBytes := serMemBuf.Bytes() | ||
| serMemLen := uint32(len(serMemBytes)) |
There was a problem hiding this comment.
I'd missed this before, but this is really very unfortunate since we now need to hold a copy of the entire memory in RAM while serializing. That was causing OOM failures originally until we started gzipping the pages individually to reduce the size.
I think we need to find a way to avoid this and actually stream the memory content. The simplest thing would be for the memory serialisation to prefix its content with the number of pages to read and then read exactly those pages back in rather than reading until EOF. Net result is the prefix becomes the number of pages (ie list length) rather than number of bytes in the memory, but now you can fully stream the content when reading and writing.
As this currently stands I think we'll run into memory issues again.
|
Pushed a couple of commits to update this with the latest changes from develop, fix the devnet failure and update how atomic file writes are done. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Overview
Moves the cannon state snapshots to a streamable binary serialization format, usable by types implementing the
Serializableinterface:The encoding scheme is just regular old flat binary - when a fixed size type is written, it's written raw, and when a dynamically sized piece of data is written, it is written following a length prefix.
Rationale
As it stands, the serialization format of state snapshots is somewhat convoluted; We first base64 encode the pages within Cannon's
Memory, JSON encode the fullStatein memory, then we gzip the JSON. This method brings us to a streamable binary format with a single compression pass for theState, which is commonly very large towards the tail end of Cannon execution.