Page MenuHomePhabricator

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

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



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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Sat, Mar 21, 02:21
deadalnix updated this revision to Diff 17090.Sat, Mar 21, 02:44

Remove unecesary include

deadalnix updated this revision to Diff 17105.Sun, Mar 22, 18:12

Remove leftover print

jasonbcox requested changes to this revision.Mon, Mar 23, 18:17
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
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?

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>

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.

768 ↗(On Diff #17105)

These classes should set __slots__ = ()
This may become relevant if functional tests end up sending thousands of ava messages:

1393 ↗(On Diff #17105)

Ditto re __slots__

This revision now requires changes to proceed.Mon, Mar 23, 18:17
deadalnix added inline comments.Mon, Mar 23, 19:21
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.Mon, Mar 23, 19:22
Fabien added a subscriber: Fabien.
Fabien added inline comments.
33 ↗(On Diff #17105)

neabled => enabled

3471 ↗(On Diff #17105)

AvalancheVote expects an uint32_t

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.

deadalnix added inline comments.Tue, Mar 24, 17:28
180 ↗(On Diff #17105)

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

deadalnix updated this revision to Diff 17154.Tue, Mar 24, 18:12

Address comments

Fabien accepted this revision.Wed, Mar 25, 08:27
jasonbcox accepted this revision.Wed, Mar 25, 18:47
jasonbcox added inline comments.
61 ↗(On Diff #17105)

Not today, no.

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