fix(manager/indexer): pruning fixes #1147

srene · 2024-10-18T11:22:47Z

PR Standards

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

--

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

Close #1145

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

omritoptix

Still in the middle of the review

omritoptix · 2024-10-20T19:48:03Z

block/pruning.go

-// PruneBlocks prune all block related data from dymint store up to (but not including) retainHeight. It returns the number of blocks pruned, used for testing.
-func (m *Manager) PruneBlocks(retainHeight uint64) (uint64, error) {
+// PruneBlocks prune all block related data from dymint store up to (but not including) retainHeight.
+func (m *Manager) PruneBlocks(retainHeight uint64) {


I believe you can DRY it out and remove the else part also. check this (didn't test it):

func (m *Manager) PruneBlocks(retainHeight uint64) { nextSubmissionHeight := m.NextHeightToSubmit() if m.IsProposer() && nextSubmissionHeight < retainHeight { m.logger.Debug("cannot prune blocks before they have been submitted. using height last submitted height for pruning", "retain_height", retainHeight, "height_to_submit", nextSubmissionHeight) retainHeight = nextSubmissionHeight } m.pruneStore("blocksync", func(from, to uint64) (uint64, error) { return m.P2PClient.RemoveBlocks(context.Background(), from, to) }, &m.State.BaseBlocksyncHeight, retainHeight) m.pruneStore("indexer", m.IndexerService.Prune, &m.State.BaseIndexerHeight, retainHeight) m.pruneStore("dymint", func(from, to uint64) (uint64, error) { return m.Store.PruneStore(from, to, m.logger) }, &m.State.BaseHeight, retainHeight) if _, err := m.Store.SaveState(m.State, nil); err != nil { m.logger.Error("save state", "err", err) } } func (m *Manager) pruneStore(storeName string, pruneFunc func(uint64, uint64) (uint64, error), baseHeight *uint64, retainHeight uint64) { pruned, err := pruneFunc(*baseHeight, retainHeight) if err != nil { m.logger.Error(fmt.Sprintf("pruning %s store", storeName), "retain_height", retainHeight, "err", err) return } m.logger.Debug(fmt.Sprintf("%s store pruned", storeName), "from", *baseHeight, "to", retainHeight, "pruned", pruned) *baseHeight = retainHeight }

added inline func for logging

omritoptix · 2024-10-20T19:58:35Z

block/pruning.go

 	}

-	m.State.BaseHeight = retainHeight
+	// store state with base heights updates
 	_, err = m.Store.SaveState(m.State, nil)


I think saving the state in the background could lead on a race condition on the state as you may save incomplete state which was changed by the applyBlock.

The m.State should only be ever accessed in either thread-safe way or by the applyBlock.

I see no reason to save pruning data on the state but on an ad-hoc object

baseheight moved to store outside state

omritoptix · 2024-10-20T20:07:44Z

indexers/txindex/kv/kv.go

 	pruned := uint64(0)
+	tobeFlushed := uint64(0)


omritoptix · 2024-10-20T20:11:02Z

indexers/txindex/kv/kv.go

+		// first all events are pruned associated to the same height
+		prunedEvents, err := txi.pruneEvents(h, batch)
+		if err != nil {
+			continue


should we log here at least something so will know we had problem pruning?

danwt · 2024-10-22T16:56:42Z

build n test failing sir

omritoptix · 2024-11-04T15:20:15Z

block/manager.go

@@ -101,6 +101,9 @@ type Manager struct {

 	// validates all non-finalized state updates from settlement, checking there is consistency between DA and P2P blocks, and the information in the state update.
 	SettlementValidator *SettlementValidator
+
+	// BaseHeight is the height of the first block we have in store after pruning.
+	BaseHeight uint64


omritoptix · 2024-11-04T15:25:04Z

store/pruning.go

 	}
-
-	prunedBlocks, err := s.pruneBlocks(from, to, logger)
+	prunedBlocks, err = s.pruneBlocks(from, to, logger)


if we e.g get erorr here, than we still save the to as base height. (i.e some stores succeeded and some not).
not sure if critical for now but probably worth opening an issue on it otherwise you have stores which are partially pruned.

i actually refactored the func to prune all indexes for every height iteration, so in case it fails it will be more likely all failed at the same height, although the solution not perfect. i created issue for this #1196

omritoptix · 2024-11-04T15:42:29Z

rpc/client/client.go

@@ -325,8 +325,12 @@ func (c *Client) GenesisChunked(context context.Context, id uint) (*ctypes.Resul
 func (c *Client) BlockchainInfo(ctx context.Context, minHeight, maxHeight int64) (*ctypes.ResultBlockchainInfo, error) {
 	const limit int64 = 20

-	minHeight, maxHeight, err := filterMinMax(
-		int64(c.node.BlockManager.State.BaseHeight),
+	baseHeight, err := c.node.BlockManager.Store.LoadBaseHeight()


currently we only set base height when pruning. it means if we never set pruning, will never have base height and we won't be able to serve this method.

when was base height saved before?

set to 1 in case baseheight not found

omritoptix · 2024-11-04T15:47:56Z

indexers/txindex/indexer_service.go

-	_, err := is.blockIdxr.Prune(from, to, is.Logger)
+func (is *IndexerService) Prune(to uint64, s store.Store) (uint64, error) {
+	// load indexer base height
+	indexerBaseHeight, err := s.LoadIndexerBaseHeight()


seems that if we fail to loa the height we just log it and the indexerBaseHeight will be set to 0.
this means tha the pruning functions will be called with from 0 (e.g blockIndexer) (and may unncessarily iterate tons of indexes.)

not sure what is the issue here, if not found it means it was never set and therefore it needs to start from 0 because it was never pruned...

omritoptix · 2024-11-04T15:49:19Z

p2p/client.go


-	if to <= from {
-		return fmt.Errorf("to height must be greater than from height: to: %d: from: %d: %w", to, from, gerrc.ErrInvalidArgument)
+	from, err := c.store.LoadBlockSyncBaseHeight()


same as my previous. from will be set to 0 which will cause very high iteration.

srene self-assigned this Oct 18, 2024

srene requested a review from a team as a code owner October 18, 2024 11:22

srene marked this pull request as draft October 18, 2024 11:22

github-actions bot added the dym-internal label Oct 18, 2024

srene marked this pull request as ready for review October 18, 2024 13:56

omritoptix requested changes Oct 20, 2024

View reviewed changes

srene force-pushed the srene/1145-fix-pruning branch from 691f18a to e6d28c7 Compare October 22, 2024 16:17

srene marked this pull request as draft October 23, 2024 07:46

srene mentioned this pull request Oct 24, 2024

feat(manager): block gossiping misbehavior detection #1117

Merged

12 tasks

srene force-pushed the srene/1145-fix-pruning branch 3 times, most recently from a27927b to 75e0ffe Compare October 30, 2024 08:51

srene marked this pull request as ready for review October 30, 2024 20:31

srene requested a review from omritoptix October 30, 2024 20:32

srene mentioned this pull request Oct 31, 2024

feat(manager): decouple drs version from commit #1182

Merged

12 tasks

omritoptix requested changes Nov 4, 2024

View reviewed changes

srene force-pushed the srene/1145-fix-pruning branch from e92551b to 9f47fed Compare November 4, 2024 16:46

srene requested a review from omritoptix November 4, 2024 16:48

srene marked this pull request as draft November 5, 2024 07:23

srene added 10 commits November 5, 2024 13:02

separate pruning services

c0c2b75

prune update

1f60040

test fix

8e3c2b4

test fix

9eb61c3

fixing tests

df2ed37

minor edit

056d469

lint fix

5d26a75

refactor base height store

561f67c

drying

b908dee

logging

b282083

srene added 21 commits November 5, 2024 13:04

lint

b92013e

fix

bcac9ba

minor edit

e60be33

fix

3a823f3

cleaning

ec5f98b

logging

d2f148b

retain to

69207d0

fix

d82a277

logging

02edb30

check before deleting

8adaee9

lint

20a7ff2

minor edit

c711ef1

fix

c510fed

improving logs

323b6cb

fix merge

cf755f9

merge fix

d19da96

rename prune func

df047ac

comments

9ad5222

refactor

0873267

remove overwrite baseheight

363aca9

go doc

4ee4cfb

srene force-pushed the srene/1145-fix-pruning branch from 2510be5 to 4ee4cfb Compare November 5, 2024 12:06

test fix

a5d4732

srene marked this pull request as ready for review November 5, 2024 12:30

remove log

3264a92

omritoptix approved these changes Nov 7, 2024

View reviewed changes

omritoptix merged commit e5e8857 into main Nov 7, 2024
5 of 6 checks passed

omritoptix deleted the srene/1145-fix-pruning branch November 7, 2024 09:23

srene added a commit that referenced this pull request Nov 10, 2024

fix(manager/indexer): pruning fixes (#1147)

4a78f54

srene added a commit that referenced this pull request Nov 11, 2024

fix(manager/indexer): pruning fixes (#1147)

e629e3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(manager/indexer): pruning fixes #1147

fix(manager/indexer): pruning fixes #1147

srene commented Oct 18, 2024

omritoptix left a comment

omritoptix Oct 20, 2024

srene Oct 26, 2024

omritoptix Oct 20, 2024

srene Oct 26, 2024

omritoptix Oct 20, 2024

srene Oct 26, 2024

omritoptix Oct 20, 2024

srene Oct 26, 2024

danwt commented Oct 22, 2024

omritoptix Nov 4, 2024

srene Nov 4, 2024

omritoptix Nov 4, 2024

srene Nov 5, 2024

omritoptix Nov 4, 2024

srene Nov 4, 2024

omritoptix Nov 4, 2024

srene Nov 4, 2024

omritoptix Nov 4, 2024

fix(manager/indexer): pruning fixes #1147

fix(manager/indexer): pruning fixes #1147

Conversation

srene commented Oct 18, 2024

PR Standards

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

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

danwt commented Oct 22, 2024

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