Add GetBlockIndex(const uint256& hash) for when the caller assumes that the block index exists for the given block hash #12897

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:GetBlockIndex changing 4 files +29 −4

practicalswift commented at 10:40 PM on April 5, 2018: contributor

Add GetBlockIndex(const uint256& hash) for when the caller assumes that the block index exists for the given block hash. As suggested by @promag in #12782.

Rationale: Explicit is better than implicit. Especially for assumptions :-)
practicalswift force-pushed on Apr 5, 2018
promag commented at 10:47 PM on April 5, 2018: member

Heh you don't waste time do you?

Concept ACK.

Maybe add a comment in LookupBlockIndex about GetBlockIndex and vice-versa?
practicalswift force-pushed on Apr 5, 2018
practicalswift commented at 10:56 PM on April 5, 2018: contributor

@promag Good point. Fixed! Please re-review :-)

in src/validation.h:433 in fa0b317581 outdated

 429 | @@ -430,13 +430,26 @@ class CVerifyDB {
 430 |  /** Replay blocks that aren't fully applied to the database. */
 431 |  bool ReplayBlocks(const CChainParams& params, CCoinsView* view);
 432 |  
 433 | +// GetBlockIndex(...) should be used instead if the caller assumes that the

promag commented at 11:04 PM on April 5, 2018:

Maybe improve a bit:

 /** Find and retrieve the block index for the given block hash.
  *  The caller must check if the block index is valid. If the block index
  *  always exists then use GetBlockIndex instead.
  *  Call with cs_main held.
  *
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] hash The block hash
  * [@return](/bitcoin-bitcoin/contributor/return/) The block index pointer if found, nullptr otherwise
  * [@see](/bitcoin-bitcoin/contributor/see/) GetBlockIndex
  */

in src/validation.h:442 in fa0b317581 outdated

 437 |      AssertLockHeld(cs_main);
 438 |      BlockMap::const_iterator it = mapBlockIndex.find(hash);
 439 |      return it == mapBlockIndex.end() ? nullptr : it->second;
 440 |  }
 441 |  
 442 | +// The caller knows if the block index exists for the given block hash.

promag commented at 11:09 PM on April 5, 2018:

 /** Retrieve the block index for the given block hash.
  *  The block index must exists otherwise an assertion fails. If the block index 
  *  can not exist then use LookupBlockIndex instead.
  *  Call with cs_main held.
  *
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] hash The block hash
  * [@return](/bitcoin-bitcoin/contributor/return/) The block index pointer
  * [@see](/bitcoin-bitcoin/contributor/see/) LookupBlockIndex
  */

promag commented at 11:24 PM on April 5, 2018: member

Alternative docblock.
meshcollider added the label Refactoring on Apr 6, 2018
Add GetBlockIndex(const uint256& hash) for when the caller assumes the block index exists for the given block hash 23f1760c59
practicalswift force-pushed on Apr 6, 2018
practicalswift commented at 9:13 AM on April 6, 2018: contributor

@promag Much better! Thanks. Updated. Please re-review :-)
promag commented at 9:23 PM on April 6, 2018: member

utACK 23f1760.

Nit, commit message could be shorter.
TheBlueMatt commented at 5:22 PM on April 17, 2018: member

Why would we want this? It looks like just needless additional interfaces and code duplication for nothing more than an assert. NACK without justification.
promag commented at 5:35 PM on April 17, 2018: member

@TheBlueMatt yes, nothing more than that. You probably prefer #12782.
practicalswift commented at 8:31 PM on April 17, 2018: contributor

Closing since lack of consensus ACK :-)
practicalswift closed this on Apr 17, 2018
practicalswift commented at 8:31 PM on April 17, 2018: contributor

@TheBlueMatt A simple assert(…) was originally suggested: see PR #12782 :-)
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#12782 Explicitly state our assumptions about LookupBlockIndex(...) return values