Page MenuHomePhabricator
Differential D5701

[avalanche] Process AvalancheResponse
ClosedPublic
Actions

Authored by deadalnix on Apr 10 2020, 23:04.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING919980f616e5: [avalanche] Process AvalancheResponse
rABC919980f616e5: [avalanche] Process AvalancheResponse
Summary

Dry run only for now.

Test Plan

Updated unit tests to do several poll/response cycles.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix created this revision.Apr 10 2020, 23:04
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 10 2020, 23:04
teamcity added a comment.Apr 10 2020, 23:05

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Apr 10 2020, 23:17

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10173: Diff 18737.Apr 10 2020, 23:17
jasonbcox requested changes to this revision.Apr 11 2020, 00:04
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/net_processing.cpp
3431 ↗(On Diff #18737)

Consider using an intermediate variable for this value and using it in both places.

3454 ↗(On Diff #18737)

I guess not relevant to this diff, but this misbehavior case clearly isn't tested.

3505 ↗(On Diff #18737)

s/verctor/vector

3518 ↗(On Diff #18737)

You added two new ban cases. Test for them.

This revision now requires changes to proceed.Apr 11 2020, 00:04
deadalnix requested review of this revision.Apr 12 2020, 14:30
deadalnix added inline comments.
src/net_processing.cpp
3431 ↗(On Diff #18737)

Which are mutually exclusive.

3518 ↗(On Diff #18737)

I don't really see the value in testing behavior that is quite clearly incorrect.

deadalnix updated this revision to Diff 18745.Apr 12 2020, 14:49

Fix typo

teamcity added a comment.Apr 12 2020, 14:50

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Apr 12 2020, 15:02

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10176: Diff 18745.Apr 12 2020, 15:02
deadalnix updated this revision to Diff 18746.Apr 12 2020, 15:04

Remove debug leftovers

teamcity added a comment.Apr 12 2020, 15:04

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Apr 12 2020, 15:15

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10177: Diff 18746.Apr 12 2020, 15:15
deadalnix updated this revision to Diff 18757.Apr 13 2020, 12:51

rebase

teamcity added a comment.Apr 13 2020, 13:17

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Apr 13 2020, 13:27

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10185: Diff 18757.Apr 13 2020, 13:28
deadalnix updated this revision to Diff 18762.Apr 13 2020, 15:53

Rebase on top of D5708

teamcity added a comment.Apr 13 2020, 15:53

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Apr 13 2020, 16:04

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10190: Diff 18762.Apr 13 2020, 16:04
jasonbcox added inline comments.Apr 13 2020, 17:48
src/net_processing.cpp
3431 ↗(On Diff #18737)

I'm referring only to the gArgs.GetBoolArg("-enableavalanche", AVALANCHE_DEFAULT_ENABLED) part.

3518 ↗(On Diff #18737)

I can see the argument for invalid-ava-response-content but not for invalid-ava-response-signature

deadalnix added inline comments.Apr 14 2020, 04:14
src/net_processing.cpp
3431 ↗(On Diff #18737)

Do you ever expect strCommand to be both equal to NetMsgType::AVAPOLL and NetMsgType::AVARESPONSE at the same time?

jasonbcox accepted this revision.Apr 15 2020, 00:56
jasonbcox added inline comments.
src/net_processing.cpp
3431 ↗(On Diff #18737)

Reread my latest comment ^^:

I'm referring only to the gArgs.GetBoolArg("-enableavalanche", AVALANCHE_DEFAULT_ENABLED) part.

But I'm not going to block this patch on this any longer.

This revision is now accepted and ready to land.Apr 15 2020, 00:56
Closed by commit rABC919980f616e5: [avalanche] Process AvalancheResponse (authored by deadalnix). · Explain WhyApr 15 2020, 15:58
This revision was automatically updated to reflect the committed changes.
deadalnix added a commit: rABC919980f616e5: [avalanche] Process AvalancheResponse.
deadalnix added a commit: rSTAGING919980f616e5: [avalanche] Process AvalancheResponse.May 5 2020, 18:58

Revision Contents
Changeset List

PathSize
src/
1 line
39 lines
test/
functional/
15 lines
test_framework/
4 lines

Diff 18829

View Options

src/avalanche.h

Loading...
View Options

src/net_processing.cpp

Loading...
View Options

test/functional/abc-p2p-avalanche.py

Loading...
View Options

test/functional/test_framework/messages.py

Loading...