Page MenuHomePhabricator

Wrap orphan handling in its own OrphanPool class
Changes PlannedPublic

Authored by schancel on Oct 16 2018, 20:26.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Summary

Move orphan pool handling code into it's own class with it's own mutex. This is the a first step in making orphan handling for flexible, and encapsulated for a better CPFP implementation in the future.

Test Plan
make VERBOSE=1 check && ./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1933 (branched from master)
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5287
Build 8636: Bitcoin ABC Buildbot (legacy)
Build 8635: arc lint + arc unit

Event Timeline

Move functions back to their rightful places.

Clean up some comment changes due to auto-linting

Reorder functions again for easier diffing

Core did various changes to this code. While it's not a bad change per se, are you really sure you want to take ownership of this ?

jasonbcox requested changes to this revision.Oct 16 2018, 21:45
jasonbcox added a subscriber: jasonbcox.

Looks mostly good. Some questions about old behavior though. I think there's an opportunity to guarantee safety for the orphan pool in this regard.

src/net_processing.cpp
838 ↗(On Diff #5436)

move this comment to the above line. silly linter

930 ↗(On Diff #5436)

A bit surprising that these calls on mapOrphanTransactionsByPrev do not have LOCK() applied. const methods should be thread safe, but use of auto here makes me wonder if that is necessarily enforced.

947 ↗(On Diff #5436)

EraseOrphanTx() calls LOCK(), so this call is safe compared to the above.

1114 ↗(On Diff #5436)

LOCK()?

2404 ↗(On Diff #5436)

LOCK()? Same logic as above.

This revision now requires changes to proceed.Oct 16 2018, 21:45
schancel retitled this revision from Wrap orphan handling in it's own OrphanPool class to Wrap orphan handling in its own OrphanPool class.

Core did various changes to this code. While it's not a bad change per se, are you really sure you want to take ownership of this ?

If necessary, I can backport them first. This isn't a substantial change at this point though.

schancel marked an inline comment as done.

Fix comment. Additional changes for locks coming later.

deadalnix requested changes to this revision.Oct 19 2018, 21:35

It looks like there are a few synchronization problems with that diff.

Core modified the orphan pool management in a different way. Investigating what hey did is probably a good idea. I like the encapsulation approach better than what core did, but it is clear that this is tricky and we may not want to take ownership of it.

src/net_processing.cpp
928 ↗(On Diff #5437)

Shouldn't this require a lock ?

1113 ↗(On Diff #5437)

dito

2403 ↗(On Diff #5437)

dito

2707 ↗(On Diff #5437)

dito

2750 ↗(On Diff #5437)

dito

4208 ↗(On Diff #5437)

I assume this is okay because CNatProcessingCLeanup is a global object of some kind, but this is sketchy.

src/test/DoS_tests.cpp
276 ↗(On Diff #5437)

I assume they assumed this was okay because it is single threaded, but meh.

This revision now requires changes to proceed.Oct 19 2018, 21:35

Looks that the orphanPool stuff was changed in this PR https://github.com/bitcoin/bitcoin/pull/11824/commits

I'd rather do this, as I don't find it incredibly disruptive for backports. WE can fix the locking.

src/net_processing.cpp
930 ↗(On Diff #5436)

Hmm yeah. Good catch. This needs to be encapsulated

schancel marked an inline comment as done.
schancel added inline comments.
src/net_processing.cpp
4208 ↗(On Diff #5437)

Probably not. All these need to be added. These were planned changes though.

Rebase and use rwcollection

Owners added a reviewer: Restricted Owners Package.Mar 24 2019, 03:42

There was a backport from core that did something very similar. While I do think the design of this patch is better than the one from core, I'd be hesitant to go for it as we'd have to maintain it rather than letting core handle that one.

There was a backport from core that did something very similar. While I do think the design of this patch is better than the one from core, I'd be hesitant to go for it as we'd have to maintain it rather than letting core handle that one.

Yes, this is rebased on top of it.