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

Nondegenerate multisignature hardware wallets #1884

Merged
merged 4 commits into from
Sep 25, 2023
Merged

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
11 of 12 checks passed
@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.

4 participants