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

LocalStateQuery: let the client acquire the immutable tip #4765

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Jan 5, 2024

Description

We will use this to make it easier for the CLI to only output answers that are not subject to rollback.

This change to LocalStateQuery is gated by NodeToClient_Version16, since that is still mutable (it enables Conway et al).

It is a breaking change for any code that involves MsgAcquire or MsgReAcquire, since the type of the argument has changed to a new (more perspicuous and expressive) ADT.

Checklist

  • Branch
    • Updated changelog files.
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • New tests are added if needed and existing tests are updated
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@nfrisby nfrisby requested a review from coot as a code owner January 5, 2024 20:38
@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 5, 2024

The CI complains that "./ouroboros-network-api was modified but its CHANGELOG was not updated". But the change is just a comment that does not deserve mention in the CHANELOG.

Do you have advice for how to proceed?

@coot coot added the local-state-query Issues / PRs related to local-state-query protocol label Jan 5, 2024
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Very nice, thank you @nfrisby.

@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 8, 2024

For "If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.", here's some quotes from a Slack discussion with @ch1bo :

[T]he serialization is backwards-compatible (I used a fresh tag for the new option, whereas the new data structure still has constructors that match the old Nothing/Just constructors and therefore use the same CBOR tag)

My general assumption is that no one downstream of Consensus is caseing over these messages (other than maybe formatting log messages, etc). So your experience should mainly be: there's a new constructor you could choose to call, but you shouldn't if the handshake version negotiated with the server (ie the local node) is too low. (Like Marcin said, if you construct it even if the negotiated version is too low, you'll get an encoder failure on the client side (eg the wallet). If a buggy custom encoder ignores the version, the node will simply disconnect due to the ill-formed message.)

Summary:

Server (ie node) Client (eg wallet) Notes
new new Both know about ImmutableTip ✔️
old old Neither knows about ImmutableTip ✔️
new old Server could handle ImmutableTip, but client won't send it ✔️
old new Our encoders error on the client side if the client logic ignores the negotiated version ✔️; if a buggy custom encoder sent it through, the node would simply disconnect due to a serialization failure ✔️

Also: the existing messages' serialization wasn't changed, but their data constructor names were improved.

@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 8, 2024

I'm assuming the panoply of darwin failures is Hydra's fault, not mine.

@nfrisby nfrisby force-pushed the nfrisby/lsq-acquire-immtip branch from 8bb6157 to 6bc58d4 Compare January 8, 2024 19:05
@coot
Copy link
Contributor

coot commented Jan 8, 2024

I'm assuming the panoply of darwin failures is Hydra's fault, not mine.

It seems so, I restarted them.

@coot coot enabled auto-merge January 11, 2024 13:20
@nfrisby nfrisby force-pushed the nfrisby/lsq-acquire-immtip branch from f30a4f1 to 6b136e9 Compare January 11, 2024 19:14
@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 11, 2024

I rebased and squashed my commits (and removed the merge commit that Marcin pushed).

We will use this to make it easier for the CLI to only output answers that are
not subject to rollback.

This change to LocalStateQuery is gated by NodeToClient_Version16, since that
is still mutable (it enables Conway et al).
@nfrisby nfrisby force-pushed the nfrisby/lsq-acquire-immtip branch from 6b136e9 to cf63266 Compare January 11, 2024 19:38
@coot coot added this pull request to the merge queue Jan 11, 2024
Merged via the queue into master with commit 80a738d Jan 11, 2024
9 of 10 checks passed
@coot coot deleted the nfrisby/lsq-acquire-immtip branch January 11, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
local-state-query Issues / PRs related to local-state-query protocol
Projects
Status: ✅ Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants