Rename "Epoch" to "Session"

[davxy]

Superseeds https://github.com/paritytech/substrate/pull/11330

This PR refines the work done by @gitwithterrence by addressing the issues left open.

The work mostly consists of a trivial 1 to 1 renaming from Epoch to Session of variables and structures.

In order to simplify the review:

• Exceptions to the trivial renaming where highlighted by my review comments below.
• I've not renamed YET the client/consensus/epochs folder. I'm going to rename it once I have the green light.

[davxy]

If we really want to do the renaming then this is a good opportunity. It is a bit annoying renaming variables and stuff ... :-)

[sam0x17]

Once the nits and merge conflicts are resolved and CI is passing looks good at least to me :)

[joepetrowski]

This is a confusing change IMO, as @rphmeier pointed out in the last PR, they are conceptually different. Naming them the same thing makes explaining BABE and GRANDPA more difficult. We have other naming collisions in Substrate too that have caused issues with users, e.g.:

• Era is in both Staking as well as the extrinsic mortality. We explain this all the time to custodians.
• transaction_version/extrinsic_version used for the serialization format and the chain's call interface (e.g. here and here).
• Derivative accounts is used for both cryptographic derivatives and on-chain derivatives.

[andresilva]

I'm a bit torn on this, I also agree that epochs and sessions are slightly different concepts (even though we use epochs to integrate with sessions) but don't really have any strong opinion either way. But I'm also not the target audience for this rename as I'm not confused by having these two different names. There's comments arguing for either side though.

[bkchr]

Naming them the same thing makes explaining BABE and GRANDPA more difficult.

Can you explain this?

pointed out in the last PR, they are conceptually different

Can someone explain this as well? What is the difference?

[andresilva]

An epoch is always measured in slots, a session is just some abstract period, e.g. aura sessions are measured in number of blocks. Session index is always contiguous while the same isn't necessarily true for epochs (since they are bound to a notion of real-world time). The concepts are tightly coupled because we do implement a session using an epoch, so there's a 1:1 mapping.

[rphmeier]

Yeah, I'm still against this change. I think it's a bad idea.

Quoting from before

IMO it was a huge mistake to ever have introduced these two different namings into the code base.

There's no guarantee that these things will be in the same code base in the future. Separating concepts decouples code, combining them (even when they aren't congruent) makes it harder to decouple in the future. You could imagine splitting the BABE logic into its own repo and then it would make more sense; it's always better to write code in a large repo in a more decoupled way.

"This crate implements BABE consensus on top of Substrate's session abstraction"

I'm quite glad that we did have the foresight to introduce these two distinct terms into the codebase.

And clearly there is a difference between session and epoch, although they are the same when you run with BABE and not when you run with Aura.

[davxy]

An epoch is always measured in slots, a session is just some abstract period, e.g. aura sessions are measured in number of blocks.

This is interesting.

So in short:

• session: bound to the concept of authority keys rotation (not described by the consensus protocol paper)
• epoch: bound to the parameters of the protocol itself.

---

As said somewhere else, I don't have strong opinions. But I just want to be coherent wrt Sassafras implementation.

In short, to avoid confusion, I would like to stick with the same terms used by Babe (since the two protocols are very closely related). Thus: -> If we are renaming Epoch to Session in Babe then I will be happy to name this "entity" Session in Sassafras as well. -> If we are not renaming then I prefer to call this "Epoch" in Sassafras

It will be even more awkward to have different names in two protocols that are so strictly connected. Not to mention that both are using sp-epochs crate.

So the choice here I think should be binary. All or nothing.

[rphmeier]

I agree, it should be consistent in the consensus implementations. I think it makes most sense to stick to the terminology of the paper and document that we use "Substrate Sessions to implement (Babe|Sassafras) Epochs".

Looking forward, it would be nice to split out the code of BABE and SASSAFRAS from Substrate as much as possible, for example as we've done with the finality-grandpa crate.

[rphmeier]

For example, we use the notion "voter_set" and "set_id" in GRANDPA, which is closer to the paper. Should we also rename this to session? Even those this also refers to a different mechanism from BABE Epochs. (Epochs drive sessions which drive voter sets on GRANDPA)

[bkchr]

Looking forward, it would be nice to split out the code of BABE and SASSAFRAS from Substrate as much as possible, for example as we've done with the finality-grandpa crate.

Why? For Grandpa it makes sense as the protocol itself is much more complicated than BABE/SASSAFRAS. I mean yes we could also make this generic and move it out, but I don't see any benefit in doing this. This would only result in more work.

I would still called epochs session, as this is the BABE/SASSAFRAS implementation in the context of Substrate and as said here:

The concepts are tightly coupled because we do implement a session using an epoch, so there's a 1:1 mapping.

When you are implementing a certain research paper, you also don't start calling all your variables using the wide variety of the Greek alphabet. The most important thing for me is consistency and consistency in Substrate would be to call it Session, not Epoch. You need to think about people approaching Substrate for the first time. These people will not start reading the BABE paper before using it. So, they will dive into the code and will see different naming for the same concept. The only thing this is doing is confusing people, nothing more. It was even said by @andresilva that we have this 1:1 mapping.

However, I will not die on this hill and I'm also okay with closing this pr.

[stale[bot]]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[bkchr]

Okay, let's close this.