search

diwakergupta / stacks-blockchain-tob-audit

GNU General Public License v3.0
0 stars 0 forks source link

Typo in implementation of compare_neighbor_uptime_health #24

Open bradlarsen opened 3 years ago

bradlarsen commented 3 years ago

On line 125 in the compare_neighbor_uptime_health function, a typo in the comparison causes the function to not work as intended:

https://github.com/trailofbits/x-audit-blockstack-core/blob/d35ef465e9fa2ce327a181117f8ca7933b9df075/src/net/prune.rs#L109-L149

Presumably if uptime_bucket_1 > uptime_bucket_2 was intended there.

This has been present since commit cd2e715d7789e09dd64f744959ad7f290e75d3b6 from May 2, 2019.

This could possibly have a detrimental effect on maintenance of a Stacks node's peer network by putting peers with wildly different uptimes into the same bucket.

diwakergupta commented 3 years ago

@bradlarsen thanks, we'd already discovered this recently and @jcnelson is fixing it in an upcoming PR.

Out of curiosity, are you running rust-clippy to uncover issues like this and e.g. #26 ? I ask because if yes, we can make that pass ourselves -- I think we'd prefer to preserve your attention for more substantive issues that linters might miss 🙏🏽

bradlarsen commented 3 years ago

@diwakergupta: Yes, this and #26 were pointed out with assistance from clippy. It's one of the quick / mechanical checklist items we like to do early on during an audit, and certainly not all that we do.

Your point regarding substantive issues is well-taken. @ESultanik and I are focusing on higher-level concerns specifically relevant to leader election in the remaining time.