Page MenuHomePhabricator
Differential D2606

Refactor ProcessGetData in anticipation of avoiding cs_main for ABC
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING5a7d15fa36c3: Refactor ProcessGetData in anticipation of avoiding cs_main for ABC
rABC5a7d15fa36c3: Refactor ProcessGetData in anticipation of avoiding cs_main for ABC
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 5084
Build 8231: Bitcoin ABC Buildbot (legacy)
Build 8230: arc lint + arc unit

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 reviewer: deadalnix. · View Herald Transcript
Herald added a subscriber: schancel. · View Herald Transcript
teamcity added a subscriber: teamcity.Feb 26 2019, 10:39

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

Fabien added a child revision: D2607: Avoid cs_main in net_processing ActivateBestChain calls.Feb 26 2019, 10:48
teamcity added a comment.Feb 26 2019, 10:51

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

Harbormaster completed remote builds in B5070: Diff 7480.Feb 26 2019, 10:51
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

teamcity added a comment.Feb 27 2019, 08:20

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

Harbormaster completed remote builds in B5084: Diff 7498.Feb 27 2019, 08:33
teamcity added a comment.Feb 27 2019, 08:33

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

deadalnix requested changes to this revision.Feb 27 2019, 12:18
deadalnix added inline comments.
src/net_processing.cpp
1253

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

1254

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

1277

dito

1359

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

Fabien edited the summary of this revision. (Show Details)Feb 27 2019, 18:04
Fabien added a parent revision: D2620: Use cached [compact] blocks to respond to getdata messages.
teamcity added a comment.Feb 27 2019, 18:04

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

teamcity added a comment.Feb 27 2019, 18:17

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

Harbormaster completed remote builds in B5095: Diff 7509.Feb 27 2019, 18:17
Fabien mentioned this in D2626: Re-layout comments in ProcessGetBlockData and ProcessGetData.Feb 28 2019, 11:21
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
Closed by commit rABC5a7d15fa36c3: Refactor ProcessGetData in anticipation of avoiding cs_main for ABC (authored by Matt Corallo <git@bluematt.me>, committed by Fabien). · Explain WhyFeb 28 2019, 18:48
This revision was automatically updated to reflect the committed changes.
Fabien mentioned this in rABC6a8b6fb9fa22: Re-layout comments in ProcessGetBlockData and ProcessGetData.Feb 28 2019, 18:57
markblundeberg mentioned this in D3374: Merge #12392: Fix ignoring tx data requests when fPauseSend is set on a peer.Jun 20 2019, 20:06
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rABC1234e9d1cab7: Merge #12392: Fix ignoring tx data requests when fPauseSend is set on a peer.Jun 20 2019, 20:23
Fabien added a commit: rSTAGING5a7d15fa36c3: Refactor ProcessGetData in anticipation of avoiding cs_main for ABC.Sep 18 2019, 22:34
Fabien mentioned this in rSTAGING6a8b6fb9fa22: Re-layout comments in ProcessGetBlockData and ProcessGetData.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rSTAGING1234e9d1cab7: Merge #12392: Fix ignoring tx data requests when fPauseSend is set on a peer.Sep 19 2019, 00:25

Revision Contents
Changeset List

PathSize
src/
379 lines

Diff 7498

View Options

src/net_processing.cpp

Loading...