Assert all the things! #8665

NicolasDorier commented at 6:57 AM on September 5, 2016: contributor

if pindexPrev was null, we would get a problem at pindexPrev->GetMedianTimePast()

dcousens commented at 7:02 AM on September 5, 2016: contributor

utACK 8a87470

Submitted a PR for your PR, see https://github.com/NicolasDorier/bitcoin/pull/1

luke-jr commented at 7:09 AM on September 5, 2016: member

This means we're not enforcing it on the genesis block? Doesn't look like a practical problem today, but feels like a booby-trap. :/

NicolasDorier commented at 7:38 AM on September 5, 2016: contributor

@luke-jr yes, there is two calls to it, one check explicitely it is not the genesis

https://github.com/NicolasDorier/bitcoin/blob/8a87470ebbd289b502e7aec5476368dcfc3c18bb/src/main.cpp#L3655

And the other is TestBlockValidity, which test a block which got mined, so obviously not the genesis.

https://github.com/NicolasDorier/bitcoin/blob/8a87470ebbd289b502e7aec5476368dcfc3c18bb/src/main.cpp#L3776

MarcoFalke added the label Refactoring on Sep 6, 2016

NicolasDorier force-pushed on Sep 9, 2016

NicolasDorier renamed this:
~~Trivial: ContextualCheckBlockHeader should never have pindexPrev equals to NULL~~
Trivial: Assert all the things!
on Sep 9, 2016

NicolasDorier commented at 2:55 AM on September 9, 2016: contributor

Added one more assert which make review more easy. This one bit me quite a few time, as pIndex sometimes refers to the block being evaluated, and sometimes to the previous one.

dcousens commented at 3:05 AM on September 9, 2016: contributor

utACK 34a37ba

NicolasDorier commented at 4:50 AM on September 9, 2016: contributor

mmh failure with last commit, investigating it. Interesting.

NicolasDorier force-pushed on Sep 9, 2016

NicolasDorier commented at 1:29 PM on September 9, 2016: contributor

yuk, the problem was that pindex->phashBlock == NULL is used as a flag to know if it is called by CreateNewBlock...

morcos commented at 8:00 PM on September 12, 2016: member

I don't think we should tag PR's like this with "Trivial" Perhaps I'm misremembering, but I thought "Trivial" was for things that only changed comments or strings that couldn't possibly change behavior. This PR takes at least a little bit of thinking about.

I don't have much of an opinion on the actual changes.

sipa commented at 8:16 PM on September 12, 2016: member

I agree, this is not trivial. Trivial is for comment and documentation changes. Not just "this does not change behaviour", but "this so obviously does not change behaviour that it does not need review".

in src/pow.cpp:None in 8eab1315f7 outdated

  11 | @@ -12,12 +12,9 @@
  12 |  
  13 |  unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& params)
  14 |  {
  15 | +    assert(pindexLast != NULL);
  16 |      unsigned int nProofOfWorkLimit = UintToArith256(params.powLimit).GetCompact();
  17 |  
  18 | -    // Genesis block

laanwj commented at 8:18 AM on September 13, 2016:

This case (Genesis block) is never actually necessary?

dcousens commented at 8:49 AM on September 13, 2016:

GetNextWorkRequired is never called with pindexLast == NULL (as written, not as tested, aka, per the code base)

NicolasDorier commented at 9:54 AM on September 13, 2016: contributor

Actually I thought that we were in the "this so obviously does not change behaviour that it does not need review" condition, until I saw that phashBlock could be NULL...

If it is not obvious then maybe this PR is not worth the review time, I can remove some commits if needed to make it trivial again.

But on the other side, I added assert to make non trivial things more visible...

dcousens commented at 10:32 AM on September 13, 2016: contributor

I think the PR is still worth it for the simple fact it adds more information/context to the code. However, perhaps as discussed on IRC, it might be better to just remove the assertions and de-reference the pointers at the call sites by using a C++ reference.

laanwj commented at 3:30 PM on September 13, 2016: member

If it is not obvious then maybe this PR is not worth the review time, I can remove some commits if needed to make it trivial again.

I think these changes are okay, let's just not call it trivial. As @morcos says the trivial denomination is for puls that don't change code, e.g. documentation typos, string typos, comment changes. Asserts generate code and can in principle create bad situations like DoS attacks if they're remote-triggerable so need to be carefully reviewed. (which doesn't mean that changes to assertions aren't worth review time)

NicolasDorier renamed this:
~~Trivial: Assert all the things!~~
Assert all the things!
on Sep 14, 2016

NicolasDorier force-pushed on Sep 14, 2016

MarcoFalke commented at 5:23 PM on September 30, 2016: member

utACK dc64a16af92bf459dde32c32cea11b266db54f04

MarcoFalke commented at 4:38 PM on December 4, 2016: member

Needs rebase.

ContextualCheckBlockHeader should never have pindexPrev to NULL cc44c8f143

pow: GetNextWorkRequired never called with NULL pindexLast 972714c956

NicolasDorier force-pushed on Feb 15, 2017

NicolasDorier commented at 1:52 AM on February 16, 2017: contributor

rebased

in src/validation.cpp:None in 61b84a0142 outdated

1738 | @@ -1739,7 +1739,10 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
1739 |                    CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck)
1740 |  {
1741 |      AssertLockHeld(cs_main);
1742 | -
1743 | +    assert(pindex);
1744 | +    // pindex->phashBlock can be null if called by CreateNewBlock

TheBlueMatt commented at 1:45 AM on February 19, 2017:

(or, generally, by TestBlockValidity)

TheBlueMatt commented at 1:46 AM on February 19, 2017: member

utACK 61b84a0142cd736a2b819b771fc843703ab8ae65 (+/- comment on comment)

Assert ConnectBlock block and pIndex are the same block 4d51e9be16

NicolasDorier force-pushed on Feb 19, 2017

NicolasDorier commented at 5:03 AM on February 19, 2017: contributor

addressed TheBlueMatt comment

NicolasDorier commented at 8:45 AM on February 23, 2017: contributor

Anything missing ?

NicolasDorier commented at 5:52 AM on March 14, 2017: contributor

not sure what to do with this PR. I think it increases readability... but it has been open since september. Do I keep open ?

laanwj commented at 9:36 AM on March 14, 2017: member

Yeah let's merge it. Asserting out is better than crashing with a NULL pointer dereference.

laanwj merged this on Mar 14, 2017

laanwj closed this on Mar 14, 2017

laanwj referenced this in commit 1b046603b3 on Mar 14, 2017

PastaPastaPasta referenced this in commit 567cceb1ef on Jan 2, 2019

PastaPastaPasta referenced this in commit 2b17fb6bad on Jan 2, 2019

PastaPastaPasta referenced this in commit 1b7268328f on Jan 2, 2019

PastaPastaPasta referenced this in commit 696f5779d2 on Jan 3, 2019

PastaPastaPasta referenced this in commit 592c691402 on Jan 21, 2019

PastaPastaPasta referenced this in commit 961c0db978 on Jan 29, 2019

PastaPastaPasta referenced this in commit c19e5416f2 on Feb 26, 2019

PastaPastaPasta referenced this in commit c4a3cd6a13 on Feb 26, 2019

UdjinM6 referenced this in commit 01ba0a40b0 on Mar 9, 2019

PastaPastaPasta referenced this in commit b0d56e8d79 on Mar 10, 2019

MarcoFalke locked this on Sep 8, 2021