parachain-system: ignore go ahead signal once upgrade is processed

[slumber]

Closes #2580

[rphmeier]

With this approach, we should be careful that we don't submit another code upgrade until the previous go-ahead signal has gone away (i.e. the block which handled the go-ahead signal is included). Otherwise we could be left with an ambiguous interpretation of the go-ahead. This should be tested.

[rphmeier]

There is also the related problem that the original issue didn't pick up - that the upgrade restriction signal isn't set until the block scheduling a code upgrade has been included. This means that the unincluded segment should also avoid scheduling code upgrades if either of the following hold:

1. The upgrade restriction signal disallows it
2. The unincluded segment schedules a code upgrade

The current logic only does (1) but we should do (2) as well, in this PR.

[rphmeier]

Logic looks correct otherwise

[slumber]

With this approach, we should be careful that we don't submit another code upgrade until the previous go-ahead signal has gone away (i.e. the block which handled the go-ahead signal is included). Otherwise we could be left with an ambiguous interpretation of the go-ahead. This should be tested.

It won't be interpreted at all while there exists a block within unincluded segment that already did this interpretation.
Scheduling another upgrade in the same segment shouldn't be a problem, although, it's very likely there'll be a restriction signal because of upgrade cooldown, e.g. 14400 blocks ~ 24 hours on Polkadot.

[slumber]

There is also the related problem that the original issue didn't pick up - that the upgrade restriction signal isn't set until the block scheduling a code upgrade has been included. This means that the unincluded segment should also avoid scheduling code upgrades if either of the following hold:

1. The upgrade restriction signal disallows it
2. The unincluded segment schedules a code upgrade

The current logic only does (1) but we should do (2) as well, in this PR.

2 is handled by PendingValidationCode storage item.

[rphmeier]

Scheduling another upgrade in the same segment shouldn't be a problem, although, it's very likely there'll be a restriction signal because of upgrade cooldown, e.g. 14400 blocks ~ 24 hours on Polkadot.

What I mean is that depending on the configuration of Polkadot right now is not future proof. If the current code breaks if parachains are "allowed" to submit a code upgrade every block by Polkadot, then the PR implementation is broken.

[slumber]

What I mean is that depending on the configuration of Polkadot right now is not future proof. If the current code breaks if parachains are "allowed" to submit a code upgrade every block by Polkadot, then the PR implementation is broken.

I didn't try to justify it with Polkadot config, this was "although"
Initial statement is that to me it looks correct to submit another upgrade after go-ahead being processed but before the processing block's inclusion.

I'll add a test for this case.

[rphmeier]

Initial statement is that to me it looks correct to submit another upgrade after go-ahead being processed but before the processing block's inclusion.

Yeah, on closer inspection it seems to function correctly as long as

2. The unincluded segment schedules a code upgrade

which is the case. Is this tested?

[slumber]

which is the case. Is this tested?

cumulus/pallets/parachain-system/src/tests.rs

Lines 424 to 439 in 974dc46

	#[test]
	fn non_overlapping() {
	BlockTests::new()
	.with_relay_sproof_builder(|_, _, builder| {
	builder.host_config.validation_upgrade_delay = 1000;
	})
	.add(123, || {
	assert_ok!(System::set_code(RawOrigin::Root.into(), Default::default()));
	})
	.add(234, || {
	assert_eq!(
	System::set_code(RawOrigin::Root.into(), Default::default()),
	Err(Error::<Test>::OverlappingUpgrades.into()),
	)
	});
	}

This works regardless of blocks being included or not.