Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol version 7 update proposal #64

Merged
merged 8 commits into from
Jun 25, 2024
Merged

Protocol version 7 update proposal #64

merged 8 commits into from
Jun 25, 2024

Conversation

td202
Copy link
Contributor

@td202 td202 commented Jun 14, 2024

Purpose

Define the specification for the protocol version 7 update.

Changes

  • Introduce P7 update.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

Copy link
Member

@DOBEN DOBEN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done.

updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
Copy link

@Victor-N-Suadicani Victor-N-Suadicani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various suggestions (and only suggestions, I don't yet have the competences to truly question anything in this document).

General question: Any reason why these protocol updates aren't using Markdown? It'd be easier to read/format them (especially here on GitHub) and it'd be simple to insert images like diagrams and such.

updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
Copy link
Contributor

@limemloh limemloh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also change the hashing structure of the block hash preparing for a light client as part of this protocol update, right?

updates/P7.txt Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
updates/P7.txt Show resolved Hide resolved
Copy link

@eb-concordium eb-concordium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@limemloh
Copy link
Contributor

@Victor-N-Suadicani

General question: Any reason why these protocol updates aren't using Markdown? It'd be easier to read/format them (especially here on GitHub) and it'd be simple to insert images like diagrams and such.

This specification needs to be hashed and signed as part of the protocol chain update. We could have used markdown, but it makes the hashing of the document a bit more complex with external resources and so on.
It also leaves out the issue of how to display markdown, with txt there is not really any doubt.

@Victor-N-Suadicani
Copy link

Victor-N-Suadicani commented Jun 18, 2024

I would say you could just as easily hash the Markdown file itself, just as a .txt file, excluding any external things linked (like images), which could be considered non-normative. Markdown looks pretty similar regardless of what you use to render it and as mentioned GitHub automatically lets you see the rendered Markdown and we use GitHub so... just kinda makes sense to me 😅. Anyways, I won't press this further if it is not desirable.

commentary/P7-commentary.txt Outdated Show resolved Hide resolved
commentary/P7-commentary.txt Outdated Show resolved Hide resolved
commentary/P7-commentary.txt Outdated Show resolved Hide resolved
commentary/P7-commentary.txt Outdated Show resolved Hide resolved
Copy link

@chportma chportma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I caught 3 instances of baker instead of validator. Apart from that, my only other comment is what I posted in slack: should we be mentioning here the cool-down time changes that we will do and undo when updating the protocol?

updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
updates/P7.txt Outdated Show resolved Hide resolved
@td202 td202 merged commit 4ca0d01 into main Jun 25, 2024
2 checks passed
@td202 td202 deleted the p7-update branch June 25, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants