Skip to content

Remove GetSpendHeight #23630

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
fanquake merged 1 commit into from
Dec 3, 2021
Merged

Remove GetSpendHeight #23630

fanquake merged 1 commit into from
Dec 3, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 29, 2021

It is unclear what the goal of the helper is, as the caller already
knows the spend height before calling the helper.

Also, in case the coins view is corrupted, LookupBlockIndex will return
nullptr. Dereferencing a nullptr is UB.

Fix both issues by removing it. Also, add a sanity check, which aborts
if the coins view is corrupted.

@maflcko maflcko mentioned this pull request Nov 29, 2021
Merged
@laanwj laanwj added the Mempool label Nov 29, 2021
darosior
darosior reviewed Nov 30, 2021
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK fa6a0f9a7c00015e70c9805faf45685ac300f61e

@maflcko maflcko mentioned this pull request Dec 1, 2021
Merged
laanwj
laanwj reviewed Dec 2, 2021
View reviewed changes
Remove GetSpendHeight
fa3942f 
It is unclear what the goal of the helper is, as the caller already
knows the spend height before calling the helper.

Also, in case the coins view is corrupted, LookupBlockIndex will return
nullptr. Dereferencing a nullptr is UB.

Fix both issues by removing it. Also, add a sanity check, which aborts
if the coins view is corrupted.
@maflcko maflcko force-pushed the 2111-noSpendHeight branch from fa6a0f9 to fa3942f Compare December 2, 2021 15:00
@laanwj
Copy link
Member

laanwj commented Dec 2, 2021

Code review ACK fa3942f

@jamesob
Copy link
Contributor

jamesob commented Dec 2, 2021

I'm pretty sure I'm ACK on this, but just to be sure (and as a helpful reminder):

  • what are the cases where coins_view.GetBestBlock() differs from chaintip? Is that only on startup when we're recovering from a crash that happened before we could write the coins down to disk?
  • do we have any reason to believe this (BestBlock != tip) could happen during mempool acceptance, or is there some kind of explicit guard against that?

ryanofsky
ryanofsky approved these changes Dec 2, 2021
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.

Code review ACK fa3942f. I'm not aware of cases where coins GetBestBlock could be different from active chain tip, and asset seems sufficient to guarantee PR doesn't change behavior if that doesn't happen.

Removing GetSpendHeight was also discussed in #21055 (comment)

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 3, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23581 (Move BlockManager to node/blockstorage by MarcoFalke)

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.

@maflcko
Copy link
Member Author

maflcko commented Dec 3, 2021

do we have any reason to believe this (BestBlock != tip) could happen during mempool acceptance, or is there some kind of explicit guard against that?

I've added the assert in this commit, if that is what you mean with guard?

@maflcko
Copy link
Member Author

maflcko commented Dec 3, 2021

To clarify on master, if the assumption didn't hold we'd silently run out of consensus (potentially via UB). With this patch the node will crash.

@fanquake fanquake merged commit 345c818 into bitcoin:master Dec 3, 2021
@maflcko maflcko deleted the 2111-noSpendHeight branch December 3, 2021 12:58
@jamesob
Copy link
Contributor

jamesob commented Dec 3, 2021

Post-merge ACK fa3942f

I was a little unclear on how this change might interact with reorgs, but I verified that the coins view's BestBlock is updated lock-step with the tip in DisconnectBlock(). It seems there is a brief period during ReplayBlocks() during which it isn't clear to me that the two are lockstep (https://github.com/jamesob/bitcoin/blob/785f9cc46a43661c63a5ec56a9e82f4ce9d42f44/src/validation.cpp#L4277-L4287), but of course ReplayBlocks() is called in init before the chainstate is fully initialized.

Otherwise (based on a grep of SetBestBlock()), the best block should be fully in sync with the tip.

@jamesob jamesob mentioned this pull request Dec 3, 2021
Merged
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 3, 2021
@fanquake
Merge bitcoin#23630: Remove GetSpendHeight
f7d9c4f
@pg156
Copy link

pg156 commented Dec 4, 2021

Otherwise (based on a grep of SetBestBlock()), the best block should be fully in sync with the tip.

Could you elaborate on this for my learning? It is not obvious to me why this is the case from the grep result. Thank you.

maflcko pushed a commit that referenced this pull request Dec 6, 2021
Merge #23661: cover DisconnectBlock with lock annotation
dca9ab4 
7da4a8f cover DisconnectBlock with lock annotation (James O'Beirne)

Pull request description:

  While reviewing #23630, I noticed that `DisconnectBlock` is uncovered by lock annotations. CoinsTip() access requires cs_main and therefore so should this function.

ACKs for top commit:
  jonatack:
    ACK 7da4a8f

Tree-SHA512: 3e2b0247c138b31deeadcd48eb3f7bc8d32c0b6bb6d6e94ccf8ea0cbbc50b1b35d83f662eee432f2bd2d87a3fe9c94604da806ec711df93298bfb0ab34a5a05b
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
@fanquake
Merge bitcoin/bitcoin#23630: Remove GetSpendHeight
beb5236 
797c7b3 Remove GetSpendHeight (MarcoFalke)

Pull request description:

  It is unclear what the goal of the helper is, as the caller already
  knows the spend height before calling the helper.

  Also, in case the coins view is corrupted, LookupBlockIndex will return
  nullptr. Dereferencing a nullptr is UB.

  Fix both issues by removing it. Also, add a sanity check, which aborts
  if the coins view is corrupted.

ACKs for top commit:
  laanwj:
    Code review ACK 797c7b3
  ryanofsky:
    Code review ACK 797c7b3. I'm not aware of cases where coins GetBestBlock could be different from active chain tip, and asset seems sufficient to guarantee PR doesn't change behavior if that doesn't happen.

Tree-SHA512: 29f65d72e116ec5a4509e0947ceeaa5bb6b7dfd5d174d3c7945cb15fa266d590c4f8b48e6385de74ef7d7c84ebd2255de902ad9c87c24955348a91b12e5bffd5
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2022
@PiRK
Remove GetSpendHeight
150344e 
Summary:
It is unclear what the goal of the helper is, as the caller already knows the spend height before calling the helper.

Also, in case the coins view is corrupted, LookupBlockIndex will return nullptr. Dereferencing a nullptr is UB.

Fix both issues by removing it. Also, add a sanity check, which aborts if the coins view is corrupted.

This is a backport of [[bitcoin/bitcoin#23630 | core#23630]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12455
@bitcoin bitcoin locked and limited conversation to collaborators Dec 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@darosior darosior darosior left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @laanwj @jamesob @DrahtBot @pg156 @ryanofsky @darosior @fanquake