[Bug] Block locator check has incorrect assumption

[HarukaMa]

🐛 Bug Report

I noticed this when I was reading the code:

https://github.com/AleoHQ/snarkOS/blob/dbca32ddfb55853f1131c74b53086ad41e6c3d3c/node/messages/src/helpers/block_locators.rs#L201-L203

Seems like this check is assuming RECENT_INTERVAL is 1. If the interval is not 1, then that check will conflict with:

https://github.com/AleoHQ/snarkOS/blob/dbca32ddfb55853f1131c74b53086ad41e6c3d3c/node/messages/src/helpers/block_locators.rs#L194-L196

and when the latest height is less than RECENT_INTERVAL * NUM_RECENTS, no recent_blocks could pass the check except empty BlockLocators.

EDIT: also

https://github.com/AleoHQ/snarkOS/blob/dbca32ddfb55853f1131c74b53086ad41e6c3d3c/node/messages/src/helpers/block_locators.rs#L191-L193

seems superfluous to me as we already have interval checks.

Steps to Reproduce

Didn't really test this.

Expected Behavior

The check should be ... != last_height * RECENT_INTERVAL.

Your Environment

N/A

[acoglio]

Indeed, various parts of the code implicitly assume that RECENT_INTERVAL is 1. In PR #3495, I've marked all those spots with TODOs.

The check *current_height <= last_height ensures that the subtraction *current_height - last_height in the interval check does not wrap around. Block locators are untrusted data coming from the outside and so the recents map may have out-of-order keys.