refactoring: add a method for determining if a block is pruned or not #13259

pull kallewoof wants to merge 1 commits into bitcoin:master from kallewoof:block-pruned-macro changing 3 files +8 −2
  1. kallewoof commented at 7:31 AM on May 17, 2018: member

    The check for whether a block is pruned or not is sufficiently obscure that it deserves a macro. It is also used in 2 places, with more coming, e.g. #10757 (turns out it was a move, not an addition).

  2. kallewoof renamed this:
    validation: add a macro for determining if a block is pruned or not
    refactoring: add a macro for determining if a block is pruned or not
    on May 17, 2018
  3. kallewoof force-pushed on May 17, 2018
  4. in src/rest.cpp:220 in e340c9c35c outdated 
     216 | @@ -217,7 +217,7 @@ static bool rest_block(HTTPRequest* req,
 217 |              return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
 218 |          }
 219 |  
 220 | -        if (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0)
 221 | +        if (BLOCK_PRUNED(pblockindex)
    Empact commented at 7:33 AM on May 17, 2018:

    Missing paren.

    kallewoof commented at 7:34 AM on May 17, 2018:

    Heh. Fixed.

  5. kallewoof force-pushed on May 17, 2018
  6. Empact commented at 7:47 AM on May 17, 2018: member

    This is a different change but for simplification IMO there's room for a HasData() or similar method on CBlockIndex, in place of ->nStatus & BLOCK_HAVE_DATA. There'd be some ~30 callers.

  7. kallewoof commented at 7:53 AM on May 17, 2018: member

    Not sure pindex->nStatus & BLOCK_HAVE_DATA is obscure enough to warrant a new method, but if it's inline it might be okay, yeah..

  8. jonasschnelli commented at 7:55 AM on May 17, 2018: contributor

    Unsure. I'm worries about the readability of the code as well as undetected nullptr accesses in future implementations.

  9. kallewoof commented at 8:21 AM on May 17, 2018: member

    @jonasschnelli The code that is replaced has to do that check already (null block index), so not sure what the concern is.

    Also, I think this helps with readability. I think the "is this block pruned? Oh let's just throw in a (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0) and check" is a lot more error prone that "oh let's just throw in a BLOCK_PRUNED(pblockindex)".

  10. laanwj added the label Refactoring on May 17, 2018
  11. laanwj commented at 10:18 AM on May 17, 2018: member

    If you insist on factoring this out, I'd prefer an inline function instead of a macro. Macros are mostly an ugly C legacy (and have some well-known issues, such as that pblockindex is evaluated multiple times).

  12. kallewoof force-pushed on May 17, 2018
  13. kallewoof commented at 10:55 AM on May 17, 2018: member

    I'm not insisting anything, but I think it would help readability since the copy-pasted if case is rather hairy.

    Replaced macro with an inline method in CBlockIndex.

  14. practicalswift commented at 11:03 AM on May 17, 2018: contributor

    Concept ACK: IsPruned() is better

  15. Empact commented at 4:12 PM on May 17, 2018: member

    Concept ACK. I would leave the fHavePruned check at the call sites though for separation of concerns. Looks like he fHavePruned check is a feature flipper / optimization, IsPruned should always be false if it is: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4500

  16. kallewoof commented at 3:38 AM on May 18, 2018: member

    @Empact I think it's okay to leave it in. It has very small impact and I think it's fine even if people check fHavePruned externally as well.

  17. kallewoof renamed this:
    refactoring: add a macro for determining if a block is pruned or not
    refactoring: add a method for determining if a block is pruned or not
    on May 19, 2018
  19. promag commented at 2:19 PM on May 19, 2018: member

    utACK a0e2648.

  20. in src/chain.h:343 in a0e2648534 outdated 
     338 | @@ -337,6 +339,11 @@ class CBlockIndex
 339 |          return ((nStatus & BLOCK_VALID_MASK) >= nUpTo);
 340 |      }
 341 |  
 342 | +    //! Check whether the block associated with this index entry is pruned or not.
 343 | +    inline bool IsPruned() const {
    sipa commented at 6:56 PM on May 19, 2018:

    Adding this to the chain module introduces a cyclic dependency between chain and validation (as fHavePruned is defined there). There's no need for this to be a CBlockIndex member as it only uses public fields, so I would suggest making it a function in validation instead.

    kallewoof commented at 3:48 AM on May 20, 2018:

    Good point. I moved it (back) to validation.h.

  21. kallewoof force-pushed on May 20, 2018
  22. kallewoof force-pushed on May 20, 2018
  23. in src/validation.h:499 in 0f80c21ba9 outdated 
     494 | @@ -495,4 +495,9 @@ bool DumpMempool();
 495 |  /** Load the mempool from disk. */
 496 |  bool LoadMempool();
 497 |  
 498 | +//! Check whether the block associated with this index entry is pruned or not.
 499 | +inline bool IsBlockPruned(const CBlockIndex* pblockindex) {
    promag commented at 3:40 PM on May 21, 2018:

    nit, { on newline.

    kallewoof commented at 11:32 PM on May 21, 2018:

    Fixed.

  24. kallewoof force-pushed on May 21, 2018
  25. Empact commented at 11:55 PM on May 21, 2018: member

    utACK dff0cf5

  26. laanwj commented at 6:29 PM on May 29, 2018: member

    Needs rebase.

  27. laanwj assigned laanwj on May 29, 2018
  28. refactor: add a function for determining if a block is pruned or not e9a1881b90
  29. kallewoof force-pushed on May 30, 2018
  30. kallewoof commented at 3:24 AM on May 30, 2018: member

    Rebased.

  31. Empact commented at 7:57 AM on June 8, 2018: member

    nit: GetBlockChecked and IsBlockPruned should probably both assert on the index not being null.

  32. Empact commented at 8:17 AM on June 8, 2018: member

    Also occurs to me that GetBlockChecked and the functionality in rest_block are identical except for the error handling. You could make GetBlockChecked suitable for both by returning the error in the form of std::optional<std::string>boost::optional<std::string> (because C++11). That would remove the need for this function because there would be no reuse.

  33. kallewoof commented at 10:42 AM on June 8, 2018: member

    That sounds like a good idea, but it means having to make GetBlockChecked public, and requires changing it around. There's also no unified way of saying what the error is, in case there are non-NOT_FOUND type errors in the future.

  34. laanwj merged this on Jun 8, 2018
  35. laanwj closed this on Jun 8, 2018
  36. laanwj referenced this in commit 121cbaacc2 on Jun 8, 2018
  37. kallewoof deleted the branch on Jun 9, 2018
  38. jasonbcox referenced this in commit a8257058a3 on Dec 6, 2019
  39. PastaPastaPasta referenced this in commit b2c3237b1c on Jun 17, 2020
  40. PastaPastaPasta referenced this in commit 000b2794a5 on Jul 2, 2020
  41. jonspock referenced this in commit a1c4ed3e23 on Oct 2, 2020
  42. jonspock referenced this in commit fa4cc2da53 on Oct 5, 2020
  43. jonspock referenced this in commit c404d98a00 on Oct 10, 2020
  44. MarcoFalke locked this on Sep 8, 2021
Contributors
kallewoofEmpactjonasschnellilaanwjpracticalswiftGreatSockpromagsipa
Labels
Refactoring
Linked (view graph)
#10757 RPC: Introduce getblockstats to plot things
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me