Warn if referenced EIPs are not included in requires preamble header

[SamWilsn]

Originally from #55

It would be nice if the bot commented on PRs that mention an EIP (either by name, or as a link) but don't include that EIP in the requires header.

This shouldn't block merging, because often other EIPs are mentioned but not strictly required.

[MicahZoltu]

I would prefer this block auto-merge and we just require people have hard dependencies on any EIP they reference. The reason for this is because someone might reference draft EIP-123 in their motivation section, and then work their EIP to final. After that EIP-123 may undergo drastic changes that makes the comment in motivation no longer make any sense.

To avoid this scenario, we have a rule that EIPs can't proceed past the status of their dependencies, so if you have EIP-123 in your requires and it is a draft, then your EIP can't move past draft. By requiring all referenced EIPs are in the requires section (including stuff referenced tangentially) we can ensure that we don't fall into the trap of a reference not making any sense.

[SamWilsn]

IMHO references != requires.

Requires is a very strong term, and simply linking to another EIP doesn't establish a "requires" relationship.

I'd define "A requires B" as "A cannot be understood without B OR A cannot be implemented without B". On the other hand, "A references B" is any weaker relationship between A and B (for example A is intended to be compatible with B, but can be implemented alone.)

So my feelings are that we either change requires: to references: in the preamble, or we change the rule to:

An EIP can't proceed past the earliest status of EIPs linked in its body or in its requires: preamble header.

[JEAlfonsoP]

Requires: as mentioned in EIP 1 template.

Not clear what is the new proposed action for Bot ?
ie: warn about eip mentioned in body but not included in requires ?
Or Autoblock depending on eips required status ?

is it possible for a PR to requires an EIP not merged (or last call or final status ?) question

I believe the word reference is confusing.

[MicahZoltu]

Using your terminology, I don't think a final EIP should reference any EIP that isn't also final. I would be fine with changing requires to references if that makes it more clear to people.

[JEAlfonsoP]

Ok, I agree, BOT can check EIPs_status (requires or References, I think that this will need a change of template ? question) and proceed/merge depending on it.

[MicahZoltu]

Lets see how other editors feel about renaming requires to references cc @gcolvin @lightclient @axic

[gcolvin]

I'd leave the requires section as is. These are normative requirements of the specification.

Internal references need not be normative requirements. They can instead be informative as to the motivation, rationale, or (as @SamWilsn points out) the compatibility of a proposal. I think keeping their status in sync with the normative references throughout the process would be just be a useless hassle, but they should probably all sync up in the end.

[MicahZoltu]

The advantage of keeping the references in sync throughout the process is that we alert authors about problems with their EIP early on, rather than waiting until the very end (Last Call => Final) to tell them that they need to make potentially significant changes. Ideally, by the time the EIP gets to Last Call it is basically done and shouldn't need any additional work, but to achieve this we need to give feedback to authors early.

[gcolvin]

For non-normative references I think a waning would do until last call, but I'm not hard over on it. I'm not sure it's all that common, and most likely such reference would be to Final EIPs anyway.

[SamWilsn]

So to summarize my understanding, we have:

(1) Don't rename requires:. Block merging if the EIP contains a link to another EIP not included in requires:.
(2) Don't rename requires:. Change rule to "An EIP can't proceed past the earliest status of EIPs linked in its body or in its requires: preamble header."
(3) Rename requires: to references:. Block merging if the EIP contains a link to another EIP not included in references:.

My preferred option would be (2).

[gcolvin]

I believe the current rule is "An EIP can't proceed past the earliest status of EIPs linked in its requires: preamble header." I want to stick with that. I want just a warning for non-normative links within the body of the text, which I think answers @MicahZoltu's concerns.

My reasoning is that informative links only serve to make a spec more understandable, so forcing them to stay in sync is unnecessary and sometimes unwarranted. E.g. some new EVM EIPs may well want to refer back to EIP-615, where important concepts were laid out, reviewed, and discussed in the community. So it is a part of the motivation and rationale for the new EIPs. But EIP-615 is currently Stagnant and likely to stay there.

[MicahZoltu]

My concern with not requiring references be in the header is that it makes it super easy to miss a reference, and we cannot easily automate the detection of "is the referenced EIP further along in the process than another EIP.

some new EVM EIPs may well want to refer back to EIP-615, where important concepts were laid out, reviewed, and discussed in the community. So it is a part of the motivation and rationale for the new EIPs. But EIP-615 is currently Stagnant and likely to stay there.

My problem with allowing this is that per the current rules, a stagnant EIP can be revived and there is no limit on the changes that can occur to an EIP. Someone could, at a future date, revive 615 and decide to take it in a completely different direction/completely rewrite it. If we have a final EIP that references 615, then that means we now have a source of mutability in that final EIP by way of mutating something that it refers to, and the reference may not make sense in the future.

I believe that Withdrawn is a terminal state, so I would accept referring to a Withdrawn EIP in a final EIP, just not Stagnant, Draft, Review, or Last Call (because all of those can change).

[gcolvin]

@MicahZoltu

I believe that Withdrawn is a terminal state, so I would accept referring to a Withdrawn EIP in a final EIP, just not Stagnant, Draft, Review, or Last Call (because all of those can change).

That would be a reasonable approach. And then it would be reasonable to simply not put Withdrawn EIPs in the header.

[gcolvin]

Possible hitch is things like the Rationale for EIP-4573: Procedures for the EVM:

This proposal uses the EIP-2315 return stack to manage calls and returns, and steals ideas from EIP-615, EIP-3336, and EIP-4200. ENTERPROC corresponds to BEGINSUB from EIP-615. Like EIP-615 it uses a frame stack to track call-frame addresses with FP as procedures are entered and left, but like EIP-3336 and EIP-3337 it moves call frames from the data stack to memory.

It's a Draft, but EIP-3336 and EIP-3337 have since gone to Stagnant and it's likely that they will stay there. The Rationale could be rewritten to plagiarize the relevant parts of those, but that's a lot of work, doesn't point the reader at their discussion threads, and doesn't give credit where credit is due. (Nick @Arachnid hasn't responded to my asking whether he wants to be co-author.)

The general problem being that Stagnant proposals may never get Withdrawn.

EDIT: Possible solution: deal with things like these manually in the rare cases where they arise.

[MicahZoltu]

In this case, my recommendation would be to move the Stagnant EIPs to Withdrawn so we can reliably link them without worry about them mutating in the future. If you are the author of the linked EIPs, then it is easy. If someone else is the author things are more complicated and I recommend duplicating the relevant content in the new EIP.

[gcolvin]

This is really looking to me like the sort of thing that won't arise that often and should be handled on a case-by-case basis

This also relates to the "no external references" rule, which I want to revisit soon.

[MicahZoltu]

I don't have data to back it up, but I would say it happens quite frequently. It is also one of those things where if we had more editors it would be less of a problem because we could introduce more nuance into the process.

[gcolvin]

What happens frequently? That EIPs stay Stagnant? I don't see that as a problem. The only problem I see is when Final EIPs would wind up referring to Stagnant issues. I.m thinking that won't happen very often.

[JEAlfonsoP]

EIP-BOT, could check required/referenced EIPs if status is not final, stop auto-merge.
whatever you decide later on, we can avoid now auto-merging if required/referenced EIP(s) is not final.

[SamWilsn]

@MicahZoltu would you be happy if referenced-but-not-required EIPs linked to a particular git commit?

[MicahZoltu]

Such links would not work with Jekyll. One could imagine someone adding support for that sort of deep-history-linking into Jekyll at which point I would support it, but that is a pretty big task I suspect.

It also makes it harder for us to switch away from Jekyll in the future, so I'm a bit negative on doing the work to implement a Jekyll addition just for this.

One could just copy the markdown and put it into ../assets/eip-1234/non-finalized-eip-5678.md and then reference that perhaps?

[gcolvin]

@MicahZoltu

One could just copy the markdown and put it into ../assets/eip-1234/non-finalized-eip-5678.md and then reference that perhaps?

Yes!

IETF standards have sections fo full Normative and Informative citations. For EIPs and copies of EIPs we could generate these sections automatically.

[JEAlfonsoP]

So far what I can see is that we agree that if required EIP(s) is (are) not final EIP-BOT will not automerge. Could you please confirm.

[SamWilsn]

So far what I can see is that we agree that if required EIP(s) is (are) not final EIP-BOT will not automerge. Could you please confirm.

Yes, that is a safe minimum to implement.

[SamWilsn]

Wait, no. I don't believe that is correct. The minimum rule we've agreed on so far is:

The bot shall not auto-merge a PR if that PR advances an EIP's status past the status of any EIP listed in its requires: preamble field.

[JEAlfonsoP]

correct..
so this is the rule:
The bot shall not auto-merge a PR if that PR advances an EIP's status pass the status of any EIP listed in its requires: preamble field.

[Pandapip1]

Superseded by ethereum/eipw#29