Skip to content

fraud proof processing using go-fraud package - #928

Merged
gupadhyaya merged 21 commits into
evstack:mainfrom
gupadhyaya:fraud_exchange_2
May 24, 2023
Merged

fraud proof processing using go-fraud package#928
gupadhyaya merged 21 commits into
evstack:mainfrom
gupadhyaya:fraud_exchange_2

Conversation

@gupadhyaya

@gupadhyayagupadhyaya commented May 9, 2023

Copy link
Copy Markdown
Contributor

Update existing fraud proof processing logic to use ProofService from go-fraud package. Also, removes the old fraud proof gossip logic.

Fixes#895 & #945

@codecov

codecovBot commented May 9, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 66.27% and project coverage change: +0.92 🎉

Comparison is base (c769503) 55.28% compared to head (c6f1ac3) 56.21%.

Additional details and impacted files
@@ Coverage Diff @@## main #928 +/- ##
==========================================
+ Coverage 55.28% 56.21% +0.92% 
==========================================
Files 68 68 Lines 10954 10940 -14 ==========================================
+ Hits 6056 6150 +94 + Misses 4017 3906 -111 - Partials 881 884 +3 
Impacted FilesCoverage Δ
p2p/client.go60.08% <ø> (+<0.01%)⬆️
types/header.go28.88% <ø> (-10.40%)⬇️
node/light.go48.59% <32.14%> (+1.34%)⬆️
state/executor.go70.38% <40.00%> (+5.93%)⬆️
block/manager.go70.02% <85.10%> (+5.27%)⬆️
node/full.go68.57% <100.00%> (+6.70%)⬆️
node/proof_service_factory.go100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@gupadhyaya
gupadhyaya marked this pull request as draft May 9, 2023 23:50
@gupadhyayagupadhyaya added the C:state-fraud-proofs Component: State Fraud Proofs (generation and validation) label May 9, 2023
@gupadhyaya
gupadhyaya changed the base branch from fraud_exchange to mainMay 10, 2023 14:29
@gupadhyaya
gupadhyayaforce-pushed the fraud_exchange_2 branch 2 times, most recently from a4912c7 to 5174b47CompareMay 17, 2023 00:59
@gupadhyaya
gupadhyaya marked this pull request as ready for review May 17, 2023 01:00
@Manav-Aggarwal
Manav-Aggarwal marked this pull request as draft May 17, 2023 16:49
@Manav-Aggarwal

Manav-Aggarwal commented May 17, 2023

Copy link
Copy Markdown
Member

Please fix failing CI/CD tests. For example, TestTxGossipingAndAggregation fails in this PR. Converted back to draft for now.

@Manav-AggarwalManav-Aggarwal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, left some comments, mostly related to refactoring/consolidation of code.

Comment threadnode/light.go
Comment threadnode/full_node_integration_test.go Outdated
Comment threadnode/full_node_integration_test.go Outdated
Comment threadblock/manager.go Outdated
Comment threadnode/full_node_integration_test.go
Comment threadnode/full_node_integration_test.go
@gupadhyaya
gupadhyaya marked this pull request as ready for review May 17, 2023 23:05
@Manav-Aggarwal

Copy link
Copy Markdown
Member

Thanks for addressing the comments! Currently, the fraud-proof generating full node proceeds to verify the fraud-proof it generates. Can we avoid that so that after generating a fraud-proof, the full node stops (previously was done in fraudProofPublishLoop)?

@gupadhyaya

Copy link
Copy Markdown
ContributorAuthor

fraudProofPublishLoop

functionality wise, the full node after generating the fraud proof will not sync any further blocks (since SyncLoop returns) and ProcessFraudProof loop eventually cancels the context. I agree that the process might not be fully stopped, but this can be fixed by simply catching the context cancel call and stopping the node gracefully.

i tried to cancel the context upon receiving ErrFraudProofGenerated in the SyncLoop, however this prevented from fraud Broadcast to fully succeed (fraud didn't get gossiped). Hence, I am not sure if there is a better place (or way) to do this. However, created an issue for it: #947

@gupadhyaya

Copy link
Copy Markdown
ContributorAuthor

fraudProofPublishLoop

functionality wise, the full node after generating the fraud proof will not sync any further blocks (since SyncLoop returns) and ProcessFraudProof loop eventually cancels the context. I agree that the process might not be fully stopped, but this can be fixed by simply catching the context cancel call and stopping the node gracefully.

i tried to cancel the context upon receiving ErrFraudProofGenerated in the SyncLoop, however this prevented from fraud Broadcast to fully succeed (fraud didn't get gossiped). Hence, I am not sure if there is a better place (or way) to do this. However, created an issue for it: #947

@Wondertan any suggestions for this? ideally, we want full nodes to broadcast the fraud proof and gracefully shutdown. However, currently canceling the context stops the broadcast for some reason.

Comment threadblock/manager.go Outdated
Comment threadblock/manager.go
Comment threadnode/full.go Outdated
Comment threadnode/full_node_integration_test.go Outdated
Comment threadnode/full_node_integration_test.go
Comment threadnode/light.go
Comment threadnode/full_node_integration_test.go
Comment threadnode/full_node_integration_test.go Outdated
@gupadhyaya
gupadhyaya merged commit e90a07b into evstack:mainMay 24, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:state-fraud-proofsComponent: State Fraud Proofs (generation and validation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update existing fraud proof gossip and handling logic to use ProofService

4 participants

@gupadhyaya@Manav-Aggarwal@MSevey@nashqueue