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

refactor!: harden verification #90

Merged
merged 7 commits into from
Aug 24, 2023
Merged

refactor!: harden verification #90

merged 7 commits into from
Aug 24, 2023

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Jul 31, 2023

We realized that non-adjacent verification might produce false negatives in Tendermint consensus. Additionally, not all chains have non-adjacent verification, and before this change, we dropped all headers that failed the non-adjacency verification. However, we should give them a second chance and keep them as sync targets.

Additionally:

  • Introduces typical verification flow. Users had to verify time, height, and chain-id themselves, but not anymore.
  • We reject known headers, and it's a hard failure now.
  • Non-adjacent verification failure is considered a soft failure.

Closes #79

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Merging #90 (84b76c3) into main (c5a5780) will increase coverage by 0.14%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   68.27%   68.41%   +0.14%     
==========================================
  Files          37       37              
  Lines        2909     2929      +20     
==========================================
+ Hits         1986     2004      +18     
- Misses        771      774       +3     
+ Partials      152      151       -1     
Files Changed Coverage Δ
p2p/helpers.go 78.26% <ø> (ø)
p2p/server.go 77.22% <ø> (-5.00%) ⬇️
p2p/subscriber.go 44.64% <0.00%> (ø)
sync/sync_head.go 68.50% <80.00%> (+6.78%) ⬆️
verify.go 96.22% <96.22%> (ø)
headertest/dummy_header.go 72.85% <100.00%> (+5.14%) ⬆️
headertest/dummy_suite.go 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

headertest/dummy_header.go Outdated Show resolved Hide resolved
p2p/subscriber.go Outdated Show resolved Hide resolved
@Wondertan Wondertan force-pushed the call-validate branch 2 times, most recently from e81c0c1 to 9e29211 Compare August 15, 2023 13:43
@Wondertan Wondertan force-pushed the verify branch 2 times, most recently from b8065f9 to bdda7bd Compare August 20, 2023 09:58
Base automatically changed from call-validate to main August 21, 2023 11:46
@Wondertan Wondertan marked this pull request as ready for review August 21, 2023 11:59
@Wondertan Wondertan changed the title refactor: harden verification refactor!: harden verification Aug 21, 2023
vgonkivs
vgonkivs previously approved these changes Aug 22, 2023
sync/sync_head.go Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
sync/verify/verify.go Outdated Show resolved Hide resolved
Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

LGTM

@Wondertan Wondertan merged commit f79c35c into main Aug 24, 2023
2 checks passed
@Wondertan Wondertan deleted the verify branch August 24, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attacks possible if we always accept non-adjacent headers
4 participants