Assert all the things! #8665
Assert all the things! #8665
Conversation
|
utACK 8a87470 Submitted a PR for your PR, see NicolasDorier#1 |
|
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. :/ |
|
@luke-jr yes, there is two calls to it, one check explicitely it is not the genesis And the other is TestBlockValidity, which test a block which got mined, so obviously not the genesis. |
NicolasDorier
changed the title
|
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. |
|
utACK 34a37ba |
|
mmh failure with last commit, investigating it. Interesting. |
|
yuk, the problem was that |
|
I don't think we should tag PR's like this with "Trivial" I don't have much of an opinion on the actual changes. |
|
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".
|
src/pow.cpp
Outdated
There was a problem hiding this comment.
This case (Genesis block) is never actually necessary?
There was a problem hiding this comment.
GetNextWorkRequired is never called with pindexLast == NULL (as written, not as tested, aka, per the code base)
|
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... |
|
I think the PR is still worth it for the simple fact it adds more information/context to the code. |
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. |
NicolasDorier
force-pushed
the
fixup
branch
from
8eab131 to
dc64a16
Compare
|
utACK dc64a16af92bf459dde32c32cea11b266db54f04 |
|
Needs rebase. |
|
rebased |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
utACK 61b84a0142cd736a2b819b771fc843703ab8ae65 (+/- comment on comment)
src/validation.cpp
Outdated
There was a problem hiding this comment.
(or, generally, by TestBlockValidity)
|
addressed TheBlueMatt comment |
|
Anything missing ? |
|
not sure what to do with this PR. I think it increases readability... but it has been open since september. Do I keep open ? |
|
Yeah let's merge it. Asserting out is better than crashing with a NULL pointer dereference. |
4d51e9b Assert ConnectBlock block and pIndex are the same block (NicolasDorier) 972714c pow: GetNextWorkRequired never called with NULL pindexLast (Daniel Cousens) cc44c8f ContextualCheckBlockHeader should never have pindexPrev to NULL (NicolasDorier) Tree-SHA512: 7cc568bf9417267c335f21ec3d1505b26e56e5b3d5f4d3dbb555279489800aaa65a3bcd7bc376e274dd102912aec16ddbb18de2e2060b2667b41eb979cd9321e
4d51e9b Assert ConnectBlock block and pIndex are the same block (NicolasDorier) 972714c pow: GetNextWorkRequired never called with NULL pindexLast (Daniel Cousens) cc44c8f ContextualCheckBlockHeader should never have pindexPrev to NULL (NicolasDorier) Tree-SHA512: 7cc568bf9417267c335f21ec3d1505b26e56e5b3d5f4d3dbb555279489800aaa65a3bcd7bc376e274dd102912aec16ddbb18de2e2060b2667b41eb979cd9321e
8 participants
if pindexPrev was null, we would get a problem at
pindexPrev->GetMedianTimePast()