feat(blockManager): refactor and use state as single source of truth for height #847

mtsitrin · 2024-05-12T19:43:32Z

Main changes:

using m.State.NextHeight() instead of Store.Height() + 1 as the expected next height
UpdateStateFromResponses was renamed to NextValSetFromResponses as that what it actually does
- ConsensusParams should be treated separately
consolidate the state update flow, in the happy flow and the post-commit failure scenario
gossip related code moved to gossip.go
updateState methods moved to state.go

PR Standards

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

Close #330
Close #634

<-- 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.

added total produced size accumolator to be used as submission trigger

…814)

…urce-of-truth

danwt

Looks good but I've a coupel questions

Why move the kv interfaces to store? Generally interfaces should be on consumer side, not producer side. (Return concrete types, accept interfaces)
What is the purpose of adding the from param to pruning? It seems to complicate without benefit

danwt · 2024-05-13T11:51:58Z

block/gossip.go

+func (m *Manager) attemptApplyCachedBlocks() error {
+	m.retrieverMutex.Lock()
+	defer m.retrieverMutex.Unlock()
+
+	for {
+		expectedHeight := m.State.NextHeight()
+
+		cachedBlock, blockExists := m.blockCache[expectedHeight]
+		if !blockExists {
+			break
+		}
+		if err := m.validateBlock(cachedBlock.Block, cachedBlock.Commit); err != nil {
+			delete(m.blockCache, cachedBlock.Block.Header.Height)
+			/// TODO: can we take an action here such as dropping the peer / reducing their reputation?
+			return fmt.Errorf("block not valid at height %d, dropping it: err:%w", cachedBlock.Block.Header.Height, err)
+		}
+
+		err := m.applyBlock(cachedBlock.Block, cachedBlock.Commit, blockMetaData{source: gossipedBlock})
+		if err != nil {
+			return fmt.Errorf("apply cached block: expected height: %d: %w", expectedHeight, err)
+		}
+		m.logger.Debug("applied cached block", "height", expectedHeight)
+
+		delete(m.blockCache, cachedBlock.Block.Header.Height)
+	}
+
+	return nil
+}


I think we should keep this in manager.go because it's used not only in gossip

where else it is used?

in syncUntilTarget
you can check usages in IDE

block/manager_test.go

block/produce.go

block/retriever.go

block/state.go

store/pruning.go

mtsitrin · 2024-05-13T12:57:41Z

What is the purpose of adding the from param to pruning? It seems to complicate without benefit

The store doesn't manage base/latest heights anymore

omritoptix

great work! minor comments.

omritoptix · 2024-05-14T13:04:54Z

types/state.go

-		Validators:                  validatorSet,
-		LastValidators:              types.NewValidatorSet(nil),
+		NextValidators:              types.NewValidatorSet(nil),
+		Validators:                  types.NewValidatorSet(nil),
 		LastHeightValidatorsChanged: genDoc.InitialHeight,


doesn't look like this param is being used

all the validators related logic will be refactored
currently it's static and not changing at all

omritoptix · 2024-05-14T13:05:59Z

types/state.go

-		Validators:                  validatorSet,
-		LastValidators:              types.NewValidatorSet(nil),
+		NextValidators:              types.NewValidatorSet(nil),
+		Validators:                  types.NewValidatorSet(nil),
 		LastHeightValidatorsChanged: genDoc.InitialHeight,

 		ConsensusParams:                  *genDoc.ConsensusParams,
 		LastHeightConsensusParamsChanged: genDoc.InitialHeight,


doesn't look like this param is being used

omritoptix · 2024-05-14T13:11:10Z

types/state.go

+
+// SetBase sets the base height if it is higher than the existing base height
+// returns OK if the value was updated successfully or did not need to be updated
+func (s *State) SetBase(height uint64) {


method not being used (though it can be used as we do set the BaseHeight in pruning.go)

I wrapped Height as it's atomic and being used all over
the base is only for pruning. I've removed the getter

omritoptix · 2024-05-14T13:11:25Z

types/state.go

+}
+
+// Base returns height of the earliest block saved in the Store.
+func (s *State) Base() uint64 {


method not being used (though it can be used as we do get the BaseHeight in pruning.go)

omritoptix · 2024-05-14T13:57:22Z

block/block.go

-	newState.BaseHeight = m.Store.Base()
-
-	_, err = m.Store.UpdateState(newState, nil)
+	_ = m.Executor.UpdateStateAfterCommit(m.State, responses, appHash, block.Header.Height, validators)


why do we return the state here and ignore it vs not returning state at all?

i'll remove the state return var

omritoptix · 2024-05-14T14:17:08Z

block/produce.go

-	} else {
-		height := m.Store.Height()
-		newHeight = height + 1
+	if !m.State.IsGenesis() {


as this if will be entered basically 100% of the times thinking if it would be cleaner to have a produceGenesisBlock and have part of the produceBlock code reusable.

refactored it a bit, take a look

danwt · 2024-05-15T13:06:29Z

there are conflicts

…urce-of-truth

danwt

Just requested a few small refactors, including moving attemptApplyCachedBlocks() out of gossip file

block/produce.go

block/pruning.go

danwt · 2024-05-15T13:25:11Z

block/state.go

+// NextValSetFromResponses updates state based on the ABCIResponses.
+func (e *Executor) NextValSetFromResponses(state *types.State, resp *tmstate.ABCIResponses, block *types.Block) (*tmtypes.ValidatorSet, error) {
 	// Dymint ignores any setValidator responses from the app, as it is manages the validator set based on the settlement consensus
 	// TODO: this will be changed when supporting multiple sequencers from the hub
-	validatorUpdates := []*tmtypes.Validator{}
+	return state.NextValidators.Copy(), nil
+}


Please inline this at all callsite: the resp and block aren't used, and it's a one liner returning error

store/pruning.go

danwt · 2024-05-15T13:29:09Z

store/pruning.go

+	if from <= 0 {
+		return 0, fmt.Errorf("from height must be greater than 0")
 	}


seems like a usess branch since if we try to fetch h=0 it will just continue

dymint/store/pruning.go

Line 33 in 953a9b8

continue

store/storeIface.go

types/state.go

mtsitrin · 2024-05-15T14:10:14Z

@danwt I'll leave further renames to other prs

danwt · 2024-05-15T15:21:05Z

ok fine re renames
when package changes it can require a rename
Btw, will you inline NextValSetFromResponses?

mtsitrin · 2024-05-15T16:33:40Z

ok fine re renames when package changes it can require a rename Btw, will you inline NextValSetFromResponses?

not planned, will be refactored quite soon anyway

…urce-of-truth

…for height (#847) Co-authored-by: danwt <[email protected]>

mtsitrin and others added 30 commits May 5, 2024 22:28

removed pending block as submit to SL retries forever

e36a9a5

added total produced size accumolator to be used as submission trigger

removed BlockBatchSize and fix UT

0d4c641

moved accumulated count to produce to be mutex protected

5803cda

refactored error handling

aef3bc4

removed healthEvents from layers. set by manager on submission skew

1e766ab

cleanup

8c4df90

fixed defaults

f3592e5

fix UT

6973282

changed accumaletd counter to be atomic

81baf80

fixed UT

24d8b85

spelling, typo, format

6c1741a

spelling

b1c0131

feat: block progress to support ibc should be managed by produceloop (#…

9364c7c

…814)

refactored the signaling

9f76e0b

added ctx support for blocking signals

c7ffbf1

cleanup. comments

a501fa2

moved indexers to own package

72a8893

renamed state and some struct fields

9815da3

moved store back to store package

5c7c454

moving height related managment to be based on State

f53ddc4

fixed store pruning

8402076

updated block manager to use state. rpc needs fix

90031e7

cleanup

d2be2ce

Merge branch 'main' into mtsitrin/634-refactor-use-state-as-single-so…

a095024

…urce-of-truth

simplified commit

d9698f8

moved gossip methods to gossip.go

051a4e7

reverted executer key

cecf106

fixed publishEvents

b896eaa

removed unused fields. saving state post commit

bb66c2c

removed unused params. cleaned the state flow

efcf193

mtsitrin linked an issue May 12, 2024 that may be closed by this pull request

refactor: use state as single source of truth #634

Closed

danwt marked this pull request as ready for review May 13, 2024 11:40

danwt requested a review from a team as a code owner May 13, 2024 11:40

danwt marked this pull request as draft May 13, 2024 11:40

danwt marked this pull request as ready for review May 13, 2024 11:47

fixed LastBlockHeight to be atomic

3992f2a

danwt requested changes May 13, 2024

View reviewed changes

avoid copying state and pass by reference

5b3cda0

mtsitrin requested a review from omritoptix May 13, 2024 12:57

mtsitrin linked an issue May 13, 2024 that may be closed by this pull request

wrong log if syncBlockManager called from full node before batches are written #788

Closed

omritoptix reviewed May 14, 2024

View reviewed changes

mtsitrin added 2 commits May 15, 2024 10:40

fixed PR comments

953a9b8

simplified genesis check on produce block

576325e

mtsitrin requested review from omritoptix and danwt May 15, 2024 07:44

Merge branch 'main' into mtsitrin/634-refactor-use-state-as-single-so…

403ec9e

…urce-of-truth

danwt requested changes May 15, 2024

View reviewed changes

pr comments

31c8906

mtsitrin requested a review from danwt May 15, 2024 14:08

mtsitrin added 2 commits May 15, 2024 20:49

Merge branch 'main' into mtsitrin/634-refactor-use-state-as-single-so…

5ab2f15

…urce-of-truth

linter

74bfba0

github-actions bot added the dym-internal label May 15, 2024

mtsitrin merged commit 73aae62 into main May 15, 2024
6 checks passed

mtsitrin deleted the mtsitrin/634-refactor-use-state-as-single-source-of-truth branch May 15, 2024 18:03

omritoptix pushed a commit that referenced this pull request May 18, 2024

feat(blockManager): refactor and use state as single source of truth …

6e93bda

…for height (#847) Co-authored-by: danwt <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blockManager): refactor and use state as single source of truth for height #847

feat(blockManager): refactor and use state as single source of truth for height #847

mtsitrin commented May 12, 2024 •

edited

Loading

danwt left a comment

danwt May 13, 2024

mtsitrin May 13, 2024

danwt May 15, 2024

mtsitrin May 15, 2024

mtsitrin commented May 13, 2024

omritoptix left a comment

omritoptix May 14, 2024

mtsitrin May 15, 2024

omritoptix May 14, 2024

omritoptix May 14, 2024

mtsitrin May 15, 2024

omritoptix May 14, 2024

omritoptix May 14, 2024

mtsitrin May 15, 2024

omritoptix May 14, 2024

mtsitrin May 15, 2024

danwt commented May 15, 2024

danwt left a comment

danwt May 15, 2024

danwt May 15, 2024

mtsitrin commented May 15, 2024

danwt commented May 15, 2024

mtsitrin commented May 15, 2024

feat(blockManager): refactor and use state as single source of truth for height #847

feat(blockManager): refactor and use state as single source of truth for height #847

Conversation

mtsitrin commented May 12, 2024 • edited Loading

PR Standards

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

danwt 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

mtsitrin commented May 13, 2024

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

danwt commented May 15, 2024

danwt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtsitrin commented May 15, 2024

danwt commented May 15, 2024

mtsitrin commented May 15, 2024

mtsitrin commented May 12, 2024 •

edited

Loading