Skip to content

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

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

Merged
edwardmack 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 all commits
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
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
16 changes: 9 additions & 7 deletions dot/sync/chain_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,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 @@ -275,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 @@ -293,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 @@ -417,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 @@ -452,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 @@ -484,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 @@ -687,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.

5 changes: 4 additions & 1 deletion dot/sync/syncer_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ func newTestSyncer(t *testing.T) *Service {
cfg.LogLvl = log.Trace
mockFinalityGadget := NewMockFinalityGadget(ctrl)
mockFinalityGadget.EXPECT().VerifyBlockJustification(gomock.AssignableToTypeOf(common.Hash{}),
gomock.AssignableToTypeOf([]byte{})).AnyTimes()
gomock.AssignableToTypeOf([]byte{})).DoAndReturn(func(hash common.Hash, justification []byte) ([]byte, error) {
return justification, nil
}).AnyTimes()
Copy link
Copy Markdown
Contributor

@jimjbrettj jimjbrettj Jun 23, 2022

Choose a reason for hiding this comment

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

Is there a way to know exactly how many times this runs?

Copy link
Copy Markdown
Contributor Author

@edwardmack edwardmack Jun 27, 2022

Choose a reason for hiding this comment

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

No, since this is in newTestSyncer which is used in multiple tests that have different conditions we don't know if this will be called or not. Ideally the tests that use this would be refactored to put the mocks in-line for each individual test so that we would know exactly how many times it's called.


cfg.FinalityGadget = mockFinalityGadget
cfg.Network = newMockNetwork()
cfg.Telemetry = mockTelemetryClient
Expand Down
47 changes: 24 additions & 23 deletions lib/grandpa/message_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,48 +553,49 @@ func (h *MessageHandler) verifyJustification(just *SignedVote, round, setID uint
return nil
}

// VerifyBlockJustification verifies the finality justification for a block
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
}

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 @@ -604,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 @@ -617,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 @@ -641,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 @@ -661,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
Copy Markdown
Contributor

@jimjbrettj jimjbrettj Jun 23, 2022

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
Copy Markdown
Contributor Author

@edwardmack edwardmack Jun 27, 2022

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