fix(manager): gossip any pending blocks not gossiped before (in case of restart or crash) #1045

srene · 2024-08-27T11:48:54Z

PR Standards

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A

In case the node crashes there maybe blocks that are not gossiped, but applied. In this case, blocks will never be gossiped (and neither added to blocksync store) and therefore not retrievable from P2P.
This pr fixes it by adding blocks to blocksync right after are produced and stored to db, this way blocks will be aways available in p2p, even if not gossiped.

Close #1044

<-- Briefly describe the content of this pull request -->

For Author:

Targeted PR against correct branch
included the correct type prefix in the PR title
Linked to Github issue with discussion and accepted design
Targets only one github issue
Wrote unit and integration tests
All CI checks have passed
Added relevant godoc comments

For Reviewer:

confirmed the correct type prefix in the PR title
Reviewers assigned
confirmed all author checklist items have been addressed

After reviewer approval:

In case targets main branch, PR should be squashed and merged.
In case PR targets a release branch, PR should be rebased.

block/manager.go

omritoptix · 2024-08-27T14:09:39Z

block/produce.go

@@ -120,6 +120,12 @@ func (m *Manager) produceApplyGossip(ctx context.Context, allowEmpty bool, nextP
 		return nil, nil, fmt.Errorf("gossip block: %w", err)
 	}

+	m.State.LastGossipedHeight = block.Header.Height


why updating it here and not in the gossipBlock function? that way seems much less error-prone to have one place (cause now for example in the node recovery function for gossiping were not updating it)

moved inside function

omritoptix · 2024-08-27T15:31:00Z

block/manager.go

@@ -328,3 +330,22 @@ func (m *Manager) setDA(daconfig string, dalcKV store.KV, logger log.Logger) err
 	m.Retriever = retriever
 	return nil
 }
+


another edge case I see here is the following: I was a sequencer previously, and than I wasn't (rotation. but continued being a full node), and than I am again. In this case, my store LastGossipedHeight wasn't updated when I was a full node (or at least I don't see it). and than I need to get stuck on gossiping potentially a big amount.

I think we need some sort of a sane skew after which we avoid gossiping (i.e if I'm 1M blocks away from the latest state, than probably it doesn't make sense).

i added an update for LastGossipedHeight inside UpdateTargetHeight, this will cause full-nodes also update LastGossipedHeight when retrieving form p2p or DA. this way, when rotating the value will be updated i think

omritoptix · 2024-08-27T15:35:39Z

block/state.go

@@ -32,6 +32,10 @@ func (m *Manager) LoadStateOnInit(store store.Store, genesis *tmtypes.GenesisDoc
 	}

 	m.State = s
+
+	if m.State.LastGossipedHeight == 0 {


worth adding a comment here explainig the reasoning

comment added

omritoptix · 2024-08-27T17:54:04Z

block/manager.go

@@ -285,6 +287,15 @@ func (m *Manager) UpdateTargetHeight(h uint64) {
 			break
 		}
 	}
+
+	// updating LastGossipedHeight to avoid gossiping long queue of blocks when sequencer rotation
+	if m.State.LastGossipedHeight < h {


3 issues I have with that:

the function is called update target height, but it actually also updates last gossiped height

the param is called last gossiped height for a full node, but it's tracking actually a potential sequencer only metric even if you're not a sequencer which may never be relevant for you. so maybe we need to make it a bit more make sense cause it feels hacky now (as if one would see that he would think it's the last height the full node got from gossiping but it's not)

we call the store save state on each iteration vs just updating it once. not sure why we need to iterate vs just saving h? it's just overriding in a loop.

in order to avoid these issues, i refactored the pr and i implemented a new and simpler approach.
the new approach relies the block is added to blocksync after is produced but not on gossip (this is not removed but it will be used only by full-nodes). this way if the block is not gossiped because a panic, it will be available through blocksync, and full-nodes will be able to sync via p2p anyway

Co-authored-by: Omri <omritoptix@gmail.com>

omritoptix · 2024-08-28T10:15:28Z

block/block.go

@@ -48,6 +50,16 @@ func (m *Manager) applyBlock(block *types.Block, commit *types.Commit, blockMeta
 			return fmt.Errorf("save block: %w", err)
 		}

+		gossipedBlock := p2p.BlockData{Block: *block, Commit: *commit}


for cleanliness I'd suggest moving this code part to a saveP2PBlock method or some other name which makes sense an add a godoc to this function.

also we why use context.TODO()? s

moved to new func

omritoptix · 2024-08-28T10:17:52Z

p2p/client.go

@@ -519,6 +519,11 @@ func (c *Client) blockGossipReceived(ctx context.Context, block []byte) {
 		c.logger.Error("Publishing event.", "err", err)
 	}
 	if c.conf.BlockSyncEnabled {
+		_, err := c.store.LoadBlockCid(gossipedBlock.Block.Header.Height)


is this for optimization purposes?
if so I see we're missing the AddBlockRecieved method in that case - is that fine?

yes, this is just to avoid storing the block twice if already stored.

srene requested a review from a team as a code owner August 27, 2024 11:48

srene marked this pull request as draft August 27, 2024 11:48

srene self-assigned this Aug 27, 2024

github-actions bot added the dym-internal label Aug 27, 2024

srene marked this pull request as ready for review August 27, 2024 12:43

omritoptix requested changes Aug 27, 2024

View reviewed changes

omritoptix reviewed Aug 27, 2024

View reviewed changes

srene requested a review from omritoptix August 27, 2024 16:14

omritoptix reviewed Aug 27, 2024

View reviewed changes

srene and others added 11 commits August 28, 2024 09:56

fix

a7ce1e0

only on restart

d237280

last gossiped height proto

010ef75

lint fix

3c61d38

Update block/manager.go

b6b4114

Co-authored-by: Omri <omritoptix@gmail.com>

lastgossipedheight update moved

47ef995

init LastGossipedHeight

9534e21

moved gossiped heigth update inside func

aab53d4

fix for seq rotation

687c0c5

fix test

dc22794

added block to blocksync when created

33a5d63

srene force-pushed the srene/1044-fix-block-not-gossiped-after-crash branch from fb1bb8c to 33a5d63 Compare August 28, 2024 09:37

srene added 3 commits August 28, 2024 11:44

removed gossip pending blocks

429d59d

cleaning

725b994

cleaning

3d803f7

srene requested a review from omritoptix August 28, 2024 09:57

omritoptix reviewed Aug 28, 2024

View reviewed changes

srene added 2 commits August 28, 2024 13:06

new saveP2PBlock func

752ff34

new saveP2PBlock func

302b880

saveP2PBlockToBlockSync func moved to p2p.go

e2ab37f

omritoptix approved these changes Aug 28, 2024

View reviewed changes

omritoptix merged commit e9709f6 into main Aug 28, 2024
6 checks passed

omritoptix deleted the srene/1044-fix-block-not-gossiped-after-crash branch August 28, 2024 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(manager): gossip any pending blocks not gossiped before (in case of restart or crash) #1045

fix(manager): gossip any pending blocks not gossiped before (in case of restart or crash) #1045

srene commented Aug 27, 2024 •

edited

Loading

omritoptix Aug 27, 2024

srene Aug 27, 2024

omritoptix Aug 27, 2024

srene Aug 27, 2024 •

edited

Loading

omritoptix Aug 27, 2024

srene Aug 27, 2024

omritoptix Aug 27, 2024 •

edited

Loading

srene Aug 28, 2024 •

edited

Loading

omritoptix Aug 28, 2024

srene Aug 28, 2024

omritoptix Aug 28, 2024

srene Aug 28, 2024

fix(manager): gossip any pending blocks not gossiped before (in case of restart or crash) #1045

fix(manager): gossip any pending blocks not gossiped before (in case of restart or crash) #1045

Conversation

srene commented Aug 27, 2024 • edited Loading

PR Standards

Opening a pull request should be able to meet the following requirements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srene Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omritoptix Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

srene Aug 28, 2024 • edited Loading

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

srene commented Aug 27, 2024 •

edited

Loading

srene Aug 27, 2024 •

edited

Loading

omritoptix Aug 27, 2024 •

edited

Loading

srene Aug 28, 2024 •

edited

Loading