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 non-determistic fakenet validator key with Go 1.20 #428

Merged
merged 9 commits into from
Mar 20, 2023

Conversation

hadv
Copy link
Contributor

@hadv hadv commented Feb 22, 2023

There's a change of package crypto/ecdsa from Go 1.20.1 that not generating deterministic keys using GenerateKey(). On the fakenet, we call get fake validator keys in 3 difference places (genesis, config and keystore) so the fake validator keys were difference, which make the error.

We need to find a way to generate deterministic keys or generating only one time and re-use in other place. The issue might happen again when user running fakenet on multiple validators then the fake validators on each fakenet node might be difference as well.

So we might to hard code the fakenet validator keys, but the issue is the total number of validators is not fixed but for testing I think we can limit to maximum 10 validators?

@hadv hadv marked this pull request as ready for review March 9, 2023 03:42
@hadv hadv requested a review from andrecronje as a code owner March 9, 2023 03:42
@hadv
Copy link
Contributor Author

hadv commented Mar 9, 2023

Note: by the way, some tests on https://github.com/Fantom-foundation/go-opera/blob/develop/gossip/mps_test.go and https://github.com/Fantom-foundation/go-opera/blob/master/gossip/heavycheck_test.go will be failed if we use random fakenet validators. It seems that we has some specific assumption on the test code

But if we generate deterministic validator keys from old version of crypto lib, then all the test passed now.

key90, key91, key92, key93, key94, key95, key96, key97, key98, key99,
key00,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would put variables keys and key00..key99 inside FakeKey to make them local as they are accessed only in FakeKey. Why you add key00 second time in L149?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, key00 actually is key100, the key00 at zero index is not used because the validator key start from 1

@hadv hadv requested a review from cyberbono3 March 13, 2023 10:50
key70, key71, key72, key73, key74, key75, key76, key77, key78, key79,
key80, key81, key82, key83, key84, key85, key86, key87, key88, key89,
key90, key91, key92, key93, key94, key95, key96, key97, key98, key99,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the type of n to uint to satisfy your condition at L211 as n can not be negative. I think, there is an idiomatic way to declare multiple variables inside FakeKey like that:

var (
     key = ...
     keys = [100]*ecdsa.PrivateKey{
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, there is an idiomatic way to declare multiple variables inside FakeKey like that:

Okay, I write code to generate this code segment so didn't notice this

I would change the type of n to uint

idx.ValidatorID type actually is uint32 to I changed the type of n to uint32 as well

Copy link
Contributor

@quan8 quan8 Mar 15, 2023

Choose a reason for hiding this comment

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

Wondering if these will work:

var seed = []byte("163f5f0f9a621d72fedd85ffca3d08d131ab4e812181e0d30ffd1c885d20aac73144c0aa4ced56dc15c79b045bc5559a5ac9");

.......

// FakeKey gets n-th fake private key.
func FakeKey(n uint32) *ecdsa.PrivateKey {
if n == 0 || n > 100 {
panic(errors.New("validator num is out of range"))
}

// shift left n characters of the seed to the end
var shifted = string(append(seed[n:], seed[:n]...))
// and get the first 64 bytes
var k = "0x" + string(shifted[:64]);

var key, err = crypto.ToECDSA(hexutil.MustDecode(k))

if err != nil {
	panic(err)
}

return key

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it might work but we want to keep the fakekey as same as when running below code on old version of golang. So we pre-generated all the first 100 keys from old version of golang and hardcode into the source like that.

	reader := rand.New(rand.NewSource(int64(n)))

	key, err := ecdsa.GenerateKey(crypto.S256(), reader)

@hadv hadv requested a review from cyberbono3 March 14, 2023 16:26
Copy link
Contributor

@cyberbono3 cyberbono3 left a comment

Choose a reason for hiding this comment

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

SGTM

@uprendis uprendis merged commit 2763740 into Fantom-foundation:develop Mar 20, 2023
@hadv hadv deleted the fakenet-genkey-fix branch March 21, 2023 15:23
@hadv hadv mentioned this pull request Mar 22, 2023
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.

4 participants