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

multisig checks and optimization #8600

Merged
merged 23 commits into from
Feb 23, 2021
Merged

multisig checks and optimization #8600

merged 23 commits into from
Feb 23, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Feb 16, 2021

Description

LegacyAminoMultisig improvements

  • Added public key uniqueness check -- though not sure if we want it, so I can remove it.
  • Introduced the CompactBitArray.Equal method to improve code flow in x/auth and not use structure fields. (PS: ideally I wanted to make the CompactBitArray attributes private, but the structure is generated with protobuf)
  • Optimized the CompactBitArray.NumTrueBitsBefore which is used on every signature validation.
  • Added more tests

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #8600 (2272eaa) into master (8db9bbb) will increase coverage by 0.00%.
The diff coverage is 90.47%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8600   +/-   ##
=======================================
  Coverage   61.47%   61.47%           
=======================================
  Files         659      659           
  Lines       37916    37928   +12     
=======================================
+ Hits        23308    23318   +10     
- Misses      12168    12169    +1     
- Partials     2440     2441    +1     
Impacted Files Coverage Δ
crypto/types/compact_bit_array.go 75.00% <89.47%> (+1.00%) ⬆️
crypto/keys/multisig/multisig.go 71.01% <100.00%> (ø)

@robert-zaremba robert-zaremba changed the title Robert/ms check multisig checks and optimization Feb 17, 2021
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

It's not worth to improve something that is

  1. Broken by design.
  2. Fated to be replaced in the near future.

Please close this PR.

@alessio alessio removed this from the v0.41.1 milestone Feb 17, 2021
@robert-zaremba
Copy link
Collaborator Author

OK, I removed the uniqueness check and updated the comments in the functions to make it clear the not unique public keys are supported.

@alessio
Copy link
Contributor

alessio commented Feb 17, 2021

I'm subscribing @ValarDragon. IIRC he's the original author of these lines.

@robert-zaremba
Copy link
Collaborator Author

I created a benchmark for the optimized function:

BenchmarkNumTrueBitsBefore/old-4         	 4124839	       285 ns/op
BenchmarkNumTrueBitsBefore/new-4         	 7612244	       154 ns/op

So 45% improvement.

@alessio
Copy link
Contributor

alessio commented Feb 17, 2021

I created a benchmark for the optimized function:

BenchmarkNumTrueBitsBefore/old-4         	 4124839	       285 ns/op
BenchmarkNumTrueBitsBefore/new-4         	 7612244	       154 ns/op

So 45% improvement.

Great! I'd love to replicate the benchmarks, do you have code to share please?

@alessio
Copy link
Contributor

alessio commented Feb 18, 2021

Conflicts must be resolved

@alessio alessio dismissed their stale review February 18, 2021 14:07

Lifiting block. Thought I'd like the original author of multisig to review before approving.

@robert-zaremba
Copy link
Collaborator Author

Conflicts must be resolved

I know, there were no conflicts before, I firstly want to have clearance on merging before going back to updating conflicts.

@robert-zaremba
Copy link
Collaborator Author

@alessio , @ValarDragon - could you approve the PR ?

@robert-zaremba
Copy link
Collaborator Author

@alessio - you were upgrading linter recently. It seams that it breaks existing code. Can you confirm?

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@alessio
Copy link
Contributor

alessio commented Feb 23, 2021

@alessio - you were upgrading linter recently. It seams that it breaks existing code. Can you confirm?

Yes, this can be merged nonetheless. Linter warnings seem minor.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Requested a small change, plus an explanation.

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 23, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Approving this on the assumption that this PR's author performed all relevant manual testing.

@mergify mergify bot merged commit 8dd6c32 into master Feb 23, 2021
@mergify mergify bot deleted the robert/ms-check branch February 23, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Crypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants