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

Fog passes latest_block_version to clients in its responses #1461

Merged

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Feb 8, 2022

This makes fog-ledger pass latest_block_version to clients in
its responses when they do KeyImage checks or get merkle proofs.

This will mean that they will get an update on this value when
they do a balance check or before they submit a transaction,
giving them a chance to potentially:

  • Pass the latest_block_version to the transaction builder, as
    envisioned in MCIP Block-Version-based Protocol Evolution mcips#26
  • Maybe signal the need for an update if in the course of a balance
    check we learn that the block version has advanced beyond what
    our software was built to support.

This is not a breaking change, this is just the part where fog
communicates the block_version which is backwards compatible,
so it is safe to merge at any time.

The non-fog clients can already ask the local ledger for the latest
block version directly.

We could add an API to consensus as well to get the latest block
header, but it doesn't seem that it's a requirement right now
so we didn't.

We could also have piped this number through fog-view but we
decided not to, since it's more plumbing and it seems that as long
as the user gets this number at least once when they do a balance
check, then it's enough.

This makes fog-ledger pass `latest_block_version` to clients in
its responses when they do `KeyImage` checks or get merkle proofs.

This will mean that they will get an update on this value when
they do a balance check or before they submit a transaction,
giving them a chance to potentially:

* Pass the `latest_block_version` to the transaction builder, as
  envisioned in MCIP mobilecoinfoundation#26.
* Maybe signal the need for an update if in the course of a balance
  check we learn that the block version has advanced beyond what
  our software was built to support.

This is not a breaking change, this is just the part where fog
communicates the `block_version` which is backwards compatible,
so it is safe to merge at any time.

The non-fog clients can already ask the ledger for the latest
block version directly.

We could add an API to consensus as well to get the latest block
header, but it doesn't seem that it's a requirement right now
so we didn't.
Copy link
Contributor

@remoun remoun left a comment

Choose a reason for hiding this comment

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

Thanks for improving the documentation, too!

ledger/db/src/ledger_trait.rs Outdated Show resolved Hide resolved
@cbeck88
Copy link
Contributor Author

cbeck88 commented Feb 8, 2022

Yeah i am a big believer in #[deny(missing_docs)]

Copy link
Contributor

@eranrund eranrund left a comment

Choose a reason for hiding this comment

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

LGTM but there is one thing I am wondering about:
We will be announcing the latest block version in the blockchain, but maybe we should also announce the maximum possible block version (e.g. there might be a period where only v1 blocks are getting into the chain, but v2 has already been implemented and is awaiting activation by a configuration flag and we want clients to start updating so that when v2 starts showing up they are aware of it).

I'm not sure if there's a non-janky way to expose that, since it's just a constant that will depend on a redeploy of the view server.

@cbeck88
Copy link
Contributor Author

cbeck88 commented Feb 8, 2022

I'm not sure if there's a non-janky way to expose that, since it's just a constant that will depend on a redeploy of the view server.

Yeah I'm not sure either -- I think it can't hurt to do that, and might give clients an extra opportunity to announce "there is an update available" or something like this

…ion`

This addresses a review suggestion from Eran
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

LGTM

#[derive(Clone, Eq, PartialEq, Message)]
pub struct QueryRequestAAD {
/// The first id of a user event to return. If you set this larger than 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

love the new comments

@cbeck88 cbeck88 merged commit fa620b3 into mobilecoinfoundation:master Feb 9, 2022
@cbeck88 cbeck88 deleted the fog-passes-block-version branch February 9, 2022 00:10
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.

6 participants