lib/grandpa: ensure grandpa catch-up logic works #2983

kishansagathiya · 2022-11-29T14:36:29Z

Changes

ensure catch-up logic works when interacting with other gossamer nodes
If round in the neighbour message is ahead of our current round by a
threshold, send a catch up request
process the catch up response, if we can't process it at the moment,
store it to process later.

Tests

Run westend local devnet with 4 substrate nodes and one gossamer node.
Let them run for a few minutes, let few rounds finish.
Stop gossamer node. Wait for few more rounds to finish (at least 2 more rounds).
Start the gossamer node.
Check through logs that it sends a catch up request to other nodes and catches up to the latest round

Issues

Fixes lib/grandpa: ensure catch-up logic works when interacting with substrate nodes #1531

Primary Reviewer

@timwu20

- ensure grandpa catch-up logic works when interacting with substrate nodes - If round in the neighbour message is ahead of our current round by a threshold, send a catch up request - process the catch up response, if we can't process it at the moment, store it to process later. Closes #1531

lib/grandpa/catch-up.go

EclesioMeloJunior · 2023-01-11T16:12:46Z

lib/grandpa/catch-up.go

+		return fmt.Errorf("failed to send catch up request: %w", err)
+	}
+
+	logger.Debugf("successfully sent a catch up request to node %s, for round number %d and set ID %d",


Suggested change

logger.Debugf("successfully sent a catch up request to node %s, for round number %d and set ID %d",

logger.Debugf("successfully sent a catch up request to peer %s, for round %d and set ID %d",

EclesioMeloJunior · 2023-01-11T16:13:58Z

lib/grandpa/catch-up.go

+	case <-c.catchUpResponseCh:
+		return nil
+	case <-timer.C:
+		return errors.New("timeout")


would be nice to have this as an sentinel error like: ErrCatchUpTimeout

EclesioMeloJunior · 2023-01-11T16:18:10Z

lib/grandpa/catch-up.go

+	logger.Debugf("successfully sent a catch up request to node %s, for round number %d and set ID %d",
+		to, round, setID)
+
+	c.waitingOnResponse.Store(true)


you call this c.waitingOnResponse.Store(true) function in sendCatchUpRequest and call here as well, should you remove this one?

EclesioMeloJunior · 2023-01-11T16:19:22Z

lib/grandpa/catch-up.go

+	}
+
+	logger.Debugf(
+		"processing catch up response with hash %s for round %d and set id %d",


Suggested change

"processing catch up response with hash %s for round %d and set id %d",

"processing catch up response with hash %s for round %d and set ID %d",

EclesioMeloJunior · 2023-01-11T16:20:10Z

lib/grandpa/catch-up.go

+		msg.Hash, msg.Round, msg.SetID)
+
+	// if we aren't currently expecting a catch up response, return
+	if !c.waitingOnResponse.Load().(bool) {


Suggested change

if !c.waitingOnResponse.Load().(bool) {

// TODO: decrease the peer reputation

if !c.waitingOnResponse.Load().(bool) {

EclesioMeloJunior · 2023-01-11T16:24:39Z

lib/grandpa/catch-up.go

+	if msg.Hash.IsEmpty() || msg.Number == 0 {
+		return ErrGHOSTlessCatchUp
+	}


this could be the second verification step in this function placed right after the if !c.grandpa.authority

EclesioMeloJunior · 2023-01-11T16:27:36Z

lib/grandpa/catch-up.go

+	c.requestsSent = make(map[peer.ID]CatchUpRequest)
+	c.lock.Unlock()
+
+	logger.Debugf("caught up to round; unpaused service and grandpa state round is %d", c.grandpa.state.round)


Suggested change

logger.Debugf("caught up to round; unpaused service and grandpa state round is %d", c.grandpa.state.round)

logger.Debugf("caught up to round; starting at round %d", c.grandpa.state.round)

EclesioMeloJunior · 2023-01-11T16:30:22Z

lib/grandpa/catch-up.go

+	return nil
+}
+
+func (c *catchUp) verifyPreCommitJustification(msg *CatchUpResponse) error {


this function already exists in lib/grandpa/message_handler.go, why not use it?

EclesioMeloJunior · 2023-01-11T16:30:37Z

lib/grandpa/catch-up.go

+	return nil
+}
+
+func (c *catchUp) verifyPreVoteJustification(msg *CatchUpResponse) (common.Hash, error) {


this function already exists in lib/grandpa/message_handler.go, why not use it?

timwu20 · 2023-01-12T22:16:08Z

lib/grandpa/catch-up.go

+	// TODO: Clean up all request sent before 5 min / (neighbour message interval)
+	c.lock.Lock()
+	_, ok := c.requestsSent[to]
+	c.lock.Unlock()
+	if ok {
+		logger.Debugf("ignoring neighbour message since we already sent a catch-up request to this peer: %s", to)
+		return nil
+	}


can we implement a TTL on this map?

Will there be cases where you could ask for multiple catch up messages to the same peer?

timwu20 · 2023-01-13T03:55:39Z

lib/grandpa/catch-up.go

@@ -0,0 +1,268 @@
+// Copyright 2021 ChainSafe Systems (ON)


can you rename to catch_up.go.

timwu20 · 2023-01-13T03:58:58Z

lib/grandpa/catch-up.go

+	}
+
+	c.lock.Lock()
+	c.requestsSent[to] = *req


Any reason why we need to copy the req here? Can this map hold *CatchupRequest?

timwu20 · 2023-01-13T04:01:29Z

lib/grandpa/catch-up.go

+	requestsSent map[peer.ID]CatchUpRequest
+
+	catchUpResponseCh chan *CatchUpResponse
+	waitingOnResponse *atomic.Value


Suggested change

waitingOnResponse *atomic.Value

waitingOnResponse *atomic.Bool

timwu20 · 2023-01-13T04:12:50Z

lib/grandpa/network.go

+	case VoteMessageType, CommitMessageType:
+		s.network.GossipMessage(msg)
+		return true, nil
+	case NeighborMessageType, CatchUpRequestMessageType, CatchUpResponseMessageType:


should we have a default case that returns an error for unsupported messages?

it seems like polkadot sends current round in neighbor message instead of last finalised round. So, I had to send a catch request with one round previous. Otherwise, polkadot would discard our request.

kishansagathiya · 2023-01-13T16:49:09Z

Current testing status,

On seeing a neighbour message, gossamer sends a catch up request. Substrate responds with catch up response.

Gossamer doesnot receive the catch message or doesn't process it properly.

timwu20 · 2023-08-14T19:38:04Z

can you mark as draft or close @kishansagathiya ?

q9f · 2024-01-12T12:05:40Z

Shall we merge this some day or put the code somewhere else?

kishansagathiya · 2024-01-16T09:07:06Z

Shall we merge this some day or put the code somewhere else?

When I tried to do this, I could not finish this task completely. I was able to send a catch up request to polkadot, I could see the request on logs of polkadot. I could see in polkadot logs that it was sending a response back, but on gossamer side I was not able to see the response. I tried to do this for quite a few days. Got some other folks to help as well, but could not get any results. And eventually gave up.

In gossamer, there is an active task of grandpa refactor is going on (in branch feat/grandpa). That might be the reason no one else has picked it up as well.

kishansagathiya · 2024-02-22T13:51:46Z

closing for too old

kishansagathiya added 8 commits November 29, 2022 20:00

Merge branch 'development' into feat/kishan/grandpa-catchup-2

d19ed2a

fixing tests

10a7312

Merge branch 'development' into feat/kishan/grandpa-catchup-2

8cdc4b5

small cleanup

a16c4d2

Merge branch 'development' into feat/kishan/grandpa-catchup-2

1afb2d1

fixed a test

d309083

fixed some more tests

6e96ee1

kishansagathiya marked this pull request as ready for review December 5, 2022 16:26

kishansagathiya requested review from noot, edwardmack, timwu20, EclesioMeloJunior, jimjbrettj and qdm12 as code owners December 5, 2022 16:26

EclesioMeloJunior requested changes Dec 6, 2022

View reviewed changes

lib/grandpa/catch-up.go Outdated Show resolved Hide resolved

lib/grandpa/catch-up.go Show resolved Hide resolved

lib/grandpa/catch-up.go Outdated Show resolved Hide resolved

kishansagathiya added 3 commits December 15, 2022 20:19

Merge branch 'development' into feat/kishan/grandpa-catchup-2

9851e2a

removed grandpa.paused

56b59bd

removed grandpa.resumed

45519e6

kishansagathiya requested a review from EclesioMeloJunior December 15, 2022 16:13

kishansagathiya added 4 commits January 4, 2023 16:49

Merge branch 'development' into feat/kishan/grandpa-catchup-2

07beeff

Merge branch 'development' into feat/kishan/grandpa-catchup-2

41453f2

fixing lint

8256f2e

fixing a test

e5c9af4

EclesioMeloJunior requested changes Jan 11, 2023

View reviewed changes

remove bestResponse

ff03fa2

timwu20 requested changes Jan 13, 2023

View reviewed changes

fixing the round number

1fd705e

it seems like polkadot sends current round in neighbor message instead of last finalised round. So, I had to send a catch request with one round previous. Otherwise, polkadot would discard our request.

Merge branch 'development' into feat/kishan/grandpa-catchup-2

c902b4f

fixing merge

13ecd5a

kishansagathiya marked this pull request as draft August 15, 2023 05:50

q9f added A-stale issue or PR is deprecated and needs to be closed. S-grandpa issues related to block finality. labels Jan 16, 2024

kishansagathiya closed this Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/grandpa: ensure grandpa catch-up logic works #2983

lib/grandpa: ensure grandpa catch-up logic works #2983

kishansagathiya commented Nov 29, 2022 •

edited

Loading

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

EclesioMeloJunior Jan 11, 2023

timwu20 Jan 12, 2023

timwu20 Jan 13, 2023

timwu20 Jan 13, 2023

timwu20 Jan 13, 2023

timwu20 Jan 13, 2023

kishansagathiya commented Jan 13, 2023

timwu20 commented Aug 14, 2023

q9f commented Jan 12, 2024

kishansagathiya commented Jan 16, 2024 •

edited

Loading

kishansagathiya commented Feb 22, 2024

	logger.Debugf("successfully sent a catch up request to node %s, for round number %d and set ID %d",
	logger.Debugf("successfully sent a catch up request to peer %s, for round %d and set ID %d",

	"processing catch up response with hash %s for round %d and set id %d",
	"processing catch up response with hash %s for round %d and set ID %d",

	if !c.waitingOnResponse.Load().(bool) {
	// TODO: decrease the peer reputation
	if !c.waitingOnResponse.Load().(bool) {

	logger.Debugf("caught up to round; unpaused service and grandpa state round is %d", c.grandpa.state.round)
	logger.Debugf("caught up to round; starting at round %d", c.grandpa.state.round)

	waitingOnResponse *atomic.Value
	waitingOnResponse *atomic.Bool

lib/grandpa: ensure grandpa catch-up logic works #2983

lib/grandpa: ensure grandpa catch-up logic works #2983

Conversation

kishansagathiya commented Nov 29, 2022 • edited Loading

Changes

Tests

Issues

Primary Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Jan 13, 2023

timwu20 commented Aug 14, 2023

q9f commented Jan 12, 2024

kishansagathiya commented Jan 16, 2024 • edited Loading

kishansagathiya commented Feb 22, 2024

kishansagathiya commented Nov 29, 2022 •

edited

Loading

kishansagathiya commented Jan 16, 2024 •

edited

Loading