A0-1323: Extend Runtime API to handle AlephBFT version

[maciejzelaszczyk]

Description

This extends the AlephSessionApi Runtime API, to handle the AlephBFT version. Storage items, getters, a setter and the declaration and implementation of appropriate methods are provided.

The solution is based on a u32 version. The scheduled version change is persisted as a StorageValue -> FinalityScheduledVersionChange. This value stores the information about a scheduled AlephBFT version change, where version_incoming is the version to be set and session is the session on which the new version will be set. A pallet_session::Session_Manager is implemented to check whether a scheduled version change has moved into the past and, if so, to record it as the current version StorageValue -> FinalityVersion. When a version change is recorded as the current version, FinalityScheduledVersionChange is cleared.

Importantly, we can always reschedule a version change, and an already set version change may move into the past as time goes by. The value of the current_session is needed to check whether the version change needs to be recorded as the current version.

In order to cancel a scheduled version change, we need to use the version change setter (scheduler) and schedule a version change which will actually not change the current version.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

Checklist:

• I have added tests
• I have created new documentation
• I have bumped the spec_version and transaction_version

[maciejzelaszczyk]

Based on a fruitful conversation with @fixxxedpoint and @kostekIV, I am converting this back to draft, as we have converged on an alternative and potentially more robust scheme to handle version updates. Will re-request review once a working version is ready.

[timorl]

LGTM.

[DamianStraszak]

My last doubt is regarding naming in this pallet. We want to use versioning not only for alephBFT but also for signature aggregation, and who knows what else in the future. I think its safe to use a more generic name here like schedule_finality_version_change or schedule_consensus_version_change instead of schedule_aleph_bft_version_change.

We ideally want to avoid changing the names ever, because it is in the runtime.

What do you think @timorl and @kostekIV ?

[kostekIV]

My last doubt is regarding naming in this pallet. We want to use versioning not only for alephBFT but also for signature aggregation, and who knows what else in the future. I think its safe to use a more generic name here like schedule_finality_version_change or schedule_consensus_version_change instead of schedule_aleph_bft_version_change.

We ideally want to avoid changing the names ever, because it is in the runtime.

What do you think @timorl and @kostekIV ?

Sounds good, either name is fine to me.

[timorl]

The names you propose sound good, although it's a bit strange that these two things (abft & justifications) get conflated – to be clear I don't have a better solution and this one seems alright, just the versioning will correspond to "official" versions in a strange way.

At the same time I'm worried about the arguments you use – changing names of functions in the runtime (or other changes there for that matter) shouldn't be that disastrous and we should probably spend some time figuring out how to do this better. I think I already mentioned this a long time ago, but it got forgotten... I'll add a task.

[DamianStraszak]

Changing the names of runtime calls (=extrinsics) is fine (if these are sudo calls, because only we need to adapt), but changing the names in the runtime API is imho VERY non-trivial, because you need to add some compatibility layer in the host, otherwise a runtime update with new API would crash the node.

We should probably add this compatibility layer when writing the host part of update logic.

That being said -- let's avoid changing names anyway, and use consensus or finality here (I would vote for the latter, as this is the nomenclature used by substrate and nicely reflects what apehbft+aggregator do).

[maciejzelaszczyk]

Changing the names of runtime calls (=extrinsics) is fine (if these are sudo calls, because only we need to adapt), but changing the names in the runtime API is imho VERY non-trivial, because you need to add some compatibility layer in the host, otherwise a runtime update with new API would crash the node.

We should probably add this compatibility layer when writing the host part of update logic.

That being said -- let's avoid changing names anyway, and use consensus or finality here (I would vote for the latter, as this is the nomenclature used by substrate and nicely reflects what apehbft+aggregator do).

finality it is :)

[DamianStraszak]

Awesome work!

[DamianStraszak]

Ehh. I approved but now I see that spec_version has not been bumped. Why is that?

[maciejzelaszczyk]

Ehh. I approved but now I see that spec_version has not been bumped. Why is that?

First time working on anything in the runtime. Have corrected this just now.

[DamianStraszak]

You must also bump transaction_version -- these things are in the PR checklist exactly because we don't want to miss them :/

[maciejzelaszczyk]

You must also bump transaction_version -- these things are in the PR checklist exactly because we don't want to miss them :/

This should be OK now.

[maciejzelaszczyk]

@DamianStraszak I have performed some final tests and there is one case that bothers me. Not a bug per se, but worth considering nonetheless. Whenever you use next_session_finality_version, you have to make sure that your call comes after the update_version_change_history call from the SessionManager. Otherwise, the returned version for the next session will not be correct for the session when the change takes hold. I can change this behavior to always return the correct version number at the cost of introducing a storage item. Any thoughts?

[maciejzelaszczyk]

Actually, this bothered me too much, so I went and implemented the change.

[maciejzelaszczyk]

Due to a discussion today with @mike1729 and @kostekIV and favorable testing so far I am merging this and will publish a separate PR with the requisite tests shortly.