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: fixing witness construction of slashing tx #193

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Oct 14, 2024

Also adapts all tests to ensure the witness construction works even if all covenant members provide signatures.

@SebastianElvis SebastianElvis requested a review from a team as a code owner October 14, 2024 09:39
@SebastianElvis SebastianElvis requested review from Lazar955 and removed request for a team October 14, 2024 09:39
@@ -274,6 +274,7 @@ func (tx *BTCSlashingTx) BuildSlashingTxWithWitness(
outputIdx uint32,
delegatorSig *bbn.BIP340Signature,
covenantSigs []*asig.AdaptorSignature,
covenantQuorum uint32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can return error early from this function i.e:

if len(covenantSigs) < covenantQuorum {
  return nil, fmt.Errorf("not enough signatures")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! As covenantSigs can contain nil (in order to have the length of covenant committee size), I added the below logic below L303

	// ensure the number of covenant signatures is at least the quorum number
	if numSigs < covenantQuorum {
		return nil, fmt.Errorf("not enough covenant signatures to reach quorum")
	}

does this make sense?

@SebastianElvis SebastianElvis merged commit c35c6a0 into main Oct 14, 2024
20 checks passed
@maurolacy maurolacy deleted the fix-cov-sig-witness branch October 14, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants