Page MenuHomePhabricator

[avalanche] Implement the challenge/response protocol
ClosedPublic

Authored by deadalnix on Nov 11 2018, 19:52.

Details

Summary

This implement the voting mechanism, with pools and responses and account for the votes properly. The peer selection is not done at this point in time.

Depends on D2045

Test Plan
make check

Updated tests and added new ones to reflect the added fonctionalities.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
peerpolling
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4072
Build 6215: Bitcoin ABC Buildbot (legacy)
Build 6214: arc lint + arc unit

Event Timeline

deadalnix created this revision.Nov 11 2018, 19:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 11 2018, 19:52
deadalnix changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Nov 11 2018, 19:52
deadalnix edited the summary of this revision. (Show Details)
deadalnix updated this revision to Diff 5758.Nov 12 2018, 00:07

Add test that ensures the event loop is sceduled properly and does its job.

jasonbcox requested changes to this revision.Nov 12 2018, 00:15
jasonbcox added a subscriber: jasonbcox.

Requesting changes only for the sanity check. Everything else is optional considering the timeline.

src/avalanche.cpp
77 ↗(On Diff #5748)

Needs check for vr == w.end() for sanity

182 ↗(On Diff #5748)

Does there need to be fuzzing so that the same node isn't selected every time?

219 ↗(On Diff #5748)

remive -> remove

220 ↗(On Diff #5748)

enevr -> never

src/avalanche.h
160 ↗(On Diff #5748)

quesries -> queries

162 ↗(On Diff #5748)

RequestReccord -> RequestRecord

src/test/avalanche_tests.cpp
317 ↗(On Diff #5748)

ti -> it

This revision now requires changes to proceed.Nov 12 2018, 00:15
deadalnix marked an inline comment as done.Nov 12 2018, 02:00
deadalnix added inline comments.
src/avalanche.cpp
182 ↗(On Diff #5748)

There is no node selection for now, so that wouldn't fuzz anything :)

deadalnix updated this revision to Diff 5766.Nov 12 2018, 12:23
deadalnix marked an inline comment as done.

Fix various comments, rebase

deadalnix updated this revision to Diff 5767.Nov 12 2018, 13:00

ava_vote => ava_votes

deadalnix updated this revision to Diff 5768.Nov 12 2018, 13:04

Fix comment

jasonbcox requested changes to this revision.Nov 12 2018, 14:22
jasonbcox added inline comments.
src/protocol.h
309 ↗(On Diff #5768)

We should use (1 << 9) since BU is using 6,7,8 for Graphene, WeakBlocks, and compact block filters:

    // NODE_GRAPHENE means the node supports Graphene blocks
    // If this is turned off then the node will not service graphene requests nor
    // make graphene requests
    NODE_GRAPHENE = (1 << 6),

    // Bits 24-31 are reserved for temporary experiments. Just pick a bit that
    // isn't getting used, or one not being used much, and notify the
    // Bitcoin Unlimited devevelopement team. Remember that service bits are just
    // unauthenticated advertisements, so your code must be robust against
    // collisions and other cases where nodes may be advertising a service they
    // do not actually support. Other service bits should be allocated via the
    // BUIP process.

    NODE_WEAKBLOCKS = (1 << 7),

    // NODE_CF indicates the node is capable of serving compact block filters to SPV clients.
    NODE_CF = (1 << 8)
};
This revision now requires changes to proceed.Nov 12 2018, 14:22
deadalnix requested review of this revision.Nov 12 2018, 14:46
deadalnix marked an inline comment as done.
deadalnix added inline comments.
src/avalanche.cpp
77 ↗(On Diff #5748)

Bracket create the element if missing, so it's never end(). But this whole thing is wong anyways.

src/protocol.h
309 ↗(On Diff #5768)

BU has a big problem designing it protocol. They use service bits like there is an infinite supply of them. Graphene doesn't require a service bit. I talked to George about this.

deadalnix marked an inline comment as done.Nov 12 2018, 14:50
deadalnix added inline comments.
src/protocol.h
309 ↗(On Diff #5768)

More over,

Bits 24-31 are reserved for temporary experiments.
jasonbcox added inline comments.Nov 12 2018, 19:09
src/protocol.h
309 ↗(On Diff #5768)

This is what I'm concerned about: https://reviews.bitcoinabc.org/D2050#inline-10963 I think it's safer to just use (1 << 9) as there's no downside to doing this.

jasonbcox requested changes to this revision.Nov 12 2018, 22:06
This revision now requires changes to proceed.Nov 12 2018, 22:06
deadalnix added a child revision: Restricted Differential Revision.Nov 13 2018, 00:42
deadalnix updated this revision to Diff 5785.Nov 13 2018, 00:52

Use bit 24 instead of 6 as it in the experiment range and this is clearly still in flight.

schancel accepted this revision.Nov 13 2018, 21:16
deadalnix updated this revision to Diff 6003.Nov 22 2018, 00:00

rebase and adapt to all change from parents

deadalnix updated this revision to Diff 6040.Nov 22 2018, 21:12

rebase and adapt to changes made to the stack

deadalnix updated this revision to Diff 6041.Nov 22 2018, 21:21

pfrom => pnode

deadalnix removed a child revision: Restricted Differential Revision.Nov 22 2018, 23:27
deadalnix changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Nov 23 2018, 01:48
Fabien requested changes to this revision.Nov 23 2018, 17:04
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/test/avalanche_tests.cpp
441

You may want to test against wrong ordering if there is more than 1 block

This revision now requires changes to proceed.Nov 23 2018, 17:04
deadalnix updated this revision to Diff 6072.Nov 24 2018, 14:05

Check against wrong ordering.

Fabien accepted this revision.Nov 24 2018, 14:26
jasonbcox accepted this revision.Nov 24 2018, 16:28
This revision is now accepted and ready to land.Nov 24 2018, 16:28
This revision was automatically updated to reflect the committed changes.