Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Deterministic genesis hash for --dev runtime #11131

Closed
ggwpez opened this issue Mar 28, 2022 · 30 comments
Closed

Deterministic genesis hash for --dev runtime #11131

ggwpez opened this issue Mar 28, 2022 · 30 comments
Assignees
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. U3-nice_to_have Issue is worth doing eventually.

Comments

@ggwpez
Copy link
Member

ggwpez commented Mar 28, 2022

When I start a substrate node in --dev node, it sometimes has different genesis hashes.
It does not happen on every start but sometimes after building? I could not really track to what it is related.

It would be nice to have deterministic genesis hashes such that import/export-block can be used for testing and in CI.
Either by fixing it or using a --seed flag.

@athei said it could be because of randomized staking nominations.

@ggwpez ggwpez added I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. U3-nice_to_have Issue is worth doing eventually. labels Mar 28, 2022
@athei
Copy link
Member

athei commented Mar 30, 2022

I think it happens on every start. The chain spec is built at runtime. This is the code I was talking about:

// stakers: all validators and nominators.
let mut rng = rand::thread_rng();
let stakers = initial_authorities
.iter()
.map(|x| (x.0.clone(), x.1.clone(), STASH, StakerStatus::Validator))
.chain(initial_nominators.iter().map(|x| {
use rand::{seq::SliceRandom, Rng};
let limit = (MaxNominations::get() as usize).min(initial_authorities.len());
let count = rng.gen::<usize>() % limit;
let nominations = initial_authorities
.as_slice()
.choose_multiple(&mut rng, count)
.into_iter()
.map(|choice| choice.0.clone())
.collect::<Vec<_>>();
(x.clone(), x.clone(), STASH, StakerStatus::Nominator(nominations))
}))
.collect::<Vec<_>>();

@shawntabrizi
Copy link
Member

@kianenigma @emostov can you use some seeded randomness or something else here that is deterministic?

@athei
Copy link
Member

athei commented Apr 1, 2022

contracts-ui relies the fact that every re-start of a --dev chain yields a different genesis hash in order to determine whether this is a new instance of a chain.

@statictype

@bkchr
Copy link
Member

bkchr commented Apr 3, 2022

contracts-ui relies the fact that every re-start of a --dev chain yields a different genesis hash in order to determine whether this is a new instance of a chain.

@statictype

This doesn't sounds like a good idea ;)

@kianenigma
Copy link
Contributor

kianenigma commented Apr 4, 2022

Both development and staging testnets are actually passing vec![] to initial_nominators, so I am not sure if this is the issue. Nonetheless, I added this for my election-testing shenanigens, and I have been doing those in a separate repo now for sometime, so @ggwpez if this is an annoyance, feel free to remove the whole randomness.

@@ -272,20 +272,12 @@ pub fn testnet_genesis(
                });

        // stakers: all validators and nominators.
-       let mut rng = rand::thread_rng();
        let stakers = initial_authorities
                .iter()
                .map(|x| (x.0.clone(), x.1.clone(), STASH, StakerStatus::Validator))
                .chain(initial_nominators.iter().map(|x| {
-                       use rand::{seq::SliceRandom, Rng};
-                       let limit = (MaxNominations::get() as usize).min(initial_authorities.len());
-                       let count = rng.gen::<usize>() % limit;
-                       let nominations = initial_authorities
-                               .as_slice()
-                               .choose_multiple(&mut rng, count)
-                               .into_iter()
-                               .map(|choice| choice.0.clone())
-                               .collect::<Vec<_>>();
+                       let nominations = initial_authorities.iter().take(MaxNominations::get() as usize).cloned().collect::<Vec<_>>();
                        (x.clone(), x.clone(), STASH, StakerStatus::Nominator(nominations))
                }))
                .collect::<Vec<_>>();

@statictype
Copy link

Contracts UI stores information about uploaded contracts in a local db, so when the chain is restarted we want to purge the db. We were using the genesis hash to check if there was a restart. Is there any value we can use that is unique to the chain instance? Worst case scenario we let the users deal with deleting contracts that are not on-chain anymore, but doing it automatically is much nicer UX

@xlc
Copy link
Contributor

xlc commented Apr 4, 2022

just use hash of block 1 instead of block 0?

@statictype
Copy link

statictype commented Apr 4, 2022

just use hash of block 1 instead of block 0?

we were doing that before substrate-contracts-node started authoring blocks on demand. now blocks are created only when a transaction is submitted

@ggwpez
Copy link
Member Author

ggwpez commented Apr 4, 2022

Is there any value we can use that is unique to the chain instance?

We can keep the old behaviour with something like --seed random.

@statictype
Copy link

that could be interesting for other reasons, but there is no guarantee people will use it and the UI needs to handle all cases

@athei
Copy link
Member

athei commented Apr 4, 2022

contracts-ui relies the fact that every re-start of a --dev chain yields a different genesis hash in order to determine whether this is a new instance of a chain.
@statictype

This doesn't sounds like a good idea ;)

I think --dev chains should actually have different genesis hashes on each startup. In contrast to a named chain spec they are supposed to be restarted regularly and hence represent different instances of a chain each time. Same for every local chain.

Where I agree with you is that relying on this implementation detail (staking nomination randomization) is bad. We should make it explicit. Just include some explicit randomness in the genesis for dev and local chains.

Then we add a --deterministic-genesis flag to resolve this issue.

@bkchr
Copy link
Member

bkchr commented Apr 4, 2022

Contracts UI stores information about uploaded contracts in a local db, so when the chain is restarted we want to purge the db. We were using the genesis hash to check if there was a restart. Is there any value we can use that is unique to the chain instance? Worst case scenario we let the users deal with deleting contracts that are not on-chain anymore, but doing it automatically is much nicer UX

How would that work on live networks? There you can not cache contracts as well? So you would need to scrape them on restart of the UI or something?

For local or dev networks, I would store something like latest block and if the chain is back at block 0, it means it was restarted. Or even less complicated, if the chain is at block 0 you know that there doesn't exists any contract, so you can delete any cache.

@bkchr
Copy link
Member

bkchr commented Apr 4, 2022

I think --dev chains should actually have different genesis hashes on each startup. In contrast to a named chain spec they are supposed to be restarted regularly and hence represent different instances of a chain each time. Same for every local chain.

I don't see why a dev chain should always have a different genesis hash. This was never the case before and only seems to be introduced by accident by @kianenigma.

@athei
Copy link
Member

athei commented Apr 4, 2022

Worst case scenario we let the users deal with deleting contracts that are not on-chain anymore

Yeah this is how polkadot.js Apps handles it currently. It is kind of annoying.

How would that work on live networks? There you can not cache contracts as well? So you would need to scrape them on restart of the UI or something?

Contracts (more precisely the metadata) is stored in persistent browser memory. So unless the user resets the cache it stays there. It is about the opposite: The chain is purged and then we still have contracts shown in the UI which do not exist on-chain. Live chains do not have this problem as they are not restarted.

For local or dev networks, I would store something like latest block and if the chain is back at block 0, it means it was restarted. Or even less complicated, if the chain is at block 0 you know that there doesn't exists any contract, so you can delete any cache.

Yes storing the latest block per endpoint might be a solution that works. If it goes backwards it was reset. But is is not reliable: If you restart the chain but only open up the UI later this event can be missed.

Another solution: Just automatically remove every contract which does not exist on chain from the UI. No need to have it stick around as "this is deleted".

@bkchr
Copy link
Member

bkchr commented Apr 4, 2022

Live chains do not have this problem as they are not restarted.

For sure, but you can remove contracts? Aka contracts are not required to stay on chain indefinitely?

@statictype
Copy link

Another solution: Just automatically remove every contract which does not exist on chain from the UI. No need to have it stick around as "this is deleted"

this could work, but better still would be to have some kind of random id that changes on restart for local/dev chains. like docker containers 🙂

@statictype
Copy link

statictype commented Apr 4, 2022

How would that work on live networks? There you can not cache contracts as well? So you would need to scrape them on restart of the UI or something?

Basically what we do is:
Every chain has it's own db, we switch between dbs depending on what endpoint we're connected to. The db name is made of the endpoint url + genesis hash. When connecting to a different node we check if the genesis hash has changed for that endpoint and if it did we create a new db and delete the old one.
There will probably be a "forget" button for contracts in the future, just to allow users to remove the contracts they don't care about anymore, but they will only be removed from the UI

@athei
Copy link
Member

athei commented Apr 4, 2022

Live chains do not have this problem as they are not restarted.

For sure, but you can remove contracts? Aka contracts are not required to stay on chain indefinitely?

A contract can self destruct if it chooses to do so. So yes, they can be removed.

@bkchr
Copy link
Member

bkchr commented Apr 4, 2022

A contract can self destruct if it chooses to do so. So yes, they can be removed.

So that needs to be supported anyway for live chains?

@athei
Copy link
Member

athei commented Apr 4, 2022

I guess in this case we would be OK to just have the user to click on "delete" as it is a rare occurrence as opposed to restarting a dev node.

@bkchr
Copy link
Member

bkchr commented Apr 5, 2022

How should the user know that the contract doesn't exist anymore? Sounds like some complicated UX to me? If the app can find this out, why not let it find out if a contract still exists?

@statictype
Copy link

Yes, it is complicated, but my goal is always to simplify the UX.
Live networks never restart, but Testnets do sometimes and local nodes restart maybe a few times / day, it's just expected behaviour.
The moment a restart happens all storage is gone (at least with --dev). It would be massively helpful for UIs to have an easy way to determine this event has occurred and invalidate all state throughout the app in one go.
In the case of Contracts UI it's about deployed contracts that are not there anymore after a restart. Let's say the user has 30 instances of different contracts and restarts the chain. Sure, we can call an RPC for each previously deployed contract and check if it's on-chain or not (probably the contractInfoOf RPC) but it would be much cleaner if we could offer the users a way to clear browser storage in one go when a chain restart is detected (instead of asking them to press a delete button 30 times or even more intrusively just remove them automatically after performing 30 RPC calls)
Because contracts can self-distruct, we can't assume that if some of them are not on-chain there has been a restart.

@bkchr
Copy link
Member

bkchr commented Apr 5, 2022

In the case of Contracts UI it's about deployed contracts that are not there anymore after a restart. Let's say the user has 30 instances of different contracts and restarts the chain. Sure, we can call an RPC for each previously deployed contract and check if it's on-chain or not (probably the contractInfoOf RPC) but it would be much cleaner if we could offer the users a way to clear browser storage in one go when a chain restart is detected (instead of asking them to press a delete button 30 times or even more intrusively just remove them automatically after performing 30 RPC calls)

I mean I get this, but I also don't see any reason why we can not just batch all these calls and then do some simple heuristic:

let num_removed_contracts;
let total_contracts;
let percent = num_removed_contracts / total_contracts;

if percent < 30 && num_removed_contracts < 5 {
     // Ask for each individually
} else {
    // Ask once for all
}

I mean I'm that far away from being anywhere an UX expert, you can not imagine, but I think you are good at this @statictype and come up with something way better than my stupid idea :D You will also don't need to use contractInfoOf. You can just use something like state_getStorageSize to find out if the data exists in the state. Should be fairly trivial and fast.

For this issue in general. @kianenigma slipped through these changes that make the chain spec of the Substrate node nondeterministic. I would say we remove this again, because there is no real need and for sure we don't add any "default" way of randomizing genesis states in some way. If someone wants this for their dev chain, like the contracts playground node (not sure how it is called), they can add some random value to the genesis storage. Then you would have the functionality you want. However, this will not happen in Substrate as any default way.

@statictype
Copy link

sure, we can find a workaround. indeed it's not ideal for UIs to rely on the randomness of the genesis hash to check for restarts.
@bkchr would a feature like a unique ID per chain instance when running in --dev mode be possible in the future?
sorry for going so off topic :)

@ggwpez
Copy link
Member Author

ggwpez commented Apr 5, 2022

I double checked the code, and @kianenigma is right. vec![] is used for nominators and therefore does not randomize.
After building two chain specs from where i got that problem from, the only difference is the wasm blob 🤦

So sadly the import-blocks command will not be able to be used in CI / Testing since in each build the runtime possibly changes, sorry I did not think about that earlier.
The fix from @kianenigma is probably still worth adding, just to make the testnet_genesis function deterministic.
As for @statictype, just relying on the genesis hash should already not work currently.

@athei
Copy link
Member

athei commented Apr 5, 2022

As for @statictype, just relying on the genesis hash should already not work currently.

I thought it does because the wasm blob is different (which in itself is in genesis). But it is an unintended side effect.

@ggwpez
Copy link
Member Author

ggwpez commented Apr 5, 2022

I thought it does because the wasm blob is different (which in itself is in genesis). But it is an unintended side effect.

Because the build is non-deterministic between rust versions or which "unintended side effect" do you mean? @athei

@athei
Copy link
Member

athei commented Apr 5, 2022

Yeah this is what I meant. But yeah they shouldn't differ between launches of the same binary.

@bkchr
Copy link
Member

bkchr commented Apr 5, 2022

@bkchr would a feature like a unique ID per chain instance when running in --dev mode be possible in the future? sorry for going so off topic :)

This is also possible, however that is again chain specific. Aka we can not change this here on the Substrate side. Every chain implementation decides on its own about the id. Which means they could use whatever id they want and could also make it random.

@ggwpez
Copy link
Member Author

ggwpez commented May 10, 2022

I will close this now since it arose from a misunderstanding of what was happening. Thanks for clearing it up!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. U3-nice_to_have Issue is worth doing eventually.
Projects
None yet
Development

No branches or pull requests

8 participants