Slightly simplify some BlockParser logic #162
Slightly simplify some BlockParser logic #162
Conversation
Also add TODO about concern in validating block header vs just block height.
utACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the check for the blockHash as well?
return !bsqStateService.getBlockAtHeight(rawBlock.getHeight()).isPresent(); | ||
private boolean isBlockAlreadyAdded(RawBlock rawBlock) { | ||
// TODO(chirhonul): shouldn't we verify that we know about the blockHash, not just that the | ||
// block heights are the same? how do we handle chainsplits otherwise? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems beyond the scope of the current PR (which is a refactor with no functional changes), which is why I added it as a TODO
here. I'd be happy to address it in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Thanks for the review! @ManfredKarrer: I'd prefer to change the functionality of Edit: I forgot to mention that I believe that the failing CI build is unrelated to this change.. the error is:
I would rerun the Travis job, but I seem to lack the permissions to do so. |
I've made the changes mentioned as a I will open the second PR when this one has been merged. |
utACK |
Also add TODO about concern in validating block header vs just
block height.