Fraud Proofs - one PR to rule them all by tzdybal · Pull Request #538 · evstack/ev-node

tzdybal · 2022-09-26T08:22:26Z

This PR wraps up entire Fraud Proof related work into single PR. It might be easier to review / apply comments this way.

Replaces: #375, #498, #500.

mergify · 2022-09-26T08:22:29Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

tzdybal · 2022-09-26T08:33:56Z

+// State witness with a list of all witness data
+message StateWitness {
+  // store level proof
+  tendermint.crypto.ProofOp proof_op = 1;


Consider renaming to just proof (as in WitnessData).

This has to be updated in https://github.com/celestiaorg/tendermint, and then pulled to rollmint with get_deps.sh.

tzdybal · 2022-09-26T08:35:17Z

@@ -0,0 +1,24 @@
+syntax = "proto3";


Is this file needed? Or it's just side-effect of running ./proto/get_deps.sh?

it's just side-effect of running ./proto/get_deps.sh

But can we remove this file safely?

tzdybal · 2022-09-26T08:42:08Z

+				}
+				// TODO: gossip fraudProof to P2P network
+				// fraudTx: current DeliverTx
+				_ = fraudProof


Gossiping code is ready - are there any blockers to execute it here?

Issue here: #552

tzdybal · 2022-09-26T08:44:43Z

+		if fraudProofsEnabled {
+			isr, err := e.getAppHash()
+			if err != nil {
+				return nil, err
+			}
+			ISRs = append(ISRs, isr)
+			isFraud := e.checkFraudProofTrigger(isr, currentIsrs, currentIsrIndex)
+			if isFraud {
+				fraudProof, err := e.generateFraudProof(&beginBlockRequest, deliverTxRequests, nil)
+				if err != nil {
+					return nil, err
+				}
+				// TODO: gossip fraudProof to P2P network
+				// fraudTx: current DeliverTx
+				_ = fraudProof
+			}
+			currentIsrIndex++
+		}


This (more or less) should be extracted as function as it's repeated 3 times.

tzdybal · 2022-09-26T08:50:22Z

+
+// Represents a single-round fraudProof
+type FraudProof struct {
+	// The block height to load state of
+	blockHeight int64
+
+	appHash []byte
+	// A map from module name to state witness
+	stateWitness map[string]StateWitness
+
+	// Fraudulent state transition has to be one of these
+	// Only one have of these three can be non-nil
+	fraudulentBeginBlock *abci.RequestBeginBlock
+	fraudulentDeliverTx  *abci.RequestDeliverTx
+	fraudulentEndBlock   *abci.RequestEndBlock
+}
+
+// State witness with a list of all witness data
+type StateWitness struct {
+	// store level proof
+	Proof    tmcrypto.ProofOp
+	RootHash []byte
+	// List of witness data
+	WitnessData []WitnessData
+}
+
+// Witness data containing a key/value pair and a SMT proof for said key/value pair
+type WitnessData struct {
+	Key   []byte
+	Value []byte
+	Proof tmcrypto.ProofOp
+}


This could be moved to separate file (fraud_proofs.go or similar).

mergify · 2022-10-01T10:57:02Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

mergify · 2022-10-01T10:59:12Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-13T07:16:28Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

Currently we need to use Manav's branch from celestiaorg/tendermint fork.

mergify · 2022-10-13T07:30:55Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-13T19:19:01Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

mergify · 2022-10-13T21:01:32Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-13T21:07:00Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-13T21:09:02Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-17T15:39:55Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-18T12:20:17Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

tzdybal · 2022-10-18T12:25:19Z

 	github.com/spf13/viper v1.13.0
 	github.com/stretchr/testify v1.8.0
-	github.com/tendermint/tendermint v0.34.21
+	github.com/tendermint/tendermint v0.34.14


We shouldn't downgrade tendermint.

tzdybal · 2022-10-18T12:26:04Z


 replace (
 	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.4
+	github.com/tendermint/tendermint => github.com/celestiaorg/tendermint v0.34.18-0.20220826223854-50069357d918


We need our tendermint version rebased on top of v0.34.21.

tzdybal · 2022-10-18T12:30:24Z

 			require.NoError(err)
 			nodeBlock, err := nodes[i].Store.LoadBlock(h)
 			require.NoError(err)
+			// Only Intermediate state roots set by block aggregator are relevant, removed for sake of comparison


Why we need to remove this? Is there any discrepancy between ISRs between aggregator and full nodes?

Test fails without explicit clearing of RawRootsList, which is strange. It's a part of a block data, so it has to be the same in all blocks. We need to figure out why it differs.

Issue here: #566

tzdybal · 2022-10-18T12:44:26Z

+		assert.GreaterOrEqual(endCnt, adjustedHeight)
+		assert.GreaterOrEqual(commitCnt, adjustedHeight)
+
+		assert.Equal(generateFraudProofCnt, beginCnt+endCnt+clientNodes)


First argument is "expected", second is "actual", so you should swap arguments to Equal.

It's not obvious why beginCnt+endCnt+clientNodes - please add comment with explanation.

Updated and added comment with explanation.

tzdybal · 2022-10-18T15:15:58Z

@@ -0,0 +1,24 @@
+syntax = "proto3";


But can we remove this file safely?

mergify · 2022-10-20T01:54:49Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-20T02:01:50Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

mergify · 2022-10-20T02:02:44Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2022-10-24T22:32:01Z

⚠️ The sha of the head commit of this PR conflicts with #500. Mergify cannot evaluate rules on this PR. ⚠️

tzdybal · 2022-10-25T09:19:02Z

All comments on this PR was resolved, but we need to close it, as I created it, instead of @Manav-Aggarwal, so I can't accept it.

Manav-Aggarwal added 18 commits July 5, 2022 16:06

Add gossip fraud proof capability over p2p (#452)

7effa8c

Add types.proto for new tendermint version with abci methods

267c1d5

Trigger Fraud Proof WIP

3f1b573

Add new abci method in mocks all across tests

49b43b4

All tests work now

f231e9c

Add fraudproof trigger

d213667

Remove fraud trigger integration test

1d9c389

Remove getAppHash from mock.on

44822f0

Write a fraudproof integration test

f7969fe

Run golang linter

9ca852c

Replace Generate with trigger

016b3a1

Add skeleton to catch up and generate fraudproof after FPGM

656d4ee

Update TODOs

d749e4c

Remove triggerFraudProofGenerationMode and refactor

3e7c1d9

Add generate fraud proof

af2e8cd

Wrap all fraudproof logic within a flag

cf43b3b

Update types.proto

08ae559

Add fraud proof verification WIP

8cfd7a7

tzdybal assigned Manav-Aggarwal Sep 26, 2022

tzdybal commented Sep 26, 2022

View reviewed changes

tzdybal marked this pull request as ready for review September 26, 2022 08:51

tzdybal requested review from adlerjohn and liamsi as code owners September 26, 2022 08:51

Merge branch 'origin/main' into manav/verify_fraud_proof

fce0c43

Update state/executor.go

c1555dd

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

Manav-Aggarwal added 2 commits October 1, 2022 07:33

Resolve some comments

f9fd481

Clear more errors

fbfee91

Merge branch 'main' into manav/verify_fraud_proof

cac373e

deps: update protobuf scripts & files to use correct version

d4f4050

Currently we need to use Manav's branch from celestiaorg/tendermint fork.

Merge branch 'main' into manav/verify_fraud_proof

043f59d

Update node/integration_test.go

e1fce5b

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

Call get_deps.sh and update abci types.proto

78b29d2

Update spelling of comparison bc linter errors

68a816d

Change rollmint ethermint to optimint

2a8aabe

Merge branch main into manav/verify_fraud_proof

599ead7

tzdybal commented Oct 18, 2022

View reviewed changes

Manav-Aggarwal added 2 commits October 19, 2022 21:50

Delete version/types.proto file

0aee362

Merge branch 'main' into manav/verify_fraud_proof

5b001aa

Manav-Aggarwal added 2 commits October 19, 2022 22:01

Add inline comment explaining integration test check

7bd10ac

Do not downgrade tendermint version

b7359ab

Update proto/get_deps.sh

34f0a31

Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl>

deps: celestiaorg/tendermint@abci_fraud_proofs

00c5837

tzdybal closed this Oct 25, 2022

Manav-Aggarwal mentioned this pull request Oct 25, 2022

Fraud Proofs - one PR to rule them all again #567

Merged

Conversation

tzdybal commented Sep 26, 2022

Uh oh!

mergify Bot commented Sep 26, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzdybal Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Oct 1, 2022

Uh oh!

mergify Bot commented Oct 1, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 13, 2022

Uh oh!

mergify Bot commented Oct 17, 2022

Uh oh!

mergify Bot commented Oct 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Oct 20, 2022

Uh oh!

mergify Bot commented Oct 20, 2022

Uh oh!

mergify Bot commented Oct 20, 2022

Uh oh!

mergify Bot commented Oct 24, 2022

Uh oh!

tzdybal commented Oct 25, 2022

Uh oh!

Reviewers

tzdybal Oct 13, 2022 •

edited

Loading