-
Notifications
You must be signed in to change notification settings - Fork 87
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
Gracefully handle decoding more summaries than statically known eras #2818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrBliss
force-pushed
the
mrBliss/handle-too-many-summaries
branch
3 times, most recently
from
December 15, 2020 12:32
c51cd3b
to
91dfb21
Compare
edsko
approved these changes
Dec 15, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, under protest :)
bors merge |
iohk-bors bot
added a commit
that referenced
this pull request
Dec 15, 2020
2818: Gracefully handle decoding more summaries than statically known eras r=mrBliss a=mrBliss Old wallets (or other clients) that are only aware of Byron and Shelley should be able to communicate with nodes until the hard fork to Allegra actually takes place. At that moment, they will stop functioning as they don't know about Allegra. The `NodeToClient` versioning mechanism takes care of this. However, we have noticed that when the update proposal for Allegra becomes stable, the old clients' `Summary` decoder starts to fail with: Summary: expected between 1 and 2 eras but got 3 This is because after the update proposal for Allegra has become stable, the node (which is compatible with Allegra) will extend its `Summary` with an `EraSummary` of the Allegra era. The `Summary` decoder of the old client does not support more `EraSummary`s (3) than eras it is aware of (2) and fails. This is /before/ the hard fork has actually happened. While the window between this and the actual hard fork, at which point the old client would stop functioning anyway, is small, it would still be nicer if this were handled gracefully. So instead of failing when we receive more `EraSummary`s than eras we statically are aware of, we ignore them. For example, a client that thinks Shelley is the final era will ignore the `EraSummary` of Allegra (and Mary) and will keep acting as if Shelley is the final era. Of course, when the hard fork to the next era actually happens, the client will stop functioning as before. Note that this fix is too late for the upcoming Allegra hard fork: only clients that haven't been upgraded to the last release will run into this problem. Upgrading them to the next release that includes this fix already makes the problem go away, as they will gain support for Allegra (and Mary). This fix will thus only help the next time, i.e., when clients are running a version that supports Allegra and Mary (and includes this fix) but not the era after that, i.e., Alonzo, and the update proposal to Alonzo has become stable. Co-authored-by: Thomas Winant <thomas@well-typed.com>
Build failed: |
Old wallets (or other clients) that are only aware of Byron and Shelley should be able to communicate with nodes until the hard fork to Allegra actually takes place. At that moment, they will stop functioning as they don't know about Allegra. The `NodeToClient` versioning mechanism takes care of this. However, we have noticed that when the update proposal for Allegra becomes stable, the old clients' `Summary` decoder starts to fail with: Summary: expected between 1 and 2 eras but got 3 This is because after the update proposal for Allegra has become stable, the node (which is compatible with Allegra) will extend its `Summary` with an `EraSummary` of the Allegra era. The `Summary` decoder of the old client does not support more `EraSummary`s (3) than eras it is aware of (2) and fails. This is /before/ the hard fork has actually happened. While the window between this and the actual hard fork, at which point the old client would stop functioning anyway, is small, it would still be nicer if this were handled gracefully. So instead of failing when we receive more `EraSummary`s than eras we statically are aware of, we ignore them. For example, a client that thinks Shelley is the final era will ignore the `EraSummary` of Allegra (and Mary) and will keep acting as if Shelley is the final era. Of course, when the hard fork to the next era actually happens, the client will stop functioning as before. Note that this fix is too late for the upcoming Allegra hard fork: only clients that haven't been upgraded to the last release will run into this problem. Upgrading them to the next release that includes this fix already makes the problem go away, as they will gain support for Allegra (and Mary). This fix will thus only help the next time, i.e., when clients are running a version that supports Allegra and Mary (and includes this fix) but not the era after that, i.e., Alonzo, and the update proposal to Alonzo has become stable.
mrBliss
force-pushed
the
mrBliss/handle-too-many-summaries
branch
from
December 15, 2020 13:43
91dfb21
to
09f6f7f
Compare
bors merge |
Build succeeded: |
dcoutts
reviewed
Dec 15, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Old wallets (or other clients) that are only aware of Byron and Shelley should
be able to communicate with nodes until the hard fork to Allegra actually takes
place. At that moment, they will stop functioning as they don't know about
Allegra. The
NodeToClient
versioning mechanism takes care of this.However, we have noticed that when the update proposal for Allegra becomes
stable, the old clients'
Summary
decoder starts to fail with:This is because after the update proposal for Allegra has become stable, the
node (which is compatible with Allegra) will extend its
Summary
with anEraSummary
of the Allegra era. TheSummary
decoder of the old client doesnot support more
EraSummary
s (3) than eras it is aware of (2) and fails. Thisis /before/ the hard fork has actually happened. While the window between this
and the actual hard fork, at which point the old client would stop functioning
anyway, is small, it would still be nicer if this were handled gracefully.
So instead of failing when we receive more
EraSummary
s than eras we staticallyare aware of, we ignore them. For example, a client that thinks Shelley is the
final era will ignore the
EraSummary
of Allegra (and Mary) and will keepacting as if Shelley is the final era. Of course, when the hard fork to the next
era actually happens, the client will stop functioning as before.
Note that this fix is too late for the upcoming Allegra hard fork: only clients
that haven't been upgraded to the last release will run into this problem.
Upgrading them to the next release that includes this fix already makes the
problem go away, as they will gain support for Allegra (and Mary).
This fix will thus only help the next time, i.e., when clients are running a
version that supports Allegra and Mary (and includes this fix) but not the era
after that, i.e., Alonzo, and the update proposal to Alonzo has become stable.