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

Check if a processed orphan is in the main chain and update its parent accordingly #1527

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

someone235
Copy link

@someone235 someone235 commented Jan 31, 2020

This PR fixes the situation where a new block is first not considered in the main chain, but then after processing orphans that depend on it the block is now on the heaviest chain, but ProcessBlock still mistakenly returns that it's not in the main chain.

Copy link
Collaborator

@jakesylvestre jakesylvestre left a comment

Choose a reason for hiding this comment

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

Looks good!

@jakesylvestre
Copy link
Collaborator

@jcvernaleo (as per #1530)

  • low priority

@jakesylvestre
Copy link
Collaborator

@someone235 can you add a description here?

@someone235 someone235 changed the title Check if a processed orphan is in the main chain and update its paren… Check if a processed orphan is in the main chain and update its parent accordingly Mar 4, 2020
@someone235
Copy link
Author

@someone235 can you add a description here?

I updated the description. I hope it's clear

@jakesylvestre
Copy link
Collaborator

Cool - will test this out tomorrow

@jakesylvestre
Copy link
Collaborator

Hey - one more thing. Looks like you'll have to rebase or force push so tests pass

@someone235 someone235 force-pushed the update-isMainChain-according-to-orphan branch from a2a7b32 to 4496974 Compare December 10, 2020 20:07
@someone235
Copy link
Author

Rebased

@coveralls
Copy link

coveralls commented Dec 10, 2020

Pull Request Test Coverage Report for Build 413871199

  • 10 of 12 (83.33%) changed or added relevant lines in 1 file are covered.
  • 17 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.7%) to 53.375%

Changes Missing Coverage Covered Lines Changed/Added Lines %
blockchain/process.go 10 12 83.33%
Files with Coverage Reduction New Missed Lines %
btcec/signature.go 6 91.64%
peer/peer.go 11 75.47%
Totals Coverage Status
Change from base Build 407350020: 1.7%
Covered Lines: 20830
Relevant Lines: 39026

💛 - Coveralls

Copy link
Collaborator

@onyb onyb left a comment

Choose a reason for hiding this comment

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

Seems good from the look of it. I think it's also interesting to use fullblocktests and write a regression test since this PR touches consensus-critical code.

@someone235
Copy link
Author

Seems good from the look of it. I think it's also interesting to use fullblocktests and write a regression test since this PR touches consensus-critical code.

TBH I probably won't have time for this anytime soon, so you can add the test if you want.

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.

None yet

5 participants