Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make Get{Quorum, MinedCommitment, InstantSendLock*, *MN*} and friends return std::optional #6446

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Dec 3, 2024

Additional Information

Breaking Changes

Work in progress

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

kwvg added 4 commits December 3, 2024 07:43
`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
@kwvg kwvg modified the milestones: 22, 22.1 Dec 3, 2024
Copy link

github-actions bot commented Dec 9, 2024

This pull request has conflicts, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant