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

Reuse rationalized sig meta #1541

Merged

Conversation

tinker-michaelj
Copy link
Collaborator

@tinker-michaelj tinker-michaelj commented Jun 5, 2021

Sonar passed with no non-license code smells here.

Related issue(s)

Summary of the change
Some minor code cleanup:

  • Remove the ScopedSigBytesProvider type, which only existed to hide whether sig bytes were coming from a SignatureMap or the now-unsupported SignatureList.
  • Remove the SigFactoryCreator type, which only existed to hide whether an outer txn or a scheduled txn was being signed (and the latter case was eliminated in the scheduled txn refactor).
  • Eliminate the unnecessary ByteString wrapper from TxnScopedPlatformSigFactory.create().

And a big performance win that avoids two kinds of repeated work being done:

  1. The list of required other-party keys was being expanded twice, once in rationalizeWithPreConsensusSigs() during AwareProcessLogic.doProcess(), and again in InHandleActivationHelper.areOtherPartiesActive().
  2. The SwirldTransaction list of TransactionSignatures was being accessed at least three times: Once to compare the expanded required signing keys during rationalization, once to construct the Function<byte[], TransactionSignature> pub-key-to-sig function for testing payer key activation, and yet again to construct the same pub-key-to-sig function for testing other-party key activation.
    • This access is extremely expensive, as it is not only synchronized, but also must return a defensive copy!
    • When the rationalized signatures didn't match the expanded signatures, a fourth access occurred.

The solution in this PR is to create a RationalizedSigMeta process object that captures the reusable by-products of Rationalization.execute(); that is,

  1. The required payer key.
  2. The required other-party keys.
  3. The canonical list of verified TransactionSignatures---no matter whether verified synchronously or asynchronously.
  4. The pub-key-to-sig function for testing key activation (backed by the above list).

So in Rationalization.execute() we set a RationalizedSigMeta instance as a field of the TxnAccessor in Rationalization. This makes reuse trivial, since every downstream component that needs any of the above can just get them from the accessor.

tinker-michaelj and others added 30 commits May 22, 2021 10:51
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
Signed-off-by: Neeharika-Sompalli <[email protected]>
@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #1541 (eab3620) into master-plus-sdk-0.15.0 (1cbe488) will increase coverage by 0.00%.
The diff coverage is 95.45%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##             master-plus-sdk-0.15.0    #1541   +/-   ##
=========================================================
  Coverage                     91.53%   91.53%           
- Complexity                     5896     5903    +7     
=========================================================
  Files                           463      461    -2     
  Lines                         17053    17058    +5     
  Branches                       1780     1784    +4     
=========================================================
+ Hits                          15609    15614    +5     
- Misses                          987      988    +1     
+ Partials                        457      456    -1     
Impacted Files Coverage Δ
...era/services/keys/StandardSyncActivationCheck.java 92.30% <ø> (-0.55%) ⬇️
...vices/legacy/services/state/AwareProcessLogic.java 34.69% <0.00%> (+0.35%) ⬆️
...edera/services/sigs/sourcing/PubKeyToSigBytes.java 100.00% <ø> (ø)
...in/java/com/hedera/services/utils/TxnAccessor.java 0.00% <0.00%> (ø)
...c/main/java/com/hedera/services/ServicesState.java 94.20% <100.00%> (-0.05%) ⬇️
...a/com/hedera/services/context/ServicesContext.java 93.45% <100.00%> (-0.07%) ⬇️
...tracts/execution/TxnAwareSoliditySigsVerifier.java 95.45% <100.00%> (ø)
.../com/hedera/services/keys/HederaKeyActivation.java 91.66% <100.00%> (+0.49%) ⬆️
...hedera/services/keys/InHandleActivationHelper.java 97.43% <100.00%> (-0.40%) ⬇️
.../main/java/com/hedera/services/sigs/Expansion.java 77.77% <100.00%> (-0.80%) ⬇️
... and 11 more

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 1cbe488...eab3620. Read the comment docs.

Signed-off-by: tinker-michaelj <[email protected]>
Base automatically changed from 01500-M0150-CreateNonBlockingHandoff to master-plus-sdk-0.15.0 June 7, 2021 16:29
tinker-michaelj added 2 commits June 7, 2021 11:43
Signed-off-by: tinker-michaelj <[email protected]>
* @param txnAccessor the accessor for the platform txn
* @param syncVerifier facility for synchronously verifying a cryptographic signature
* @param keyOrderer facility for listing Hedera keys required to sign the gRPC txn
* @param pkToSigFnProvider source of crypto sigs for the simple keys in the Hedera key leaves
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: @param... for sigFactoryCreator is missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added!

@@ -0,0 +1,96 @@
package com.hedera.services.utils;

Copy link
Member

Choose a reason for hiding this comment

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

copyright missing

import java.util.function.Function;

import static com.hedera.services.keys.HederaKeyActivation.pkToSigMapFrom;

Copy link
Member

Choose a reason for hiding this comment

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

javadoc for the class will be helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added!

@@ -0,0 +1,105 @@
package com.hedera.services.sigs;

Copy link
Member

Choose a reason for hiding this comment

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

copyright missing

@@ -0,0 +1,64 @@
package com.hedera.services.utils;

Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added :/

tinker-michaelj added 2 commits June 8, 2021 09:50
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM !!

@tinker-michaelj tinker-michaelj merged commit 8e583d7 into master-plus-sdk-0.15.0 Jun 8, 2021
@tinker-michaelj tinker-michaelj deleted the 01501-M0150-ReuseRationalizedSigMeta branch June 8, 2021 17:15
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.

3 participants