Gossip Fraud Proofs to P2P network after generation when needed by Manav-Aggarwal · Pull Request #643 · evstack/ev-node

Manav-Aggarwal · 2022-12-02T17:24:58Z

Overview

Gossip generated Fraud Proofs to P2P network

Closes: #552

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

tzdybal

Now I noticed, that the entire gossiping flow is invalid.
newFraudProofValidator should be kind of similar to newTxValidator - we should verify the fraud proof, and only if it was a valid fraud proof, we should propagate it (return true in validation function returned from newFraudProofValidator).

Right now, we always return true, so all fraud proofs are propagated in gossip network, and we also re-publish them in fraudProofPublishLoop.

I think we don't need this loop at all - the only proof we need to "publish" is the one we generated, and currently our policy is to stop after generating.

Manav-Aggarwal · 2023-01-17T19:53:25Z

Follow up issue: #693

codecov-commenter · 2023-01-17T19:58:16Z

Codecov Report

Base: 54.59% // Head: 55.07% // Increases project coverage by +0.47% 🎉

Coverage data is based on head (799a2b9) compared to base (018e42a).
Patch coverage: 52.49% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   54.59%   55.07%   +0.47%     
==========================================
  Files          52       52              
  Lines       10372     9851     -521     
==========================================
- Hits         5663     5425     -238     
+ Misses       3840     3618     -222     
+ Partials      869      808      -61

Impacted Files	Coverage Δ
config/config.go	`88.88% <ø> (ø)`
conv/addr.go	`100.00% <ø> (ø)`
conv/config.go	`47.05% <ø> (ø)`
da/celestia/celestia.go	`50.00% <ø> (ø)`
da/celestia/mock/server.go	`49.68% <ø> (ø)`
da/grpc/grpc.go	`73.13% <ø> (ø)`
da/mock/mock.go	`73.75% <ø> (ø)`
da/registry/registry.go	`100.00% <ø> (ø)`
mempool/v1/mempool.go	`86.09% <ø> (ø)`
node/light_client.go	`0.00% <ø> (ø)`
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tzdybal

I left few comments. Refactoring error type can be done as followup issue.

nashqueue

LGTM, except that we should not gossip if we get an error from the comment above.

commit f6dd61a Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Tue Jan 17 14:58:31 2023 -0500 Revert integration test changes commit e496fc0 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Tue Jan 17 14:51:59 2023 -0500 Fix rollmint.pb.go commit fd2fde6 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Tue Jan 17 14:37:04 2023 -0500 Associate issue with rewriting TestFraudProofTrigger commit b49aa51 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Tue Jan 17 13:49:56 2023 -0500 Move expected valid apphash after generation to during generation commit d57302d Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Thu Jan 5 14:48:50 2023 -0500 Update error commit 7706677 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Tue Dec 13 00:01:04 2022 -0500 fix linter commit 325dc2c Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Mon Dec 12 23:46:57 2022 -0500 gossip fp before stopping commit eddd7e3 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Sun Dec 11 00:51:06 2022 -0500 stop executor after fraud commit 7b447cf Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Sat Dec 10 22:17:22 2022 -0500 add more logging for generating fraud proof commit 35a1a2c Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Sat Dec 10 22:13:55 2022 -0500 Fix deliverTxRequests size commit 01c83cc Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Sat Dec 10 16:54:41 2022 -0500 halt full node and add logging messages commit d150e0d Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Wed Dec 7 18:29:38 2022 -0500 make channel unbufferred commit edc2c8e Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Fri Dec 2 17:28:23 2022 -0500 Add verifyFraudProof to tests commit c719751 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Fri Dec 2 16:57:50 2022 -0500 Rename expectedAppHash to expectedValidAppHash commit bd3ff36 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Fri Dec 2 16:55:29 2022 -0500 Add expected valid app hash to verify fraud proof commit 433279a Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Fri Dec 2 13:26:22 2022 -0500 gossip fraud proof through FraudProofOutChan commit 33eae45 Author: Manav Aggarwal <manavaggarwal1234@gmail.com> Date: Fri Dec 2 12:22:53 2022 -0500 Gossip state fraud proof

tzdybal

LGTM

nashqueue

LGTM

Manav-Aggarwal linked an issue Dec 2, 2022 that may be closed by this pull request

Gossip Fraud Proofs to P2P network after generation when needed #552

Closed

Manav-Aggarwal changed the title ~~Gossip state fraud proof~~ Gossip Fraud Proofs to P2P network after generation when needed Dec 2, 2022

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from 3409ec5 to 0bd8ae2 Compare December 2, 2022 18:26

Manav-Aggarwal requested a review from gupadhyaya December 2, 2022 18:27

Manav-Aggarwal self-assigned this Dec 2, 2022

Manav-Aggarwal requested review from S1nus and nashqueue December 2, 2022 21:56

Manav-Aggarwal marked this pull request as ready for review December 2, 2022 22:28

Manav-Aggarwal requested a review from tzdybal as a code owner December 2, 2022 22:28

tzdybal suggested changes Dec 6, 2022

View reviewed changes

Comment thread state/executor.go Outdated

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from 0e4504d to f66f07a Compare December 13, 2022 04:47

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from ba78f4d to 3dbdd74 Compare January 5, 2023 19:49

Manav-Aggarwal mentioned this pull request Jan 17, 2023

Fix/Add integration test for fraud proofs #693

Closed

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from bdb6736 to e496fc0 Compare January 17, 2023 19:52

Manav-Aggarwal requested a review from tzdybal January 17, 2023 20:00

nashqueue previously approved these changes Jan 20, 2023

View reviewed changes

Manav-Aggarwal dismissed nashqueue’s stale review via ed74871 January 23, 2023 19:36

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from f6dd61a to ed74871 Compare January 23, 2023 19:36

Manav-Aggarwal requested review from nashqueue and tuxcanfly and removed request for S1nus January 23, 2023 19:37

tzdybal suggested changes Jan 24, 2023

View reviewed changes

Comment thread block/manager.go

Comment thread node/full.go Outdated

tzdybal mentioned this pull request Jan 24, 2023

Create ErrApplyBlock with Unwrap method. #707

Closed

nashqueue previously approved these changes Jan 24, 2023

View reviewed changes

Manav-Aggarwal dismissed nashqueue’s stale review via 4e0e174 January 24, 2023 16:53

Manav-Aggarwal requested review from nashqueue and tzdybal January 24, 2023 17:04

Do not gossip after failing to serialize

739cfce

Manav-Aggarwal force-pushed the manav/gossip_state_fp branch from fa23864 to 739cfce Compare January 24, 2023 17:50

panic when unable to serialize

799a2b9

tzdybal approved these changes Jan 24, 2023

View reviewed changes

nashqueue approved these changes Jan 24, 2023

View reviewed changes

Manav-Aggarwal merged commit 0b3dc6a into main Jan 24, 2023

Manav-Aggarwal deleted the manav/gossip_state_fp branch January 24, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gossip Fraud Proofs to P2P network after generation when needed#643

Gossip Fraud Proofs to P2P network after generation when needed#643
Manav-Aggarwal merged 3 commits intomainfrom
manav/gossip_state_fp

Manav-Aggarwal commented Dec 2, 2022 •

edited

Loading

Uh oh!

tzdybal left a comment

Uh oh!

Uh oh!

Manav-Aggarwal commented Jan 17, 2023

Uh oh!

codecov-commenter commented Jan 17, 2023 •

edited

Loading

Uh oh!

tzdybal left a comment

Uh oh!

Uh oh!

Uh oh!

nashqueue left a comment

Uh oh!

tzdybal left a comment

Uh oh!

nashqueue left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Manav-Aggarwal commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Manav-Aggarwal commented Jan 17, 2023

Uh oh!

codecov-commenter commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nashqueue left a comment

Choose a reason for hiding this comment

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

nashqueue left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Manav-Aggarwal commented Dec 2, 2022 •

edited

Loading

codecov-commenter commented Jan 17, 2023 •

edited

Loading