Details

Reviewers

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

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.May 21 2021, 06:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 21 2021, 06:53

PiRK requested review of this revision.May 21 2021, 06:53

Harbormaster completed remote builds in B15775: Diff 28562.May 21 2021, 07:11

PiRK mentioned this in D9509: [avalanche] Send the proof master pubkey with AVAHELLO.May 21 2021, 07:24

Fabien added a subscriber: Fabien.May 26 2021, 07:42

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

mingsu awarded a token.May 26 2021, 08:37

rebase

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

Harbormaster completed remote builds in B15810: Diff 28612.May 26 2021, 13:19

deadalnix added a subscriber: deadalnix.May 27 2021, 14:01

deadalnix added inline comments.

src/net_processing.cpp
3939 ↗	(On Diff #28612)	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.

Harbormaster completed remote builds in B15834: Diff 28644.May 27 2021, 15:31

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.

Harbormaster completed remote builds in B15838: Diff 28657.May 28 2021, 08:57

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