Fixes duplicate contractmetav0 custom sections when users add meta on build #2095
Conversation
this still creates a new custom section with all the meta, when i want to replace the existing one
don't include the length at the front of the vec
elizabethengelman
changed the title
There was a problem hiding this comment.
This change isn't quite what I meant in #1721, but it's better!
The issue was reporting that:
the cli is writing a whole new section per entry, but it should be joining the new entries together and writing them in one new entry
The issue was that when appending N new meta after build, the file would contain N new custom sections, instead of 1 new custom section. The intent was to still append a new custom section without modifying any of the existing custom sections, but to add all the meta entries into 1 new custom section.
The change in this PR is a bit different because it is fully decoding the file and rewriting the entire file. I had avoided rewriting the file in the past out of a fear we could introduce a problem with the contract by fiddling with the existing sections. But we were likely going to end up needing to rewrite files at some point anyway someday, and this change looks good and straight forward.
Rewriting has the outcome that the resulting file should be a tiny bit smaller than if we kept with appending a 2nd section.
I like that we've shifting from wasm-gen to wasm-encoder, because the former has not had updates for years and the latter is maintained by the Bytecode Alliance.
Few issues inline.
| for payload in WasmParser::new(0).parse_all(&wasm_bytes) { | ||
| match payload.unwrap() { | ||
| Payload::CustomSection(section) => { | ||
| if section.name() == META_CUSTOM_SECTION_NAME { | ||
| let updated_meta = self.append_new_meta(&existing_meta).unwrap(); |
There was a problem hiding this comment.
This code assumes that the file contains only a single contractmetav0 custom section. While that happens to be the case today during build, it's somewhat brittle to behave unexpected if that changes about how LLVM (what rustc uses to compile and construct the wasm file) builds custom sections in the future. If that changed, this logic would rewrite the existing meta multiple times. This logic would be more resilient I think if it ensure only a single meta section was writtne.
There was a problem hiding this comment.
For how to address this, I suggest iterating over the entire file, and collecting the contents of all contractmetav0 into a Vec<u8> and not adding them into the new module. All other sections, including all other custom sections, can be added via the raw section logic that exists in the PR further down. Then, serialize the new meta appending it into the collected contents of existing meta sections, and add the contractmetav0 section with the collected + new meta into the module. Following that process should result in only a single meta section no matter how many existed prior to this code being called.
| fn append_new_meta(&self, existing_meta: &[ScMetaEntry]) -> Result<Vec<u8>, Error> { | ||
| let mut updated_meta = existing_meta.to_owned(); |
There was a problem hiding this comment.
This line makes a copy of the vec in memory, which is unnecessary because the only use of the vec being passed in is to append. So you can accept a &mut Vec<ScMetaEntry> and modify it directly to avoid the copy.
| let mut wasm_bytes = fs::read(target_file_path).map_err(Error::ReadingWasmFile)?; | ||
| // get existing wasm bytes | ||
| let wasm_bytes = fs::read(target_file_path).map_err(Error::ReadingWasmFile)?; | ||
| let existing_meta: Vec<ScMetaEntry> = Spec::new(&wasm_bytes).unwrap().meta; |
There was a problem hiding this comment.
Why do we use the Spec class to read the meta out of the wasm_bytes? I would expect that we wouldn't need to parse the existing meta. The new meta can be encoded to XDR on its own and appended to the existing meta's bytes without decoding the existing meta.
What
Closes #1721
Updates the logic to add new contract meta data to the contract wasm on build so that there is only one
contractmetav0custom section.Here's a diff of the before (on the left) vs after with this branch (on the right): https://www.diffchecker.com/fshP64p8/
These are the commands that were run to get the outputs for the diff:
Output from uploading and deploying the resulting wasm:
Why
This will make it so the wasm isn't larger than it needs to be with duplicate
contractmetav0sections. This hopefully will also make it easier for the data team to fetch contract data.Known limitations
This does reencode the whole wasm file, instead of just appending a new section. Given the tooling, this was the only way I could figure out how to remove the default
contractmetav0and include just the new, updated section with the user's meta.