solidity implementation for ics23 #58

lightyear15 · 2021-11-29T12:04:05Z

Code is splitted in 4 different libraries, following the layout of
functions and files of the go implementation

Ops -> go/ops.go
Proof -> go/proof.go
Compress -> go/compress.go
Ics23 -> go/ics23.go

Missing parts of the current implementation:

sha512 hash option: solidity does not have built-in function for this
hash. A solidity implementation would be utterly inefficient and very
costly (gas-wise)
compress functions: this implementation is meant be used for
verification, not for proof generation
ProofSpec for tendermint and iavl definition: again, this
implementation is just to verify, not to generate
Decompress functions are present but not working: currently
investigating a potention buf in the protobuf runtime
Batch verification functions are temporarily disabled due to above

Code is splitted in 4 different libraries, following the layout of functions and files of the go implementation - Ops -> go/ops.go - Proof -> go/proof.go - Compress -> go/compress.go - Ics23 -> go/ics23.go Missing parts of the current implementation: - sha512 hash option: solidity does not have built-in function for this hash. A solidity implementation would be utterly inefficient and very costly (gas-wise) - compress functions: this implementation is meant be used for verification, not for proof generation - ProofSpec for tendermint and iavl definition: again, this implementation is just to verify, not to generate - Decompress functions are present but not working: currently investigating a potention buf in the protobuf runtime - Batch verification functions are temporarily disabled due to above

lightyear15 · 2021-11-29T12:11:08Z

Hello @adizee @andynog
you might be interested in this

@ethanfrey ,
Here is my PR with the solidity implementation.
I know I promised to use truffle, but after a few hours trying to write the first JS unitest I had a breakdown 😄 , so I decided to move to brownie for a much better user experience.
The framework it's released under MIT as well so I hope this change won't cause any license problem.

I am investigating the issue with the Protobuf decompress functions, trying to tailor the the simplest possible example for the maintainers of the solidity-protobuf to start the investigation.

ethanfrey · 2021-11-29T17:33:58Z

Missing parts of the current implementation:

sha512 hash option: solidity does not have built-in function for this
hash. A solidity implementation would be utterly inefficient and very
costly (gas-wise)

Agree. No know implementations use this. It was just added for completeness.

compress functions: this implementation is meant be used for
verification, not for proof generation

Agree.

ProofSpec for tendermint and iavl definition: again, this
implementation is just to verify, not to generate

The ProofSpec is needed to verify. You don't need to export it, but the test cases should have access to those proof specs so they can verify the same data. (You don't need a solidity object for it if you don't want)

Decompress functions are present but not working: currently
investigating a potention buf in the protobuf runtime

These only apply to batch proofs. Current ICS / Relayer usage seems to just include a series of existence proofs and no batching/compression, so simply throwing an error "not yet implemented" and leaving an issue to add them seems fine

Batch verification functions are temporarily disabled due to above

ethanfrey · 2021-11-29T17:34:59Z

The framework it's released under MIT as well so I hope this change won't cause any license problem.

MIT is good.

I will try to review soon, but it is fine to merge with the described functionality if it works

adlerjohn · 2021-12-01T13:51:17Z

I will take a look at this, but might be a couple days.

lightyear15 · 2021-12-03T08:28:33Z

FYI:
With the help of the maintainers, I found out why decompress function doesn't work.
datachainlab/solidity-protobuf#23
The current solidity-protobuf compiler does not support packed repeated fields.
As explained in the issue there is a workaround: adding attribute [packed=false] to field path

... 🤔 ...
I wonder if this bug is also affecting message InnerSpec

ethanfrey

The test code looks nice and proper.

I cannot review solidity, so I would take someone else on that, but from my end, we could merge this as is (tested) and then have another pass with a solidity reviewer to ensure there are no issues in that code.

ethanfrey · 2021-12-07T17:36:54Z

sol/tests/test_ics23compress.py

+
+@pytest.mark.parametrize("fname", [os.path.join(IAVL_TEST_DIR, fname) for fname in cases_fname])
+@pytest.mark.skip(reason="potential bug in ProtobufRuntime")
+def test_iavl_decompress(compress, proof, fname):


Amazed at how expressive these tests can be with those decorators. Nice stuff.

Shame they are skipped. But compress is not essential for v1

ethanfrey · 2021-12-07T17:39:46Z

sol/tests/test_ics23ops.py

+    key = base64.b64decode(case["Key"] or "")
+    value = base64.b64decode(case["Value"] or "")
+    if case["IsErr"]:
+        with brownie.reverts():


Does this ensure it throws an exception?

ethanfrey · 2021-12-07T17:40:51Z

sol/tests/test_ics23vectors.py

+    return accounts[0].deploy(Proof_UnitTest)
+
+
+@pytest.mark.parametrize("fname", [os.path.join(IAVL_TEST_DIR, fname) for fname in cases_fname])


ethanfrey · 2021-12-07T17:42:23Z

@lightyear15 sorry for the long delay.

If you want, I can merge it. Just let me know.
Otherwise, feel free to get a reviewer for the solidity stuff first

lightyear15 · 2021-12-07T18:54:33Z

Hello @ethanfrey ,
thanks a lot for your review.
I will talk to ICF and see how they want to proceed

sol/contracts/ics23Proof.sol

sol/contracts/ics23Ops.sol

sol/contracts/ics23.sol

Changing the return values of functions throughout the libraries to avoid using ``require`` and ``revert``. Function callers will receive an enum encoding the Error

Separating Mock contracts from libraries

sol/.gitattributes

adlerjohn · 2022-01-09T20:18:33Z

sol/contracts/ProtoBufRuntime.sol

@@ -0,0 +1,2990 @@
+// SPDX-License-Identifier: Apache-2.0


The biggest issue here is the use of this library. It is a protobuf codec that may or may not follow a non-existent protobuf spec, not an ADR-027 codec. The latter is highly recommended (read: required) to avoid the possibility of malleability attacks such as this one. You should be using the following libraries to deal with protobuf.

https://github.com/celestiaorg/protobuf3-solidity

https://github.com/celestiaorg/protobuf3-solidity-lib

HI @adlerjohn ,
the problem with the celestiaorg compiler is that, as reported in the README, it does not support oneof needed in
BatchEntry
CompressedBatchEntry
and most importantly, in
CommitmentProof

oneof can be implemented as a library! Much better than fixing the currently-used library to be ADR-027-compliant. PRs welcome.

I have talked to the datachainlab/solidity-protobuf compiler's maintainers.
They have plans to make it ADR-27 compliant soon.
I think I will just sit and wait for it 😁

That works too!

sol/contracts/ics23Proof.sol

lightyear15 · 2022-01-31T14:35:11Z

Datachain lab have updated their compiler which now is ADR-27 compliant.
The changes to the branch updated the compiled sol libraries with the new version of the compiler.
This PR should be ready for review as all the @adlerjohn comments have been addressed

schnetzlerjoe · 2022-09-09T05:31:47Z

@ethanfrey is work on this complete? Curious!

hu55a1n1 · 2022-09-12T13:15:05Z

Hi @adlerjohn and @mkaczanowski, would you be able to take a look to see if your comments have been addressed and approve the PR if you find the time?

crodriguezvega · 2023-09-22T09:48:24Z

Is there still interest in merging this PR?

lightyear15 · 2023-09-22T09:54:53Z

Is there still interest in merging this PR?

not on my side

crodriguezvega · 2023-09-22T11:37:07Z

not on my side

Ok, thank you. Then I will close the PR for now.

ethanfrey approved these changes Dec 7, 2021

View reviewed changes

ethanfrey mentioned this pull request Dec 7, 2021

WIP: Solidity Implementation #12

Closed

mkaczanowski reviewed Dec 9, 2021

View reviewed changes

sol/contracts/ics23Proof.sol Outdated Show resolved Hide resolved

mkaczanowski reviewed Dec 10, 2021

View reviewed changes

sol/contracts/ics23Ops.sol Outdated Show resolved Hide resolved

mkaczanowski reviewed Dec 10, 2021

View reviewed changes

sol/contracts/ics23.sol Outdated Show resolved Hide resolved

first partial round of review

67821e5

mkaczanowski reviewed Dec 10, 2021

View reviewed changes

sol/contracts/ics23.sol Outdated Show resolved Hide resolved

Giulio added 2 commits December 14, 2021 09:17

2nd round of reviews

725563f

Changing the return values of functions throughout the libraries to avoid using ``require`` and ``revert``. Function callers will receive an enum encoding the Error

3rd round of review

2ce6204

Separating Mock contracts from libraries

adlerjohn suggested changes Jan 9, 2022

View reviewed changes

Giulio added 3 commits January 13, 2022 11:49

first round of PR review

36d0587

updating with last solidity-protobuf compiler

83f9d68

updating the decompress functions with ErrorEnum

1e41626

crodriguezvega closed this Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solidity implementation for ics23 #58

solidity implementation for ics23 #58

lightyear15 commented Nov 29, 2021

lightyear15 commented Nov 29, 2021

ethanfrey commented Nov 29, 2021

ethanfrey commented Nov 29, 2021

adlerjohn commented Dec 1, 2021

lightyear15 commented Dec 3, 2021

ethanfrey left a comment

ethanfrey Dec 7, 2021

ethanfrey Dec 7, 2021

ethanfrey Dec 7, 2021

lightyear15 Dec 7, 2021

ethanfrey Dec 7, 2021

ethanfrey commented Dec 7, 2021

lightyear15 commented Dec 7, 2021

adlerjohn Jan 9, 2022

lightyear15 Jan 11, 2022 •

edited

Loading

adlerjohn Jan 11, 2022

lightyear15 Jan 13, 2022

adlerjohn Jan 13, 2022

lightyear15 commented Jan 31, 2022

schnetzlerjoe commented Sep 9, 2022

hu55a1n1 commented Sep 12, 2022

crodriguezvega commented Sep 22, 2023

lightyear15 commented Sep 22, 2023

crodriguezvega commented Sep 22, 2023

		return accounts[0].deploy(Proof_UnitTest)


		@pytest.mark.parametrize("fname", [os.path.join(IAVL_TEST_DIR, fname) for fname in cases_fname])

solidity implementation for ics23 #58

solidity implementation for ics23 #58

Conversation

lightyear15 commented Nov 29, 2021

lightyear15 commented Nov 29, 2021

ethanfrey commented Nov 29, 2021

ethanfrey commented Nov 29, 2021

adlerjohn commented Dec 1, 2021

lightyear15 commented Dec 3, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Dec 7, 2021

lightyear15 commented Dec 7, 2021

Choose a reason for hiding this comment

lightyear15 Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightyear15 commented Jan 31, 2022

schnetzlerjoe commented Sep 9, 2022

hu55a1n1 commented Sep 12, 2022

crodriguezvega commented Sep 22, 2023

lightyear15 commented Sep 22, 2023

crodriguezvega commented Sep 22, 2023

lightyear15 Jan 11, 2022 •

edited

Loading