Page MenuHomePhabricator

[avalanche] Ensure block added to avalanche have the proper acceptance.
ClosedPublic

Authored by deadalnix on Nov 22 2018, 13:42.

Details

Summary

This ensure the state within avalanche is consistent with the outside world.

Depends on D2142

Test Plan

Adapted test cases.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avaaddaccept
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4073
Build 6217: Bitcoin ABC Buildbot (legacy)
Build 6216: arc lint + arc unit

Event Timeline

deadalnix changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Nov 22 2018, 13:42
deadalnix edited the summary of this revision. (Show Details)
deadalnix added a parent revision: Restricted Differential Revision.
deadalnix edited parent revisions, added: D2046: [avalanche] Implement the challenge/response protocol; removed: Restricted Differential Revision.Nov 22 2018, 21:42
deadalnix edited the summary of this revision. (Show Details)

Only poll blocks worth polling.

deadalnix changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Nov 22 2018, 23:26
deadalnix added a child revision: Restricted Differential Revision.
Fabien requested changes to this revision.Nov 22 2018, 23:32
Fabien added a subscriber: Fabien.

Maybe you could also add a test to check that you won't poll an invalid block

src/test/avalanche_tests.cpp
158

New blocks are now considered accepted

170

Please remove the "now" from the comment

315

It is accepted

This revision now requires changes to proceed.Nov 22 2018, 23:32
Fabien requested changes to this revision.Nov 23 2018, 18:37
Fabien added inline comments.
src/test/avalanche_tests.cpp
158 โ†—(On Diff #6047)

nit, remove "are"

315 โ†—(On Diff #6047)

I think it's accepted instead of rejected ?

This revision now requires changes to proceed.Nov 23 2018, 18:37
jasonbcox requested changes to this revision.Nov 25 2018, 07:27
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/avalanche_tests.cpp
34 โ†—(On Diff #6077)

Having a really hard time reading this variable name. Is it supposed to be vraccepter?

223 โ†—(On Diff #6077)

I think we should stick to zero-indexing to prevent future errors. I think AVALANCHE_FINALIZATION_SCORE - 1 reads better.

This revision now requires changes to proceed.Nov 25 2018, 07:27

The rest of these changes look good.

deadalnix added inline comments.
src/test/avalanche_tests.cpp
223 โ†—(On Diff #6077)

The index right now indicate how many confirmations were made. Zero based index represent nothing, so I fail to see haw it makes the code more meaningful in any way.

Fabien requested changes to this revision.Nov 25 2018, 19:57

Don't you want to add a test to ensure that invalid blocks are not polled ?

src/test/avalanche_tests.cpp
34 โ†—(On Diff #6090)

vraccpeted => vraccepted

This revision now requires changes to proceed.Nov 25 2018, 19:57

Add a check for invalid blocks

jasonbcox added inline comments.
src/test/avalanche_tests.cpp
223 โ†—(On Diff #6077)

I guess that makes sense.

This revision is now accepted and ready to land.Nov 27 2018, 18:07
This revision was automatically updated to reflect the committed changes.