ADR-40: update on multi-store refactor and IBC proofs #10191

roysc · 2021-09-17T11:10:10Z

Description

Update on the proposal for the RootStore and how to handle the new proof scheme to support IBC.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

roysc · 2021-09-17T11:11:14Z

@robert-zaremba I've made some updates on your ADR update branch to cover my root store proposal and what we've discussed on IBC in a little more depth

roysc · 2021-09-17T11:14:28Z

Would also like to solicit @AdityaSripal to check that I've correctly summarized the IBC situation 🙏

robert-zaremba

Thanks for improving the design!

docs/architecture/adr-040-storage-and-smt-state-commitments.md

robert-zaremba · 2021-09-17T14:30:40Z

docs/architecture/adr-040-storage-and-smt-state-commitments.md

+    GetStoreCache(StoreKey, CommitKVStore) CommitKVStore
+    Unwrap(StoreKey) CommitKVStore
+    Reset()
+}


Can we merge these interfaces?

Do we need RootStorePersistentCache?

I think it is desired, but on second thought it probably won't look like this. I will just remove it for now and revisit

Root store should be used only by the app. We can add interfaces if we need later when implementing. Then we can create a new PR to update the design. Let's start from the minimal interface. RootStore should not allow to write objects into it. Only the "normal" stores should allow writes.

robert-zaremba · 2021-09-17T15:06:41Z

docs/architecture/adr-040-storage-and-smt-state-commitments.md


+This workaround can be used until the IBC connection code is fully upgraded and supports single-element commitment proofs.


how about keeping the paragraph: "the RootStore will have only two stores....".
Maybe we can be also more specific:

IBC store key is 01 (2 bits)

"general" store key is 1 (1 bit).
Not sure though if we can only add as much as 1-2 bits though.

Ah, I think I may have misunderstood Aditya's comment - I had thought ibc/clients, ibc/connections were the prefixes of separate stores. Now, if I've got it right, ibc is the only store prefix for IBC data, and the rest are just parts of the key paths. I'll update it.

I'm not sure I understand the prefixes you're proposing though. If you mean how the data is partitioned in the DB, that doesn't seem to belong at this level of design, also I don't think it's feasible to have prefixes smaller than a byte - it would shift the alignment of all the following data.

For IBC proofs, I believe the IBC data must be under the ibc store key, and the general data can just use the empty string.

Yes, my understanding is that there is one store for IBC: the ibc/. Then they have a 3 groups of objects: clients, connections, channels.
So my proposal is that in the root store we have 2 stores:

IBC with 01 prefix (2 bits or 1 byte). AFAIU, IBC supports changing the prefix without doing x/ibc update -- it requires a migration only.

"general" store for everything else.

@AdityaSripal , @colin-axner - could you validate this?

IBC does not support changing the prefix unfortunately because the prefix is in the connection which is non-upgradable.

Can the separate IBC store retain the ibc/ prefix?

yes, if we can't change it then we will keep it.

Co-authored-by: Robert Zaremba <[email protected]>

robert-zaremba · 2021-09-22T15:59:57Z

docs/architecture/adr-040-storage-and-smt-state-commitments.md

+    InitialVersion  uint64
+
+    ReservePrefix(StoreKey, StoreType)
+}


TODO: validate if we really need all these interfaces. It seams to me that the app only needs the CommitRootStore.

We let's add this TODO into the code and merge it - we can do that check in the "main" PR.

robert-zaremba

Pre-Approving. This is a good update, let's add TODOs in the "code" and merge it and continue the discussion in the main PR.

Co-authored-by: Robert Zaremba <[email protected]>

…ert/adr-40-update

* adr-40: use prefix store instead of multistore * add note about prefix.Store * Update SC and SS setup information and historical versions sepc * add note about key prefix optimization * rephrased the changes related to multistore * Apply suggestions from code review Co-authored-by: Ryan Christoffersen <[email protected]> * Update docs/architecture/adr-040-storage-and-smt-state-commitments.md * Update docs/architecture/adr-040-storage-and-smt-state-commitments.md * Update docs/architecture/adr-040-storage-and-smt-state-commitments.md Co-authored-by: Aleksandr Bezobchuk <[email protected]> * design update * update merkle proofs * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <[email protected]> * reword huffman compression paragraph * ADR-40: update on multi-store refactor and IBC proofs (#10191) * Update on multistore refactor and IBC proof * cleanup whitespace * Update docs/architecture/adr-040-storage-and-smt-state-commitments.md Co-authored-by: Robert Zaremba <[email protected]> * revise for PR * add todo * Update docs/architecture/adr-040-storage-and-smt-state-commitments.md Co-authored-by: Robert Zaremba <[email protected]> Co-authored-by: Robert Zaremba <[email protected]> * review updates * add todo for protobuf message type compression * add link to a discussion * guarantee atomic commit with IBC workaround proposal * adding more links to references * Apply suggestions from code review Co-authored-by: Roy Crihfield <[email protected]> * reword the module key compression part Co-authored-by: Ryan Christoffersen <[email protected]> Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Roy Crihfield <[email protected]>

Update on multistore refactor and IBC proof

6f16386

roysc requested review from aaronc and alexanderbez as code owners September 17, 2021 11:10

github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Sep 17, 2021

cleanup whitespace

497c3e7

robert-zaremba reviewed Sep 17, 2021

View reviewed changes

i-norden mentioned this pull request Sep 17, 2021

ADR-040 Implementation: Review and refactor MultiStore #10192

Closed

7 tasks

Update docs/architecture/adr-040-storage-and-smt-state-commitments.md

bb3e72e

Co-authored-by: Robert Zaremba <[email protected]>

roysc mentioned this pull request Sep 20, 2021

docs: adr-40: reduce multistore and make it atomic #9355

Merged

9 tasks

revise for PR

1cf052d

robert-zaremba reviewed Sep 22, 2021

View reviewed changes

robert-zaremba approved these changes Sep 22, 2021

View reviewed changes

roysc and others added 3 commits September 24, 2021 16:30

add todo

7d2ecce

Update docs/architecture/adr-040-storage-and-smt-state-commitments.md

05363d5

Co-authored-by: Robert Zaremba <[email protected]>

Merge remote-tracking branch 'upstream/robert/adr-40-update' into rob…

d66e5f7

…ert/adr-40-update

robert-zaremba merged commit 3f841fd into cosmos:robert/adr-40-update Oct 1, 2021

roysc deleted the roysc/adr-040-update-adr branch February 15, 2022 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR-40: update on multi-store refactor and IBC proofs #10191

ADR-40: update on multi-store refactor and IBC proofs #10191

roysc commented Sep 17, 2021

roysc commented Sep 17, 2021

roysc commented Sep 17, 2021

robert-zaremba left a comment

robert-zaremba Sep 17, 2021

roysc Sep 20, 2021 •

edited

Loading

robert-zaremba Sep 22, 2021

robert-zaremba Sep 17, 2021

roysc Sep 20, 2021 •

edited

Loading

robert-zaremba Sep 22, 2021

AdityaSripal Sep 24, 2021

robert-zaremba Sep 28, 2021

robert-zaremba Sep 22, 2021

robert-zaremba left a comment


		This workaround can be used until the IBC connection code is fully upgraded and supports single-element commitment proofs.

ADR-40: update on multi-store refactor and IBC proofs #10191

ADR-40: update on multi-store refactor and IBC proofs #10191

Conversation

roysc commented Sep 17, 2021

Description

Author Checklist

Reviewers Checklist

roysc commented Sep 17, 2021

roysc commented Sep 17, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Sep 17, 2021

Choose a reason for hiding this comment

roysc Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Sep 22, 2021

Choose a reason for hiding this comment

robert-zaremba Sep 17, 2021

Choose a reason for hiding this comment

roysc Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Sep 22, 2021

Choose a reason for hiding this comment

AdityaSripal Sep 24, 2021

Choose a reason for hiding this comment

robert-zaremba Sep 28, 2021

Choose a reason for hiding this comment

robert-zaremba Sep 22, 2021

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

roysc Sep 20, 2021 •

edited

Loading

roysc Sep 20, 2021 •

edited

Loading