Skip to content

Conversation

@murisi
Copy link
Collaborator

@murisi murisi commented Sep 11, 2023

Describe your changes

  • Combined Signature and MultiSignature to reduce logic duplication. Specifically:
    • Decompression logic no longer needs to be separately implemented for both types of signatures
    • Hardware wallet would no longer need to implement separate hashers, storage logic, and serializers for both types of signatures
  • Allow signer to be specified in signature section, otherwise indexing data would need to be sent to hardware wallet for nondegenerate multisig transactions
  • Added decompression logic in order to eliminate the decompression hacks we are using in namada-interface (we should explore in future whether this can be used to reduce size of signature sections on protocol)
  • Using map instead of set to store indexed signatures in order to remedy the signature double counting issues (multiple valid signatures can exist for given hash and public key).
  • Eliminated signature double counting across different sections by storing successfully verified indices in a set.

Indicate on which release or other PRs this topic is based on

Namada v0.22.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@murisi murisi changed the title Allow public key to be specified in signature section. Enable nondegenerate multisignature hardware wallets Sep 11, 2023
@murisi murisi changed the title Enable nondegenerate multisignature hardware wallets Nondegenerate multisignature hardware wallets Sep 11, 2023
@murisi murisi force-pushed the murisi/multisig-fixes branch from c1aafa9 to 3c7bdba Compare September 13, 2023 08:51
@murisi murisi marked this pull request as ready for review September 13, 2023 09:48
@murisi murisi requested review from Fraccaman and grarco September 13, 2023 09:54
Fraccaman
Fraccaman previously approved these changes Sep 13, 2023
let amt_verified = usize::from(amt_verifieds.is_err())
+ verified_pks.len()
- prev_verifieds;
x.consume(VERIFY_TX_SIG_GAS_COST * amt_verified as u64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that given this rework it would be slightly better to move the gas accounting part directly into the verify_signature method of Signature, to have a highest possible resolution. But we can do this later

grarco
grarco previously approved these changes Sep 13, 2023
Copy link
Collaborator

@grarco grarco left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@murisi murisi dismissed stale reviews from grarco and Fraccaman via 6eff4a9 September 15, 2023 13:12
@murisi murisi force-pushed the murisi/multisig-fixes branch from 998fd82 to 6eff4a9 Compare September 15, 2023 13:12
@murisi murisi mentioned this pull request Sep 20, 2023
murisi added a commit that referenced this pull request Sep 20, 2023
Fraccaman added a commit that referenced this pull request Sep 25, 2023
* origin/murisi/multisig-fixes:
  Added changelog entry.
  Added support for compressed signatures coming from hardware wallets.
  Combining Signature and MultiSignature sections to deduplicate hardware wallet code.
  Allow public key to be specified in signature section.
@brentstone brentstone merged commit 640bd1a into main Sep 25, 2023
@brentstone brentstone deleted the murisi/multisig-fixes branch September 25, 2023 17:19
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.

5 participants