Clear container after move
ClosedPublic
Actions

Authored by castarco on Jan 19 2019, 12:27.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGff5296fa2cfb: Clear container after move
rABCff5296fa2cfb: Clear container after move

Summary

Clear parents after move in
DisconnectedBlockTransactions::addForBlock to ensure a known state
before reusing the variable (the standard enforces a valid state, but
not necessarely a known & specific one).

Signed-off-by: Andres Correa Casablanca <andres@thirdhash.com>

Test Plan

Running the current tests should be enough.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

castarco created this revision.Jan 19 2019, 12:27

Owners added a reviewer: Restricted Owners Package.Jan 19 2019, 12:27

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 19 2019, 12:27

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.

castarco added inline comments.Jan 19 2019, 12:33

src/txmempool.h
398 ↗	(On Diff #6743)	Removing the `const` qualifier allows the instances of this class to be movable.

Harbormaster completed remote builds in B4579: Diff 6743.Jan 19 2019, 12:39

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

deadalnix requested changes to this revision.Jan 19 2019, 13:11

deadalnix added inline comments.

src/txmempool.cpp
1333 ↗	(On Diff #6743)	Fair enough, even though std::move is known to leave the moved datastructure empty, I don't think the spec enforces it.
src/txmempool.h
398 ↗	(On Diff #6743)	Changing these midway would be catastrophic. This is a good constraint to enforce. Implementing a move constructor seems like a better approach.

This revision now requires changes to proceed.Jan 19 2019, 13:11

castarco added inline comments.Jan 20 2019, 11:33

src/txmempool.h
398 ↗	(On Diff #6743)	Unfortunately, the move constructor does not fully solve the problem, there's need for the move assignment operator, which is automatically deleted by the existence of the `const` qualifier. I think removing this qualifier is not very dangerous because this properties are private and the class does not have methods with side effects declared. Another option is to undo this change and enforce a known state on `parents` by calling `parents.clear()`.