Skip to content

init: Remove dead code in LoadChainTip #15999

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
Merged
maflcko merged 1 commit into from
May 17, 2019
Merged

init: Remove dead code in LoadChainTip #15999

maflcko merged 1 commit into from
May 17, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 9, 2019 •
edited
Loading

LoadChainTip sets ::ChainActive() based on pcoinsTip's best block. LoadChainTip is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.

Fixes #15967 Inconsistent locking behavior in LoadChainTip

@maflcko maflcko force-pushed the 1905-initNoDead branch from fa9b40e to fa32fd0 Compare May 9, 2019 22:00
@DrahtBot
Copy link
Contributor

DrahtBot commented May 10, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15921 (Tidy up ValidationState interface by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@gmaxwell
Copy link
Contributor

gmaxwell commented May 10, 2019

This is essentially just reverting b0f3249 PR doesn't seem to address why this prior change was inapplicable.

@maflcko
Copy link
Member Author

maflcko commented May 10, 2019

See the immediate follow-up commit 13ab353 that disabled the code. So the code was never executed and tested as part of a release. (And if it ever were executed it would immediately crash with an assert failure anyway.)

If you want to enable that code, it needs to be properly reviewed and tested.

@practicalswift
Copy link
Contributor

practicalswift commented May 12, 2019

Concept ACK

promag
promag reviewed May 13, 2019
View reviewed changes
@maflcko maflcko force-pushed the 1905-initNoDead branch from fa32fd0 to fa86c8a Compare May 13, 2019 15:54
@promag
Copy link
Contributor

promag commented May 13, 2019

utACK fa86c8a.

@practicalswift
Copy link
Contributor

practicalswift commented May 13, 2019

utACK fa86c8a

ryanofsky
ryanofsky approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK fa86c8a. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:

bitcoin/src/init.cpp

Line 1573 in 2d16fb7

is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull();
, and the assert should prevent it from being erroneously called in the future.

jamesob
jamesob approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@jamesob jamesob left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK fa86c8a

Single use of LoadChainTip() is here, and is only called when pcoinsTip has contents; verify with git grep LoadChainTip\(.

@Empact
Copy link
Contributor

Empact commented May 17, 2019

utACK fa86c8a

@laanwj
Copy link
Member

laanwj commented May 17, 2019 •
edited
Loading

utACK fa86c8a
It's only confusing to have this if it isn't ever reached—the code can always be re-introduced later when necessary.

@maflcko maflcko merged commit fa86c8a into bitcoin:master May 17, 2019
maflcko pushed a commit that referenced this pull request May 17, 2019
Merge #15999: init: Remove dead code in LoadChainTip
a822a0e 
fa86c8a init: Remove dead code in LoadChainTip (MarcoFalke)

Pull request description:

  `LoadChainTip` sets `::ChainActive()` based on `pcoinsTip`'s best block. `LoadChainTip` is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.

  Fixes #15967  Inconsistent locking behavior in LoadChainTip

ACKs for commit fa86c8:
  promag:
    utACK fa86c8a.
  practicalswift:
    utACK fa86c8a
  Empact:
    utACK fa86c8a
  laanwj:
    utACK fa86c8a
  ryanofsky:
    utACK fa86c8a. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:
  jamesob:
    utACK fa86c8a

Tree-SHA512: 8961c0e579800a52038ac5655478468852faac055299b64d6cfdf0c213d3bf09669c4889467d09d93457f6c8b073967bb0475a137f77ddd3a3a3c03ad90001c4
@maflcko maflcko deleted the 1905-initNoDead branch May 17, 2019 11:47
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 19, 2020
Merge #15999: init: Remove dead code in LoadChainTip
094184b 
Summary:
fa86c8aec6 init: Remove dead code in LoadChainTip (MarcoFalke)

Pull request description:

  `LoadChainTip` sets `::ChainActive()` based on `pcoinsTip`'s best block. `LoadChainTip` is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.

  Fixes #15967  Inconsistent locking behavior in LoadChainTip

ACKs for commit fa86c8:
  promag:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d.
  practicalswift:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
  Empact:
    utACK bitcoin/bitcoin@fa86c8a
  laanwj:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
  ryanofsky:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:
  jamesob:
    utACK bitcoin/bitcoin@fa86c8a

Tree-SHA512: 8961c0e579800a52038ac5655478468852faac055299b64d6cfdf0c213d3bf09669c4889467d09d93457f6c8b073967bb0475a137f77ddd3a3a3c03ad90001c4

Backport of Core [[bitcoin/bitcoin#15999 | PR15999]]

Our only call for `LoadChainTip()` is [[ https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/init.cpp$2526-2528 | here ]] and requires `pcoinsTip` to not be null.

Test Plan:
  ninja
  ninja check-all

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, deadalnix

Subscribers: Fabien, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6631
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@ftrader
Merge #15999: init: Remove dead code in LoadChainTip
d225244 
Summary:
fa86c8aec6 init: Remove dead code in LoadChainTip (MarcoFalke)

Pull request description:

  `LoadChainTip` sets `::ChainActive()` based on `pcoinsTip`'s best block. `LoadChainTip` is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.

  Fixes #15967  Inconsistent locking behavior in LoadChainTip

ACKs for commit fa86c8:
  promag:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d.
  practicalswift:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
  Empact:
    utACK bitcoin/bitcoin@fa86c8a
  laanwj:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
  ryanofsky:
    utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:
  jamesob:
    utACK bitcoin/bitcoin@fa86c8a

Tree-SHA512: 8961c0e579800a52038ac5655478468852faac055299b64d6cfdf0c213d3bf09669c4889467d09d93457f6c8b073967bb0475a137f77ddd3a3a3c03ad90001c4

Backport of Core [[bitcoin/bitcoin#15999 | PR15999]]

Our only call for `LoadChainTip()` is [[ https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/init.cpp$2526-2528 | here ]] and requires `pcoinsTip` to not be null.

Test Plan:
  ninja
  ninja check-all

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, deadalnix

Subscribers: Fabien, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6631
kwvg added a commit to kwvg/dash that referenced this pull request Oct 22, 2021
@kwvg
merge bitcoin#15999: Remove dead code in LoadChainTip
51f73b5
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@kwvg @pravblockc
merge bitcoin#15999: Remove dead code in LoadChainTip
ec20de1
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@promag promag promag left review comments

@jamesob jamesob jamesob approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent locking behavior in LoadChainTip

9 participants

@maflcko @DrahtBot @gmaxwell @practicalswift @promag @Empact @laanwj @jamesob @ryanofsky