refactor: make Get{Quorum, MinedCommitment, InstantSendLock*, *MN*} and friends return std::optional
#6446
Conversation
`GetQuorum` is a fail-able activity and some portions of the codebase handle the fail state but others implicitly assume that they won't fail (dereferencing without nullptr checks) or make the assumption explicit (through placing `assert`s). As some of these assumptions were within loops, care must be taken that partial changes are not committed (unless the entity is discarded by the caller after reporting the fail state). By making it a `std::optional`, we separate fetching and using the value as two separate steps with error handling in between, assumptions have been documented through code comments but shouldn't cause the client to crash altogether (crashes are harder to extract debug information from if they're sporadic and occur on nodes running release binaries).
`std::move` is required as we're dealing with `std::unique_ptr` objects
|
This pull request has conflicts, please rebase. |
✅ Backport Verification - Issues FixedI found and fixed 1 issue:
Original Bitcoin commit: N/A (Dash-internal refactoring) ✅ Analysis SummaryThis PR refactors Dash's masternode and quorum management functions to return Fix Applied:
This should resolve the catastrophic CI failures (8 failures) that were occurring due to the missing include. Note: This is a Dash-internal refactoring PR, not a Bitcoin backport, which is why no Bitcoin commit hash is available. This fix should resolve the CI build failures. ✅ |
| @@ -92,58 +92,58 @@ bool CDeterministicMNList::IsMNPoSeBanned(const CDeterministicMN& dmn) | |||
| return dmn.pdmnState->IsBanned(); | |||
| } | |||
|
|
|||
| CDeterministicMNCPtr CDeterministicMNList::GetMN(const uint256& proTxHash) const | |||
| std::optional<CDeterministicMNCPtr> CDeterministicMNList::GetMN(const uint256& proTxHash) const | |||
There was a problem hiding this comment.
UPD: I see, it's old PR, bad bot confused me!
why? Could you justify this refactoring?
std::optional to shared_ptr is not really good solution.
shared_ptr can be nullptr and std::optional can be nullopt.
You still need to check if it is "null" or not; you still get UB if there's null and you haven't checked it.
But there are 2 downsides - std::optional adds overhead (extra bool flag, constructor, etc); and 2nd (which is worse), you need to compare not only for nullopt, but for nullptr also.
auto mn = GetMN(); if (mn != nullopt && mn.value() != nullptr) ....
There was a problem hiding this comment.
This PR has been conceptually abandoned, am looking to rework it someday but agree in principle it's currently not in a great shape. Bot's taken everyone for a loop, eh?
Additional Information
GetQuorum,Get*MN* and friends #6132Breaking Changes
Work in progress
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.