Page MenuHomePhabricator
Differential D2117

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

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

Details

Reviewers
Fabien
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING1f50cf0e7ef3: [avalanche] Ensure block added to avalanche have the proper acceptance.
rABC1f50cf0e7ef3: [avalanche] Ensure block added to avalanche have the proper acceptance.
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 created this revision.Nov 22 2018, 13:42
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 22 2018, 13:42
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)
deadalnix updated this revision to Diff 6042.Nov 22 2018, 21:43

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
deadalnix updated this revision to Diff 6047.Nov 23 2018, 00:21

Fix comments

teamcity added a subscriber: teamcity.Nov 23 2018, 00:23

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

teamcity added a comment.Nov 23 2018, 00:31

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

Harbormaster completed remote builds in B4075: Diff 6047.Nov 23 2018, 00:32
deadalnix added a child revision: D2122: [avalanche] Enforce cooldown request present in AvalancheResponse.Nov 23 2018, 02:13
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
deadalnix updated this revision to Diff 6073.Nov 24 2018, 14:52

Fix comments

teamcity added a comment.Nov 24 2018, 14:53

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

teamcity added a comment.Nov 24 2018, 15:01

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

Harbormaster completed remote builds in B4097: Diff 6073.Nov 24 2018, 15:01
deadalnix updated this revision to Diff 6077.Nov 24 2018, 18:00

rebase

teamcity added a comment.Nov 24 2018, 18:01

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

teamcity added a comment.Nov 24 2018, 18:10

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

Harbormaster completed remote builds in B4100: Diff 6077.Nov 24 2018, 18:10
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
jasonbcox added a comment.Nov 25 2018, 07:27

The rest of these changes look good.

deadalnix marked an inline comment as done.Nov 25 2018, 17:00
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.

deadalnix updated this revision to Diff 6090.Nov 25 2018, 17:02

Fix vairable name

teamcity added a comment.Nov 25 2018, 17:04

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

teamcity added a comment.Nov 25 2018, 17:12

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

Harbormaster completed remote builds in B4111: Diff 6090.Nov 25 2018, 17:12
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
deadalnix edited parent revisions, added: D2142: [avalanche] consistently use avanodeid in avalanche's test.; removed: D2046: [avalanche] Implement the challenge/response protocol.Nov 27 2018, 12:59
deadalnix edited the summary of this revision. (Show Details)
deadalnix updated this revision to Diff 6128.Nov 27 2018, 13:05

Add a check for invalid blocks

teamcity added a comment.Nov 27 2018, 13:06

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

teamcity added a comment.Nov 27 2018, 13:15

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

Harbormaster completed remote builds in B4142: Diff 6128.Nov 27 2018, 13:15
Fabien accepted this revision.Nov 27 2018, 13:28
jasonbcox accepted this revision.Nov 27 2018, 18:07
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
Closed by commit rABC1f50cf0e7ef3: [avalanche] Ensure block added to avalanche have the proper acceptance. (authored by deadalnix). · Explain WhyNov 27 2018, 19:01
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
2 lines
44 lines
test/
68 lines

Diff 6042

View Options

src/avalanche.h

Loading...
View Options

src/avalanche.cpp

Loading...
View Options

src/test/avalanche_tests.cpp

Loading...