Page MenuHomePhabricator

[avalanche] verify signature on avahello reception
ClosedPublic

Authored by PiRK on May 21 2021, 06:53.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC819d42939969: [avalanche] verify signature on avahello reception
Summary

Verify the peer's signature using his delegated pubkey.
Add functional tests for the AvaHello handshake sequence.
Add a missing return at the end of the AvaHello branch in net_processing.cpp

Depends on D9564

Test Plan

ninja check-functional

Event Timeline

PiRK requested review of this revision.May 21 2021, 06:53
Fabien added inline comments.
src/net_processing.cpp
4025 ↗(On Diff #28562)

Out of scope here but at some point we might want a dedicated BCLog::AVALANCHE category

test/functional/abc_p2p_avalanche.py
119 ↗(On Diff #28562)

Nit: if this is only use in send_avahello then you can define it there and avoid the leading _ which is not consistent with what is done in the codebase

412 ↗(On Diff #28562)

Style nit: you can use interface.wait_for_disconnect() to test the ban

rebase

Simplify TestNode.sendHello. Use waitfordisconnect to check banning behavior (reduces the test fragility if the logging changes)

deadalnix added inline comments.
src/net_processing.cpp
3939

This might become very spammy, no? In any case, this doesn't really help with anything because it doesn't have the relevant information to figure out what is going on.

remove logging on successfull signature verification. This makes it a difficult to test the success in the functional test. For the time being, checking that the good p2p interface is still connected after the bad one is disconnected is better than no test at all.

Improve the check that the good interface is still connected, using sync_with_ping. This raises AssertionError if no pong is received in response to ping.

Fabien requested changes to this revision.May 31 2021, 07:11
Fabien added inline comments.
test/functional/abc_p2p_avalanche.py
403 ↗(On Diff #28657)

Please do both: check the node is disconnected AND the reason it is disconnected. Otherwise you're not sure what you are testing, here you are looking for invalid-avahello-signature but you if you get an invalid-delegation the test will still pass.

408 ↗(On Diff #28657)

Move next to the good interface test instead of having it dandling

This revision now requires changes to proceed.May 31 2021, 07:11

address review:

  • move sync_and_ping next to the corresponding code
  • add test for log message again to verify the reason the node is banned for
This revision is now accepted and ready to land.May 31 2021, 08:41