Use MakeUnique to construct objects owned by unique_ptrs #14211

Use MakeUnique to construct objects owned by unique_ptrs.

Rationale:

• MakeUnique ensures exception safety in complex expressions.
• MakeUnique gives a more concise statement of the construction.

<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

• #14224 (Add -fsanitize=integer Travis job. Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations. by practicalswift)
• #14010 (tests: Setup chain parameters (globalChainParams) when performing fuzzing initialization by practicalswift)
• #13311 (Don't edit Chainparams after initialization by jtimon)
• #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK. Could mention std::make_unique is only available in c++14 so MakeUnique is a temporary replacement.

@promag Good point! Added. Please re-review :-)

utACK https://github.com/bitcoin/bitcoin/commit/81f81ec022bce22c916b2964f5e38c318a828d9a :-)

utACK da15d9d3d9451b5cb3af137305a702174a9202b3

@donaloconnor Thanks for the review! I'm afraid you didn't review the latest commit. Would you mind reviewing da15d9d3d9451b5cb3af137305a702174a9202b3? :-)

utACK da15d9d.

utACK https://github.com/bitcoin/bitcoin/commit/da15d9d3d9451b5cb3af137305a702174a9202b3

Thanks, but this can be changed the next time someone touches the function(s). No need for a separate patch.

Pull requests without a rationale and clear improvement may be closed immediately.

Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly.

• Any test improvements or new tests that improve coverage are always welcome.
• Features are always welcome, but might be rejected early in the review process.
• Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed.
• Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they significantly improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the developer notes, stylistic code changes are usually rejected.
• All changes should have accompanying unit tests (see src/test/) or functional tests (see test/). PR authors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change.

Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time.

Feel free to submit the test and documentation changes separately, but we don't have the resources right now to review refactoring of core code. Also I think we are not affected by "complex expressions" that require MakeUnique to ensures exception safety.

we don't have the resources right now to review refactoring of core code

Could we have a tag to distinguish PRs that were closed to due lack of resources from PRs that were closed due to more substantive issues (like brokenness or lack of agreement)? It might be useful to track where we are falling behind, and maybe nice to take edge off rejection for people who submit such PRs, sort of https://en.wikipedia.org/wiki/It%27s_not_you,_it%27s_me

@ryanofsky A closed pull request due to lack of author resources is tagged with "up for grabs". A closed pull request due to lack of reviewer resources is usually left open until it is reviewed or closed due to a merge conflict and then also tagged with "up for grabs".

For refactoring pull requests, I don't see the need to place them "up for grabs", since the same topics come up frequently every couple of months.

Yes, I'm not suggesting as tagging them as "up for grabs", I'm suggesting a new tag like "insufficient resources" to distinguish low priority PRs from PRs with actual substantive problems.