[F104] Deduplicates requested operation IDs before cloning/assembling responses

[peterjah]

• 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

[bilboquet]

That looks good.
But I think we could catch this issue earlier, in massa-protocol-worker/src/handlers/block_handler/messages.rs:287 in

impl Deserializer<BlockMessage> for BlockMessageDeserializer {
    fn deserialize<'a, E: ParseError<&'a [u8]> + ContextError<&'a [u8]>>()

applying the same principle, Operations deserializer would become something like:

BlockInfoType::Operations => self
    .operation_ids_deserializer
    .deserialize(rest)
    .map(|(rest, operation_ids)| {
        let deduplicated: Vec<OperationId> = operation_ids
            .into_iter()
            .collect::<PreHashSet<_>>()
            .into_iter()
            .collect();
        (rest, AskForBlockInfo::Operations(deduplicated))
    }),

what do you think?

[Leo-Besancon]

Second opinion: I think we could go with @bilboquet's approach if we don't intend to punish the peer for duplicated ops.
(in the issue recomandations: optionally treat excessive duplicates as a peer offense). If we want to punish our peer, I think the current location of this dedup makes more sence.

I could go either way personally.

[bilboquet]

Reading my comment a second time, I realize I was not clear.
It was not a counter proposal: I was suggesting to do it in both places.

EDIT: never mind, that's dumb. My "proposal for an early filter" would prevent any action against the guilty peer.
So it's probably better to keep the PR version and decide what to do with peer.
(thanks leo for the remark)