Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fraud: add peer that sent an invalid fraud proof to black list #966

Merged
merged 8 commits into from
Aug 3, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jul 29, 2022

Resolves #954

@vgonkivs vgonkivs added area:fraud kind:fix Attached to bug-fixing PRs labels Jul 29, 2022
@vgonkivs vgonkivs self-assigned this Jul 29, 2022
@vgonkivs vgonkivs force-pushed the fix_fraud_proof_propagation branch 4 times, most recently from c14f5b1 to dd7d034 Compare July 29, 2022 16:54
@vgonkivs vgonkivs force-pushed the fix_fraud_proof_propagation branch 3 times, most recently from 4fe25b2 to 4c7e9f8 Compare July 29, 2022 17:08
@vgonkivs vgonkivs force-pushed the fix_fraud_proof_propagation branch from 4c7e9f8 to a7f1150 Compare July 29, 2022 17:23
@vgonkivs vgonkivs marked this pull request as ready for review July 29, 2022 17:23
@vgonkivs vgonkivs requested a review from Bidon15 July 29, 2022 17:24
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Nice!

One last thing we should to care about is testing/mocking implementations of Proofs so that we can test network behaviours, like in this tests, without relying on a specific Proof type.

fraud/service_test.go Outdated Show resolved Hide resolved
fraud/service_test.go Outdated Show resolved Hide resolved
fraud/interface.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 1, 2022

One last thing we should to care about is testing/mocking implementations of Proofs so that we can test network behaviours, like in this tests, without relying on a specific Proof type.

Yes, I agree. But let's make it in scope of separate PR. IMO, it's not related to this one. Will create and post here an issue.

@vgonkivs vgonkivs force-pushed the fix_fraud_proof_propagation branch from f470ec9 to afc8e07 Compare August 1, 2022 11:29
@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 1, 2022

#971

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2022

Codecov Report

Merging #966 (0896179) into main (da5a5a4) will decrease coverage by 0.02%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #966      +/-   ##
==========================================
- Coverage   58.52%   58.50%   -0.03%     
==========================================
  Files         131      132       +1     
  Lines        7766     7947     +181     
==========================================
+ Hits         4545     4649     +104     
- Misses       2747     2820      +73     
- Partials      474      478       +4     
Impacted Files Coverage Δ
fraud/service.go 69.56% <76.92%> (+8.69%) ⬆️
fraud/helpers.go 76.92% <100.00%> (ø)
fraud/store.go 64.70% <100.00%> (+2.20%) ⬆️
cmd/flags_misc.go 42.59% <0.00%> (-11.80%) ⬇️
node/settings.go 31.37% <0.00%> (-4.19%) ⬇️
node/services/service.go 80.66% <0.00%> (-2.79%) ⬇️
cmd/env.go 76.47% <0.00%> (ø)
service/state/core_access.go 18.32% <0.00%> (ø)
header/metrics.go 0.00% <0.00%> (ø)
ipld/plugin/nmt.go 41.53% <0.00%> (+1.43%) ⬆️
... and 4 more

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

@vgonkivs vgonkivs force-pushed the fix_fraud_proof_propagation branch from 149fc1a to 92071a1 Compare August 1, 2022 14:33
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Nice!

Further #971 should prettify things a bit.

fraud/service_test.go Outdated Show resolved Hide resolved
fraud/service_test.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Everything looks generally okay but I don't understand the reasoning behind adding the from peer.ID param to the validator func instead of just getting the peer from the msg.ReceivedFrom field -- what does this change?

Also need to go through tests - they are quite intense so i'm gonna do another pass through just for the test

fraud/service.go Show resolved Hide resolved
fraud/service_test.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

So the tests are quite long and hard to follow what's going on and why - it would be nice to clean them up a bit, reduce LOC as much as possible (for example, no need to define bServ if it's only used in one place, just pass mdutils.Bserv() directly as a param). The tests inside sync_test.go are really nice to follow, for example.

fraud/service_test.go Show resolved Hide resolved
fraud/service_test.go Show resolved Hide resolved
fraud/service_test.go Outdated Show resolved Hide resolved
fraud/service_test.go Show resolved Hide resolved
fraud/service_test.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 2, 2022

msg.ReceivedFrom field is the address of node that have created a Fraud Proof. But it could be a case when other node receives a Fraud Proof and regossips it further

@renaynay

@vgonkivs vgonkivs requested a review from renaynay August 2, 2022 09:22
fraud/service_test.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs merged commit 0fbc94f into celestiaorg:main Aug 3, 2022
@vgonkivs vgonkivs deleted the fix_fraud_proof_propagation branch January 9, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fraud kind:fix Attached to bug-fixing PRs
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

fraud: blacklist republisher of the incorrect message, not the original publisher
4 participants