Skip to content

add flag to enable fraud proofs#622

Merged
tzdybal merged 1 commit intoevstack:mainfrom
gupadhyaya:fraud_proof_flag
Nov 30, 2022
Merged

add flag to enable fraud proofs#622
tzdybal merged 1 commit intoevstack:mainfrom
gupadhyaya:fraud_proof_flag

Conversation

@gupadhyaya
Copy link
Copy Markdown
Contributor

@gupadhyaya gupadhyaya commented Nov 22, 2022

Fixes #619 by adding a flag to enable fraud proofs. Note that, there is a change in the default behavior after this commit is merged, where previously by default the fraud proofs were enabled, but now they will be disabled and the flag --rollmint.experimental_insecure_fraud_proofs must be passed enable fraud proofs.

@gupadhyaya gupadhyaya requested a review from tzdybal as a code owner November 22, 2022 17:21
@gupadhyaya
Copy link
Copy Markdown
Contributor Author

gupadhyaya commented Nov 22, 2022 •
edited
Loading

still working on fixing the failing integration tests.

@tzdybal Since previously the fraud proofs were enabled by default (fraudProofsEnabled = true), where there any dependencies that assumed this behavior?

@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Nov 23, 2022

Since previously the fraud proofs were enabled by default (fraudProofsEnabled = true), where there any dependencies that assumed this behavior?

IIRC no. @Manav-Aggarwal please confirm.

@Manav-Aggarwal
Copy link
Copy Markdown
Member

Manav-Aggarwal commented Nov 25, 2022

Since previously the fraud proofs were enabled by default (fraudProofsEnabled = true), where there any dependencies that assumed this behavior?

IIRC no. @Manav-Aggarwal please confirm.

Yep that's correct. No dependencies assuming it.

@gupadhyaya gupadhyaya changed the title [wip] add flag to enable fraud proofs add flag to enable fraud proofs Nov 28, 2022
@gupadhyaya
Copy link
Copy Markdown
Contributor Author

gupadhyaya commented Nov 28, 2022

@tzdybal @Manav-Aggarwal i think this PR is ready for review.

nashqueue
nashqueue suggested changes Nov 28, 2022
View reviewed changes
Comment thread config/config.go
tzdybal
tzdybal reviewed Nov 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I would only to test code with fraud proofs both enabled and disabled.

Comment thread config/defaults.go
Comment thread state/executor_test.go Outdated
@tzdybal tzdybal requested a review from nashqueue November 29, 2022 11:43
add flag to enable fraud proofs
5689e32 
minor fix to fix the integration tests

expanding the executor tests to cover both fraud proof enable and disable cases
nashqueue
nashqueue approved these changes Nov 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nashqueue nashqueue requested a review from tzdybal November 30, 2022 11:32
tzdybal
tzdybal approved these changes Nov 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🚀

@tzdybal tzdybal enabled auto-merge (squash) November 30, 2022 19:42
@tzdybal tzdybal merged commit dcb2092 into evstack:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Manav-Aggarwal Manav-Aggarwal Awaiting requested review from Manav-Aggarwal

2 more reviewers

@tzdybal tzdybal tzdybal approved these changes

@nashqueue nashqueue nashqueue approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gupadhyaya gupadhyaya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Create flag to enable fraud proofs

4 participants

@gupadhyaya @tzdybal @Manav-Aggarwal @nashqueue