search

dwellir-public / polkadot-operator

The Polkadot Juju operator/charm
Apache License 2.0
1 stars 1 forks source link

Removed the need for validator-address config param #46

Closed Maharacha closed 8 months ago

Maharacha commented 8 months ago

In the first implementation of this, we choose to use a config param because it would take too long time to the update-status hook to find the address. However, the trade off with that is that we need to rely on that the correct address is set in the config. I did a benchmark to see how long time it actually takes. On Kusama, which has 1000 validators it took 20 seconds to do the lookup. That is worst case if the node is not validating so that it has to check all the 1000 addresses. Polkadot has 297 validators, and the parachains usually only have a few collators. So I think we can actually go with this, which is a much more secure way of knowing if a node is validating or not. And it will only do this lookup when --validator/--collator is in the service args.

Maharacha commented 8 months ago

I have only tested this locally yet. I will test it on at least one validator and one collator in prod as well.

jonathanudd commented 8 months ago

find-validator-address action is still there since update-status only shows yes/no. With the action you get both address and session key.

This action will only return a address if the node is validating this era right? But this was the case before as well just reminding myself on how this works.

Maharacha commented 8 months ago

find-validator-address action is still there since update-status only shows yes/no. With the action you get both address and session key.

This action will only return a address if the node is validating this era right? But this was the case before as well just reminding myself on how this works.

Yes, same behavior as before.

Maharacha commented 8 months ago

I think this looks good. But I get reminded every time I look at this code that creating the ServiceArgs object downloads new spec files each time. Created a card for this https://nextcloud.dwellir.com/apps/deck/#/board/2/card/1803

Yeah, I also got reminded about that PR while testing this one.

jakobilobi commented 8 months ago

Please target this PR at develop :pray:

jonathanudd commented 8 months ago

Please target this PR at develop 🙏

Does the github action you added still not work? :thinking:

jonathanudd commented 8 months ago

Please target this PR at develop 🙏 Does the github action you added still not work? 🤔

It's not merged to main yet so that's probably why.

jakobilobi commented 8 months ago

I have only tested this locally yet. I will test it on at least one validator and one collator in prod as well.

Did you do this yet?

If so, let's merge to develop and do a revision release soon.

Maharacha commented 8 months ago

I have only tested this locally yet. I will test it on at least one validator and one collator in prod as well.

Did you do this yet?

If so, let's merge to develop and do a revision release soon.

Have not tested it yet. Can do tomorrow.

Maharacha commented 8 months ago

I have only tested this locally yet. I will test it on at least one validator and one collator in prod as well.

Did you do this yet? If so, let's merge to develop and do a revision release soon.

Have not tested it yet. Can do tomorrow.

@jakobilobi it's now tested