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

P2p message signing component #4460

Merged

Conversation

ssd04
Copy link
Contributor

@ssd04 ssd04 commented Sep 13, 2022

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

  • For metachain signature improvement in consensus, a separate component have to be created in order to handle the p2p message serialization and verification

Proposed Changes

  • Create a separate component in p2p package

Testing procedure

  • Standard system test

@ssd04 ssd04 self-assigned this Sep 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (feat/optimise-consensus-sigcheck@cdc7507). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@                         Coverage Diff                         @@
##             feat/optimise-consensus-sigcheck    #4460   +/-   ##
===================================================================
  Coverage                                    ?   75.84%           
===================================================================
  Files                                       ?      645           
  Lines                                       ?    85198           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?    64619           
  Misses                                      ?    15781           
  Partials                                    ?     4798           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

return err
}

err = pubsub.VerifyMessageSignature(pubsubMsg)
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not need this. Please use the p2p/crypto/p2pSigner component available on feat/multisigner branch

https://github.com/ElrondNetwork/elrond-go/tree/feat/multisigner/p2p/crypto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to use p2p signer component

)

type messageVerifier struct {
marshaller marshal.Marshalizer
Copy link
Contributor

Choose a reason for hiding this comment

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

the serializer used in the p2p subsystem is not the one used in elrond-go. This needs to be refactored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored to use internal marshaller

"github.com/stretchr/testify/require"
)

func TestNewMessageVerifier(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

beside this, we might create an integration test in which we have 2 connected peers that and verify their received messages with the new component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created the integration test

mv, err := messagecheck.NewMessageVerifier(args)
require.Nil(t, err)

messagesBytes, err := mv.Serialize(messages)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think messages and expected messages are switched (names)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AdoAdoAdo AdoAdoAdo merged commit 16a2fb0 into feat/optimise-consensus-sigcheck Sep 15, 2022
@AdoAdoAdo AdoAdoAdo deleted the p2p-message-signing-component branch September 15, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants