Add version, partial state indicator, and checksum to payload file (intermediate migration file) #5438
Conversation
This commit also added --allow-partial-state-from-payload-file flag to execution-state-extract program. User needs to specify this flag allow partial payload file as input (e.g. not all accounts) in order to prevent accidental use of partial state during real migration. The primary use case for payload file is for development, testing, and debugging of migrations because it allows migrating a subset of payloads instead of all payloads.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5438 +/- ##
==========================================
+ Coverage 56.00% 56.04% +0.03%
==========================================
Files 1026 1026
Lines 99940 100033 +93
==========================================
+ Hits 55973 56064 +91
Misses 39677 39677
- Partials 4290 4292 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // newPayloadFileHeader() returns payload header, consisting of: | ||
| // - magic bytes (2 bytes) | ||
| // - version (2 bytes) | ||
| // - flags (2 bytes) |
There was a problem hiding this comment.
curious why flag needs 2 bytes? isn't 1 byte is enough?
There was a problem hiding this comment.
Yes, 1 byte for flag is enough for now. But I ran into a situation in onflow/atree where all flag bits got used, so I use 2 bytes here to avoid this potential issue since it is only 1 extra byte written only once to a file.
| exportedPayloadCount, err := util.CreatePayloadFile( | ||
| log, | ||
| outputPayloadFile, | ||
| payloads, | ||
| exportPayloadsByAddresses, | ||
| false, // payloads represents entire state. |
There was a problem hiding this comment.
Can we create another CreatePayloadFileWithPartialStateFlag and make CreatePayloadFile to call CreatePayloadFileWithPartialStateFlag with partialStateFlag: false, so that we don't have to add this default value if not needed.
There was a problem hiding this comment.
Maybe not in this case because most of the non-test calls of CreatePayloadFile() uses variable returned from another function call (not hard-coded) to indicate if input payloads are for partial or full state. So If we split it into two functions, we need to check this boolean and decide which function to call at higher level.
Closes #5437
This PR updates the payload file produced by the state extraction program.
--allow-partial-state-from-payload-fileflag toexecution-state-extractprogram.User needs to specify this flag allow partial payload file as input (e.g. not all accounts) in order to prevent accidental use of partial state during real migration.
The primary use case for payload file is for development, testing, and debugging of migrations because it allows migrating a subset of payloads instead of all payloads.
Thanks @zhangchiqing for suggestion to add checksum to this payload file, etc.
Caveats (outside scope of this PR)
Outside the scope of this PR, CRC-32 should eventually be replaced by similar hash used by input files (currently also CRC-32).
Since the input files (checkpoint files) currently use CRC-32, the output file (payload file) also uses CRC-32. We can update the payload file when the checkpoint file format is changed to replace CRC-32 with hash as previously suggested at: