feat(ADR-028): refunding transaction fee for certain txs #125
Conversation
SebastianElvis
changed the title
SebastianElvis
requested review from
KonradStaniec,
RafilxTenfen,
gitferry,
Lazar955 and
a team
and removed request for
a team
| // if all messages in the tx are refundable, refund the tx | ||
| if len(refundableMsgHashList) == len(tx.GetMsgs()) { | ||
| err := d.k.RefundTx(ctx, feeTx) | ||
| if err != nil { |
There was a problem hiding this comment.
why not return the error if it fails to refund?
There was a problem hiding this comment.
If this returns error then the tx will be rejected and all resulting state transition will be reverted. It's a bit unexpected to fail the tx due to fee refund, but tbh not sure if it's a good thing to do. If RefundTx fails it might mean something is really wrong. Will think more about it tmr
| @@ -41,6 +41,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
|
|
|||
| ### State Machine Breaking | |||
|
|
|||
| * [#125](https://github.com/babylonlabs-io/babylon/pull/125) Implement ADR-028 and | |||
There was a problem hiding this comment.
this should go to unreleased
| @@ -92,6 +92,11 @@ func (ms msgServer) InsertBTCSpvProof(ctx context.Context, req *types.MsgInsertB | |||
| return nil, err | |||
| } | |||
|
|
|||
| // At this point, the BTC checkpoint is considered the first valid submission | |||
There was a problem hiding this comment.
this is not exactly true as it may be as well not first submission. If the intention is to refund only first one, the place to do so would be elsewere.
On the other hand, I thought we want to refund all valid submissions ?
There was a problem hiding this comment.
Yeah this is a typo we want to refund all valid submissions (more specifically, every unique pair of BTC txs). Will clarify in a follow-up
| @@ -466,6 +470,14 @@ func (ms msgServer) AddCovenantSigs(goCtx context.Context, req *types.MsgAddCove | |||
| params, | |||
| ) | |||
|
|
|||
| // at this point, the covenant signatures are verified and are not duplicated. | |||
| // Thus, we can safely consider this message as refundable | |||
| // NOTE: currently we refund tx fee for covenant signatures even if the BTC | |||
There was a problem hiding this comment.
hmm this is not entirely true tbh. We have check for quorum super early in the handler:
if btcDel.HasCovenantQuorums(params.CovenantQuorum) {
ms.Logger(ctx).Debug("Received covenant signature after achieving quorum", "covenant pk", req.Pk.MarshalHex())
return &types.MsgAddCovenantSigsResponse{}, nil
}
so this won't be refunded. And refunding it would require a bit of refactor.
Either way I think we cannot refund such case, as then covenant committe member could send his signature forever and bloating chain with it.
There was a problem hiding this comment.
Hmm I see. So this won't get refunded in the current impl
I actually think we should refund when covenant quorum is reached. Otherwise for each BTC delegation the covenant member has a rough probability of (covenant size - covenant quorum) / covenant size to pay some tx fee, so running a covenant emulator won't be free. But the goal of refunding is to ensure running covenant emulator does not incur tx fee.
Either way I think we cannot refund such case, as then covenant committe member could send his signature forever and bloating chain with it.
Does this attack vector exist? for each BTC delegation a covenant member can only send 1 valid covenant signature, and subsequent ones will be rejected and won't get refunded (ref).
There was a problem hiding this comment.
oh you are right.
So if we reject duplicates early and not refund them this is fine.
Though handler need refactor either way, as we should refund signatures after quorum only if they are valid and pass all checks.
| // iterate over all messages in the tx, and record whether they are refundable | ||
| for _, msg := range tx.GetMsgs() { | ||
| msgHash := types.HashMsg(msg) | ||
| if d.k.HasRefundableMsg(ctx, msgHash) { |
There was a problem hiding this comment.
I think we need to filter our the duplicated form the message list or do refunds only in case when there is 1 message in transaction.
Consider case, if finality provider woud create transactions with finality votes:
- First vote would be valid finality vote which would be indexable for refund
- All other votes would be duplicates
Transaction would be successful as duplicated vote is not an error:
if err == nil && existingSig.Equals(req.FinalitySig) {
ms.Logger(ctx).Debug("Received duplicated finiality vote", "block height", req.BlockHeight, "finality provider", req.FpBtcPk)
// exactly same vote already exists, return success to the provider
return &types.MsgAddFinalitySigResponse{}, nil
}
Now finality provider can bloat the ledger for free i.e he can send transactions with 100 duplicated votes and not pay anything.
There was a problem hiding this comment.
In this example, upon duplicated finality signature the function will return directly without calling IndexRefundableMsg, so this condition won't be met and the tx won't get refunded. Or did I miss anything?
There was a problem hiding this comment.
Per offline discussion, this is an issue: the problem is that these messages share the same hash and then all of them are considered refundable in the posthandler. Will address this issue by doing the following:
- deduplicate hashes in posthandler before comparing the length
- return error upon duplicated finality signature to be extra safe, as this gets more sensitive in security
This follow-up PR fixes the comments in #125, including - changelog and typo comments in BTCCheckpoint module - allow covenant signatures after covenant quorum are reached, and return error if the covenant signature is duplicated. This is to ensure covenants won't have operational cost when submitting covenant sig late, and avoid refunding to duplicated covenant signatures - reject duplicated finality signature. This is to avoid refunding to duplicated covenant signatures - add a dedup check in the PostHandler, to ensure one won't exploit the refunding by having many duplicated messages in a single tx. Also added a fuzz test for this.
Resolves https://github.com/babylonlabs-io/pm/issues/64
This PR introduces the functionality of refunding fee for certain txs, including BTC headers/checkpoints in BTC timestamping protocol, and finality signature, BTC delegation inclusion proof, covenant signatures, undelegation, selective slashing evidence in BTC staking protocol.
The implementation leverages a new key-only KV store in the incentive module for storing refundable messages, and a new PostHandler for refunding a tx if all msgs in it are refundable.
Along the way, this PR also adds dependency from BTC light client / BTC staking modules to the incentive module, and adds relevant e2e tests to ensure that the tx fee refunding indeed works.