chore: Add inclusion height to early unbonding event #228
Conversation
gitferry
added
consensus breaking
gitferry
requested review from
KonradStaniec,
samricotta and
jrwbabylonlab
and removed request for
a team
| 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; | ||
| // inclusion_height is the height of the block in which the spend_staking_tx is included | ||
| uint32 inclusion_height = 2; |
There was a problem hiding this comment.
So this makes me a bit nervous, as due to re-orgs is is possible that this inclusion height can change. So we can be left with data is database that do not match reality.
In other words, I am fine with putting this info in event which is kind of ephemeral thing and we can treat it as inclusion height at the time of receiving unbonding transaction on BTC but I am not too sure about storing this data in database.
There was a problem hiding this comment.
I agree that re-orgs can cause mismatch issues. The tricky thing is that in our current flow, we deal with undelegate in message handler but deal with the relevant event in BeginBlocker. This means that the inclusion height is passed via db write and read. If we don't want to store the height in db, we would have to emit the early unbonding event in the message handler directly. So, two options:
- leave it what it is with a note that the inclusion height is subjective to re-orgs, maybe rename it to
reported_inclusion_heightas it is only used for emitting events for now - do not store the inclusion height but emit the early unbonding event in the message handler. This will change the logic of handling delegation events a bit and make the early unbonding a tx event
I'll try with option 2 to see if it could work without significant change
There was a problem hiding this comment.
@gitferry for (2), does it only work for new events, or it can be replayed?
There was a problem hiding this comment.
2 would most probably work but then: expiration event would be block event, and early unbonding would be transaction event. It is a bit inconsistent from api perspective, but I prefer that over storing inconsistent data in state tbh.
There was a problem hiding this comment.
it can be replayed just like other events
There was a problem hiding this comment.
Thanks @jrwbabylonlab and @KonradStaniec for the discussion. I have changed it to (2) in 59bfc27.
@jrwbabylonlab please keep in mind that early unbonding will be a tx event while expired unbonding is still a block event.
Btw, do we want to backport it to release/v0.14.x?
gitferry
changed the title
gitferry
removed
the
consensus breaking
gitferry
force-pushed
the
chore/add-unbonding-height-in-event
branch
from
8dd0da3 to
59bfc27
Compare
Closes #225. * added unbonding start height to early unbonding event * changed that early unbonding event is emitted as a tx event
Closes #225.