Paths

Table of Contentst

Differential D2606

Refactor ProcessGetData in anticipation of avoiding cs_main for ABC
ClosedPublic
Actions

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

Tags

None

Subscribers

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 5095
Build 8253: Bitcoin ABC Buildbot (legacy)
Build 8252: 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

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

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

Address feedback + rebase

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

Harbormaster completed remote builds in B5084: Diff 7498.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 ↗	(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

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.

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

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

Path

Size

src/

net_processing.cpp

398 lines

Diff 7509

src/net_processing.cpp

Loading...