Encapsulate Nil Checks

[rauljordan]

💎 Issue

Background

One pattern we should opt for across our repo is to rely more on package-level errors rather than putting the burden of nil checks in certain situations on the caller. For example, a common pattern seen across the codebase is this:

st, err := beaconDB.StateByRoot(ctx, root)
if err != nil {
  return err
}
if st == nil || st.IsNil() {
  ...
}

One time, there was a PR with a bug because we forgot to add a nil check. Another time, there was another failure because we forgot state needs two nil checks. Pushing this check onto callers increases the probability. Go is already notorious for nil checks, which are risky if forgotten. Instead, we should aim to encapsulate them as much as possible and rely on error values as a sane alternative.

For example:

// beacon-chain/db/kv/state.go
var ErrNotFound = errors.New("not found")

func (s *Store) StateByRoot(root [32]byte) (*State, error) {
  state := s.db.view(root)
  if st == nil || st.IsNil() {
    return nil, ErrNotFound
  }
}

// elsewhere.
st, err := beaconDB.StateByRoot(ctx, root)
switch err {
  case kv.ErrNotFound:
    // Handle...
  default:
    return err
}

Notes

It is important to audit all uses of this nil encapsulation we are changing, however, as it is possible we introduce a bug in doing so. For example, some code paths might rely on error checking differently, and we may need to handle those error values in a sane manner. Additionally, some other code paths might actually rely on nil values returned from the db method to branch into certain execution. We should be careful with this change.

[rkapka]

It would be neat to have a [dream]NotNil<T> generic type[/dream] signalling that the caller does not have to perform nil checks.

[pgagala]

I can try to do that (if that is not uber urgent and doesn't have to be delivered very fast)

[rkapka]

@pgagala I think it would be nice if we could do this incrementally, package by package or so. One huge PR will be very hard to maintain, especially merging with develop might become a nightmare.

[pgagala]

I think that is a good idea

[pgagala]

so far I was able to introduce some changes https://github.com/pgagala/prysm/tree/encapsulate-nil-checks-9454
. I checked beacon-chain package without state and sync packages someone can overtake that if that is urgent because now I won't have much time unfortunately

[victordeleau]

I would gladly take over if that's a possibility. I can finish the beacon-chain package and make a PR for it. Then move on to the next package.

[terencechain]

I would gladly take over if that's a possibility. I can finish the beacon-chain package and make a PR for it. Then move on to the next package.

Sounds good. Thanks for your help. I will also recommend to do it per beacon-chain sub package (ie blockchain, rpc.. etc). beacon-chain itself is an enormous package

[rauljordan]

See https://github.com/prysmaticlabs/prysm/pull/9976/files#diff-867448a2b93d350184511d77435350937f71ea1ac154cefa13cdb3ce2bce26daR1 for an awesome pattern on how to do this !

[rauljordan]

#9976