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

Murisi/restructure tx draft #1462

Merged
merged 139 commits into from
Jun 5, 2023
Merged

Murisi/restructure tx draft #1462

merged 139 commits into from
Jun 5, 2023

Conversation

murisi
Copy link
Collaborator

@murisi murisi commented May 24, 2023

This is an attempt to achieve #1255 with the tip of main merged in. Overall the main changes introduced by this branch are:

  • Instead of having an outer transaction structure containing an inner transaction, there is now a single transaction structure containing a flat list of sections.
  • Instead of wrapping inner transactions in a structure that supplies a signature, the unsigned data is added as a section to the transaction at the same level as it corresponding signature section.
  • The WASM code now receives a Tx object instead of a data byte vector. This is because the data section's signature is stored in a different section, and because the extra data also resides in a separate section.
  • Encryption is now done separately after all signing is complete - in particular this means that there are no longer any signatures over encrypted data.
  • Upgraded the MASP client and validation code to use the latest MASP crate.
  • Added a builder section to Txs where the auxiliary inputs that were used to construct a MASP transaction can be stored. This may be useful for hardware wallets when validating a given Transaction object.

Additional notes:

murisi and others added 30 commits January 27, 2023 12:52
tzemanovic and others added 19 commits May 21, 2023 11:57
…tomas/improve-cli-commission-rate' and 'tomas/rename-tx-reveal-arg' (#1431, #1432, #1434, #1436)

* tomas/rm-empty-keys-mod:
  app/wallet: rm empty keys mod

* tomas/missing-args-def:
  cli: add missing `--wallet-alias-force` and `--alias-force`

* tomas/improve-cli-commission-rate:
  shared/tx/submit_validator_commission_change: print err even with force

* tomas/rename-tx-reveal-arg:
  cli: rename Tx arg `tx_code_path` to `tx_reveal_code_path`
* tomas/fix-err-typos:
  shared/tx: fix err variant typos
Namada 0.15.4

* tag 'v0.15.4':
  Namada 0.15.4
  changelog: add #1407
  Fixes e2e tests
  changelog: add #1399
  Tendermint consensus params settable in Namada config
  Updates `InternalStats` display
  changelog: add #1405
  process_proposal: fix typos in test names
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
  Logs validation error in `process_proposal`
* aleks/wallet-zeroize:
  [ci] wasm checksums update
  Add changelog
  fix: zeroize passphrases
* tiago/main/rm-abciplus-cargo-deps:
  Remove ABCI++ commands from the Makefile
  Update Cargo lock file
  Remove ABCI++ deps from Cargo files
  Revert "Makefile: Add arguments to e2e tests"
  Revert "Makefile: `NAMADA_E2E_USE_PREBUILT_BINARIES` default"
  Makefile: `NAMADA_E2E_USE_PREBUILT_BINARIES` default
  Makefile: Add arguments to e2e tests
  Makefile: `NAMADA_E2E_USE_PREBUILT_BINARIES` default
  Makefile: Add arguments to e2e tests
  fix: ci run with maint-* target branch
  Remove accidental artifact
  Apply suggestions from code review
  missed spelling mistakes
  fixed a bunch of typos and attended comments
  Update documentation/specs/src/base-ledger/governance.md
  added incentives for stewards
  somg governance changes
  fixed some governance ya dig
  slight fix
  ammendments that will need ammending
  proposals named correctly
  no more mentions of the council
  new changes
  some changes to pgf specs
@murisi
Copy link
Collaborator Author

murisi commented May 24, 2023

pls update wasm

@tzemanovic tzemanovic mentioned this pull request May 24, 2023
@Fraccaman Fraccaman merged commit 917eaa4 into main Jun 5, 2023
@Fraccaman Fraccaman deleted the murisi/restructure-tx-draft branch June 5, 2023 03:18
/// Transaction data that needs to be sent to hardware wallets
Data(Data),
/// Transaction data that does not need to be sent to hardware wallets
ExtraData(Code),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be ExtraData(Code)?

Suggested change
ExtraData(Code),
ExtraData(Data),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is meant to be ExtraData(Code). The reason why is that the extra data section shares a greater resemblance to the the code section than it does to the data section. More specifically, extra data sections frequently contain too many bytes, just like the WASM code in code sections are frequently too large. Hence both extra data and code sections are "compressed" by first taking the SHA-256 hash of their payload and putting that into the section instead. This is in contrast to the data section, which is mostly unique to each transaction, and is never compressed because hardware wallets need to render all contained data (which would not be possible with just a hash).

/// A transaction timestamp
pub timestamp: DateTimeUtc,
/// The SHA-256 hash of the transaction's code section
pub code_hash: crate::types::hash::Hash,
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a privacy concern, as mentioned by @Fraccaman

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code_hash field of Header is salted. That is, the WASM code hash is appended to a salt, and then hashed in order to not reveal what kind of transaction is being done. See:

pub struct Code {
. cc @Fraccaman

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.

7 participants