Page MenuHomePhabricator

Refactor ProcessGetData in anticipation of avoiding cs_main for ABC
ClosedPublic

Authored by Fabien on Feb 26 2019, 10:38.

Details

Summary

This is a refactor only, no functional change.
Partial backport of core PR11824 (commit 66aa1d5)

Depends on D2605 and D2620

Test Plan
make check
./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11824_part2
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5095
Build 8253: Bitcoin ABC Buildbot (legacy)
Build 8252: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Feb 26 2019, 20:18
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/net_processing.cpp
1246 ↗(On Diff #7480)

consensusParams is redundant with the config that's already passed in

1405 ↗(On Diff #7480)

Remove since it's not used in this function if you apply my above comment

This revision now requires changes to proceed.Feb 26 2019, 20:18
src/net_processing.cpp
1264 ↗(On Diff #7480)

This was moved in the commit you referenced: https://github.com/bitcoin/bitcoin/pull/11824/commits/66aa1d58a158991a8014a91335b5bc9c00062f56

I noticed D2607 actually moves it but refers to a different commit. Can you comment on if this is a full backport of commit 66aa1d or just parts of it?

src/net_processing.cpp
1264 ↗(On Diff #7480)

This is a full backport of commit 66aa1d5.
This part is not really "moved above" in the original commit, it was already there because used for segwit stuff that is not implemented in abc.
I kept this commit a pure refactoring, so the shared pointer is left where it stands from our original code, with the narrower possible scope.

You're right when you say that I move it in D2607. This is because there is a functional change in this commit which require a larger scope for a_recent_block.

src/net_processing.cpp
1264 ↗(On Diff #7480)

I see now. That PR is super hard to read on Github. The rest of what you said cleared it up.

Cheers

Address feedback + rebase

deadalnix requested changes to this revision.Feb 27 2019, 12:18
deadalnix added inline comments.
src/net_processing.cpp
1253 ↗(On Diff #7498)

It seems like there are some other backport missing here as there is compact block support missing.

1254 ↗(On Diff #7498)

This doesn't seem to match the patch from core.

1277 ↗(On Diff #7498)

dito

1359 ↗(On Diff #7498)

It's probably worth doing a relayout on these comments, but this can be in another diff.

This revision now requires changes to proceed.Feb 27 2019, 12:18
Fabien planned changes to this revision.Feb 27 2019, 14:24
This revision is now accepted and ready to land.Feb 28 2019, 17:54
This revision was automatically updated to reflect the committed changes.