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.

Sign up for GitHub

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

Jump to bottom

Avoid costly stream operations while iterating over SignaturePairs in the sigMap #1456

Merged
merged 6 commits into from
May 25, 2021
Merged

Avoid costly stream operations while iterating over SignaturePairs in the sigMap #1456

merged 6 commits into from
May 25, 2021

Conversation

Neeharika-Sompalli
Copy link
Member

Related issue(s):
Closes #1423

Summary of the change:
Removed stream() operation being used to iterate over the SignaturePairs in the sigMap. This is done because modifying this to use a simple for loop and using Arrays.equals() improved performance

Neeharika-Sompalli added 2 commits May 21, 2021 13:18
remove using stream and replace using a for loop
e4b05c0 
Signed-off-by: Neeharika-Sompalli <[email protected]>
remove var
7cff352 
Signed-off-by: Neeharika-Sompalli <[email protected]>
@Neeharika-Sompalli Neeharika-Sompalli changed the title Avoid costly Avoid costly stream operations while iterating over SignaturePairs in the sigMap May 21, 2021
@codecov
Copy link

codecov bot commented May 21, 2021 •
edited
Loading

Codecov Report

Merging #1456 (cc0dfd3) into master (251ce9d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1456      +/-   ##
============================================
+ Coverage     91.19%   91.21%   +0.01%     
- Complexity     5868     5870       +2     
============================================
  Files           459      459              
  Lines         16916    16919       +3     
  Branches       1771     1772       +1     
============================================
+ Hits          15427    15432       +5     
+ Misses         1020     1019       -1     
+ Partials        469      468       -1
Impacted Files Coverage Δ
...services/sigs/sourcing/SigMapPubKeyToSigBytes.java 100.00% <100.00%> (ø)
.../com/hedera/services/sigs/utils/PrecheckUtils.java 85.71% <0.00%> (+14.28%) ⬆️
...ervices/sigs/sourcing/DefaultSigBytesProvider.java 100.00% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 251ce9d...cc0dfd3. Read the comment docs.

qnswirlds
qnswirlds suggested changes May 21, 2021
View reviewed changes
Copy link
Contributor

@qnswirlds qnswirlds left a comment

Choose a reason for hiding this comment

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

Please see inline comments

hedera-node/src/main/java/com/hedera/services/sigs/sourcing/SigMapPubKeyToSigBytes.java Outdated Show resolved Hide resolved
hedera-node/src/main/java/com/hedera/services/sigs/sourcing/SigMapPubKeyToSigBytes.java Outdated
final int length = pubKeyPrefix.length;
if (Arrays.equals(pubKeyPrefix, 0, length, pubKey, 0, length)) {
if (sigBytes != EMPTY_SIG) {
throw new KeyPrefixMismatchException("Source signature map is ambiguous for given public key!");
Copy link
Contributor

Choose a reason for hiding this comment

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

For this throw, also add the info of pubKeyPrefix and pubKey. Need a test to cover this throw with the expected info.

Copy link
Collaborator

@tinker-michaelj tinker-michaelj May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There is at least one unit test where we create an intentionally ambiguous sigMap and verify the KeyPrefixMismatchException is thrown, here: SigVerifierRegressionTest.throwsOnInvalidSigMap().

Apparently it was unstable in CI because it has,

		assumeFalse(isInCircleCi);

But @Neeharika-Sompalli you could try removing the assumeFalse(isInCircleCi) from the SigVerifierRegressionTest...at some point I could start running the SmartContract*Test unit tests in CircleCI for no obvious reason, maybe this can too now! 😁

Copy link
Member Author

@Neeharika-Sompalli Neeharika-Sompalli May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@tinker-michaelj Sure, uncommented the line. SigMapPubKeyToSigBytesTest.rejectsNonUniqueSigBytes also checks for the KeyPrefixMismatchException

tinker-michaelj
tinker-michaelj previously approved these changes May 21, 2021
View reviewed changes
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

LGTM...I believe the semantics are unchanged, we can run the SigVerifierRegressionTest mentioned below for extra test coverage if we want.

hedera-node/src/main/java/com/hedera/services/sigs/sourcing/SigMapPubKeyToSigBytes.java Outdated Show resolved Hide resolved
hedera-node/src/main/java/com/hedera/services/sigs/sourcing/SigMapPubKeyToSigBytes.java Outdated
final int length = pubKeyPrefix.length;
if (Arrays.equals(pubKeyPrefix, 0, length, pubKey, 0, length)) {
if (sigBytes != EMPTY_SIG) {
throw new KeyPrefixMismatchException("Source signature map is ambiguous for given public key!");
Copy link
Collaborator

@tinker-michaelj tinker-michaelj May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There is at least one unit test where we create an intentionally ambiguous sigMap and verify the KeyPrefixMismatchException is thrown, here: SigVerifierRegressionTest.throwsOnInvalidSigMap().

Apparently it was unstable in CI because it has,

		assumeFalse(isInCircleCi);

But @Neeharika-Sompalli you could try removing the assumeFalse(isInCircleCi) from the SigVerifierRegressionTest...at some point I could start running the SmartContract*Test unit tests in CircleCI for no obvious reason, maybe this can too now! 😁

ljianghedera
ljianghedera previously approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@ljianghedera ljianghedera left a comment

Choose a reason for hiding this comment

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

LGTM

address review comments
c8fa94f 
Signed-off-by: Neeharika-Sompalli <[email protected]>
ljianghedera
ljianghedera previously approved these changes May 24, 2021
View reviewed changes
Copy link
Contributor

@ljianghedera ljianghedera left a comment

Choose a reason for hiding this comment

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

LGTM

Add unit test for sigBytesFor method
5d51c4f 
Signed-off-by: Neeharika-Sompalli <[email protected]>
@Neeharika-Sompalli Neeharika-Sompalli merged commit 60a2b10 into master May 25, 2021
@Neeharika-Sompalli Neeharika-Sompalli deleted the 1423-M-avoid-stream-reading-sigMap branch May 25, 2021 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhishek-hedera abhishek-hedera abhishek-hedera left review comments

@qnswirlds qnswirlds qnswirlds approved these changes

@ljianghedera ljianghedera ljianghedera left review comments

@tinker-michaelj tinker-michaelj tinker-michaelj left review comments

@anighanta anighanta Awaiting requested review from anighanta

@rbair23 rbair23 Awaiting requested review from rbair23

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid expensive stream operations when sourcing public key's sig from sigMap
5 participants
@Neeharika-Sompalli @qnswirlds @tinker-michaelj @ljianghedera @abhishek-hedera