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 signing issue if the message is leading with 0x00 #284

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

zargarzadehm
Copy link
Contributor

for issue #264

@ZhAnGeek
Copy link
Collaborator

ZhAnGeek commented Jan 4, 2024

Hi @zargarzadehm Thanks for submitting codes! It will work well, however The PR will change m from big.Int to bytes, it will change the interface of NewLocalParty which are highly depended by upstreams. I would recommend use big.Int.FillBytes() when necessary. Could you have an update on it accordingly?

@zargarzadehm
Copy link
Contributor Author

Hi @zargarzadehm Thanks for submitting codes! It will work well, however The PR will change m from big.Int to bytes, it will change the interface of NewLocalParty which are highly depended by upstreams. I would recommend use big.Int.FillBytes() when necessary. Could you have an update on it accordingly?

yeah, I will update ASAP!

@zargarzadehm
Copy link
Contributor Author

@ZhAnGeek
I tried using big.Int.FillBytes() for testing, but I came across an issue regarding the variable size of the signData. Since signData is non-deterministic, I couldn't create a buffer for it. Moreover, I observed that big.Int.BitLen returns the size of the slice without considering leading zeros.

@ZhAnGeek
Copy link
Collaborator

ZhAnGeek commented Jan 10, 2024

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

@zargarzadehm
Copy link
Contributor Author

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.

If you have another option for discussion instead of Github tell me!

@ZhAnGeek
Copy link
Collaborator

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.

If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

@ZhAnGeek ZhAnGeek closed this Jan 10, 2024
@ZhAnGeek ZhAnGeek reopened this Jan 10, 2024
@zargarzadehm
Copy link
Contributor Author

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.
If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

We can have an optional parameter FullBytesLen to NewLocalParty for the size of bigInt and if the size is set we will use FillBytes otherwise use Bytes (like the old function)

@ZhAnGeek
Copy link
Collaborator

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.
If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

We can have an optional parameter FullBytesLen to NewLocalParty for the size of bigInt and if the size is set we will use FillBytes otherwise use Bytes (like the old function)

Sounds great! we could do that way.

ackratos
ackratos previously approved these changes Jan 11, 2024
@zargarzadehm
Copy link
Contributor Author

@ZhAnGeek

I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
	msg *big.Int,
	params *tss.Parameters,
	key keygen.LocalPartySaveData,
	keyDerivationDelta *big.Int,
	out chan<- tss.Message,
	end chan<- *common.SignatureData,
	fullBytesLen int,
)

@@ -61,14 +61,20 @@ func (round *finalization) Start() *tss.Error {
round.data.S = padToLengthBytesInPlace(sumS.Bytes(), bitSizeInBytes)
round.data.Signature = append(round.data.R, round.data.S...)
round.data.SignatureRecovery = []byte{byte(recid)}
round.data.M = round.temp.m.Bytes()
common.Logger.Infof("checkkkkkkk %v", round.temp.fullBytesLen == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

,logger removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@@ -95,7 +95,7 @@ signing:
go updater(parties[dest[0].Index], msg, errCh)
}

case <-endCh:
case xx := <-endCh:
Copy link
Collaborator

Choose a reason for hiding this comment

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

msg or signdata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@ZhAnGeek
Copy link
Collaborator

ZhAnGeek commented Jan 12, 2024

@ZhAnGeek

I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
	msg *big.Int,
	params *tss.Parameters,
	key keygen.LocalPartySaveData,
	keyDerivationDelta *big.Int,
	out chan<- tss.Message,
	end chan<- *common.SignatureData,
	fullBytesLen int,
)

Ahh I suggested you can change it to optional parameters as well, this is an interface use for HD, should be dependent

@zargarzadehm zargarzadehm reopened this Jan 12, 2024
@zargarzadehm zargarzadehm changed the title fix signing issue if the message is leading with 0x00 WIP: fix signing issue if the message is leading with 0x00 Jan 12, 2024
@zargarzadehm zargarzadehm changed the title WIP: fix signing issue if the message is leading with 0x00 fix signing issue if the message is leading with 0x00 Jan 12, 2024
@zargarzadehm
Copy link
Contributor Author

@ZhAnGeek
I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
	msg *big.Int,
	params *tss.Parameters,
	key keygen.LocalPartySaveData,
	keyDerivationDelta *big.Int,
	out chan<- tss.Message,
	end chan<- *common.SignatureData,
	fullBytesLen int,
)

Ahh I suggested you can change it to optional parameters as well, this is an interface use for HD, should be dependent

ok, as I know there isn't optional input in Golang functions but as an alternative solution I used Variadic Parameters for FullBytesLength parameter and the merge is ready for review!

Copy link
Collaborator

@ZhAnGeek ZhAnGeek left a comment

Choose a reason for hiding this comment

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

lg

@ZhAnGeek ZhAnGeek merged commit c0de534 into bnb-chain:master Jan 15, 2024
1 check passed
@zargarzadehm
Copy link
Contributor Author

zargarzadehm commented Jan 15, 2024

@ZhAnGeek
Do you have any plans for new release? As I see there are around 7 PR after latest release!

@ZhAnGeek
Copy link
Collaborator

@zargarzadehm
New release has been launched, thank you! https://github.com/bnb-chain/tss-lib/releases/tag/v2.0.2

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

3 participants