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

Do not return errors for duplicated signatures #449

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

KonradStaniec
Copy link
Contributor

  • pr removes erros for duplicated finality and covenant signatures
  • add testcase for duplicate covenant signatures

Note:
It also adds todo to refactor adding covenant signatures to delegation as it is not optimal. This can be done in separate pr to not block update on finality provider.

Copy link
Contributor

@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.

Lgtm!

@@ -205,7 +205,7 @@ func (d *BTCDelegation) AddCovenantSigs(covPk *bbn.BIP340PubKey, sigs []asig.Ada
}
// ensure that this covenant member has not signed the delegation yet
if d.IsSignedByCovMember(covPk) {
return ErrDuplicatedCovenantSig
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add log here for now and we can add metrics for motoring in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea this is a bit which requires refactor. Currently this is a functin which is defined on BTCDelegation which does not have access to logger (and it should not have it)

In general this whole bit require moving those:

	if d.HasCovenantQuorums(quorum) {
		return nil
	}
	// ensure that this covenant member has not signed the delegation yet
	if d.IsSignedByCovMember(covPk) {
		return nil
	}

those checks up the stack to Keeper or Msg_server , and addint proper logging there.

I would do it in next pr, so finality provider can update with this breaking change quickly. Next pr will be not breaking (it will be just some code golf)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah makes sense. Thanks for the explaining!

@@ -79,7 +79,7 @@ func (ud *BTCUndelegation) AddCovenantSigs(
}

if ud.IsSignedByCovMember(covPk) {
return ErrDuplicatedCovenantSig
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add log here for now and we can add metrics for motoring in the future

@@ -205,7 +205,7 @@ func (d *BTCDelegation) AddCovenantSigs(covPk *bbn.BIP340PubKey, sigs []asig.Ada
}
// ensure that this covenant member has not signed the delegation yet
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment can be cut or rephrased

@KonradStaniec KonradStaniec merged commit d166d40 into dev Feb 5, 2024
4 checks passed
@KonradStaniec KonradStaniec deleted the do-not-return-errors-for-duplicated-signatures branch February 5, 2024 14:26
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.

None yet

2 participants