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.
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
docs: ADR-049 state sync hooks #10976
docs: ADR-049 state sync hooks #10976
Changes from 7 commits
805f653
8eb5147
3b52898
4d05187
6b34562
86bee96
5689001
7e46ff9
bf67fc3
7990098
50ef809
aea3ea2
bbc3c52
d3fe008
9d82396
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Here there would be one
SnapshotExtensionMeta
for x/wasm module, maybe a second one for Ethermint module, etc?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.
Yes, one for each extension snapshotter. You can have multiple snapshotters.
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.
add a comment and hope that can help clarify the question here
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.
format is globally unique?
like x/wasm needs one, ethermint another, agoric a 3rd?
Or is this used within one namespace?
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.
It’s used within the snapshotter/namespace.
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.
They don't need to globally unique. They need to be unique in the state machine. So if you are 3 different modules that use different types of state sync packets you need formats 1, 2, 3. I don't see why they should be global
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.
yes, they are not global, they can be 1 at the same time.
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.
@zmanian and @yihuang you are saying different things.
This was more or less what I meant by globally unique (on the chain). That you cannot have 2 modules on the same chain both using format 1.
This is what I understood the answer to be, that
(name, format)
must be unique within this state machine. (And if we keep name globally unique, then we only have to consider format in the context of our own module).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.
To clarify there are two formats at play in the snapshot:
SnapshotExtensionPayload
message. So they are not related to one another.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.
technically speaking we can have duplicated
name
s in the stream, it just means the snapshotter'sRestore
methods will be called multiple times. But I don't think that makes much sense, maybe we should even validate to prevent that situation.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.
This is somewhat confusing. I recommend updating the field comment to be very explicit.
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.
Up. It would be good to clarify if
name
is unique, or(name, format)
is. IMO first one is simpler.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.
name
is unique in current implementation.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.
What's the ordering of these extensions in the stream if we register multiple in
RegisterExtensions
? Alphabetical by name?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.
It's alphabetical by name in current implementation.
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.
Sounds good, would like to see some wording for this. i suppose
name
should be unique then (ref: #10976 (comment))?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.
yes
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.
Ah, key point here. Thanks
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.
It might be worth mentioning that the wire format of the snapshot is backward compatible when there's no
ExtensionSnapshotter
registered.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.
sure, I think this is significant for application chains that don't have additional state to take snapshots.
So for application chains of this kind, this ADR should be completely backwards compatible?
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.
Yes, let's add some wording that the stream is backwards-compatible if no extension is registered.