[bugfix] txnotifier: delete ntfn by confirm height always #4620
Conversation
When we cancel a confirmation request, we should remove the request from the height map regardless of the current height. Otherwise we end up in the situation when the height is reached, the notification is attempted sent which results in a crash.
This test addition would cause the txnotifier to crash prior to the previous commit.
| @@ -763,9 +763,7 @@ func (n *TxNotifier) CancelConf(confRequest ConfRequest, confID uint64) { | |||
| if confSet.details != nil { | |||
| confHeight := confSet.details.BlockHeight + | |||
| ntfn.NumConfirmations - 1 | |||
| if confHeight <= n.currentHeight { | |||
There was a problem hiding this comment.
@wpaulino Just to make sure: was this condition here for a good reason?
There was a problem hiding this comment.
not 100% sure myself, but i think your change is correct. doesn't make sense to continue tracking a canceled request
There was a problem hiding this comment.
It looks like I actually meant to write the opposite condition. If the confirmation height has not been reached, then we should delete it as it hasn't been notified yet to prevent the notification, but if it has, then there's no need to delete it as that's already done once the notification is dispatched. Either way, it should be fine to just always delete now.
| @@ -763,9 +763,7 @@ func (n *TxNotifier) CancelConf(confRequest ConfRequest, confID uint64) { | |||
| if confSet.details != nil { | |||
| confHeight := confSet.details.BlockHeight + | |||
| ntfn.NumConfirmations - 1 | |||
| if confHeight <= n.currentHeight { | |||
There was a problem hiding this comment.
not 100% sure myself, but i think your change is correct. doesn't make sense to continue tracking a canceled request
| @@ -763,9 +763,7 @@ func (n *TxNotifier) CancelConf(confRequest ConfRequest, confID uint64) { | |||
| if confSet.details != nil { | |||
| confHeight := confSet.details.BlockHeight + | |||
| ntfn.NumConfirmations - 1 | |||
| if confHeight <= n.currentHeight { | |||
There was a problem hiding this comment.
It looks like I actually meant to write the opposite condition. If the confirmation height has not been reached, then we should delete it as it hasn't been notified yet to prevent the notification, but if it has, then there's no need to delete it as that's already done once the notification is dispatched. Either way, it should be fine to just always delete now.
When we cancel a confirmation request, we should remove the request from
the height map regardless of the current height. Otherwise we end up in
the situation when the height is reached, the notification is attempted
sent which results in a crash.