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.

Sign up for GitHub

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

Jump to bottom

fraud: implement fraud sync #917

Merged
merged 28 commits into from
Sep 7, 2022
Merged

fraud: implement fraud sync #917

merged 28 commits into from
Sep 7, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jul 18, 2022 •
edited
Loading

Resolves #536

  • add unit test
  • add swamp test
  • update ADR

@vgonkivs vgonkivs added area:fraud kind:feat Attached to feature PRs labels Jul 18, 2022
@vgonkivs vgonkivs self-assigned this Jul 18, 2022
@vgonkivs vgonkivs force-pushed the implement_fraud_sync branch 4 times, most recently from 261749d to f566783 Compare August 3, 2022 13:00
@vgonkivs vgonkivs force-pushed the implement_fraud_sync branch 4 times, most recently from 6ab8387 to 74f4f7e Compare August 5, 2022 16:46
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022 •
edited
Loading

Codecov Report

Merging #917 (8181c1a) into main (c696009) will decrease coverage by 0.84%.
The diff coverage is 47.98%.

@@            Coverage Diff             @@
##             main     celestiaorg/celestia-node#917      +/-   ##
==========================================
- Coverage   57.58%   56.74%   -0.85%     
==========================================
  Files         133      135       +2     
  Lines        8307     8965     +658     
==========================================
+ Hits         4784     5087     +303     
- Misses       3050     3342     +292     
- Partials      473      536      +63
Impacted Files Coverage Δ
fraud/pb/proof.pb.go 36.61% <39.06%> (+3.09%) ⬆️
fraud/requester.go 48.14% <48.14%> (ø)
fraud/sync.go 59.43% <59.43%> (ø)
fraud/service.go 76.00% <73.52%> (-0.09%) ⬇️
fraud/registry.go 90.47% <100.00%> (+5.86%) ⬆️
fraud/store.go 58.33% <100.00%> (+2.45%) ⬆️
node/components.go 90.90% <100.00%> (+0.21%) ⬆️
node/services/service.go 81.76% <100.00%> (+1.09%) ⬆️
header/core/listener.go 52.83% <0.00%> (-5.67%) ⬇️

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 implement_fraud_sync branch 3 times, most recently from 2e827d8 to d6cefff Compare August 8, 2022 09:31
@vgonkivs vgonkivs requested a review from Bidon15 August 8, 2022 09:34
Bidon15
Bidon15 reviewed Aug 8, 2022
View reviewed changes
fraud/interface.go Outdated Show resolved Hide resolved
fraud/requester.go Outdated Show resolved Hide resolved
fraud/service.go Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the implement_fraud_sync branch 2 times, most recently from 0ca476b to 755c6e9 Compare August 9, 2022 17:17
@vgonkivs vgonkivs requested a review from Bidon15 August 9, 2022 17:24
@vgonkivs vgonkivs marked this pull request as ready for review August 9, 2022 18:22
@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 9, 2022

start syncing was rework in 2c232ee

It was added as a separate commit in order to show the difference between two approaches. @Wondertan , please take a look at diff.

Bidon15
Bidon15 reviewed Aug 10, 2022
View reviewed changes
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

some small nits and 🚀

fraud/pb/proof.proto Outdated Show resolved Hide resolved
fraud/service.go Outdated Show resolved Hide resolved
node/tests/fraud_test.go Outdated Show resolved Hide resolved
Wondertan
Wondertan reviewed Sep 6, 2022
View reviewed changes
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.

Peer cache review

fraud/sync.go Show resolved Hide resolved
fraud/sync.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Sep 6, 2022
View reviewed changes
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.

🙏

@Wondertan
Copy link
Member

What's the status of the broken test here? Is it coming from this PR?

@vgonkivs
Copy link
Member Author

vgonkivs commented Sep 7, 2022

No. Fraud sync does not affect full reconstruction test.

@vgonkivs
Copy link
Member Author

vgonkivs commented Sep 7, 2022

#1058

renaynay
renaynay reviewed Sep 7, 2022
View reviewed changes
fraud/service_test.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Sep 7, 2022
View reviewed changes
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.

Looks fine, made one minor comment re using context.TODO in test

One thing is: I don't like this: f.registerProofTopics(getRegisteredProofTypes()...); I'm okay with merging as is, but I hope we can make this a bit nicer.

distractedm1nd
distractedm1nd previously approved these changes Sep 7, 2022
View reviewed changes
fraud/sync.go Show resolved Hide resolved
fraud/service.go Show resolved Hide resolved
renaynay
renaynay approved these changes Sep 7, 2022
View reviewed changes
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.

morge

@vgonkivs vgonkivs merged commit 5ae17af into celestiaorg:main Sep 7, 2022
distractedm1nd pushed a commit to renaynay/celestia-node that referenced this pull request Sep 19, 2022
@vgonkivs @distractedm1nd
fraud: implement fraud sync (celestiaorg#917)
bc7e379 
* fraud: add fraud message

* fraud: implement fraud sync logic

* fraud: start syncing proofs once node checks the local storage

* doc: update an adr

* fraud: add peer cache to avoid multiple requests to the same peer

* fraud: join topics for all available proof types
distractedm1nd pushed a commit to distractedm1nd/celestia-node that referenced this pull request Sep 21, 2022
@vgonkivs @distractedm1nd
fraud: implement fraud sync (celestiaorg#917)
fd48dbd 
* fraud: add fraud message

* fraud: implement fraud sync logic

* fraud: start syncing proofs once node checks the local storage

* doc: update an adr

* fraud: add peer cache to avoid multiple requests to the same peer

* fraud: join topics for all available proof types
@vgonkivs vgonkivs deleted the implement_fraud_sync branch January 9, 2023 13:48
@vgonkivs vgonkivs mentioned this pull request Aug 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@distractedm1nd distractedm1nd distractedm1nd left review comments

@renaynay renaynay renaynay approved these changes

@Wondertan Wondertan Wondertan approved these changes

@liamsi liamsi Awaiting requested review from liamsi liamsi is a code owner

@Bidon15 Bidon15 Awaiting requested review from Bidon15

Assignees

@vgonkivs vgonkivs

Labels
area:fraud kind:break! Attached to breaking PRs kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fraud: FraudSync
6 participants
@vgonkivs @codecov-commenter @Bidon15 @Wondertan @renaynay @distractedm1nd