NO-TICK: Add capability of turning Block into FinalizedBlock.#172
Conversation
This is necessary for node syncing/joining catch-up mechanism. We want to download `Block`(s) (linear chain blocks) and execute them.
ProtoBlock hash may be a default `Digest` for finalized blocks created out of the linear chain blocks while height is universal and the same on all nodes.
afck
left a comment
There was a problem hiding this comment.
Approving… but I'm more and more convinced that it was a mistake to add parent_hash to ProtoBlock in the first place. Can we remove it?
afck
left a comment
There was a problem hiding this comment.
Awesome! Looks good to me!
(Although I'm sure we'll realize tomorrow why this was a mistake. 😅)
You could probably even re-enable a too_many_arguments lint somewhere now.
Deploy buffer shouldn't warn about this case b/c it's never a replay attack (no deploys).
You were right, I hit the same problem I had when I tried to fix the node over the weekend – deploy buffer was logging I added a logic to detect these cases and not log errors. See 3082e05. If you're still OK with merging it, let me know. |
| // TODO: Events are not guaranteed to be handled in order, so this could happen! | ||
| error!("finalized block that hasn't been processed!"); | ||
| let is_empty_proto = block == ProtoBlock::empty_random_bit_false() | ||
| || block == ProtoBlock::empty_random_bit_true(); |
There was a problem hiding this comment.
Why not define some block.is_empty() helper function?
afck
left a comment
There was a problem hiding this comment.
Yes, if I understand correctly I'm still fine with merging this: Those error messages didn't really make sense, did they?
I think the complication that removing the parent hash will add is that we now will need to give a list of not-yet-finalized ancestors to the deploy buffer after all, instead of just one, because it points to its ancestors anyway?
Yes, for the scenario that I am describing they didn't make any sense.
At the moment, deploy buffer is given |
There was a problem hiding this comment.
Actually, do you mind if we talk about this again tomorrow, before merging? I'm still pretty confused.
Is the issue really about empty blocks specifically? Or did it just show up because all empty blocks are equal? And it will happen for other equal proto blocks, too — which can happen legitimately, because two sibling blocks are allowed to have the same value?
(If someone else approves it, feel free to merge it, of course! I'm just currently unable to wrap my head around this. 😅)
|
It's only about empty proto blocks – although there are two variants of empty proto block:
If two proto blocks like this are created in a row then the error will be logged. |
Co-authored-by: JC <jc@pnkr.io>
This is necessary for node syncing/joining catch-up mechanism. We want to download
Block(s) (linear chain blocks) and execute them.