feat(manager): state update mishbehavior detection #1130

srene · 2024-10-10T15:50:52Z

PR Standards

ADR x: State update misbehavior detection

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

--

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

Close #1106

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

I think the DRS in-memory management makes the code cumbersome and unnecessary complicated vs just saving DRS per height (like we do with block, commit, proposer) etc and just get it where necessary.

we should strive to keep the code simple vs micro optimizations.

omritoptix · 2024-10-26T13:53:37Z

block/block.go

@@ -122,7 +122,15 @@ func (m *Manager) applyBlock(block *types.Block, commit *types.Commit, blockMeta
 				m.logger.Error("pruning channel full. skipping pruning", "retainHeight", retainHeight)
 			}
 		}
-
+		if responses.EndBlock.RollappParamUpdates != nil {
+			newDRS := m.DRSVersionHistory.AddDRSVersion(block.Header.Height, responses.EndBlock.RollappParamUpdates.Version)


Few questions/comments here:

can we have plz a comment explainig why are we saving the history

I get the optimization here but what happens if we get a reboot? than we save a DRS which is not acutally new

I don't think there is an inherent problem just saving the DRS per height (even if not unique - as anyway you don't prevent it now due to (2))

drs history replaced by storing drs per height.

omritoptix · 2024-10-26T13:56:56Z

block/validate.go

+// LoadDrsHistoryFromStore loads drs history from store
+func (m *Manager) LoadDrsHistoryFromStore() error {
+	drsHistory, err := m.Store.LoadDRSVersionHistory()
+	if errors.Is(err, gerrc.ErrNotFound) {
+		m.logger.Info("failed to find drs history in the store, creating new")
+		m.DRSVersionHistory = &types.DRSVersionHistory{}
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("get drs version history: %w", err)
+	}
+
+	m.DRSVersionHistory = drsHistory
+
+	return nil
+}


Suggested change

// LoadDrsHistoryFromStore loads drs history from store

func (m *Manager) LoadDrsHistoryFromStore() error {

drsHistory, err := m.Store.LoadDRSVersionHistory()

if errors.Is(err, gerrc.ErrNotFound) {

m.logger.Info("failed to find drs history in the store, creating new")

m.DRSVersionHistory = &types.DRSVersionHistory{}

return nil

} else if err != nil {

return fmt.Errorf("get drs version history: %w", err)

}

m.DRSVersionHistory = drsHistory

return nil

}

// LoadDrsHistoryFromStore loads drs history from store

func (m *Manager) LoadDrsHistoryFromStore() error {

drsHistory, err := m.Store.LoadDRSVersionHistory()

if errors.Is(err, gerrc.ErrNotFound) {

m.logger.Info("failed to find drs history in the store, creating new")

drsHistory = &types.DRSVersionHistory{}

} else if err != nil {

return fmt.Errorf("get drs version history: %w", err)

}

m.DRSVersionHistory = drsHistory

return nil

}

this has been removed

block/manager.go

omritoptix · 2024-10-26T14:09:38Z

block/validate.go

+		return fmt.Errorf("get drs version history: %w", err)
+	}
+
+	m.DRSVersionHistory = drsHistory


as to my other comment on the applyBlock - not sure the optimization of saving it in memory and than only saving the uniqe changes in store (which I'm not sure is actually the case) is necessary for optimization.
I'd simply just save the DRS per height and also get rid of the new manager memeber (i.e DRSVersionHistory)

omritoptix · 2024-10-26T14:10:03Z

block/manager.go

@@ -161,6 +164,11 @@ func NewManager(
 		return nil, fmt.Errorf("get initial state: %w", err)
 	}

+	err = m.LoadDrsHistoryFromStore()


as to my other comment - don't think maintining this in memory is necessary

drs history replaced by drs per height in store

omritoptix · 2024-10-26T14:17:27Z

block/block.go

+		if responses.EndBlock.RollappParamUpdates != nil {
+			newDRS := m.DRSVersionHistory.AddDRSVersion(block.Header.Height, responses.EndBlock.RollappParamUpdates.Version)
+			if newDRS {
+				_, err = m.Store.SaveDRSVersionHistory(m.DRSVersionHistory, nil)


I think we should simply save DRS per height in the store (so the key is per height, similiar to how we save the proposer changes. And prune it when necessary. it will simplify all the DRS management and validation imo)

block/submit.go

da/celestia/celestia.go

omritoptix

Approved. 2 issues im not sure about but we can address it in seperate issues.

srene · 2024-10-28T12:47:49Z

da batch is written to celestia without including drs version and last batch, since it is not included when mashall / unmmarshal, but its used when submitting to sl to generate the block descriptors. added an issue to improve it #1171

srene requested a review from a team as a code owner October 10, 2024 15:50

srene self-assigned this Oct 10, 2024

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

srene marked this pull request as draft October 10, 2024 15:51

srene force-pushed the srene/1106-state-update-mishbehavior branch from 29d9dff to 62ed34e Compare October 11, 2024 11:00

This was linked to issues Oct 11, 2024

move drs version to block descriptors instead of state info #1132

Closed

Detection: State Update Misbehavior - Impl #1106

Closed

srene force-pushed the srene/1106-state-update-mishbehavior branch 5 times, most recently from 3133aa3 to cd01dd7 Compare October 15, 2024 10:12

srene marked this pull request as ready for review October 15, 2024 10:12

omritoptix mentioned this pull request Oct 16, 2024

feat(rollapp): move DRS version to be part of the block descriptor dymensionxyz/dymension#1311

Merged

29 tasks

srene marked this pull request as draft October 21, 2024 12:14

srene force-pushed the srene/1128-validation-rpc branch from 651e920 to 5552b77 Compare October 22, 2024 10:39

srene force-pushed the srene/1106-state-update-mishbehavior branch from e6a6e50 to 6726b93 Compare October 22, 2024 13:15

srene marked this pull request as ready for review October 22, 2024 13:28

srene marked this pull request as draft October 23, 2024 08:48

srene mentioned this pull request Oct 23, 2024

feat(manager): block gossiping misbehavior detection #1117

Merged

12 tasks

srene force-pushed the srene/1128-validation-rpc branch from c2d36f1 to a62066c Compare October 24, 2024 14:20

srene changed the base branch from srene/1128-validation-rpc to main October 24, 2024 14:31

srene added 8 commits October 24, 2024 16:51

new sl tests

1d51f79

files rename

339b897

added drs and dr validation

e82151f

drs version

15b6c3c

new frauds

794bd8f

new da errors

39a2c81

da mocks

ac71019

fixing tests + missing state

38447d3

srene marked this pull request as draft October 25, 2024 11:33

srene added 11 commits October 25, 2024 16:10

fixing tests

46e2e82

lint fix

bb9b3d4

move drs outside state

17ceed5

fix

b895e4a

missing file

0eb4fe6

store drs

883adce

simplifying

92b0f73

lint fix

eb88317

tests

29f6beb

tests

e4a1653

lint

19ef2fc

srene marked this pull request as ready for review October 25, 2024 21:14

srene added 2 commits October 25, 2024 23:25

move func

6761019

go doc

df07ccd

omritoptix requested changes Oct 26, 2024

View reviewed changes

srene added 4 commits October 28, 2024 12:14

simplify drs version

bae2927

lint fix

c9c699e

fix tests

c41f8e4

add drs pruning

9ca3a82

omritoptix reviewed Oct 28, 2024

View reviewed changes

block/submit.go Show resolved Hide resolved

remove retrieve from celestia without commitment

c1ec86b

omritoptix reviewed Oct 28, 2024

View reviewed changes

da/celestia/celestia.go Show resolved Hide resolved

omritoptix previously approved these changes Oct 28, 2024

View reviewed changes

remove retrieve without commitment

1006203

srene dismissed omritoptix’s stale review via 1006203 October 28, 2024 12:21

omritoptix approved these changes Oct 28, 2024

View reviewed changes

omritoptix merged commit 314003c into main Oct 28, 2024
6 checks passed

omritoptix deleted the srene/1106-state-update-mishbehavior branch October 28, 2024 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manager): state update mishbehavior detection #1130

feat(manager): state update mishbehavior detection #1130

srene commented Oct 10, 2024 •

edited

Loading

omritoptix left a comment

omritoptix Oct 26, 2024

srene Oct 28, 2024

omritoptix Oct 26, 2024

srene Oct 28, 2024

omritoptix Oct 26, 2024

srene Oct 28, 2024

omritoptix Oct 26, 2024

srene Oct 28, 2024

srene Oct 28, 2024

omritoptix Oct 26, 2024

srene Oct 28, 2024

omritoptix left a comment

srene commented Oct 28, 2024

feat(manager): state update mishbehavior detection #1130

feat(manager): state update mishbehavior detection #1130

Conversation

srene commented Oct 10, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omritoptix left a comment

Choose a reason for hiding this comment

srene commented Oct 28, 2024

srene commented Oct 10, 2024 •

edited

Loading