feat: Implement ADR-29 - generalized unbonding #215
Conversation
KonradStaniec
added
api breaking
KonradStaniec
requested review from
Lazar955,
samricotta,
gitferry and
SebastianElvis
and removed request for
a team
|
added more reviewers than normal as this is pretty substantial change to the protocol |
| // spend_stake_tx_inclusion_block_hash is the block hash of the block in which | ||
| // spend_stake_tx was included | ||
| bytes spend_stake_tx_inclusion_block_hash = 2; | ||
| // spend_stake_tx_inclusion_index is the index of spend_stake_tx in the block | ||
| uint32 spend_stake_tx_inclusion_index = 3; |
There was a problem hiding this comment.
Do we need to keep L123-127 in the KV store? It seems not as they are only be used upon verification but nothing else
There was a problem hiding this comment.
tbh my main use for those fields was to make it possible to easily check this transaction on Bitcoin node i.e when you have blockhash and transaction index you can retrieve tx from BTC node even if it does not have --txindex flag.
So later it is easier to check unbondings (normal or not) against BTC network , and the cost is only additional 40bytes of data. Do you think it is useful or overkill from node perspective ?
There was a problem hiding this comment.
It's a bit overkill imo -- since we have millions of BTC delegations these extra fields might still incur some cost in the states. Also since we know the tx hash, we can find relevant block that contains this tx using Bitcoind APIs. Wdyt?
There was a problem hiding this comment.
It's a bit overkill imo -- since we have millions of BTC delegations these extra fields might still incur some cost in the states
For 5m delegation it will be around 50MB of data which seems negligible, but I understand your point: 50MB here, 50MB there and it adds up.
In 74db17e:
- I have removed it from the state
- though I left it in the event, as this is not part of the state and this piece of data maybe important for somebody
| message DelegatorUnbondingInfo { | ||
| // spend_stake_tx is the transaction which spent the staking output. It is | ||
| // filled only if spend_stake_tx is different than unbonding_tx registered | ||
| // on the Babylon chain. | ||
| bytes spend_stake_tx = 1; | ||
| } |
There was a problem hiding this comment.
Do we still need this struct given that it only contains 1 field?
There was a problem hiding this comment.
yup, this signals whether unbonding actually happened i.e
if del.BtcUndelegation.DelegatorUnbondingInfo != nil
delegation is considered unbonded.
And if
len(del.BtcUndelegation.DelegatorUnbondingInfo.SpendStakeTx) > 0
then unbonding happened through unexpected unbonding transaction i.e the transaction different that del.BtcUndelegation.UnbondingTx
Assuming most of unbondings will happen through unbonding transaction committed to Babylon, this modelling enables us to save space i.e we do not store stake spending transaction if it happens to be already known unbonding transaction.
There was a problem hiding this comment.
OK sounds good. Probably we can wrap some functions for BTCDelegation to signal these in the future 👍
| // - spend_stake_tx_inclusion_block_hash: the block hash of the block in which | ||
| // spend_stake_tx was included | ||
| // - spend_stake_tx_sig_inclusion_index: the index of spend_stake_tx in the block |
There was a problem hiding this comment.
Nit: remove outdated documentation.
Highlights:
MsgBTCUndelegatecontains stake spending transaction as well as inclusion proofBTCDelegationcontains news fieldDelegatorUnbondingInfoinstead of old staker signature fieldDelegatorUnbondingInfocontains information about in which header unbonding happened. Also, ifStakeSpendingTxin the message was different that registeredUnbondingTx,DelegatorUnbondingInfocontains whole stake spending tx.