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 handling of the restart during unbonding #20

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

KonradStaniec
Copy link
Collaborator

Fixes: #18

Fixes handling of the case in which program crashes in between sending of unbonding transactions and unbonding transaction confirmation.

Note: this is still not perfect, as it may happen that:

  • user requests unbonding
  • call is returned to the user about success
  • program crashes before unbonding tx is sent to btc
  • to handle this properly new state would need to be introduces UNBONDING_REQUESTED but this would be breaking change to existing db that we want to avoid rn.

Follow up task: #19

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Nice fix! A refactoring and unit tests can be done in a separate PR

Comment on lines 680 to 682
// delegation was send to Babylon and activated by covenants, check wheter we:
// - did not spent tx beore restart
// - did not sent unbonding tx before restart
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// delegation was send to Babylon and activated by covenants, check wheter we:
// - did not spent tx beore restart
// - did not sent unbonding tx before restart
// delegation was sent to Babylon and activated by covenants, check whether we:
// - did not spend tx before restart
// - did not send unbonding tx before restart

}

if !stakingOutputSpent {
continue
Copy link
Member

Choose a reason for hiding this comment

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

we can add a comment here explaining we don't need to do anything if the output is not spent

return err
}

// unbonding tx is in mempool, wait for confirmation and infrom event
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// unbonding tx is in mempool, wait for confirmation and infrom event
// unbonding tx is in mempool, wait for confirmation and inform event

Comment on lines 714 to 716
if err := app.txTracker.SetTxSpentOnBtc(stakingTxHash); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

seems we can have app.MustSetTxSpentOnBtc to avoid embedded if condition and panic upon err

Comment on lines 727 to 729
if err := app.txTracker.SetTxSpentOnBtc(stakingTxHash); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

same here

@KonradStaniec KonradStaniec merged commit 17fad51 into dev Aug 9, 2024
8 checks passed
@KonradStaniec KonradStaniec deleted the fix/improve-restart-after-unbonding branch August 9, 2024 06:55
@gitferry gitferry restored the fix/improve-restart-after-unbonding branch August 9, 2024 15:45
KonradStaniec added a commit that referenced this pull request Aug 12, 2024
* Fix handling of the restart during unbonding
gitferry pushed a commit that referenced this pull request Aug 13, 2024
* Fix handling of the restart during unbonding
huynaism added a commit that referenced this pull request Nov 8, 2024
filippos47 pushed a commit that referenced this pull request Nov 22, 2024
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.

2 participants