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

kallewoof · 2018-05-17T07:31:38Z

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).

Empact · 2018-05-17T07:33:11Z

src/rest.cpp

Missing paren.

Heh. Fixed.

Empact · 2018-05-17T07:47:32Z

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.

kallewoof · 2018-05-17T07:53:46Z

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..

jonasschnelli · 2018-05-17T07:55:39Z

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

kallewoof · 2018-05-17T08:21:18Z

@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)".

laanwj · 2018-05-17T10:18:08Z

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).

kallewoof · 2018-05-17T10:55:10Z

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.

practicalswift · 2018-05-17T11:03:52Z

Concept ACK: IsPruned() is better

Empact · 2018-05-17T16:12:20Z

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

kallewoof · 2018-05-18T03:38:11Z

@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.

GreatSock · 2018-05-19T12:51:22Z

utACK a0e2648

promag · 2018-05-19T14:19:49Z

utACK a0e2648.

sipa · 2018-05-19T18:56:03Z

src/chain.h

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.

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

promag · 2018-05-21T15:40:11Z

src/validation.h

nit, { on newline.

Empact · 2018-05-21T23:55:48Z

utACK dff0cf5

laanwj · 2018-05-29T18:29:24Z

Needs rebase.

kallewoof · 2018-05-30T03:24:14Z

Rebased.

Empact · 2018-06-08T07:57:21Z

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

Empact · 2018-06-08T08:17:21Z

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.

kallewoof · 2018-06-08T10:42:11Z

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.

… pruned or not e9a1881 refactor: add a function for determining if a block is pruned or not (Karl-Johan Alm) Pull request description: 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). Tree-SHA512: b9aeb60663e1d1196df5371d5aa00b32ff5d4cdea6a77e4b566f28115cce09570c18e45e4b81a4033f67c4135c8e32c027f67bae3b75c2ea4564285578a3f4dd

Summary: "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)." This is a backport from Core PR13259: bitcoin/bitcoin#13259 Test Plan: ninja check ./test/functional/test_runner.py Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4624

…lock is pruned or not e9a1881 refactor: add a function for determining if a block is pruned or not (Karl-Johan Alm) Pull request description: 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. bitcoin#10757~~ (turns out it was a move, not an addition). Tree-SHA512: b9aeb60663e1d1196df5371d5aa00b32ff5d4cdea6a77e4b566f28115cce09570c18e45e4b81a4033f67c4135c8e32c027f67bae3b75c2ea4564285578a3f4dd

Summary: "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)." This is a backport from Core PR13259: bitcoin/bitcoin#13259 Test Plan: ninja check ./test/functional/test_runner.py Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4624

kallewoof changed the title ~~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 May 17, 2018

kallewoof force-pushed the block-pruned-macro branch from 5887dca to e340c9c Compare May 17, 2018 07:32

Empact reviewed May 17, 2018

View reviewed changes

kallewoof force-pushed the block-pruned-macro branch from e340c9c to 87a4c78 Compare May 17, 2018 07:34

laanwj added the Refactoring label May 17, 2018

kallewoof force-pushed the block-pruned-macro branch from 87a4c78 to a0e2648 Compare May 17, 2018 10:53

kallewoof changed the title ~~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 May 19, 2018

sipa reviewed May 19, 2018

View reviewed changes

kallewoof force-pushed the block-pruned-macro branch 2 times, most recently from 5d6f03d to 0f80c21 Compare May 20, 2018 03:59

promag reviewed May 21, 2018

View reviewed changes

kallewoof force-pushed the block-pruned-macro branch from 0f80c21 to dff0cf5 Compare May 21, 2018 23:32

laanwj self-assigned this May 29, 2018

refactor: add a function for determining if a block is pruned or not

e9a1881

kallewoof force-pushed the block-pruned-macro branch from dff0cf5 to e9a1881 Compare May 30, 2018 03:24

laanwj merged commit e9a1881 into bitcoin:master Jun 8, 2018

kallewoof deleted the block-pruned-macro branch June 9, 2018 01:53

Bushstar mentioned this pull request Jun 12, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#343

Merged

dagurval mentioned this pull request Sep 11, 2019

refactoring: add a method for determining if a block is pruned… BitcoinUnlimited/BitcoinUnlimited#1915

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

kallewoof commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact May 17, 2018

Choose a reason for hiding this comment

Uh oh!

kallewoof May 17, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented May 17, 2018

Uh oh!

kallewoof commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 17, 2018

Uh oh!

kallewoof commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented May 17, 2018

Uh oh!

practicalswift commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 17, 2018

Uh oh!

kallewoof commented May 18, 2018

Uh oh!

GreatSock commented May 19, 2018

Uh oh!

promag commented May 19, 2018

Uh oh!

sipa May 19, 2018

Choose a reason for hiding this comment

Uh oh!

kallewoof May 20, 2018

Choose a reason for hiding this comment

Uh oh!

promag May 21, 2018

Choose a reason for hiding this comment

Uh oh!

kallewoof May 21, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented May 21, 2018

Uh oh!

laanwj commented May 29, 2018

Uh oh!

kallewoof commented May 30, 2018

Uh oh!

Empact commented Jun 8, 2018

Uh oh!

Empact commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kallewoof commented May 17, 2018 •

edited

Loading

kallewoof commented May 17, 2018 •

edited

Loading

kallewoof commented May 17, 2018 •

edited

Loading

laanwj commented May 17, 2018 •

edited

Loading

practicalswift commented May 17, 2018 •

edited

Loading

Empact commented Jun 8, 2018 •

edited

Loading