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

R4R: Add proof verification #3191

Closed
wants to merge 9 commits into from
Closed

R4R: Add proof verification #3191

wants to merge 9 commits into from

Conversation

abelliumnt
Copy link

The merkle proof can be divided into two category: existence proof and absence proof. They have difference verification methods. Current, the proof verification will use wrong method to verify absence proof if the query key doesn't exist.

@zmanian
Copy link
Member

zmanian commented Dec 25, 2018

Can we write a test case for this?

@abelliumnt
Copy link
Author

I added a new commit to enable distrust mode in lcd test. Then the currently lcd test can cover the code for proof verification.

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

looks like lcd test is failing and should be fixed (in test_cover)

@cwgoes
Copy link
Contributor

cwgoes commented Jan 2, 2019

This is probably failing because of the upstream issue - tendermint/tendermint#2862.

@mossid
Copy link
Contributor

mossid commented Jan 8, 2019

The test is failing because the validator set is different in some of the tests. Before TestBonding, InitializaTestLCD takes 1, in result generating same validator set. In TestBonding, the argument becomes 2 to test validator bonding. tests.WaitForHeight queries http://localhost:port/blocks/latest to the lcd. The problem is that the lcd is not reset at the end of each TestXXX, so the previous tests' trusted commits are not cleaned up. So when the validator set is different from the previous ones(TestBond in this case), the lcd query is requested to proof a signed header whose validator set is different from the stored one with the same height, returns error, and panics on tests/util.go when attempt to unmarshal it as a json value.

@jackzampolin jackzampolin mentioned this pull request Jan 8, 2019
5 tasks
@jackzampolin
Copy link
Member

An alternate implementation: #3236

@jackzampolin
Copy link
Member

@mossid how do we fix that issue?

@abelliumnt
Copy link
Author

@mossid
In lcd tests, the lcd home directories are readomly generated. So it is impossible that two lcd processes share the same home directory.
Besides, the failed unit test is unbondingTest, if you launch the single test, it will pass. If you run all tests together, then it will fail. That's very strange.

@jackzampolin
Copy link
Member

@HaoyangLiu The tests are dependent on eachother. I did some work trying to parallelize them about a month ago without much luck.

@jackzampolin
Copy link
Member

@HaoyangLiu Is this PR ready to merge? It appears to be still failing tests.

@abelliumnt
Copy link
Author

@jackzampolin
I still working with @mossid to fix the failed unit test.

@jackzampolin
Copy link
Member

@HaoyangLiu Thanks for the update!

@cwgoes cwgoes self-assigned this Jan 29, 2019
@cwgoes cwgoes added this to the v0.31.0 (Launch RC) milestone Jan 30, 2019
@cwgoes
Copy link
Contributor

cwgoes commented Jan 30, 2019

OK, I tried to make a downstream PR - https://github.com/HaoyangLiu/cosmos-sdk/pull/15 - but it looks like CI isn't working, so I'm just going to split this into a separate branch.

@cwgoes cwgoes mentioned this pull request Jan 30, 2019
5 tasks
@cwgoes
Copy link
Contributor

cwgoes commented Jan 30, 2019

Closing in favor of #3449 - but thank you @HaoyangLiu!

@cwgoes cwgoes closed this Jan 30, 2019
@abelliumnt
Copy link
Author

@cwgoes
Great! Recently I was busy with irishub development, so I missed to fix the failed lcd tests. Now Are all lcd tests passed?

@cwgoes
Copy link
Contributor

cwgoes commented Jan 31, 2019

@cwgoes
Great! Recently I was busy with irishub development, so I missed to fix the failed lcd tests. Now Are all lcd tests passed?

Yes - fixed in #3449, now merged.

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