feat: Import all SMT files into cosmos-sdk without any build errors #232

Manav-Aggarwal · 2022-07-15T21:09:21Z

Description

Closes: #231

For Cevmos' Block Fraud Proofs we need to have a SMT based store. We want to try and backport the one from v0.46.x.

Follow up would be in ##111

Please use manav/backport_smt-add_files...vulcanize:cosmos-sdk:release-v0.46.0-smt-0.0.5-alpha for comparing this to the vulcanize implementation if useful for review

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)

liamsi

It looks like the bulk changes here are from https://github.com/vulcanize/cosmos-sdk/releases/tag/v0.46.0-smt-0.0.5-alpha?

Can we instead cherry-pick changes and then resolve merge conflicts? Even if github does not give any real guarantees about the commits being the same it will help reviewing the PR a bit IMO.

jbowen93 · 2022-07-19T13:42:24Z

It looks like the bulk changes here are from https://github.com/vulcanize/cosmos-sdk/releases/tag/v0.46.0-smt-0.0.5-alpha?

Can we instead cherry-pick changes and then resolve merge conflicts? Even if github does not give any real guarantees about the commits being the same it will help reviewing the PR a bit IMO.

Would you prefer cherry picking the commits from Vulkanize and then layering commits on top of that to resolve changes needed for v0.46 -> v0.45?

Manav-Aggarwal · 2022-07-22T10:11:46Z

Looks like there's a race condition in a test sometimes but unable to reproduce locally. Will move on to #111 and build on top of this for now. Please feel free to suggest any possible ways to resolve this.

Things tried: adding a sync.Mutex lock/unlock around write and GetKVStore in cache_store.go

Manav-Aggarwal · 2022-07-22T17:03:52Z

For review: manav/backport_smt-add_files...vulcanize:cosmos-sdk:release-v0.46.0-smt-0.0.5-alpha

Manav-Aggarwal · 2022-07-22T17:08:25Z

It looks like the bulk changes here are from https://github.com/vulcanize/cosmos-sdk/releases/tag/v0.46.0-smt-0.0.5-alpha?

Can we instead cherry-pick changes and then resolve merge conflicts? Even if github does not give any real guarantees about the commits being the same it will help reviewing the PR a bit IMO.

Please correct me if I am wrong but wouldn't that mean looking at manav/backport_smt...vulcanize:cosmos-sdk:release-v0.46.0-smt-0.0.5-alpha and cherry-picking commits? It seems like there are 1k+ commits there which might not be worth the effort

adlerjohn

Can you rebase manav/backport_smt against the latest default branch? As-is, this PR is useless since it's based on a stale Cosmos SDK release.

.github/workflows/labeler.yml

evan-forbes · 2022-07-25T07:37:41Z

store/cachemulti/store.go

-	traceContext types.TraceContext
+	traceWriter       io.Writer
+	traceContext      types.TraceContext
+	traceContextMutex sync.RWMutex


should the mutex be a pointer?

Should that affect functionality in this case? I tried searching how mutexes are used in the cosmos-sdk currently and it seems like there are plenty of places where they are not pointers:
https://github.com/cosmos/cosmos-sdk/blob/2646b474c7beb0c93d4fafd395ef345f41afc251/snapshots/store.go#L30
https://github.com/cosmos/cosmos-sdk/blob/2646b474c7beb0c93d4fafd395ef345f41afc251/types/config.go#L21

Just rebased on top of 0.45.5 instead, and that seems to have fixed the issue. Thanks Evan! Appreciate your help :)

…store_test

Manav-Aggarwal · 2022-07-25T14:18:24Z

Can you rebase manav/backport_smt against the latest default branch? As-is, this PR is useless since it's based on a stale Cosmos SDK release.

Rebased to 0.45.5 which is what ethermint uses

liamsi · 2022-07-31T09:48:59Z

What is the status of this PR? Ethermint now uses 0.46

Manav-Aggarwal · 2022-08-01T05:05:58Z

What is the status of this PR? Ethermint now uses 0.46

good from my side, waiting on review

liamsi · 2022-08-01T05:25:57Z

I don't really follow. I thought this backports code from 0.46 to an older version of the SDK? Why would that still be necessary when you'd be now using 0.46 directly? Are you sure this is good to go as is @Manav-Aggarwal?

liamsi · 2022-08-01T05:35:50Z

Also, related to this:

Please correct me if I am wrong but wouldn't that mean looking at manav/backport_smt...vulcanize:cosmos-sdk:release-v0.46.0-smt-0.0.5-alpha and cherry-picking commits? It seems like there are 1k+ commits there which might not be worth the effort

You should compare the diff of their work from where they forked off and not against your branch in our fork. If you look at
cosmos/cosmos-sdk@main...vulcanize:cosmos-sdk:release-v0.46.0-smt-0.0.5-alpha you can see they are only 86 commits, and mostly related to the SMT stuff the vlucanize team has been working on only.

As a general note: I would not take the changes of others and commit them under a different account if avoidable (you lose the git history by that and it attributes code to the wrong account). There might be scenarios where this is unavoidable maybe. But git would allow different strategies here at least: merging one branch into another, or the aforementioned cherry-picking. another option is interactive rebase (with picking the relevant changes).

Manav-Aggarwal · 2022-08-01T23:43:48Z

I don't really follow. I thought this backports code from 0.46 to an older version of the SDK? Why would that still be necessary when you'd be now using 0.46 directly? Are you sure this is good to go as is @Manav-Aggarwal?

Since cosmos-sdk v0.46 supports tendermint v0.34 now, this PR is not needed. We'll be closing this PR and prioritizing #245 directly in cosmos-sdk 0.46

Manav-Aggarwal changed the title ~~Manav/backport smt add files~~ feat: Import all SMT files into cosmos-sdk without any build errors Jul 15, 2022

Manav-Aggarwal linked an issue Jul 15, 2022 that may be closed by this pull request

Import all SMT files into cosmos-sdk without any build errors #231

Closed

Manav-Aggarwal self-assigned this Jul 15, 2022

Manav-Aggarwal added the block-fraud-proofs Block Fraud Proofs (Generation and verification) label Jul 15, 2022

Manav-Aggarwal requested review from liamsi and jbowen93 July 15, 2022 21:37

Manav-Aggarwal marked this pull request as ready for review July 15, 2022 21:39

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch from b8cf96f to 27ccc2f Compare July 15, 2022 22:02

liamsi requested a review from adlerjohn July 17, 2022 13:30

liamsi reviewed Jul 17, 2022

View reviewed changes

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch 4 times, most recently from 68ab1f1 to 9aa377e Compare July 22, 2022 03:44

Manav-Aggarwal added the C: Cevmos Changes related to the cevmos branches label Jul 22, 2022

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch 3 times, most recently from e37f7c9 to a292da8 Compare July 22, 2022 09:19

Manav-Aggarwal changed the title ~~feat: Import all SMT files into cosmos-sdk without any build errors~~ !feat: Import all SMT files into cosmos-sdk without any build errors Jul 22, 2022

Manav-Aggarwal changed the title ~~!feat: Import all SMT files into cosmos-sdk without any build errors~~ feat: Import all SMT files into cosmos-sdk without any build errors Jul 22, 2022

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch from 553781e to e0baa95 Compare July 22, 2022 09:50

adlerjohn reviewed Jul 23, 2022

View reviewed changes

.github/workflows/labeler.yml Outdated Show resolved Hide resolved

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch 2 times, most recently from e648aa4 to cb35c3a Compare July 25, 2022 07:10

evan-forbes reviewed Jul 25, 2022

View reviewed changes

Manav-Aggarwal added 14 commits July 25, 2022 09:23

Add in-memory KV database db

abd4b85

Add SMT (needs internal and pruning util files so added them too)

4f55a66

Update dockerfile

e582584

Add proof and new store types for SMT

aa96454

QueryResult changes

25f3aaf

No pruning errors -> replace pruningtypes

aae5c15

Fix all pruning, snapshot, and migration errors

f6a223f

Add SMT proof type support

15f61fc

lint

f9cbde3

Fix pruning test in store

d0f9d26

Attempt fixing data race issue

a7bc26a

Add SMT to string in store types and remove data race condition from …

50d1c11

…store_test

Add mutex lock to tracingContext

42c660a

Finish rebase

207b382

Manav-Aggarwal force-pushed the manav/backport_smt-add_files branch from cb35c3a to 207b382 Compare July 25, 2022 14:15

adlerjohn marked this pull request as draft July 31, 2022 12:43

Manav-Aggarwal closed this Aug 1, 2022

liamsi deleted the manav/backport_smt-add_files branch August 2, 2022 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Import all SMT files into cosmos-sdk without any build errors #232

feat: Import all SMT files into cosmos-sdk without any build errors #232

Manav-Aggarwal commented Jul 15, 2022 •

edited

Loading

liamsi left a comment

jbowen93 commented Jul 19, 2022

Manav-Aggarwal commented Jul 22, 2022 •

edited

Loading

Manav-Aggarwal commented Jul 22, 2022

Manav-Aggarwal commented Jul 22, 2022

adlerjohn left a comment

evan-forbes Jul 25, 2022

Manav-Aggarwal Jul 25, 2022

Manav-Aggarwal Jul 25, 2022

Manav-Aggarwal commented Jul 25, 2022

liamsi commented Jul 31, 2022 •

edited

Loading

Manav-Aggarwal commented Aug 1, 2022

liamsi commented Aug 1, 2022

liamsi commented Aug 1, 2022 •

edited

Loading

Manav-Aggarwal commented Aug 1, 2022

feat: Import all SMT files into cosmos-sdk without any build errors #232

feat: Import all SMT files into cosmos-sdk without any build errors #232

Conversation

Manav-Aggarwal commented Jul 15, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

liamsi left a comment

Choose a reason for hiding this comment

jbowen93 commented Jul 19, 2022

Manav-Aggarwal commented Jul 22, 2022 • edited Loading

Manav-Aggarwal commented Jul 22, 2022

Manav-Aggarwal commented Jul 22, 2022

adlerjohn left a comment

Choose a reason for hiding this comment

evan-forbes Jul 25, 2022

Choose a reason for hiding this comment

Manav-Aggarwal Jul 25, 2022

Choose a reason for hiding this comment

Manav-Aggarwal Jul 25, 2022

Choose a reason for hiding this comment

Manav-Aggarwal commented Jul 25, 2022

liamsi commented Jul 31, 2022 • edited Loading

Manav-Aggarwal commented Aug 1, 2022

liamsi commented Aug 1, 2022

liamsi commented Aug 1, 2022 • edited Loading

Manav-Aggarwal commented Aug 1, 2022

Manav-Aggarwal commented Jul 15, 2022 •

edited

Loading

Manav-Aggarwal commented Jul 22, 2022 •

edited

Loading

liamsi commented Jul 31, 2022 •

edited

Loading

liamsi commented Aug 1, 2022 •

edited

Loading