ETCM-626 - remove Option.get when searching for the best block #924
Conversation
bsuieric
requested review from
AnastasiiaL,
dzajkowski,
jvdp,
leo-bogastry,
robinraju and
ten15bit
src/main/scala/io/iohk/ethereum/consensus/ethash/EthashBlockCreator.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/consensus/ethash/EthashMiner.scala
Outdated
Show resolved
Hide resolved
leo-bogastry
left a comment
leo-bogastry
left a comment
There was a problem hiding this comment.
Nice to see all those gets removed 👍
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@bsuieric you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.
There was a problem hiding this comment.
This looks like a good fix overall.
Double-checking: All the new uses of .get are in test code? If they're in actual production code that seems worrisome, but not a big deal if it's only in test code.
IME it's useful to think about how any given issue arose and how it can be prevented in the future. In this case, it looks like it would due to a casual use of .get. Since .get undermines the safety Scala provides with Option, it should generally be regarded with suspicion. It may be worth getting a lint rule setup to flag all uses of .get like this for further review.
Reviewed with ❤️ by PullRequest
bsuieric
force-pushed
the
ETCM-626-remove-best-block-option-get
branch
from
14e0b4b to
cc2a8ae
Compare
There was a problem hiding this comment.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
Unsafe use of Option.get causes node desync when getting new best block