Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: duplicate state history #151

Closed
wants to merge 7 commits into from

Conversation

gusin13
Copy link
Collaborator

@gusin13 gusin13 commented Feb 2, 2025

Fixes - #115

This is the usual flow of events

  1. delegation created
  2. inclusion proof received
  3. quorem reached and so on

Now there are 2 staking flows

  1. preaproval flow
  2. old/legacy flow

in the old flow babylon emits state in inclusion proof event
while in the new flow babylon emits state in inclusion proof event

the problem was indexer used to initialize state when 1.delegation was created
this created double entry in old flow because when inclusion proof is received, babylon will again send state.

so in this pr, i have created a new state which is the initial state when delegation is created, we need not store this in state history, the state history will be inserted once the inclusion proof is received so

  1. Verified -> Active (in case of preaprooval )
  2. Pending -> Active (in case of old flow)

notice there is no Pending state now in the pre-approval flow

@gusin13 gusin13 marked this pull request as ready for review February 2, 2025 20:22
@gusin13 gusin13 marked this pull request as draft February 2, 2025 20:31
@@ -10,6 +10,7 @@ import (
type DelegationState string

const (
StateCreated DelegationState = "CREATED" // Initial state when a delegation is first created in the indexer before receiving inclusion proof or covenant signatures
Copy link
Collaborator

@jrwbabylonlab jrwbabylonlab Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gusin13 why not we skip the state update if detected the previous state + sub state is the same?
Changing the pending to created here would break the indexer + api + FE contract on state transitions, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes if we add new state then it will break the api contract

in general my thinking was if the prev state/substate combination is same then it is a bug in indexer and shouldn't happen, so indexer shouldn't handle this case

there is another way to solve the problem without making a new state, as this problem only occurs in old flow, so

  1. when delegation is created - we mark as Pending (already being done before this pr)
  2. when inclusion proof received - babylon emits Pending and indexer w/o checking inserts Pending again, in this case I simply check if this is old flow don't insert Pending

even after the above solution if there are duplicates then its another bug

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, the more I feel like it's a bug on BBN side. Why does babylon emit pending state twice from difference messages?
Does the inclusion proof received event require emitting the pending state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, so babylon actually doesn't emit twice, it only emits Pending state in the old flow in the inclusion proof event
so actually the transition in babylon events is like this
Verified -> Active (new flow)
Pending -> Active (old flow)

but the confusing thing is Babylon has a GetStatus method which returns the current status of delegation, so if the covenant quoram has not reached it treats it as Pending

so internally query wise, Babylon returns Pending (in both flows) but emits Pending only in the old flow.

the indexer should ideally when the delegation is created should use Pending state by default (becasue the quorem hasn't been achieved yet, we were already doing this before this pr)

@@ -100,7 +100,7 @@ func TestStakingEarlyUnbonding(t *testing.T) {
stakingMsgTxHash := stakingMsgTx.TxHash()

// Wait for delegation to be PENDING in Indexer DB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this comment be relevant after these changes ?

@@ -4,6 +4,7 @@ on:
push:
branches:
- 'main'
- '115-duplicate-entries-in-state_history-2'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please elaborate why this change is required, or you using it while developing

@gusin13
Copy link
Collaborator Author

gusin13 commented Feb 3, 2025

Sorry guys, realized in offline discussion this is not needed. The duplicated event is indeed legit.

@gusin13 gusin13 closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants