Paths

Table of Contentst

[avalanche] Delegate node management to caller
ClosedPublic
Actions

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

Details

Reviewers

jasonbcox
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING80acc8fd3b66: [avalanche] Delegate node management to caller
rABC80acc8fd3b66: [avalanche] Delegate node management to caller

Summary

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

Repository

rABC Bitcoin ABC

Branch

avaselectnodes

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4137
Build 6345: Bitcoin ABC Buildbot (legacy)
Build 6344: arc lint + arc unit

Event Timeline

deadalnix created this revision.Nov 26 2018, 23:39

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 26 2018, 23:39

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B4137: Diff 6121!Nov 26 2018, 23:46

jasonbcox edited the summary of this revision. (Show Details)Nov 27 2018, 00:34

jasonbcox requested changes to this revision.Nov 27 2018, 00:51

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/avalanche.cpp
80	tha -> that
84	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	Probably best to introduce a constant called INVALID_NODE or NODE_UNKNOWN for `-1`.
295	use the constant here

This revision now requires changes to proceed.Nov 27 2018, 00:51

deadalnix marked an inline comment as done.Nov 27 2018, 13:37

deadalnix added inline comments.

src/avalanche.cpp
84	Why would it get stuck ?

Fix typo

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B4143: Diff 6129!Nov 27 2018, 13:46

Fabien edited the summary of this revision. (Show Details)Nov 27 2018, 14:03

deadalnix added a child revision: D2143: [avalanche] Expire queries once they reached their timeout..Nov 27 2018, 15:29

Fabien added a subscriber: Fabien.Nov 27 2018, 17:23

Fabien added inline comments.

src/avalanche.cpp
79 ↗	(On Diff #6129)	recieved => received

jasonbcox added inline comments.Nov 27 2018, 17:58

src/avalanche.cpp
84	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.

Back on your queue because tests fail.

src/avalanche.cpp
276	I still think this should be a constant.
295	ping

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

Add NO_NODE constant instead of using magic numbers.
Fix typos

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4165: Diff 6161.Nov 29 2018, 15:11

Fabien added inline comments.Nov 29 2018, 15:52

src/avalanche.h
39 ↗	(On Diff #6161)	What do you think of moving this to net.h ? This definition could be useful outside of avalanche.

deadalnix marked an inline comment as done.Nov 29 2018, 16:04

deadalnix added inline comments.

src/avalanche.h
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.