chore(lib/grandpa): update grandpa message tracker to store messages with map of maps, also store commit messages

[noot]

Changes

• update grandpa message tracker to store vote messages with map of maps to prevent duplicates
• also update message tracker to store commit messages (that we haven't synced the block for yet), so when we sync it, we can handle the commit message
• minor sync fixes

Tests

go test ./lib/grandpa -short

Issues

• closes [GRANDPA] Message tracker use map of maps to avoid duplicates #1718

Primary Reviewer

• @EclesioMeloJunior

[arijitAD]

Remove this line.

[noot]

done

[arijitAD]

uint32(goal - start)

[arijitAD]

Can we merge this and the below code and decide on the request size and make the request code common?

	var requestSize int32
	if goal-int64(start) < int64(blockRequestSize) {
		start := best.Int64() + 1
		requestSize = int32(goal - start)
	} else {
		m := start % uint64(blockRequestSize)
		start = start - m + 1
		requestSize = ...
	}

	for i := 0; i < numRequests; i++ {
		if start > uint64(goal) {
			return
		}
		
		if requestSize / blockRequestSize == 0 {
			currentRequestSize = requestSize
		} else {
			currentRequestSize = blockRequestSize
		}
		
		... // logic
		start += uint64(currentRequestSize)
	}

[noot]

goal and start have different types so they need to both be casted to uint32 before they can be subtracted

[arijitAD]

I think both of them are int64. Let me recheck.

[noot]

updated!

[noot]

@arijitAD they aren't :(

../../dot/network/sync.go:457:25: invalid operation: goal - start (mismatched types int64 and uint64)

[arijitAD]

My bad I was checking it inside the if condition where the variable `goal` was getting shadowed.

[arijitAD]

Add a switch case,

func (h *MessageHandler) handleMessage(from peer.ID, m GrandpaMessage) (network.NotificationsMessage, error) {
	logger.Trace("handling grandpa message", "msg", m)

	switch msg := m.(type) {
	case *VoteMessage:
		if h.grandpa == nil {
			return nil, nil
		}

		// send vote message to grandpa service
		go func() {
			h.grandpa.in <- &networkVoteMessage{
				from: from,
				msg:  msg,
			}
		}()

		return nil, nil
	case *CommitMessage:
		return nil, h.handleCommitMessage(msg)
	case *NeighbourMessage:
		return nil, h.handleNeighbourMessage(from, msg)
	case *catchUpRequest:
		return h.handleCatchUpRequest(msg)
	case *catchUpResponse:
		return nil, h.handleCatchUpResponse(msg)
	}
	
	return nil, ErrInvalidMessageType
}

[noot]

updated

[arijitAD]

This channel is used to receive data
in <-chan *types.Block
We should close the channel at the sender's end.

func (bs *BlockState) UnregisterImportedChannel(id byte) {
	bs.importedLock.Lock()
	defer bs.importedLock.Unlock()

	if ch, ok := bs.imported[id]; !ok {
		close(ch)
	}
	delete(bs.imported, id)
	err := bs.importedBytePool.Put(id)
	if err != nil {
		logger.Error("failed to unregister imported channel", "error", err)
	}
}

instead of

func (t *tracker) stop() {
	close(t.stopped)
	t.blockState.UnregisterImportedChannel(t.chanID)
	close(t.in)
}

Reference:

One general principle of using Go channels is don't close a channel from the receiver side and don't close a channel if the channel has multiple concurrent senders. In other words, we should only close a channel in a sender goroutine if the sender is the only sender of the channel.

[noot]

yeah I think this relies on Ed's refactor in #1736 so I'd rather not change it here, I agree with you though that the whole notifier code needs to be refactored. I can change it here if you'd like but it overlaps with Ed's PR

[EclesioMeloJunior]

I commented on this on Ed's PR #1736 (comment)

[arijitAD]

Change to

	voteMsg, has := t.voteMessages[v.msg.Message.Hash]
	if !has {
		voteMsg = make(map[ed25519.PublicKeyBytes]*networkVoteMessage)
		t.voteMessages[v.msg.Message.Hash] = voteMsg
	}

	voteMsg[v.msg.Message.AuthorityID] = v

Code to verify

func TestMessageTracker_MapInsideMap(t *testing.T) {
	mm := make(map[common.Hash]map[ed25519.PublicKeyBytes]*networkVoteMessage)
	hash := common.NewHash([]byte{0})

	voteMsg, ok := mm[hash]
	require.False(t, ok)

	voteMsg = make(map[ed25519.PublicKeyBytes]*networkVoteMessage)
	mm[hash] = voteMsg

	AuthorityID := ed25519.PublicKeyBytes(common.MustHexToHash("0x34602b88f60513f1c805d87ef52896934baf6a662bc37414dbdbf69356b1a691"))
	voteMsg[AuthorityID] = &networkVoteMessage{}

	voteMsg, ok = mm[hash]
	require.True(t, ok)

	_, ok = voteMsg[AuthorityID]
	require.True(t, ok)
}

[noot]

added a test for the maps in addVote

[arijitAD]

Better indentation

func (t *tracker) handleBlocks() {
	handleBlocks := func(b *types.Block) {
		t.mapLock.Lock()
		defer t.mapLock.Unlock()

		h := b.Header.Hash()
		if vms, has := t.voteMessages[h]; has {
			for _, v := range vms {
				_, err := t.handler.handleMessage(v.from, v.msg)
				if err != nil {
					logger.Warn("failed to handle vote message", "message", v, "error", err)
				}
			}

			delete(t.voteMessages, h)
		}

		if cm, has := t.commitMessages[h]; has {
			_, err := t.handler.handleMessage("", cm)
			if err != nil {
				logger.Warn("failed to handle commit message", "message", cm, "error", err)
			}

			delete(t.commitMessages, h)
		}
	}
	
	for {
		select {
		case b := <-t.in:
			if b == nil {
				continue
			}
			handleBlocks(b)
		case <-t.stopped:
			return
		}
	}
}

[EclesioMeloJunior]

Or create a function func (t *tracker) handle(b *types.Block) and call it inside the case:

for { 
  select { 
  case b := <-t.in: 
     if b == nil { continue }
     t.handle(b)
...

[noot]

updated to have a separate handleBlock function

[EclesioMeloJunior]

Issue created to this TODO -> #1771

[noot]

nice, I'm still unsure about this todo, after the sync refactor it hopefully won't be necessary, but we'll see

[arijitAD]

Suggested change

	start = uint64(best.Int64() + 1)
	start = best.Uint64() + 1

[noot]

updated

[arijitAD]

Suggested change

	t.voteMessages[v.msg.Message.Hash] = make(map[ed25519.PublicKeyBytes]*networkVoteMessage)
	msgs = t.voteMessages[v.msg.Message.Hash]
	msgs = make(map[ed25519.PublicKeyBytes]*networkVoteMessage)
	t.voteMessages[v.msg.Message.Hash] = msgs

[noot]

updated

[codecov]

Codecov Report

Merging #1769 (362ef19) into development (66b1410) will increase coverage by 0.00%.
The diff coverage is 77.27%.

@@             Coverage Diff              @@
##           development    #1769   +/-   ##
============================================
  Coverage        59.09%   59.09%           
============================================
  Files              184      184           
  Lines            19383    19390    +7     
============================================
+ Hits             11454    11459    +5     
+ Misses            5951     5944    -7     
- Partials          1978     1987    +9     

Flag	Coverage Δ
unit-tests	59.09% <77.27%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/grandpa/message_tracker.go	72.00% <69.69%> (-15.88%)	⬇️
lib/grandpa/message_handler.go	56.89% <82.35%> (-1.09%)	⬇️
dot/network/sync.go	63.70% <84.61%> (+1.47%)	⬆️
lib/grandpa/grandpa.go	59.87% <100.00%> (-0.32%)	⬇️
lib/grandpa/vote_message.go	80.50% <100.00%> (ø)
lib/grandpa/network.go	52.89% <0.00%> (-1.66%)	⬇️
lib/grandpa/message.go	59.31% <0.00%> (-1.38%)	⬇️
lib/blocktree/blocktree.go	55.61% <0.00%> (-1.13%)	⬇️
... and 1 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 66b1410...362ef19. Read the comment docs.

[github-actions]

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀