[F26] fix(consensus-worker): preserve pruned future/dep-waiting block…

[peterjah]

…s as Discarded and harden mark_invalid_block

• document all added functions
• try in sandbox /simulation/labnet
  • if part of node-launch, checked using the resync_check flag
• unit tests on the added/changed features
  • make tests compile
  • make tests pass
• add logs allowing easy debugging in case the changes caused problems
• if the API has changed, update the API specification

[Leo-Besancon]

Same as the other consensus-related PR, I'm not doing deep reviews. The implementation looks fine to me.

[bilboquet]

That looks good to me.
As Léo asked Damir to review, let's wait a bit.
If Damir has to much to do, I think that we could approve this PR.

[Leo-Besancon]

LGTM, waiting on other reviews (ping @damip) before merging

[damip]

Two unrelated things in here, they get different answers.

1. mark_invalid_block no-op when consensus has no entry: correct, keep it, and it matters more than the issue says. This is not a race on a stale wishlist entry, it is the normal missing-parent flow. Consensus wishlists a parent P it has never seen (P -> None in get_block_wishlist, nothing in blocks_state). Protocol fetches P's header and keeps it to itself (wishlisted headers are not registered to consensus), then the op list, then the ops. If the committed ops exceed the max block size, gather_missing_block_ops calls mark_block_as_invalid(P) -> mark_invalid_block(P) -> transition_map on None -> panic!("Invalid transition from None to Discarded"). Anyone can sign P (no draw check on a zero-endorsement header) and put whatever they want in its op list. A staker needs one slot to send a header with P as parent to each node, and every node that got it dies. Please say that in the PR description and add a test: mark invalid a block consensus never had, no panic.

2. Turning size evictions into Discarded(Stale): please revert, both in prune_slot_waiting and in prune_waiting_for_dependencies, and put back Option<DiscardReason>. Details inline. Short version: Discarded is a verdict, it is terminal and it is inherited by every child (check_header discards a block whose parent is Discarded). A size eviction is not a verdict. The "blackhole" from the issue is a one-slot delay: the forgotten block is refetched through the wishlist as soon as a child references it. With Discarded(Stale) the child is discarded instead, so the change removes the recovery path it was supposed to protect. No upside.

To be clear on severity: in practice eviction only happens in states where the node is already lost (over 2048 waiters means 32+ periods of backlog, consensus catch-up is dead after force_keep_final_periods = 5 anyway, the node ends up in NeedSync either way). So this is not a "will desync nodes" objection, it is a "wrong direction, zero benefit, don't" objection.

[peterjah]

@damip @Leo-Besancon @bilboquet
Suggested comments and tests added

[Leo-Besancon]

LGTM

[bilboquet]

LGTM

Dismissed as stale