Improve reindex_chain readability and small fixes

[JoseSK999]

A few suggested changes:

1. Better readability for reindex_chain
2. It seems that maybe_reorg is saving the branch tip with its parent height, so I add 1
3. accept_header should perhaps return if there's a database error
4. verify_block_transactions should just return Ok(()), instead of Ok(true), since any other outcome is returned as Err

I also found an issue in validate_block:
We check if block.header.prev_blockhash != prev_block.block_hash(). But prev_block is fetched with header.prev_blockhash, so this check always passes! I don't know what is the desired behavior here.

[Davidson-Souza]

Hi @JoseSK999 thanks for the PR. Sorry for the delay, I was traveling this last week, but I should take a loot at this tomorrow.

[Davidson-Souza]

I also found an issue in validate_block:
We check if block.header.prev_blockhash != prev_block.block_hash(). But prev_block is fetched with header.prev_blockhash, so this check always passes! I don't know what is the desired behavior here.

You're right, this is completely broken. Nice catch!

Btw, the CI error is totally unrelated.

[JoseSK999]

Thanks! Should that part be removed or we need to check whether the block extends an orphan chain?

[Davidson-Souza]

I think it's important to check if this block is indeed the next in our chain. I think the right way to do that is to use the validation_index. block_header.prev_block should be equal to validation_index.

[JoseSK999]

Thanks, I will try to do it this weekend (traveling rn as well)

[JoseSK999]

@Davidson-Souza I think we are already checking if the block is the next in the chain in connect_block. The block is validated only if it's HeadersOnly and if its height is the same as self.get_validation_index()? + 1. So it must be the case that the block to validate extends the last validated one (we check the previous block hash before storing the header's height).

Actually there's a small issue in get_validation_index: we fetch the validation_index disk header, so it can only be FullyValid. Right now we are also checking if it's HeadersOnly, I think we should panic in this case as well.

[Davidson-Souza]

Yes, you're right. We'll never call validate_block in a block that's not the next one. I'm ok with just removing this check in validate_block at all.

[JoseSK999]

Done!

[Davidson-Souza]

ACK Davidson-Souza@06f3835