eth,pm: avoid watchPoolSizeChange nil pointer error

[kyriediculous]

What does this pull request do? Explain your changes. (required)
This issue might be caused by an unexpected return value from the ETH provider.

The error is caused by the following expression

if poolSize := sm.tm.GetTranscoderPoolSize(); poolSize.Cmp(lastPoolSize) != 0

Both values are fetched from TimeWatcher.GetTranscoderPoolSize()

The value of TimeWatcher.transcoderPoolSize is initialised on node startup and refetched from the ETH provider when events are received.

The contract binding BondingManagerSession.GetTranscoderPoolSize() returns the following return *(new(*big.Int)), err.
This means that even though the return value (a pointer) can't be nil because it is initialised , but value the pointer refers to can still be and this is what is returned. If for some reason there is a nil error and an unexpected value from the contract call a nil pointer could still occur.

Specific updates (required)

• Wrap the contract call in case it returns nil, nil

How did you test each of these updates (required)

Does this pull request close any open issues?
Fixes #1828

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[yondonfu]

This means that even though the return value (a pointer) can't be nil because it is initialised , but value the pointer refers to can still be and this is what is returned. If for some reason there is a nil error and an unexpected value from the contract call a nil pointer could still occur.

Did you actually confirm that the contract binding is returning a nil value even when the error is nil?

The nil pointer error in #1828 is being trigger in this line. I believe poolSize could be nil or lastPoolSize could be nil. The latter makes more sense to me because the first time that the pool size in the TimeWatcher is set is in the Watch() method which is called in a goroutine. So, if lastPoolSize is initialized in LocalSenderMonitor before the pool size is initialized in the TimeWatcher then lastPoolSize would be nil.

I think this indicates another problem - LocalSenderMonitor depends on state in TimeWatcher that might not be initialized yet. One way to address this particular problem could be to move the state initialization logic (such as this line) into the TimeWatcher constructor which is called synchronously before Watch() is called in a goroutine. Additionally, it may make sense to default the return value of TimeWatcher.GetTranscoderPoolSize() to some non-nil value to guard against nil pointer errors just to be safe (i.e. 0).

[kyriediculous]

Did you actually confirm that the contract binding is returning a nil value even when the error is nil?

No but it could be possible if a ethereum client is used with potential non-default behaviour here I guess (or some erroneous code). You're probably right that a properly implemented client likely doesn't return nil, nil here.

So yeah a race condition is more likely to be the cause here.

[yondonfu]

No but it could be possible if a ethereum client is used with potential non-default behaviour here I guess (or some erroneous code). You're probably right that a properly implemented client likely doesn't return nil, nil here.

I think the contract binding implementation should guarantee that the big.Int value is always non-nil.

[yondonfu]

Looks pretty good. Just 2 minor comments.

[yondonfu]

LGTM after squashing