Page MenuHomePhabricator

[avalanche] Add handling of ava_poll command in the network layer
ClosedPublic

Authored by deadalnix on Mar 21 2020, 02:21.

Details

Summary

This is not quite suffiscient by itself as it has no DoS protection, but can answer queries about blocks.

In order to avoid any problem, we make sure that it is gated behind a flag that is turned off by default.

Test Plan

Added an integration test that poll the node and check answers.
ran bitcoind -help to verify avalanche option do not show up.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avanetwork
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9957
Build 17768: Default Diff Build & Tests
Build 17767: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Mar 23 2020, 18:17
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/avalanche.cpp
180 ↗(On Diff #17105)

Looking at this function by itself, it looks like this should be a reference. But I see the only call site is initializing it in-place. I'm still relatively new to rvalue references, so is this the right place to use one? If not, why?

src/init.cpp
1025 ↗(On Diff #17105)

Ideally this would be declarative similar to -enablebip61: -enableava
But I can see the value in having the help text for ava be grouped together or easily greppable.

Perhaps the only improvement I can offer is -avaenable to keep the tense consistent.
Over time, we can move towards a format of -<group><attribute>

test/functional/abc-p2p-avalanche.py
29 ↗(On Diff #17105)

Since we expect ava to respond quickly, isn't a 60 second timeout way larger than expected, even during a failure? From experience, I know waiting a full minute unnecessarily for a test to fail is a burden on debugging. Although not universal, we have reduced timeouts in some tests due to this.

47 ↗(On Diff #17105)

Call it what it is rather than "fake node": "node interface"

61 ↗(On Diff #17105)

Something like this as assert_ava_response would be useful in the test_framework utils IMO, but that can wait until we have more tests like this one.

69 ↗(On Diff #17105)

Give names to the error codes, at least within this test.

test/functional/test_framework/messages.py
768 ↗(On Diff #17105)

These classes should set __slots__ = ()
This may become relevant if functional tests end up sending thousands of ava messages: https://docs.python.org/2.5/ref/slots.html

1393 ↗(On Diff #17105)

Ditto re __slots__

This revision now requires changes to proceed.Mar 23 2020, 18:17
test/functional/abc-p2p-avalanche.py
61 ↗(On Diff #17105)

How do you know this is useful in the utils? Is there some other piece of code that do need it?

Fabien requested changes to this revision.Mar 23 2020, 19:22
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/avalanche.h
33 ↗(On Diff #17105)

neabled => enabled

src/net_processing.cpp
3471 ↗(On Diff #17105)

AvalancheVote expects an uint32_t

test/functional/test_framework/messages.py
795 ↗(On Diff #17105)

Note that this is treated as an int while the node is treating it as an unsigned.
This is not really an issue since you don't use any "meaningful" unsigned values that would change the sign, but it's maybe a good idea to use an int everywhere, or use constants for the -1, 0, 1 values.

src/avalanche.cpp
180 ↗(On Diff #17105)

Which is a perfectly good reason to not use them when not strictly needed.

jasonbcox added inline comments.
test/functional/abc-p2p-avalanche.py
61 ↗(On Diff #17105)

Not today, no.

This revision is now accepted and ready to land.Mar 25 2020, 18:47