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

Remove wasm code from tx #1297

Merged
merged 19 commits into from
Apr 19, 2023
Merged

Remove wasm code from tx #1297

merged 19 commits into from
Apr 19, 2023

Conversation

yito88
Copy link
Member

@yito88 yito88 commented Apr 14, 2023

Remove tx/vp wasm code from a transaction

A transaction has the hash instead of the actual wasm code.
Each node stores wasms in the storage and fetches them according to hashes included in requested transactions.

@yito88
Copy link
Member Author

yito88 commented Apr 15, 2023

pls update wasm

@yito88
Copy link
Member Author

yito88 commented Apr 16, 2023

pls update wasm

@yito88 yito88 marked this pull request as ready for review April 16, 2023 10:54
juped added a commit that referenced this pull request Apr 16, 2023
…nt-0.15

* namada/yuji/wasm_hash:
  add changelog
  fix vp wasm tests
  fix vp_implicit test
  for CI
  [ci] wasm checksums update
  fix wasm cache file path
  fix e2e test: invalid_transactions
  fix vp whitelist checking
  [ci] wasm checksums update
  for client
  for vp handling
  modify wasm cache
@juped juped mentioned this pull request Apr 16, 2023
tzemanovic
tzemanovic previously approved these changes Apr 17, 2023
Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo the small comments LGTM! Since we're going this way, I think we could also pre-compile all the wasms in init_chain in a follow up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are the genesis hashes removed here? they are not directly related to the whitelist; they are an integrity check on the genesis bundle, so we die on genesis if the wrong wasms are present

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the genesis hashes should be exactly the same as the hashes on the whitelists. Do you mean that we shouldn't die on genesis even if they mismatch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, if whitelists are empty, we need to check hashes with them. I will revert the hash members and add the hash checking.

@juped
Copy link
Member

juped commented Apr 17, 2023

pls update wasm

juped added a commit that referenced this pull request Apr 17, 2023
…nt-0.15

* namada/yuji/wasm_hash:
  add changelog
  fix vp wasm tests
  fix vp_implicit test
  for CI
  [ci] wasm checksums update
  fix wasm cache file path
  fix e2e test: invalid_transactions
  fix vp whitelist checking
  [ci] wasm checksums update
  for client
  for vp handling
  modify wasm cache
@yito88 yito88 requested review from juped and tzemanovic April 17, 2023 21:02
@juped juped merged commit 47f44f0 into main Apr 19, 2023
@juped juped deleted the yuji/wasm_hash branch April 19, 2023 13:32
@grarco grarco mentioned this pull request May 9, 2023
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

Successfully merging this pull request may close these issues.

3 participants