swarm/network: Use different privatekey for bzz overlay in sim

[nolash]

Depends on #1302

A swarm node has different values for swarm overlay address and p2p connectivity id. However, in simulations the same value is used. This has in some cases led to confusion.

This PR provides a function to generate a swarm overlay address that is deterministically dependent on the p2p id (more precisely, its private key) but where the resulting value is different. The private key will be available through a respective simulation bucket.

When generating networks using the swarm/network/simulation package, this change is now built in (see swarm/network/simulation/node.go:AddNode). However, when booting from snapshots, it is currently up to the client code defining the node.Service implementations to explicitly choose different values.

An example of the latter is implemented for the swarm/pss/prox_test.go. Corresponding changes for other simulation code will be amended in future PR(s).

[holisticode]

just a minor thing I believe

[frncmx]

Could you extract []byte{0x62, 0x7a, 0x7a, 0x62, 0x7a, 0x7a, 0x62, 0x7a} into a variable so that we don't have magic constants? What these hex number anyway? I don't know what they mean.

[nolash]

Hint: hex.

And I tend to think that a bit of cryptic easter egg fun in the code raises spirits. You don't agree, perhaps?

[frncmx]

I see these are hex numbers, but still magic constants. <- 🔥

So, I guess I disagree.
As I feel we already have enough cryptic code to understand. 😉
Let's make fun of solving the already existing puzzles first. 👍

[nolash]

Let's make fun of solving the already existing puzzles first.

That would be magic.

[zelig]

fun is ok bzz, but why are we doing this again? what was wrong with this?

[frncmx]

Okay. getCmdParams is splitting the test name into params. Well, I disagree with the naming. It does not deal with command line arguments so Cmd is improper. But please, do use table driven tests instead. You don't need string splitting and parsing that way.

[zelig]

minor comments

[zelig]

fun is ok bzz, but why are we doing this again? what was wrong with this?

[nolash]

fun is ok bzz, but why are we doing this again? what was wrong with this?

@zelig we should not use the same key for p2p and swarm. We should enforce that separation both in binary (which wasn't done either, in fact) and in sims. bzz bzz.

[nolash]

@frncmx it's still "in process" ;) but thanks, yes, I will mind the comments.

[frncmx]

@frncmx did you have any further comments, please? thanks.

After yesterday stand-up I thought that you will address the last remaining comment about BzzKeyFromConfig. That is extracting the magic array into a meaningful variable.

I understand that you think it's a funny Easter egg. Please keep in mind, that you write your code for others to understand and not to entertain them.

---

After looking at BzzKeyFromConfig() even more I understand even less. So, please try to fix my ignorance.

• All the other places we generate the bzz key from the public key and not from the private key. Why do we use the private key here?
• Further you say that ecdsa.GenerateKey() requires entropy as the second parameter is a slice called rand. However we don't really provide entropy but some deterministically crafted data. So the comment is confusing for me.
• Could we generate the bzzkey as just is done in cmd/swarm/main.go:237 - keys() , i.e., bzzkey := crypto.Keccak256Hash(pubkey).Hex() as that seems simpler?

[nolash]

@frncmx

All the other places we generate the bzz key from the public key and not from the private key. Why do we use the private key here?

Did you actually read the description of the PR? Please read again along with ethereum/go-ethereum#19309 . If it is still not clear, I will try to add a more thorough explanation.

Further you say that ecdsa.GenerateKey() requires entropy as the second parameter is a slice called rand. However we don't really provide entropy but some deterministically crafted data. So the comment is confusing for me.

I will add a more precise comment.

Could we generate the bzzkey as just is done in cmd/swarm/main.go:237 - keys() , i.e., bzzkey := crypto.Keccak256Hash(pubkey).Hex() as that seems simpler?

No. In fact, cmd/swarm/main.go should also be changed to use the same function fo generating the key from private key as the others that were changed. Again, please read the description.

Please keep in mind, that you write your code for others to understand and not to entertain them.

I'm not sure if patronizing me is very useful.

[frncmx]

Did you actually read the description of the PR? Please read again along with ethereum#19309 . If it is still not clear, I will try to add a more thorough explanation.

Yes, I read. To my understanding the description just says that the overlay address and p2p connectivity id should be different in simulations. As it used to be the same before. The description also says we should use the private key, but does not talk about the why.

As I've limited understanding of the code under change; I thought you might fill in the blanks for me. As switching from public key to private seemed like a radical change.

I will add a more precise comment.

Thanks.

No. In fact, cmd/swarm/main.go should also be changed to use the same function fo generating the key from private key as the others that were changed. Again, please read the description.

I read the description again. I might be mistaken, but I did not find the reference that we intended to change the key generation at other places. Also, I'm not sure when do we want to do that? Is that part of this change effort?

I'm not sure if patronizing me is very useful.

I did not mean that. I just don't understand why such a simple thing - like creating a variable called padding - results in a huge push-back from you. As I pointed out I find that code part hard to read and that could be easily fixed by a variable. If such simple changes require the amount of energy (explanations) we are currently putting in, that makes me very uncomfortable while reviewing your changes. In this specific case I feel I way exceeded the reasonable amount of energy spent on that one line. It just seems meaningless. So do as you wish.

[nolash]

As switching from public key to private seemed like a radical change.

Ahh, so there is a misunderstanding here. We are not generating it from the private key, still from the public key. But the name of this method is stale, from the point where it was actually returning the bzzkey. I've changed the method name. The only thing this method does is to generate the private key from whose public key the bzzkey will be created.

The pattern with "deterministic randomness" is used several places in our code. I prefer that kind of approach personally.

that makes me very uncomfortable while reviewing your changes.

I don't mean to make you feel uncomfortable. My apologies. But I simply don't agree with you that we can't risk a bit of fun once in awhile. And we have to be able to cope with disagreement, @frncmx . If there is a difference of opinion on something, we leave it to a third party to be the tiebreaker. And the tie was broken. There was no need to push this further.

That said, I've changed the comment description above this line. It should be pretty clear now that the given data is inconsequential, aswell as not pointing out that it's not actual entropy. Thanks for pointing that out.

[nolash]

Merged upstream as ethereum/go-ethereum#19313