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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Feb 26 2019, 10:38
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 26 2019, 10:38
Herald added a subscriber: schancel. · View Herald Transcript
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
jasonbcox added inline comments.Feb 26 2019, 20:21
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?

Fabien added inline comments.Feb 26 2019, 20:47
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.

jasonbcox added inline comments.Feb 26 2019, 23:45
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

Fabien updated this revision to Diff 7498.Feb 27 2019, 08:19

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
Fabien updated this revision to Diff 7509.Feb 27 2019, 18:03

Rebase

deadalnix accepted this revision.Feb 28 2019, 14:20
jasonbcox accepted this revision.Feb 28 2019, 17:54
This revision is now accepted and ready to land.Feb 28 2019, 17:54
This revision was automatically updated to reflect the committed changes.