grandpa: avoid importing unnecessary justifications

[andresilva]

When importing a block if it includes a GRANDPA justification we will always process it. If a peer is serving us justifications for every block then we will invariably verify them and import them which can lead to a significant slowdown in block import speed. We don't need justifications for every block and therefore this is wasteful. After this PR we will only import GRANDPA justifications every 512 blocks (besides the mandatory ones, i.e. when authorities change).

Fixes problems like paritytech/polkadot-sdk#13.

polkadot companion: paritytech/polkadot#7484

[skunert]

This will solve the slow import issue when syncing with nodes that are sending more justifications as expected.

However its a bit different than what we discussed earlier, correct? Since JUSTIFICATION_IMPORT_PERIOD and the config are separate, this will even help nodes that have justification_period set to 1. I think previously we had discussed using the configured justification_period for import too.

[davxy]

Another option could be to add another parameter in the GrandpaParams.
Something like separating justification_import_period and justification_production_period.
As another tweak, both may also be defined as Options. If one is None => Use a default const value (like 512)
Just to not have the import period as a constant while the production a non constant.

(feel free to ignore... this is just an opinionated nit 😃)

[andresilva]

The GrandpaParams is only used when running a validator, whereas in this case the configuration needs to be passed to the GrandpaBlockImport when setting up the pipeline (which in service is also done beforehand). The reason I didn't add a parameter there to configure this was to avoid having a breaking change that will need to be handled by everyone when setting up the service. I can add the parameter though (I was a bit lazy and didn't want to deal with this).

[bkchr]

I would probably go with the parameter, to make it configurable like we have for the creation.

[andresilva]

Updated to make the justification import period configurable.

[jasl]

A dumb question: what if justification_import_period can not exact division justification_generation_period?

[andresilva]

@jasl Both options will be used independently. justification_generation_period will be used when following the GRANDPA protocol (i.e. when fully synced) to create justifications from commits at the end of the round, e.g. if it is set to 1 then the node will create and persist a justification for all the GRANDPA rounds. justification_import_period will be used when importing blocks that may come with a justification attached (usually only during initial sync), e.g. if it is set to 1 then it will always import the justifications that are served with the block. If you set justification_generation_period to 1 then when you are serving blocks for someone else to sync they will always have a justification attached (not necessarily always due to the way GRANDPA works), but on the other side if that node has justification_import_period set to e.g. 512 it will discard those justifications and only import them every 512 blocks.

[jasl]

@jasl Both options will be used independently. justification_generation_period will be used when following the GRANDPA protocol (i.e. when fully synced) to create justifications from commits at the end of the round, e.g. if it is set to 1 then the node will create and persist a justification for all the GRANDPA rounds. justification_import_period will be used when importing blocks that may come with a justification attached (usually only during initial sync), e.g. if it is set to 1 then it will always import the justifications that are served with the block. If you set justification_generation_period to 1 then when you are serving blocks for someone else to sync they will always have a justification attached (not necessarily always due to the way GRANDPA works), but on the other side if that node has justification_import_period set to e.g. 512 it will discard those justifications and only import them every 512 blocks.

So in our case, we could set justification_generation_period to 1 but set justification_import_period to 512 to mitigate our problem and keep the current behavior?

[andresilva]

@jasl Yes, that would work for your own node to generate as much justifications as possible while not affecting other nodes on the network, i.e. those nodes would ignore those justifications when doing initial sync.

[andresilva]

bot merge

[paritytech-processbot]

Error: Github API says paritytech/polkadot#7484 is not mergeable

[bkchr]

bot merge force