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

chore(lib/grandpa): integrate scale into grandpa #1811

Merged
merged 242 commits into from
Sep 30, 2021
Merged

chore(lib/grandpa): integrate scale into grandpa #1811

merged 242 commits into from
Sep 30, 2021

Conversation

jimjbrettj
Copy link
Contributor

Changes

  • Remove the old scale library from grandpa
  • Remove as much custom serialization logic as possible from grandpa

Tests

go test ./dot/types -v
go test ./dot/state -v -short
go test ./lib/grandpa -v -short

Issues

Primary Reviewer

jimjbrettj and others added 30 commits July 15, 2021 13:07
@jimjbrettj
init test to check docket tests
7d6f434
@jimjbrettj
check block announce integration
b3ea436
@jimjbrettj
fixes to block announce handshake
28ce9c6
@jimjbrettj
test block announce
8d37eda
@jimjbrettj
update handshake
59a466b
@jimjbrettj
test block announce
e781bb7
@jimjbrettj
fix block announce encoding
9ed3203
@jimjbrettj
block announce update
844ee88
@jimjbrettj
add logs for debug node start
b118f9e
@jimjbrettj
add more logs for debug node start
aa0be0c
@jimjbrettj
try different encoding
8e7814f
@jimjbrettj
wip debug
53a6505
@noot
fix BlockAnnounceMessage.Decode
4400d5e
@noot
add go.sum
7dca93f
@noot
add nil checks
7a3c03e
@jimjbrettj
wip
b2f2a14
@jimjbrettj
merge
9064fe4
@jimjbrettj
wip/get stress tests working
0db1caa
@jimjbrettj
wip debug block announce
0f7ed44
@jimjbrettj
wip
882a40e
@jimjbrettj
undo changes to begin with Digest encoding
2fcaec0
@jimjbrettj
VDT for single digestItem works
3091954
@jimjbrettj
decode digest
b885973
@jimjbrettj
changes to scale
81003b1
@jimjbrettj
fix header encoding
34440ba
@jimjbrettj
interface changes and pass tests
e6e72c7
@jimjbrettj
Merge branch 'development' into jimmy/fixNetworkScaleIntegration
33cf659
@jimjbrettj
update digest encoding
37e896e
@jimjbrettj
Merge branch 'jimmy/fixNetworkScaleIntegration' of github.com:ChainSa…
1520c5a 
…fe/gossamer into jimmy/fixNetworkScaleIntegration
@jimjbrettj
proper header encoding
c915fcc
noot
noot reviewed Sep 28, 2021
View reviewed changes
dot/state/grandpa_test.go Outdated Show resolved Hide resolved
noot
noot reviewed Sep 28, 2021
View reviewed changes
dot/types/grandpa.go Outdated
Comment on lines 120 to 123
func (v *GrandpaVoter) Encode() ([]byte, error) {
enc := []byte{}
b := v.Key.Encode()
enc = append(enc, b...)
Copy link
Contributor

Choose a reason for hiding this comment

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

does the new SCALE package work with custom encodings? for example, if scale.Marshal(*GrandpaVoter) is called, will this function be called by SCALE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it does not. I also do not believe the GrandpaVoter is scale encoded because of the ed25519.PublicKey (Don't think the length is the leading byte of the encoded pubkey)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it's always a []byte of length 32. unfortunately the ed25519 has it just as []byte instead of [32]byte so we can't directly SCALE encode it. maybe as a next step for SCALE we can add custom type encodings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that could be a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

this method should just be

return scale.Marshal(v)

if you change ed25519.PublicKey to [32]byte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could just delete it all together and just directly marshal/unmarshal. Would also allow us to get rid of the custom encode and decode functions for GrandpaVoters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into this now actually. Could be a nice change to include in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a huge change maybe do it in another PR, since I think the crypto/ed25519 golang lib we're using has some nice functions that we currently use (but maybe it's not a huge change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call, I will do this in a separate PR

noot
noot reviewed Sep 28, 2021
View reviewed changes
lib/grandpa/grandpa.go Outdated Show resolved Hide resolved
noot
noot reviewed Sep 28, 2021
View reviewed changes
lib/grandpa/message.go Outdated Show resolved Hide resolved
noot
noot reviewed Sep 28, 2021
View reviewed changes
lib/grandpa/network.go Outdated

if err != nil {
return nil, err
return nil, fmt.Errorf("message type not recognised")
Copy link
Contributor

Choose a reason for hiding this comment

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

why not ErrInvalidMessageType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change

noot
noot reviewed Sep 28, 2021
View reviewed changes
lib/grandpa/network.go Outdated Show resolved Hide resolved
noot
noot reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@noot noot 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!! basically just a question about the decodeMessage function, I think everything else is fine

timwu20
timwu20 reviewed Sep 28, 2021
View reviewed changes
dot/types/grandpa.go
"fmt"
"io"

"github.com/ChainSafe/gossamer/lib/common"
"github.com/ChainSafe/gossamer/lib/crypto/ed25519"
"github.com/ChainSafe/gossamer/lib/scale"
scale2 "github.com/ChainSafe/gossamer/pkg/scale"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need both scale packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well right now encoding/decoding for the grandpaVoter is using custom logic (since its not scale encoded I do not believe) and its uses the old scale to encode/decode. Dot/types is the integration I will finish after this one is merged so I was planning to fix it then but if you want it in this pr I can do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we have an issue for this that's fine.

lib/grandpa/types.go Outdated
primaryProposal subround = 2
Prevote Subround
Precommit Subround = 1
PrimaryProposal Subround = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these public now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So scale can access them when decoding/encoding. I believe this is necessary but I can change if I am wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

The values don't need to be public, only the attribute names if they are being used as an attribute in a struct.

noot
noot approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@noot noot 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, would be nice to either add SCALE custom encoding or change our crypto/ed25519/PublicKey type to be [32]byte so that it can be used with new SCALE, maybe wanna open issues for these?

@timwu20
Copy link
Contributor

timwu20 commented Sep 29, 2021

looks good, would be nice to either add SCALE custom encoding or change our crypto/ed25519/PublicKey type to be [32]byte so that it can be used with new SCALE, maybe wanna open issues for these?

I prefer the latter. If it was a [32]byte that would encode/decode easily.

timwu20
timwu20 reviewed Sep 29, 2021
View reviewed changes
dot/types/grandpa.go Outdated
v.ID = id
return nil
}

// Encode will encode the GrandpaVoter
func (v *GrandpaVoter) Encode() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you update all the receiver functions.

Suggested change
func (v *GrandpaVoter) Encode() ([]byte, error) {
func (gv *GrandpaVoter) Encode() ([]byte, error) {

EclesioMeloJunior
EclesioMeloJunior reviewed Sep 29, 2021
View reviewed changes
lib/grandpa/message.go
func justificationToCompact(just []*SignedVote) ([]*Vote, []*AuthData) {
precommits := make([]*Vote, len(just))
authData := make([]*AuthData, len(just))
func justificationToCompact(just []SignedVote) ([]Vote, []AuthData) {
Copy link
Member

Choose a reason for hiding this comment

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

this function name does not correspond to the action IMO, I guess splitJustification or fromJustificationToVoteAndAuth

Copy link
Contributor Author

@jimjbrettj jimjbrettj Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense to me. @noot or @timwu20 do you have thoughts on this change?

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 this method should be a receiver function of a new type called type SignedVotes []SignedVote.

Copy link
Contributor

Choose a reason for hiding this comment

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

I named it this way to match substrate's naming, so I think it's ok to leave as is but up to your judgement

@noot noot merged commit 33059ad into development Sep 30, 2021
@noot noot deleted the jimmy/grandpaIntegration branch September 30, 2021 09:25
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021
@jimjbrettj @timwu20
chore(lib/grandpa): integrate scale v2 into grandpa (ChainSafe#1811)
a303598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EclesioMeloJunior EclesioMeloJunior EclesioMeloJunior left review comments

@noot noot noot approved these changes

@timwu20 timwu20 timwu20 approved these changes

@arijitAD arijitAD Awaiting requested review from arijitAD

@edwardmack edwardmack Awaiting requested review from edwardmack

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

Successfully merging this pull request may close these issues.
4 participants
@jimjbrettj @timwu20 @EclesioMeloJunior @noot