chore: tx fee refunding follow-up #130
Conversation
SebastianElvis
requested review from
KonradStaniec and
RafilxTenfen
and removed request for
a team
| ms.Logger(ctx).Debug("Received duplicated finality vote", "block height", req.BlockHeight, "finality provider", req.FpBtcPk) | ||
| // exactly same vote already exists, return error | ||
| // this is to secure the tx refunding against duplicated messages | ||
| return nil, types.ErrDuplicatedFinalitySig |
There was a problem hiding this comment.
I would wait for @gitferry review here as he may have some insight how this will influence finality provider side
There was a problem hiding this comment.
I think we will need to add this error as expected error here. After that it should be good. Will wait for Gai to confirm this
There was a problem hiding this comment.
I agree with @SebastianElvis that we can add the duplicate error as expected in the finality provider. I guess this applies in covenant emulator sending duplicate sigs as well. Added issue babylonlabs-io/finality-provider#84 and babylonlabs-io/covenant-emulator#17 to track this
| if status != types.BTCDelegationStatus_PENDING { | ||
| ms.Logger(ctx).Debug("Received covenant signature after the BTC delegation is already expired", "covenant pk", req.Pk.MarshalHex()) | ||
| return &types.MsgAddCovenantSigsResponse{}, nil | ||
| if status == types.BTCDelegationStatus_UNBONDED { |
There was a problem hiding this comment.
Just note here: I guess this change allow to accept more signatures than the quorum. I imagine slasher or some other tool may relay of there being only quorum of signatures when building witness for sending slashing transactions. This may require some updates there. ( I definitely remember staker, assumes there is only quorum of sigs on Babylon chain)
There was a problem hiding this comment.
Hmm will check when bumping peripheral repositories. In which situation BTC staker will need to rely on this behaviour? As far as I know in other repositories (eg vigilante) it does not even rely on how many covenant signatures are in a BTC delegation, as slashing only uses one of all covenant signatures. Did I miss anything?
There was a problem hiding this comment.
What do you mean by as slashing only uses one of all covenant signatures ?
The problem I have in mind is for example here: https://github.com/babylonlabs-io/vigilante/blob/main/btcstaking-tracker/btcslasher/slasher_utils.go#L344.
We build the wittness from all covenant signatures in the delegation. Up until now it was fine, because we accepted only quorum of signatures in the delegation.
After this change this is not fine now we can have more that quorum signatures in delegation, but BTC script expects the witness to have exactly quorum of signatures.
There was a problem hiding this comment.
Hmm right, maybe some programs (esp. those involving slashing) might assume somewhere that there is only a quorum number of cov sigs. Good point 👍
| ms.Logger(ctx).Debug("Received duplicated finality vote", "block height", req.BlockHeight, "finality provider", req.FpBtcPk) | ||
| // exactly same vote already exists, return error | ||
| // this is to secure the tx refunding against duplicated messages | ||
| return nil, types.ErrDuplicatedFinalitySig |
There was a problem hiding this comment.
I agree with @SebastianElvis that we can add the duplicate error as expected in the finality provider. I guess this applies in covenant emulator sending duplicate sigs as well. Added issue babylonlabs-io/finality-provider#84 and babylonlabs-io/covenant-emulator#17 to track this
Pr: #130 allowed for accepting covenant signatures after quorum is reached. This change introduces regression i.e covenant signatures accepted after quorum is reached also generated voting power events, this could lead to weird results in processing voting power events. This pr fixes that by making sure that events of any kind are generated only if this is first time quorum is reached.
[context](babylonlabs-io/babylon#130) Closes #123
This follow-up PR fixes the comments in #125, including