Fix mem access violation merkleblock

[Christewart]

Fixing a possible memory access violation in CPartialMerkleTree::CalcHash().

This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them.

This was found with #8469, specifically using this generator which will cause a memory access violation on this test case.

[dcousens]

If it shouldn't happen, maybe an assert is better?

[pstratem]

@TheBlueMatt what was the behavior before if the elements list was empty?

[pstratem]

wait no that's @sipa

[pstratem]

nvm it returned 0 so this maintains the original behavior

[TheBlueMatt]

Probably best to assert that vTxid has non-zero length, as that would represent generating a tree for an invalid block.

[Christewart]

Changed to what @dcousens and @TheBlueMatt suggested. I think that makes sense at the end of the day. Also, from what I can tell, that assert cannot be hit from the p2p code.

[NicolasDorier]

I would have set fBad, so we don't risk later to reach a network reachable assert(0) by inadvertance. :)

[jnewbery]

Looks good. Lightly tested ACK 51802cc

if you're adding an assert, can you put some commenting in explaining the assumptions that you're testing (either around the assert or the function declaration).

Also please remove the merge commits from this PR.

EDIT: cool that this was found using your property-based testing. I've been meaning to review #8469 for a while.

[Christewart]

@jnewbery Addressed the concerns you had.

If you want to chat about 8469 don't be afraid to ping me on irc. I think it will allow us to streamline testing AND exhaustively test invariants better than we are now.

[NicolasDorier]

why no just setting fBad and returning ? I really don't like having an assert, this code might be reached from the network one day for some reason (maybe unsuspecting new feature). Putting an assert here is a tragedy waiting to happen.

[jnewbery]

@NicolasDorier - I had the same concerns as you, so I spent some time reading around the code to convince myself that this was ok. This code is only called from the CMerkleBlock constructor, which is only called from the gettxoutproof RPC or in response to a MSG_FILTERED_BLOCK getdata request. In both cases, we're constructing the Merkle Block from a block that's already in our mapBlockIndex. Therefore to hit this assert, we'd need there to be an invalid block in the mapBlockIndex, which would mean:

• our internal data is corrupted; and
• we've probably fallen pretty badly out of consensus

in which case, asserting is appropriate.

As for concerns about future code calling this without knowing about the assert - they'd have to be constructing a merkle block for a block that they hadn't already validated, which is a very bad idea! Just to make sure, Chris has now also added a comment above the assert to clarify assumptions.

Hope that allays your concerns!

[TheBlueMatt]

utACK bbdd771, though it would be nice to add a comment to CMerkleBlock noting that the block containing transactions will be asserted-on.

[Christewart]

@TheBlueMatt Added the comment, is this ready for merge?

EDIT: Also rebased

[TheBlueMatt]

Wait, what is asserted? Also note extra space at EOL here.

[gmaxwell]

utACK.

[TheBlueMatt]

utACK 8276e70

[sipa]

utACK 8276e70