feat: block production misbehavior detection #1071

faultytolly · 2024-09-11T09:46:23Z

PR Standards

~~https://www.notion.so/dymension/ADR-x-generic-Dymint-fraud-handling-7425a9ef065b458c989404c42be6a6cd~~
~~https://www.notion.so/dymension/ADR-x-STF-fraud-detection-105a4a51f86a80f88225da364d798ddd~~
~~https://www.notion.so/dymension/ADR-x-timestamp-fraud-detection-105a4a51f86a80478c68d385b254459e~~

https://www.notion.so/dymension/ADR-L2-Block-Production-Misbehavior-10da4a51f86a806ab08cf0a4e1cecc37

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

--

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

Close #XXX

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

fraud/handler.go

block/manager.go

@@ -171,8 +180,8 @@
 		}()

 		// P2P Sync. Subscribe to P2P received blocks events
-		go uevent.MustSubscribe(ctx, m.Pubsub, "applyGossipedBlocksLoop", p2p.EventQueryNewGossipedBlock, m.onReceivedBlock, m.logger)
-		go uevent.MustSubscribe(ctx, m.Pubsub, "applyBlockSyncBlocksLoop", p2p.EventQueryNewBlockSyncBlock, m.onReceivedBlock, m.logger)
+		go uevent.MustSubscribe(ctx, m.Pubsub, "applyGossipedBlocksLoop", p2p.EventQueryNewGossipedBlock, m.OnReceivedBlock, m.logger)


block/manager.go

-		go uevent.MustSubscribe(ctx, m.Pubsub, "applyGossipedBlocksLoop", p2p.EventQueryNewGossipedBlock, m.onReceivedBlock, m.logger)
-		go uevent.MustSubscribe(ctx, m.Pubsub, "applyBlockSyncBlocksLoop", p2p.EventQueryNewBlockSyncBlock, m.onReceivedBlock, m.logger)
+		go uevent.MustSubscribe(ctx, m.Pubsub, "applyGossipedBlocksLoop", p2p.EventQueryNewGossipedBlock, m.OnReceivedBlock, m.logger)
+		go uevent.MustSubscribe(ctx, m.Pubsub, "applyBlockSyncBlocksLoop", p2p.EventQueryNewBlockSyncBlock, m.OnReceivedBlock, m.logger)


types/validation.go

danwt · 2024-09-18T17:20:47Z

Please link the adr(s) and issue(s)

danwt

Looking good

Aside from comments on the code, the main thing here

please over document everything wrt. security implications
what is envisioned for fraud handler?
dependence on accuracy of proposer addr

The fraud handler is going to need to check that everything is actually a fraud, rather than operator mistake (e.g. in wrong app-hash case) and that the fraud came from a particular slashable person. How do you envision that given the currently supplied data?

danwt · 2024-09-18T17:20:00Z

block/executor.go

@@ -19,6 +19,18 @@ import (
 // default minimum block max size allowed. not specific reason to set it to 10K, but we need to avoid no transactions can be included in a block.
 const minBlockMaxBytes = 10000

+type ExecutorI interface {


dont need I suffix in go

~~and interfaces should be defined in consumer package~~

nvm it is already

block/fraud.go

block/manager.go

block/manager_test.go

block/block.go

types/validation_test.go

types/errors.go

fraud/types.go

block/block.go

types/errors.go

types/validation.go

block/fraud.go

danwt · 2024-10-17T10:57:45Z

block/fraud.go

+func (f FreezeHandler) HandleFault(ctx context.Context, fault error) {
+	uevent.MustPublish(ctx, f.m.Pubsub, &events.DataHealthStatus{Error: fault}, events.HealthStatusList)
+}


what is the intended behaviour?
This will just log the error string? what about dumping to file etc, is that coming in another pr?

I never heard about those features

in theory this is what was commented, to set the node freeze by sending the event.

how does sending this event 'freeze' anything? what does freeze even mean?

danwt · 2024-10-17T11:05:33Z

types/validation.go

+	if c.Height != header.Height {
+		return NewErrInvalidBlockHeightFraud(c.Height, header.Height)
+	}


invalid block height fraud is a bit undefined IMO
this is an invalid commit fraud

let me know your preferred name

why is there ErrFraudHeightMismatch and ErrInvalidBlockHeightFraud?
it's not clear what the intention is so hard to choose a name

invalid height is fine as a name but why are there two?

And the invalid height is only relevant when comparing to DA batch surely?

One is for the header, the other from the commit.

danwt · 2024-10-17T11:06:48Z

types/validation.go

 func (c *Commit) ValidateWithHeader(proposerPubKey tmcrypto.PubKey, header *Header) error {
 	if err := c.ValidateBasic(); err != nil {
-		return err
+		return NewErrInvalidSignatureFraud(err)


none of the frauds returned here will be caught in p2p case right? because there is no handler

dymint/p2p/validator.go

Lines 83 to 86 in 88ba1fe

if err := gossipedBlock.Validate(v.proposerGetter.GetProposerPubKey()); err != nil {

v.logger.Error("Failed to validate gossiped block.", "height", gossipedBlock.Block.Header.Height, "error", err)

return false

}

it will when attemptApplyCachedBlocks

but they are already rejected at p2p level by this code? so they will never reach it

danwt · 2024-10-17T11:08:30Z

types/errors.go

+type ErrLastHeaderHashMismatch struct {
+	Expected       [32]byte
+	LastHeaderHash [32]byte
+}


don't you need more info to validate this

I did not heard that the error must contain all the info to validate. Is that documented?

I've asked @omritoptix for some confirmation that nothing was changed but I believe the original intention was that the frauds have to be actionable, so yes

types/errors.go

danwt

Looks like the scope has drifted a bit from what I originally understood

I've posted on slack to get this confirmed

https://dymensionworkspace.slack.com/archives/C07HV4329LJ/p1729509868648489

danwt · 2024-10-21T11:06:08Z

block/fraud.go

+func (f FreezeHandler) HandleFault(ctx context.Context, fault error) {
+	uevent.MustPublish(ctx, f.m.Pubsub, &events.DataHealthStatus{Error: fault}, events.HealthStatusList)
+}


how does sending this event 'freeze' anything? what does freeze even mean?

types/validation.go

danwt · 2024-10-21T11:11:32Z

types/validation.go

 func (c *Commit) ValidateWithHeader(proposerPubKey tmcrypto.PubKey, header *Header) error {
 	if err := c.ValidateBasic(); err != nil {
-		return err
+		return NewErrInvalidSignatureFraud(err)


but they are already rejected at p2p level by this code? so they will never reach it

danwt · 2024-10-21T11:12:10Z

types/validation.go

+	nextHeight := state.NextHeight()
+	if b.Header.Height != nextHeight {
+		return NewErrFraudHeightMismatch(state.NextHeight(), b.Header.Height, b)
 	}


ok can you please document that? defensive code is really hard to understand

danwt · 2024-10-21T11:29:11Z

types/errors.go

+type ErrLastHeaderHashMismatch struct {
+	Expected       [32]byte
+	LastHeaderHash [32]byte
+}


I've asked @omritoptix for some confirmation that nothing was changed but I believe the original intention was that the frauds have to be actionable, so yes

danwt · 2024-10-21T11:31:12Z

types/validation.go

+	if c.Height != header.Height {
+		return NewErrInvalidBlockHeightFraud(c.Height, header.Height)
+	}


why is there ErrFraudHeightMismatch and ErrInvalidBlockHeightFraud?
it's not clear what the intention is so hard to choose a name

invalid height is fine as a name but why are there two?

And the invalid height is only relevant when comparing to DA batch surely?

types/validation.go

Faulty Tolly added 3 commits September 11, 2024 11:45

include fraud checking

b42b2a8

update test

bf0e7eb

first check on mockFraud Handler

c7939c7

github-actions bot added the dym-internal label Sep 12, 2024

github-advanced-security bot found potential problems Sep 12, 2024

View reviewed changes

fraud/handler.go Fixed Show fixed Hide fixed

Faulty Tolly added 2 commits September 15, 2024 17:11

fraud check done

7fb5faa

add fraud check on p2p layer

bb1ebfb

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

Faulty Tolly added 3 commits September 16, 2024 15:39

even received check

1bb5ffe

fix tests

11c6703

add validation for block timestamp fraud

242863e

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

types/validation.go Fixed Show fixed Hide fixed

Faulty Tolly added 5 commits September 17, 2024 09:47

include test for block validation and fraud

07277e1

include fraud test errors

2c147dd

fix tests

a45ef32

fix units

c58d16d

remove generic handler

427abd7

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

types/validation.go Fixed Show fixed Hide fixed

fix linter

32fb103

faultytolly marked this pull request as ready for review September 17, 2024 13:32

faultytolly requested a review from a team as a code owner September 17, 2024 13:32

reordering imports by hand

58cf2a8

danwt requested review from srene and danwt September 19, 2024 10:44

danwt requested changes Sep 19, 2024

View reviewed changes

danwt changed the title ~~feat: include fraud detection handling~~ feat: detect future timestamp and STF frauds Sep 19, 2024

danwt reviewed Sep 19, 2024

View reviewed changes

fraud/types.go Outdated Show resolved Hide resolved

danwt mentioned this pull request Sep 19, 2024

Rollapp detected fraud handling - ADR #1085

Closed

pr comments 1

a9ca873

Faulty Tolly added 5 commits October 16, 2024 15:47

add missing lib

ec1a825

Merge remote-tracking branch 'origin/main' into feat/adr-fraud-handling

e5a767d

add to state serialization

dec030c

include param to last header hash to UpdateStateFromApp

a978587

Add fraud handler to manager.

1ae1c11

danwt reviewed Oct 17, 2024

View reviewed changes

Faulty Tolly added 9 commits October 17, 2024 18:39

Fix units

fc85b29

solve some issues based on the PR comments

806fc8c

comment applyBlockWithFraudHandling

710f523

remove comment

40e5f8b

simplify apply block with block fraud handling

684c515

cleanup the code and duplications on validate

decd83e

update comments

e740570

expected heights

ad473d1

updated fraud info

2e27ce7

faultytolly requested review from srene, danwt and mtsitrin October 21, 2024 09:12

danwt requested changes Oct 21, 2024

View reviewed changes

danwt reviewed Oct 21, 2024

View reviewed changes

types/validation.go Outdated Show resolved Hide resolved

Faulty Tolly added 3 commits October 22, 2024 11:57

Merge remote-tracking branch 'origin/main' into feat/adr-fraud-handling

aa74bec

update branch with validator set

166ef41

move err invalid signature fraud

adffd84

danwt approved these changes Oct 22, 2024

View reviewed changes

danwt mentioned this pull request Oct 22, 2024

Remove invalid signature 'fraud' #1160

Open

srene approved these changes Oct 22, 2024

View reviewed changes

omritoptix approved these changes Oct 22, 2024

View reviewed changes

omritoptix merged commit 9aef1d0 into main Oct 22, 2024
6 checks passed

omritoptix deleted the feat/adr-fraud-handling branch October 22, 2024 13:20

omritoptix linked an issue Oct 26, 2024 that may be closed by this pull request

Detection: L2 Block production Misbehavior - Impl #1105

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: block production misbehavior detection #1071

feat: block production misbehavior detection #1071

faultytolly commented Sep 11, 2024 •

edited by danwt

Loading

danwt commented Sep 18, 2024

danwt left a comment

danwt Sep 18, 2024

danwt Sep 19, 2024 •

edited

Loading

danwt Sep 19, 2024

danwt Oct 17, 2024

faultytolly Oct 17, 2024

faultytolly Oct 17, 2024

danwt Oct 21, 2024

danwt Oct 17, 2024

faultytolly Oct 17, 2024

danwt Oct 21, 2024

faultytolly Oct 22, 2024

danwt Oct 17, 2024

faultytolly Oct 17, 2024

danwt Oct 21, 2024

danwt Oct 17, 2024

faultytolly Oct 17, 2024

danwt Oct 21, 2024

danwt left a comment

danwt Oct 21, 2024

danwt Oct 21, 2024

danwt Oct 21, 2024

danwt Oct 21, 2024

danwt Oct 21, 2024

	if err := gossipedBlock.Validate(v.proposerGetter.GetProposerPubKey()); err != nil {
	v.logger.Error("Failed to validate gossiped block.", "height", gossipedBlock.Block.Header.Height, "error", err)
	return false
	}

feat: block production misbehavior detection #1071

feat: block production misbehavior detection #1071

Conversation

faultytolly commented Sep 11, 2024 • edited by danwt Loading

PR Standards

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

danwt commented Sep 18, 2024

danwt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwt Sep 19, 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

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

faultytolly commented Sep 11, 2024 •

edited by danwt

Loading

danwt Sep 19, 2024 •

edited

Loading