fix: don't unpin blocks that may show up again

[jsdw]

This builds on #1367.

The intention behind the original unpin logic is that blocks that will never show up again in any future event can now be safely unpinned. The logic was flawed though; we'd unpin all flagged blocks when a finalized event comes in, even if some of those may later appear in another finalized event.

The intention behind this fix is to track of which of our pinned blocks have been pruned by the backend, and only actually unpin those blocks for real, leaving other blocks pinned until they eventually are pruned by the backend too.

[lexnv]

Looks good to me! I think one adjustment should be made to never_unpin_new_block_before_finalized to make it pass (drop(_i0) the initialized event)

Just to double check I got this right, we store an extra bit of information in the pinning details, the can_be_unpinned.
We then set can_be_unpinned = true for Finalized and Pruned events. Then use this flag in combination with the UnpinFlags to make a call to chainHead_unpin.

And this works for the following edge-case:

• NewBlockRef kept around
• FinalizedBlockRef (can_be_unpinned = true) and block ref dropped immediately -> hash is not added to the UnpinFlags yet
• NewBlockRef is used as normal; then dropped and this time is added to the UnpinFlags
• the next finalized event will clear out the block generating an unpin

In other words, this is safe for NewBlock references that outlive Finalized references 👍

[tadeohepperle]

Great job figuring this out, the entire pinning/unpinning API feels quite complicated.

[jsdw]

Looks good to me! I think one adjustment should be made to never_unpin_new_block_before_finalized to make it pass (drop(_i0) the initialized event)

I had a general pass over this test just now to clarify it all a little so hopefully it makes sense now :)

Just to double check I got this right, we store an extra bit of information in the pinning details, the can_be_unpinned.
We then set can_be_unpinned = true for Finalized and Pruned events. Then use this flag in combination with the UnpinFlags to make a call to chainHead_unpin.

Yup; so we should only actually unpin blocks that we've flagged we are no longer using (ie in the UnpinFlags set) and that are marked as can_be_unpinned (because we don't expect them to occur meaningfully in any future events now (the exception is that they can come up as "parent hashes" of blocks, but it'd be a pain to handle that properly so we haven't bothered).

NewBlockRef kept around

It should be yup (there's a test for this)

FinalizedBlockRef (can_be_unpinned = true) and block ref dropped immediately -> hash is not added to the UnpinFlags yet

I think this will be ok; pinned but finalized blocks should now have can_be_unpinned: true set, and so when they are eventually dropped, then when the next finalization event occurs we'll see that they can be properly unpinned now and tell the backend to unpin.

NewBlockRef is used as normal; then dropped and this time is added to the UnpinFlags

These blocks should now continue to be kept pinned until they are finalized or pruned, at which point they will now be unpinned on the next finalized event after that.

Great job figuring this out, the entire pinning/unpinning API feels quite complicated.

Yeah; it's a bit hairy for sure; the goal is to hide this hairyness away behind this abstraction and hopefully everything above it should "just work" without needing to fuss over any details (but of course, if we can simplify it then that'd be great too!) :)

[niklasad1]

I'm not a fan of the naming pin_unpinnable_block_at it just confuses me but I don't have any good suggestion for it... maybe I'm just not used to this language.

[jsdw]

Yeah; I couldn't think of a good name either! I'm trying to say "pin a block that can be unpinned" versus "pin a block that can't yet be unpinned"

[niklasad1]

I would prefer an enum here called BlockStatus such as:

enum BlockStatus {
  /// The block has been seen in a finalized block or a pruned block, it's ok to be unpinned and future events are of no interest
  Processed
  /// Waiting for the block to be finalized or pruned.
  Pending
}

[jsdw]

I guess I feel like the boolean is clear enough here (can the block be unpinned; true or false), and then the functions you call to pin a block hide it anyway (but their name is another issue :D). So maybe having a nenum and only one pin function combined might be clearer hmmm

[jsdw]

I had a quick go at adding an enum and it felt messier to me offhand so I left it as a bool for now :)

[niklasad1]

The logic looks good to me but not sure what's going on in the CI/tests..