fix: nodes keep out of sync when missing gossiped block (issue #284) #540

srene · 2023-12-20T11:02:23Z

PR Standards

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

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.

…of libp2p

srene · 2023-12-20T11:16:47Z

Fixing the following issue #284.
The problem is when nodes are disconnected or join the network with some delay, they are unable to receive missing blocks and keep out of sync till next batch submission.
The solution enables that nodes can receive previously missed blocks when disconnected and keep up with the block height.
The solution relies in the following:

Modification to go-libp2p-pubsub to emit gossip of cached messages to any node when the number of nodes of the network is small (below the degree D), otherwise no gossip (periodic broadcast of previously received messages) is received by nodes.
Caching of out-of-order received blocks, to apply them later when missing block received.
Modification of default gossipsub parameters to enable previous blocks caching

omritoptix · 2023-12-20T11:23:45Z

@srene tests fail

codecov · 2023-12-20T13:46:49Z

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (d2b2cdf) 52.46% compared to head (4b4a6e0) 52.43%.

Files	Patch %	Lines
block/block.go	31.81%	13 Missing and 2 partials ⚠️
block/manager.go	11.76%	15 Missing ⚠️
block/retriever.go	25.00%	2 Missing and 1 partial ⚠️
config/config.go	0.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   52.46%   52.43%   -0.04%     
==========================================
  Files         100      100              
  Lines       14717    14764      +47     
==========================================
+ Hits         7722     7742      +20     
- Misses       5886     5911      +25     
- Partials     1109     1111       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

srene · 2023-12-20T14:20:43Z

@omri build successful with go1.19 after gossipsub downgrade

omritoptix

IMO applyBlock should only applyBlock and it's being called from the sequencer or full nodes but adding caching logic there is not relevant for some of the callers (like the sequncer).

You can simply check in the ApplyBlockCallback before even calling the applyBlock if it should be cached (by the store height etc) . If it’s correct apply it. If not add to cache. And than every time I get a block, assuming my cache is not empty I can try and call AttemptApplyCachedBlocks (or some better name)

This function should probably be called in 2 situations:

once we get a new gossiped block
once we get a block from the DA

omritoptix · 2023-12-27T15:01:46Z

block/retriever.go

@@ -50,19 +50,22 @@ func (m *Manager) syncUntilTarget(ctx context.Context, syncTarget uint64) error
 			return err
 		}

-		if settlementBatch.StartHeight != currentHeight+1 {
+		//code commented out because it may be the case the start height from the da batch is not exactly the next block from the last received


you can simply delete the entire lines. no need for leaving it with comments.

omritoptix · 2023-12-27T15:01:51Z

block/retriever.go

-		if currentHeight != settlementBatch.EndHeight {
+
+		//code commented out because it may be the case the current height is higher than da height after applying pending blocks, in case of missing blocks
+		/*if currentHeight != settlementBatch.EndHeight {


you can simply delete the entire lines. no need for leaving it with comments.

omritoptix · 2023-12-28T10:38:52Z

block/block.go

@@ -112,6 +113,30 @@ func (m *Manager) applyBlock(ctx context.Context, block *types.Block, commit *ty
 	return nil
 }

+func (m *Manager) checkPrevCachedBlocks(ctx context.Context) error {


Probably best to create a mutex on the this function as can be called from both DA code and gossiped blocks callback which may lead to a Race condition.

true. mutex added

omritoptix · 2023-12-28T10:40:00Z

block/block.go

@@ -112,6 +113,30 @@ func (m *Manager) applyBlock(ctx context.Context, block *types.Block, commit *ty
 	return nil
 }

+func (m *Manager) checkPrevCachedBlocks(ctx context.Context) error {


as this function is not only checking it's also applying I think the name is confusing. Also when it says "Prev" blocks it can be confusing as it has been previously cached but those blocks can be in the future (i.e not yet applied)

name changed to attemptApplyCachedBlocks

omritoptix · 2023-12-28T10:40:58Z

block/manager.go

-		m.logger.Debug("Failed to apply block", "err", err)
+
+	if block.Header.Height != m.store.Height()+1 {
+		// We crashed after the commit and before updating the store height.


This comment is actually not accurate as we can cache blocks which just came out of order regardless to any crash.

omritoptix · 2023-12-28T10:42:18Z

block/manager.go

+
+	if block.Header.Height != m.store.Height()+1 {
+		// We crashed after the commit and before updating the store height.
+		m.prevBlock[block.Header.Height] = &block


We should probably try to apply the cached blocks even if we just applied a block because and not just if the blocks are out of order. for example, if we got previously block at (store height + 2) and put it into cache and now we got storeHeight + 1 (which means we won't go into the "if") we can still apply the cached block and no need to wait for the next out-of-order block in order to try it..

I would simply add at the end of the function a check, if the block we have in cache is == m.storeHeight + 1 then run ApplyCachedBlock.

true. having function call in the if condition may need an extra block to apply all cached blocks. it's been changed

omritoptix · 2023-12-28T10:43:34Z

block/manager.go

+		m.logger.Debug("Caching block", "block height", block.Header.Height, "store height", m.store.Height())
+		err := m.checkPrevCachedBlocks(context.Background())
+		if err != nil {
+			m.logger.Debug("Error applying previous cached blocks", "err", err)


Im not sure if Error is correct here as it’s not necessarily due to error - it’s mostly based on attempting. So maybe “failed” . especially when you log it with a "debug"

log message changed

omritoptix · 2023-12-28T10:44:05Z

block/manager.go

+		// We crashed after the commit and before updating the store height.
+		m.prevBlock[block.Header.Height] = &block
+		m.prevCommit[block.Header.Height] = &commit
+		m.logger.Debug("Caching block", "block height", block.Header.Height, "store height", m.store.Height())


IMO makes more send to move the “caching block” message before the actual caching and not after.

omritoptix · 2023-12-28T10:45:02Z

block/manager.go

+		m.prevBlock[block.Header.Height] = &block
+		m.prevCommit[block.Header.Height] = &commit
+		m.logger.Debug("Caching block", "block height", block.Header.Height, "store height", m.store.Height())
+		err := m.checkPrevCachedBlocks(context.Background())


Why context.background() is being passed? seems like it’s not being used.

it is actually used when calling applyBlock. there is no context in the manager so context.background() is used by now

srene added 6 commits December 20, 2023 10:16

replace for go-libp2p-pubsub repo pointing to dymension repo instead …

28e0b95

…of libp2p

updating versions of required go-libp2p-pubsub dependencies

5c0d60d

removing reference to libp2p old libraries (go-libp2p-core)

712e50f

removing references to deprecated peer id Pretty function

f741746

caching blocks to apply them later when received out-of-order

08f36ec

increasing gossipsub cache size

9b42765

srene requested a review from a team as a code owner December 20, 2023 11:02

downgrading libp2p to be compatible with go 1.19

cab1d26

checking error after applying block

88666b1

srene and others added 2 commits December 27, 2023 08:36

wrong checks after syncing from da

1895c79

setting gossipsub cache size from config parameters

849431e

srene mentioned this pull request Dec 27, 2023

GossipSub default configurations #521

Closed

omritoptix reviewed Dec 27, 2023

View reviewed changes

srene added 3 commits December 28, 2023 09:34

refactoring caching blocks with new function checkPrevCachedBlocks

735a31e

removing commented code

5773d82

adding missing error check

a951998

omritoptix reviewed Dec 28, 2023

View reviewed changes

srene added 2 commits December 28, 2023 12:38

applying cached blocks in all cases + minor edits

b0029b5

caching blocks only when higher than store height

4b4a6e0

omritoptix approved these changes Dec 28, 2023

View reviewed changes

omritoptix linked an issue Dec 28, 2023 that may be closed by this pull request

Full nodes can't catch up on sequencer block gossiping once out of sync #284

Closed

omritoptix merged commit 14ae6fd into dymensionxyz:main Dec 28, 2023
4 of 5 checks passed

srene added a commit that referenced this pull request Jan 2, 2024

fix: nodes keep out of sync when missing gossiped block (#540)

be28b41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: nodes keep out of sync when missing gossiped block (issue #284) #540

fix: nodes keep out of sync when missing gossiped block (issue #284) #540

srene commented Dec 20, 2023 •

edited

Loading

srene commented Dec 20, 2023

omritoptix commented Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

srene commented Dec 20, 2023 •

edited

Loading

omritoptix left a comment

omritoptix Dec 27, 2023

srene Dec 28, 2023

omritoptix Dec 27, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

omritoptix Dec 28, 2023

srene Dec 28, 2023

fix: nodes keep out of sync when missing gossiped block (issue #284) #540

fix: nodes keep out of sync when missing gossiped block (issue #284) #540

Conversation

srene commented Dec 20, 2023 • edited Loading

PR Standards

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

srene commented Dec 20, 2023

omritoptix commented Dec 20, 2023

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

srene commented Dec 20, 2023 • edited Loading

omritoptix left a 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

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 Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading

srene commented Dec 20, 2023 •

edited

Loading