epoching API: validator lifecycle #109

SebastianElvis · 2022-09-01T02:07:53Z

This PR aims at implementing the validator lifecycle API. In this API, upon a validator address, it outputs the entire lifecycle of this validator, including

the height when the validator is created and becomes bonded
- which happens upon MsgWrappedCreateValidator is handled
the height when the validator becomes unbonding
- which happens upon MsgWrappedUndelegate is handled
the height when the validator becomes unbonded
- which happens when the epoch carrying this validator's MsgWrappedUndelegate msg is checkpointed

Technical approaches

Upon receiving MsgWrappedCreateValidator, this validator enters state ValStateCreateRequestSubmitted at current height
Upon handling MsgWrappedCreateValidator successfully, this validator enters state ValStateBonded at current height
Upon receiving MsgWrappedUndelegate, this validator enters state ValStateUnbondingRequestSubmitted at current height
Upon handling MsgWrappedUndelegate successfully, this validator enters state ValStateUnbonding at current height
Upon unbonding mature validators, each of the validator enters state ValStateUnbonded at current height
All of the above stuff is recorded to KVStore
A new API "/babylon/epoching/v1/validator_lifecycle/{val_addr}" is implemented for reading this KVStore

Misc

I also marked @gitferry as reviewer since this PR touches the checkpointing module in the following. Feel free to have a look and provide comments:

this PR adds a new function InitGenValLifecycle to initialise the lifecycle of the genesis validators
this PR renames genesis_bls.go to genesis_val.go due to the above modification
this PR adds a TODO on porting verification rules in the staking module to WrappedCreateValidator. These are needed for filtering out malformed requests.
this PR adds two functions to the expected epoching keeper for validator lifecycle recording

I also marked @vitsalis as reviewer since this API will be used by the explorer

SebastianElvis · 2022-09-01T09:29:06Z

Just fixed a bug on recording heights of bonded -> unbonding. Only when the validator's operator undelegates from the validator, the validator becomes unbonding from bonded. Previous implementation does not impose such limit, but refreshes the height upon any undelegation of its delegations.

Question: do we need to record the height for delegations undelegating from validators?

vitsalis

Overall, I believe that this validator lifecycle structure needs some extra checks to verify correctness (e.g. see my unbonding height vs unbonded height comment below). A more useful and less complex data structure would be one that contains the validator address, an event (Create|Bonded|Unbonding|Unbonded), and a height.

Then, someone that wants to see the lifecycle of the validator can query and get a list of events corresponding to that validator in the form:

{
val_addr: [(create, 10), (bonded, 13), (unbonding, 27), (unbonded, 42), (bonded, 103), ...]
}

This would also simplify storage and the query endpoint since the validator address could be used as a key.

This would require some computation from the front-end to calculate the current status of the validator, but it would be just traversing the list and implementing a very simple state machine.

vitsalis · 2022-09-01T10:22:39Z

proto/babylon/epoching/v1/epoching.proto

@@ -29,3 +29,26 @@ message QueuedMessage {
    // TODO: after we bump to Cosmos SDK v0.46, add MsgCancelUnbondingDelegation
  }
 }
+
+message ValidatorLifecycle {


What happens if a validator address has multiple bonding/unbonding requests over time?

vitsalis · 2022-09-01T10:29:18Z

x/epoching/keeper/val_lifecycle.go

+	case types.ValStateUnbonding:
+		lc.UnbondingHeight = uint64(ctx.BlockHeight())
+	case types.ValStateUnbonded:
+		lc.UnbondedHeight = uint64(ctx.BlockHeight())


Shouldn't we panic if the UnbondingHeight has not been set here? Accordingly with the above cases.

Such sanity checks on the validator's state transition have been done elsewhere (e.g., in the staking module), right?

SebastianElvis · 2022-09-01T10:58:01Z

Overall, I believe that this validator lifecycle structure needs some extra checks to verify correctness (e.g. see my unbonding height vs unbonded height comment below). A more useful and less complex data structure would be one that contains the validator address, an event (Create|Bonded|Unbonding|Unbonded), and a height.

Then, someone that wants to see the lifecycle of the validator can query and get a list of events corresponding to that validator in the form:
{
val_addr: [(create, 10), (bonded, 13), (unbonding, 27), (unbonded, 42), (bonded, 103), ...]
}
This would also simplify storage and the query endpoint since the validator address could be used as a key.

This would require some computation from the front-end to calculate the current status of the validator, but it would be just traversing the list and implementing a very simple state machine.

Thanks for the comment and totally agree on this! Yeah it's possible that a validator can get bonded/unbonded back and forth. I think your way of recording lists of events is more simple and generic. Will adapt to this approach. 👍

aakoshh · 2022-09-01T22:22:26Z

x/epoching/keeper/msg_server.go

+	}
+	if _, err := sdk.ValAddressFromBech32(msg.Msg.ValidatorDstAddress); err != nil {
+		return nil, err
+	}


Why do we need these extra validations now, but not before this PR?

Without these validations users can submit malformed msgs, eg msgs with empty fields. This was a bug in the code and was not tested. I found this in this PR because when handling these APIs we need the validator address inside the msg, which was allowed to be empty.

aakoshh · 2022-09-01T22:23:18Z

x/epoching/keeper/val_lifecycle.go

+	k.SetValLifecycle(ctx, valAddr, &lc)
+}
+
+// InitValState adds a state for an existing validator lifecycle, including bonded, unbonding and unbonded


Suggested change

// InitValState adds a state for an existing validator lifecycle, including bonded, unbonding and unbonded

// UpdateValState adds a state for an existing validator lifecycle, including bonded, unbonding and unbonded

aakoshh

Ideally the explorer's backend would build up this database from the events in the blocks, similarly to how Vitalis suggested. I was wondering if for such cases there should be a separate module that subscribes to these event; but events are probably not visible to modules.

SebastianElvis · 2022-09-01T22:33:41Z

Ideally the explorer's backend would build up this database from the events in the blocks, similarly to how Vitalis suggested. I was wondering if for such cases there should be a separate module that subscribes to these event; but events are probably not visible to modules.

Agree. In the long term perhaps we need a new module (say metrics) to subscribe a lot of hooks and do statistics. Im also not sure what's the standard way of doing statistics in Cosmos SDK. For the sake of demo perhaps we can have this API for now.

aakoshh · 2022-09-01T22:39:53Z

Sure, for the demo you can have temporary queries, I just thought that if it was easy to add a module (which I don't think it is, unless you use hooks but even that's more than the normal event emission) then it would be very clear which parts of the app exist only to stand in for a missing explorer backend, and it would be easy to remove it later.

SebastianElvis · 2022-09-02T07:04:03Z

Thanks for the comments Akosh and Vitalis! Now this PR is significantly simplified by using hooks and adapts the data structure that is more generic. Specifically, this PR:

adapts the new structure for ValidatorLifecycle, which includes the validator's address and a series of its state transitions, including created, bonded, unbonding, unbonded, and removed
moves all invocations of RecordNewValState to hooks in the staking module. This significantly simplifies the work and makes most of the code for the validator lifecycle maintenance to a single file. The only outlier is the ckpt-assisted unbonding, which does not have a hook and is put under modified_staking.go
avoids touching checkpointing module as much as possible. Now it only modifies minor stuff and added a TODO.
fixes some minor issues/comments

Please feel free to have a look again :)

aakoshh

Great!

aakoshh · 2022-09-03T06:04:16Z

x/epoching/keeper/modified_staking.go

+					k.stk.RemoveValidator(ctx, val.GetOperator())
+				}
+
+				// Babylon modification: record the height when this validator becomes unbonded


Great, thanks for adding these comments!

- Update delegation and undelegation to new btc transactions format

SebastianElvis added 5 commits August 31, 2022 17:22

init

c381929

fix CI

a5f87b7

fix

e9174be

minor

18affe4

init

c5360a2

SebastianElvis requested a review from vitsalis September 1, 2022 02:08

SebastianElvis added 8 commits September 1, 2022 12:18

distinguish created vs bonded

eee5c63

minor

02f314c

init impl

eab8cab

pass CI

1d57202

record ckpt-assisted unbonding time

fe7c550

minor fixes

0f4ff6f

created -> bonded

e974af6

comments and minor fixes

a2b7c3a

SebastianElvis marked this pull request as ready for review September 1, 2022 07:15

SebastianElvis requested review from aakoshh and gitferry September 1, 2022 07:15

record height only when operator undelegates

9fffb70

vitsalis reviewed Sep 1, 2022

View reviewed changes

aakoshh reviewed Sep 1, 2022

View reviewed changes

SebastianElvis added 4 commits September 2, 2022 09:40

Merge branch 'main' into epoching-epoch-events-api

527b82d

tests

54a3127

prepare for fixing CI

91cf670

reject msgs during 0 epoch

7189c38

SebastianElvis added 10 commits September 2, 2022 11:26

reject msgs in 0epoch

39485d2

update event

d592381

fix CI

abe11a4

Merge branch 'main' into epoching-epoch-events-api

146068f

tidy

b73ba34

Merge branch 'epoching-epoch-events-api' into epoching-val-lifecycle

39ee618

mock stuff

83b3aff

fix multiple

dd80914

tidy

0ab0c47

go mod tidy

86d9ffb

SebastianElvis requested review from aakoshh and vitsalis September 2, 2022 07:04

aakoshh approved these changes Sep 3, 2022

View reviewed changes

Base automatically changed from epoching-epoch-events-api to main September 4, 2022 23:30

SebastianElvis added 3 commits September 5, 2022 09:41

Merge branch 'main' into epoching-val-lifecycle

b0be710

resolve duplicated stuff when resolving conflicts

5277a46

minor

33d3094

SebastianElvis merged commit eb75898 into main Sep 4, 2022

SebastianElvis deleted the epoching-val-lifecycle branch September 4, 2022 23:53

vitsalis pushed a commit that referenced this pull request Jan 21, 2024

Update delegation/undelegation to new btc primitives (#109)

7e54265

- Update delegation and undelegation to new btc transactions format

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epoching API: validator lifecycle #109

epoching API: validator lifecycle #109

SebastianElvis commented Sep 1, 2022 •

edited

Loading

SebastianElvis commented Sep 1, 2022

vitsalis left a comment

vitsalis Sep 1, 2022

vitsalis Sep 1, 2022

SebastianElvis Sep 2, 2022

SebastianElvis commented Sep 1, 2022

aakoshh Sep 1, 2022

SebastianElvis Sep 1, 2022

aakoshh Sep 1, 2022

aakoshh left a comment

SebastianElvis commented Sep 1, 2022

aakoshh commented Sep 1, 2022

SebastianElvis commented Sep 2, 2022 •

edited

Loading

aakoshh left a comment

aakoshh Sep 3, 2022

	// InitValState adds a state for an existing validator lifecycle, including bonded, unbonding and unbonded
	// UpdateValState adds a state for an existing validator lifecycle, including bonded, unbonding and unbonded

epoching API: validator lifecycle #109

epoching API: validator lifecycle #109

Conversation

SebastianElvis commented Sep 1, 2022 • edited Loading

Technical approaches

Misc

SebastianElvis commented Sep 1, 2022

vitsalis left a comment

Choose a reason for hiding this comment

vitsalis Sep 1, 2022

Choose a reason for hiding this comment

vitsalis Sep 1, 2022

Choose a reason for hiding this comment

SebastianElvis Sep 2, 2022

Choose a reason for hiding this comment

SebastianElvis commented Sep 1, 2022

aakoshh Sep 1, 2022

Choose a reason for hiding this comment

SebastianElvis Sep 1, 2022

Choose a reason for hiding this comment

aakoshh Sep 1, 2022

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

SebastianElvis commented Sep 1, 2022

aakoshh commented Sep 1, 2022

SebastianElvis commented Sep 2, 2022 • edited Loading

aakoshh left a comment

Choose a reason for hiding this comment

aakoshh Sep 3, 2022

Choose a reason for hiding this comment

SebastianElvis commented Sep 1, 2022 •

edited

Loading

SebastianElvis commented Sep 2, 2022 •

edited

Loading