Page MenuHomePhabricator

Refactor ProcessGetData in anticipation of avoiding cs_main for ABC

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



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

Depends on D2605 and D2620

Test Plan
make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox requested changes to this revision.Feb 26 2019, 20:18
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
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
1264 ↗(On Diff #7480)

This was moved in the commit you referenced:

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?

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.

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.


Address feedback + rebase

deadalnix requested changes to this revision.Feb 27 2019, 12:18
deadalnix added inline comments.
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)


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.