Page MenuHomePhabricator

[avalanche] Delegate node management to caller

Authored by deadalnix on Nov 26 2018, 23:39.



So far, AvalancheProcessor tried to find apropriate nodes by itself. This diff delegates the responsability to the caller, which will add peer usables for avalanche using the addPeer method.

Now peers are stored in a datastructure tracking the data relevent for the peer and stored in a multi_index in order to be able to query quickly the nodes that are available for polling.

Depends on D2137

Test Plan

Updated tests to reflect behavioral changes.

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

jasonbcox requested changes to this revision.Nov 27 2018, 00:51
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
80 ↗(On Diff #6121)

tha -> that

84 ↗(On Diff #6121)

This needs to enforce a minimum cooldown of at *least* AVALANCHE_TIME_STEP_MILLISECONDS + 1 (or some other, safer value) so that runEventLoop doesn't get stuck looping forever.

276 ↗(On Diff #6121)

Probably best to introduce a constant called INVALID_NODE or NODE_UNKNOWN for -1.

295 ↗(On Diff #6121)

use the constant here

This revision now requires changes to proceed.Nov 27 2018, 00:51
deadalnix added inline comments.
84 ↗(On Diff #6121)

Why would it get stuck ?

Fabien added inline comments.
79 ↗(On Diff #6129)

recieved => received

84 ↗(On Diff #6121)

I misread. I was thinking that a small enough cooldown in an AvalancheResponse could get the loop in runEventLoop() to execute on the same nodeId multiple times. Looks like the test at // Running two iterration of the event loop so that vote gets triggerd on A // and B. covers this case though, so we're good.

jasonbcox requested changes to this revision.Nov 27 2018, 22:23

Back on your queue because tests fail.

276 ↗(On Diff #6121)

I still think this should be a constant.

295 ↗(On Diff #6121)


This revision now requires changes to proceed.Nov 27 2018, 22:23

Add NO_NODE constant instead of using magic numbers.
Fix typos

39 ↗(On Diff #6161)

What do you think of moving this to net.h ? This definition could be useful outside of avalanche.

deadalnix added inline comments.
39 ↗(On Diff #6161)

That convention is not used through the codebase. If it become the case, then yes. Until then, defining things with a limited scope is better.

This revision is now accepted and ready to land.Nov 29 2018, 17:08
This revision was automatically updated to reflect the committed changes.