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

blockchain: Add ReconsiderBlock() #2196

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Conversation

Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Jun 7, 2024

Adds ReconsiderBlock() on blockchain that will clear up any block validation statuses and re-validate the block. If the block is part of a longer chain, then the chain will reorganize to that chain.

Replaces #2181

reorganizeChain() used to handle the following:
1: That the blocknodes being disconnected/connected indeed to connect
   properly without errors.
2: Perform the actual disconnect/connect of the blocknodes.

The functionality of 1, the validation that the disconnects/connects can
happen without errors are now refactored out into
verifyReorganizationValidity.

This is an effort made so that ReconsiderBlock() can call
verifyReorganizationValidity and set the block status of the
reconsidered chain and return nil even when an error returns as it's ok
to get an error when reconsidering an invalid branch.
ReconsiderBlock reconsiders the validity of the block for the passed
in blockhash. The behavior of the function mimics that of Bitcoin Core.

The invalid status of the block nodes are reset and if the chaintip that
is being reconsidered has more cumulative work, then we'll validate the
blocks and reorganize to it. If the cumulative work is lesser than the
current active chain tip, then nothing else will be done.
// code assumes that it's directly modifying the database so the cache
// will be left in an inconsistent state. It needs to be flushed beforehand
// in order for that to not happen.
err := b.db.Update(func(dbTx database.Tx) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Outstanding question is whether we need to do this still?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It wasn't needed here (the PR for when it was added) since the flush on disconnectBlock would always ensure that the right state would be saved to disk.

No need for it to be here and it's ok to remove it.

@coveralls
Copy link

coveralls commented Jun 7, 2024

Pull Request Test Coverage Report for Build 9423714813

Details

  • 129 of 169 (76.33%) changed or added relevant lines in 1 file are covered.
  • 15 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.04%) to 57.057%

Changes Missing Coverage Covered Lines Changed/Added Lines %
blockchain/chain.go 129 169 76.33%
Files with Coverage Reduction New Missed Lines %
blockchain/chain.go 3 74.78%
connmgr/connmanager.go 3 86.27%
peer/peer.go 9 73.86%
Totals Coverage Status
Change from base Build 9348961734: 0.04%
Covered Lines: 29680
Relevant Lines: 52018

💛 - Coveralls

@kcalvinalvin
Copy link
Collaborator

Verified that this PR is just a cleanup for the commits:

c14546b369dde37d868985f1112468b70d52967f
7c04bd86ac54fbe404e1085e569a35d83ea370e2

from #2181

Looks fine for me

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌴

@Roasbeef Roasbeef merged commit cd5e5ba into btcsuite:master Jun 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants