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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- refactor-orphan
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3612 Build 5299: Bitcoin ABC Buildbot (legacy) Build 5298: arc lint + arc unit
Event Timeline
Comment Actions
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 ?
Comment Actions
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. |
Comment Actions
If necessary, I can backport them first. This isn't a substantial change at this point though.
Comment Actions
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. |
Comment Actions
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 |
| src/net_processing.cpp | ||
|---|---|---|
| 4208 ↗ | (On Diff #5437) | Probably not. All these need to be added. These were planned changes though. |
Comment Actions
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.