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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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)

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 added inline comments.
src/avalanche.cpp
182 ↗(On Diff #5748)

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

deadalnix marked an inline comment as done.

Fix various comments, rebase

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 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 added inline comments.
src/protocol.h
309 ↗(On Diff #5768)

More over,

Bits 24-31 are reserved for temporary experiments.
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

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

rebase and adapt to all change from parents

rebase and adapt to changes made to the stack

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 ↗(On Diff #6041)

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

Check against wrong ordering.

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