[Cleanup] Discrepancy in block request removals.

[acoglio]

There is a discrepancy between remove_block_request_to_peer() (singular 'request') and remove_block_requests_to_peer() (plural 'requests') in how they treat a particular situation. (I would expect the two to do the same thing for each height, the singular just for the given height, and the plural for all heights.)

The situation:

• The requests map has an entry for height h with a singleton set {ip} of peer socket addresses.
• The responses map has an entry for height h with some block.

The discrepancy:

• The "singular" function, when called on h, removes the h entry from the requests map, because:
  • peek_next_block() returns None, because is_request_complete is false — the ip has not been removed yet.
  • can_revoke is thus true.
  • After removing the ip from the map, can_revoke stays true.
• The "plural" function, when deciding whether to retain or remove the h entry from the requests map, it retains it, because:
  • retain is true, because the responses map has a block at h.

[joske]

@acoglio which behaviour do you consider correct?

[acoglio]

I'm not sure which behavior is better. I haven't done a detailed study of the liveness properties of the implementation yet, which is what these behaviors may affect (not safety). Perhaps others have more context/ideas on this.