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

fix(lib/grandpa): Storing Justification Allows Extra Bytes (GSR-13) #2618

Merged
merged 6 commits into from
Jun 29, 2022
Merged

fix(lib/grandpa): Storing Justification Allows Extra Bytes (GSR-13) #2618

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8abfdba
testing to create justification
edwardmack Jun 22, 2022
6d93ce5
implement fix and tests for GSR-13
edwardmack Jun 22, 2022
5d9d0ee
fix integration test mock
edwardmack Jun 22, 2022
1f49294
remove testing/debugging logging
edwardmack Jun 23, 2022
28a6f27
refactor test to use generator instead of hex string
edwardmack Jun 27, 2022
209e046
lint
edwardmack Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
implement fix and tests for GSR-13
  • Loading branch information
@edwardmack
edwardmack committed Jun 22, 2022
commit 6d93ce5d70e255db1a966b766d3aa8036a4b0b37
2 changes: 1 addition & 1 deletion chain/dev/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ sync = ""
digest = ""

[init]
genesis = "./chain/dev/genesis-spec_test.json"
genesis = "./chain/dev/genesis-spec.json"

[account]
key = "alice"
Expand Down
4 changes: 2 additions & 2 deletions dot/sync/chain_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,13 @@ func (s *chainProcessor) handleJustification(header *types.Header, justification
return
}

err := s.finalityGadget.VerifyBlockJustification(header.Hash(), justification)
returnedJustification, err := s.finalityGadget.VerifyBlockJustification(header.Hash(), justification)
if err != nil {
logger.Warnf("failed to verify block number %d and hash %s justification: %s", header.Number, header.Hash(), err)
return
}

err = s.blockState.SetJustification(header.Hash(), justification)
err = s.blockState.SetJustification(header.Hash(), returnedJustification)
if err != nil {
logger.Errorf("failed tostore justification: %s", err)
return
Expand Down
30 changes: 9 additions & 21 deletions dot/sync/chain_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,21 +236,7 @@ func Test_chainProcessor_handleBody(t *testing.T) {
processor.handleBody(testBody)
})
}
func Test_chainProcessor_handleJustification_two(t *testing.T) {
ctrl := gomock.NewController(t)

mockHeader := types.NewEmptyHeader()
mockJust := common.MustHexToBytes(
"0x05000000000000008306f3c4dda0211e863e52c8bc15a3251e5ba38c7dd2f241f896653665e5e7e0080000000c8306f3c4dda0211e863e52c8bc15a3251e5ba38c7dd2f241f896653665e5e7e0080000001fd54b3502e84c96a1ab890d507789266c4611d784d0169a575e120874271d075b6717ab5a7eb7d9618c3916ecaf4310286163fc77152be2919c981701b9aa0ed17c2d7823ebf260fd138f2d7e27d114c0145d968b5ff5006125f2414fadae698306f3c4dda0211e863e52c8bc15a3251e5ba38c7dd2f241f896653665e5e7e0080000002187956a30a233c1bf4d9e4e0b63eb887641f58758432915b2a5e3ab66543942e752767111fc75dcfbe3955d71c443adbc156e67c82fa2ae9b6a16b014d7690488dc3417d5058ec4b4503e0c12ea1a0a89be200fe98922423d4334014fa6b0ee8306f3c4dda0211e863e52c8bc15a3251e5ba38c7dd2f241f896653665e5e7e008000000dad739bb86c09539662075845873c109c29f6c13e9a0ac771f08c4b58e96c5a2198e387fb3f18a2506c28f2c2a151044d1ae9dc40f83efa73cdfcf9a37615c09439660b36c6c03afafca027b910b4fecf99801834c62a5e6006f27d978de234f")
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(mockHeader.Hash(), mockJust)
cp := chainProcessor{
blockState: NewMockBlockState(ctrl),
finalityGadget: mockFinalityGadget,
}
cp.handleJustification(mockHeader, mockJust)

}
func Test_chainProcessor_handleJustification(t *testing.T) {
t.Parallel()

Expand All @@ -272,7 +258,8 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
"invalid justification": {
chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor {
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`x`)).Return(errors.New("error"))
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash,
[]byte(`x`)).Return(nil, errors.New("error"))
return chainProcessor{
finalityGadget: mockFinalityGadget,
}
Expand All @@ -289,7 +276,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
mockBlockState := NewMockBlockState(ctrl)
mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`xx`)).Return(errors.New("fake error"))
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`xx`)).Return(nil)
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`xx`)).Return([]byte(`xx`), nil)
return chainProcessor{
blockState: mockBlockState,
finalityGadget: mockFinalityGadget,
Expand All @@ -307,7 +294,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
mockBlockState := NewMockBlockState(ctrl)
mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`1234`)).Return(nil)
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`1234`)).Return(nil)
mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`1234`)).Return([]byte(`1234`), nil)
return chainProcessor{
blockState: mockBlockState,
finalityGadget: mockFinalityGadget,
Expand Down Expand Up @@ -431,7 +418,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
3})
3}).Return([]byte{1, 2, 3}, nil)
mockStorageState := NewMockStorageState(ctrl)
mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, nil)
mockBlockImportHandler := NewMockBlockImportHandler(ctrl)
Expand Down Expand Up @@ -466,7 +453,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
3})
3}).Return([]byte{1, 2, 3}, nil)
mockStorageState := NewMockStorageState(ctrl)
mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, mockError)
return chainProcessor{
Expand Down Expand Up @@ -498,7 +485,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
3})
3}).Return([]byte{1, 2, 3}, nil)
mockStorageState := NewMockStorageState(ctrl)
mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, nil)
mockBlockImportHandler := NewMockBlockImportHandler(ctrl)
Expand Down Expand Up @@ -701,7 +688,8 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
mockTelemetry.EXPECT().SendMessage(gomock.Any()).AnyTimes()
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(
common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a"), justification)
common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a"),
justification).Return(justification, nil)
return chainProcessor{
blockState: mockBlockState,
babeVerifier: mockBabeVerifier,
Expand Down
2 changes: 1 addition & 1 deletion dot/sync/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ type BabeVerifier interface {

// FinalityGadget implements justification verification functionality
type FinalityGadget interface {
VerifyBlockJustification(common.Hash, []byte) error
VerifyBlockJustification(common.Hash, []byte) ([]byte, error)
}

// BlockImportHandler is the interface for the handler of newly imported blocks
Expand Down
7 changes: 4 additions & 3 deletions dot/sync/mock_interface_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 25 additions & 28 deletions lib/grandpa/message_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,52 +553,49 @@ func (h *MessageHandler) verifyJustification(just *SignedVote, round, setID uint
return nil
}

// VerifyBlockJustification verifies the finality justification for a block
// todo(ed): either make justification *justification, or return justification so that
// the marshaled just is used
func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byte) error {
// VerifyBlockJustification verifies the finality justification for a block, returns scale encoded justification with
// any extra bytes removed.
func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byte) ([]byte, error) {
fj := Justification{}
err := scale.Unmarshal(justification, &fj)
if err != nil {
return err
return nil, err
}
tmpJust, err := scale.Marshal(fj)
copy((justification), tmpJust[:])
//fmt.Printf("just %x\n", just2)

setID, err := s.grandpaState.GetSetIDByBlockNumber(uint(fj.Commit.Number))
if err != nil {
return fmt.Errorf("cannot get set ID from block number: %w", err)
return nil, fmt.Errorf("cannot get set ID from block number: %w", err)
}

has, err := s.blockState.HasFinalisedBlock(fj.Round, setID)
if err != nil {
return err
return nil, err
}

if has {
return fmt.Errorf("already have finalised block with setID=%d and round=%d", setID, fj.Round)
return nil, fmt.Errorf("already have finalised block with setID=%d and round=%d", setID, fj.Round)
}

isDescendant, err := isDescendantOfHighestFinalisedBlock(s.blockState, fj.Commit.Hash)
if err != nil {
return err
return nil, err
}

if !isDescendant {
return errVoteBlockMismatch
return nil, errVoteBlockMismatch
}

auths, err := s.grandpaState.GetAuthorities(setID)
if err != nil {
return fmt.Errorf("cannot get authorities for set ID: %w", err)
return nil, fmt.Errorf("cannot get authorities for set ID: %w", err)
}

// threshold is two-thirds the number of authorities,
// uses the current set of authorities to define the threshold
threshold := (2 * len(auths) / 3)

if len(fj.Commit.Precommits) < threshold {
return ErrMinVotesNotMet
return nil, ErrMinVotesNotMet
}

authPubKeys := make([]AuthData, len(fj.Commit.Precommits))
Expand All @@ -608,7 +605,7 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt

equivocatoryVoters, err := getEquivocatoryVoters(authPubKeys)
if err != nil {
return fmt.Errorf("could not get valid equivocatory voters: %w", err)
return nil, fmt.Errorf("could not get valid equivocatory voters: %w", err)
}

var count int
Expand All @@ -621,20 +618,20 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
// check if vote was for descendant of committed block
isDescendant, err := s.blockState.IsDescendantOf(hash, just.Vote.Hash)
if err != nil {
return err
return nil, err
}

if !isDescendant {
return ErrPrecommitBlockMismatch
return nil, ErrPrecommitBlockMismatch
}

pk, err := ed25519.NewPublicKey(just.AuthorityID[:])
if err != nil {
return err
return nil, err
}

if !isInAuthSet(pk, auths) {
return ErrAuthorityNotInSet
return nil, ErrAuthorityNotInSet
}

// verify signature for each precommit
Expand All @@ -645,16 +642,16 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
SetID: setID,
})
if err != nil {
return err
return nil, err
}

ok, err := pk.Verify(msg, just.Signature[:])
if err != nil {
return err
return nil, err
}

if !ok {
return ErrInvalidSignature
return nil, ErrInvalidSignature
}

if _, ok := equivocatoryVoters[just.AuthorityID]; ok {
Expand All @@ -665,30 +662,30 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
}

if count+len(equivocatoryVoters) < threshold {
return ErrMinVotesNotMet
return nil, ErrMinVotesNotMet
}

err = verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number))
if err != nil {
return err
return nil, err
}

for _, preCommit := range fj.Commit.Precommits {
err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number))
if err != nil {
return err
return nil, err
}
}

err = s.blockState.SetFinalisedHash(hash, fj.Round, setID)
if err != nil {
return err
return nil, err
}

logger.Debugf(
"set finalised block with hash %s, round %d and set id %d",
hash, fj.Round, setID)
return nil
return scale.Marshal(fj)
Copy link
Contributor

Choose a reason for hiding this comment

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

A little confused, just for my own understanding where are the extra bytes trimmed here? Is it just by unmarshalling then marshalling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the extra bytes are removed when the un-marshaled Justification is marshaled.

}

func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number uint) error {
Expand Down
Loading