babylonlabs-io / babylon-contract

CosmWasm smart contracts for Babylon integration
Other
10 stars 6 forks source link

eots: fix tests for verifying EOTS signatures from Go #32

Closed SebastianElvis closed 3 months ago

SebastianElvis commented 3 months ago

Resolves #28

The test_serialize test is flaky because it assumes Go implementation always provides secret randomness that corresponds to a public randomness with even y coordinate, however this is not the case. This PR fixes this by enforcing the sec rand to always correspond to pub rand with even y coord.

Note that the fact that Go implementation might give secrand corresponding to pubrand with even y coordinate does not affect the security of our Rust implementation, because

This is reflected in the fixed test_serialize where assertions around verify/extract have passed.

NOTE: @KonradStaniec Do you think we should enforce secrand to correspond to pubrand with even y coordinate here? Seems like a good thing to do

maurolacy commented 3 months ago

Do you think we should enforce secrand to correspond to pubrand with even y coordinate here? Seems like a good thing to do

Second that.

KonradStaniec commented 3 months ago

NOTE: @KonradStaniec Do you think we should enforce secrand to correspond to pubrand with even y coordinate here? Seems like a good thing to do

tbh I am not so sure my main objections to it are:

Protocol wise: in our EOTS signing we committed to the BTC flavour of signing i.e the version described in https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki. This version has few characteristics like:

This means that our signing algorithm and signing verification algorithm must account for those choices (the same way as BTC) i.e even if key is odd, the signing algorithm will assume it is even and do the proper transformations. Those are:

R.ToAffine()
if R.Y.IsOdd() {
k.Negate()
}

So this brings us to the proposed change. If our generators, will start to only generate keys with even Y coordinates, we will stop testing code paths which are triggered when Y coordinates are odd, but in reality we will encounter such keys in working system as people will generate keys through other ways

Code wise (less important): I would say that this is unexpected behaviour for the caller if the function called RandGen suddenly decides that it will only generate randomness with even coordinate.

KonradStaniec commented 3 months ago

Thinking on this longer, it probably means that flaky test (failing when point parity is odd) actually reveals that implementation in contract here, is not compatible with the implementation in go.

maurolacy commented 3 months ago

If this is the Bitcoin behaviour, I agree we should keep it.

As you said, let's just make sure the Rust impl is compatible with the Go one.

SebastianElvis commented 3 months ago

So

maurolacy commented 3 months ago

Good to know, thanks.