fix: fpd panics on jailed finality provider #78
Conversation
gitferry
requested review from
SebastianElvis,
RafilxTenfen,
KonradStaniec and
filippos47
| Use: "unjail-finality-provider", | ||
| Aliases: []string{"ufp"}, | ||
| Short: "Unjail the given finality provider.", | ||
| Example: fmt.Sprintf(`fpd unjail-finality-provider [eots_pk]--daemon-address %s ...`, defaultFpdDaemonAddress), |
There was a problem hiding this comment.
| Example: fmt.Sprintf(`fpd unjail-finality-provider [eots_pk]--daemon-address %s ...`, defaultFpdDaemonAddress), | |
| Example: fmt.Sprintf(`fpd unjail-finality-provider [eots_pk] --daemon-address %s ...`, defaultFpdDaemonAddress), |
gitferry
force-pushed
the
fix/fp-panic-when-jailed
branch
from
2aaae63 to
7ed2c90
Compare
gitferry
force-pushed
the
fix/fp-panic-when-jailed
branch
from
7ed2c90 to
171cc6d
Compare
| @@ -247,6 +246,11 @@ func (fpm *FinalityProviderManager) StartFinalityProvider(fpPk *bbntypes.BIP340P | |||
| } | |||
|
|
|||
| if err := fpm.addFinalityProviderInstance(fpPk, passphrase); err != nil { | |||
| if errors.Is(err, ErrFinalityProviderJailed) { | |||
| fpm.logger.Error("failed to start finality provider", zap.Error(err)) | |||
| // do not return error | |||
There was a problem hiding this comment.
Same question here. This seems like an error to me
There was a problem hiding this comment.
The point of not returning error is that we still want to start the app even if we fail to start the fp instance due to fp jailed. But I agree that we should not handle the error here. I changed to handle the error in the application level in 0eb53b3
| if errors.Is(err, ErrFinalityProviderJailed) { | ||
| fp.MustSetStatus(proto.FinalityProviderStatus_JAILED) | ||
| } |
There was a problem hiding this comment.
Do you think this should be done here or inside bootstrap()?
There was a problem hiding this comment.
Good point. Moved the error handler to bootstrap()
| @@ -247,6 +246,11 @@ func (fpm *FinalityProviderManager) StartFinalityProvider(fpPk *bbntypes.BIP340P | |||
| } | |||
|
|
|||
| if err := fpm.addFinalityProviderInstance(fpPk, passphrase); err != nil { | |||
| if errors.Is(err, ErrFinalityProviderJailed) { | |||
| fpm.logger.Error("failed to start finality provider", zap.Error(err)) | |||
| // do not return error | |||
There was a problem hiding this comment.
Same question here. This seems like an error to me
| msg := &finalitytypes.MsgUnjailFinalityProvider{ | ||
| Signer: bc.mustGetTxSigner(), | ||
| FpBtcPk: bbntypes.NewBIP340PubKeyFromBTCPK(fpPk), | ||
| } |
There was a problem hiding this comment.
Do you think if it's beneficial to check whether the jail_until is passed before submitting this tx? If the operator submits unjail tx when the FP hasn't passed the jailing period, the tx will still consume gas but the FP won't get unjailed, which is not expected
There was a problem hiding this comment.
Good point. I thought gas would not be confused as it will be rejected in the mempool level? Created an issue #79 to track this as this needs introduction of signing info queries and will be handled in a separate pr
In the previous implementation, once a finality provider is jailed, when the fpd is restarted, it will panic due to error from Babylon when fast sync. This PR fixed this issue with the following changes: 1. We implemented `unjail` finality provider CLI other than using the one from Babylon. This implementation will set the fp instance status to `inactive` after `unjail` tx is successfully sent (will later updated to active if the fp has voting power). 2. Once `jailed` error is detected while starting a fp instance, it will fail but the fpd will not panic, meaning that the loop for updating stored fp status continues running. Now the flow of jailing/unjailing becomes the follows: 1. fpd detects jailing via err when sending a fp sig or a loop for checking fp status 2. once `jailed` detected, fpd terminates the fp instance without terminating the program 3. the operator checks fp signing info to get the `jail_until` via `babylond q finality signing-info [fp-pk-hex]` 4. after the `jail_until` is passed, the operator can unjail the fp by executing `fpd unjail-finality-provider [fp-pk-hex]` 5. if everything goes well, the fp will continue sending finality votes if it has voting power after a period of waiting for state transition
In the previous implementation, once a finality provider is jailed, when the fpd is restarted, it will panic due to error from Babylon when fast sync. This PR fixed this issue with the following changes:
unjailfinality provider CLI other than using the one from Babylon. This implementation will set the fp instance status toinactiveafterunjailtx is successfully sent (will later updated to active if the fp has voting power).jailederror is detected while starting a fp instance, it will fail but the fpd will not panic, meaning that the loop for updating stored fp status continues running.Now the flow of jailing/unjailing becomes the follows:
jaileddetected, fpd terminates the fp instance without terminating the programjail_untilviababylond q finality signing-info [fp-pk-hex]jail_untilis passed, the operator can unjail the fp by executingfpd unjail-finality-provider [fp-pk-hex]