Polynetwork support #908

vaivaswatha · 2020-11-19T16:16:47Z

Scilla library functions for polynetwork support

anton-trunov

Hey, I understand this is a WIP PR, still I think some early input might be helpful

src/stdlib/Conversions.scillib

vaivaswatha · 2020-12-20T05:13:03Z

Marking as ready for review. The main contract Polynetwork.scilla is more of a template than production ready. So it may be ok to ignore cosmetic issues in the contract, but any bug findings are appreciated. The library itself (Polynetwork.scillib) on the other hand needs thorough review.

renlulu · 2020-12-20T05:57:31Z

Marking as ready for review. The main contract Polynetwork.scilla is more of a template than production ready. So it may be ok to ignore cosmetic issues in the contract, but any bug findings are appreciated. The library itself (Polynetwork.scillib) on the other hand needs thorough review.

Hey, I moved your contract Polynetwork.scilla (with some common design and enhancement) into this repo: https://github.com/Zilliqa/zilliqa-contracts/tree/main/contracts. This will be the main repository for polynetwork as you can see it forked from their repository. Anyways, many thanks to your hard work (both on scilla library support and contract itself). End 2 end testing will be continuing recently. Will sync with you along the way! Thanks!

anton-trunov

I'm wondering why in a lot of not-related test cases we got a constant increase of gas usage by about 500 units?

tests/eval/good/polynetwork_extract_bystr2.scilexp

tests/eval/good/polynetwork_extract_bystr3.scilexp

tests/eval/good/polynetwork_next_var_uint2.scilexp

tests/eval/good/polynetwork_next_var_uint3.scilexp

tests/eval/good/polynetwork_next_var_uint4.scilexp

tests/eval/good/polynetwork_next_var_uint5.scilexp

tests/eval/good/polynetwork_next_var_uint6.scilexp

tests/eval/good/polynetwork_next_var_uint7.scilexp

vaivaswatha · 2020-12-20T09:50:25Z

I'm wondering why in a lot of not-related test cases we got a constant increase of gas usage by about 500 units?

This is because we have a new library (two actually) and the eval-runner executable includes all libraries it can find to it's environment.

anton-trunov · 2020-12-20T09:56:57Z

the eval-runner executable includes all libraries it can find to it's environment.

Ah, I see, thanks. Looks like this should be fixed at some point. I would expect having more libraries in the tests in the future and checking unrelated changes does not seem like a lot of fun to me :)

vaivaswatha · 2020-12-20T10:05:06Z

the eval-runner executable includes all libraries it can find to it's environment.

Ah, I see, thanks. Looks like this should be fixed at some point. I would expect having more libraries in the tests in the future and checking unrelated changes does not seem like a lot of fun to me :)

I agree. At that point we must include a new test that will include all libraries though, and keep it updated when new libraries are added. This ensures there are no name clashes in our own libraries. Such a clash is now flagged by these implicit "all imports"

vaivaswatha · 2020-12-20T10:10:39Z

Reference: https://github.com/polynetwork/eth-contracts/blob/master/contracts/core/cross_chain_manager/logic/EthCrossChainManager.sol

jjcnn

I haven't finished my review yet, but I wanted to make my initial comments visible.

I'll do my best to get the review done before I go on Christmas leave tomorrow evening, but if not I'll get it done between Christmas and New Years.

jjcnn · 2020-12-21T11:30:34Z

src/stdlib/Conversions.scillib

+  | BigEndian
+
+(* does not throw exceptions *)
+let osubstr : ByStr -> Uint32 -> Uint32 -> Option ByStr =


Suggested change

let osubstr : ByStr -> Uint32 -> Uint32 -> Option ByStr =

let substr_safe : ByStr -> Uint32 -> Uint32 -> Option ByStr =

We have an unsafe substr and the o prefix indicates return of an option value. Since the name was suggested by @anton-trunov , let's see if he's ok with the new name too. If yes, I'll do this change.

I'm fine with renaming.

src/stdlib/Conversions.scillib

vaivaswatha · 2021-01-05T14:48:43Z

Based on offline discussion, ignoring @jjcnn 's comments above, except for renaming osubstr to substr_safe. Thereby abandoning #922 too.

jjcnn

Review finished. I have some concerns both in terms of what the code does, and how it fits into our general process, but I don't know how to fix either.

I'd like to have a chat about it before I approve the PR, even if no changes are needed.

jjcnn · 2021-01-06T02:10:36Z

src/stdlib/Polynetwork.scillib

+  let ignore = builtin div zero_uint32 zero_uint32 in
+  default_val
+
+let some_exn =


Hmm... This naming scheme is consistent with Jane Street's libraries, so maybe we should use substr_no_exn in the other function? I'm not sure - we're probably not consistent about things anyway.

src/stdlib/Polynetwork.scillib

jjcnn · 2021-01-06T06:14:52Z

src/stdlib/Polynetwork.scillib

+  fun (msg : ByStr) =>
+  fun (sig : ByStr64) =>
+  fun (recid : Uint32) =>
+    let pk = builtin ecdsa_recover_pk msg sig recid in


I just looked up what this builtin does, and I honestly don't understand it.

This is documented here https://scilla.readthedocs.io/en/latest/scilla-in-depth.html?highlight=ecdsa#crypto-built-ins.

That's what I looked up, and that's the thing I don't understand. :-(

src/stdlib/Polynetwork.scillib

jjcnn · 2021-01-07T01:43:47Z

tests/contracts/Polynetwork.scilla

+    let nil_msg = Nil {Message} in
+    Cons {Message} msg nil_msg
+
+contract Polynetwork(thisChainID : Uint64)


As far as I can tell this contract corresponds to the Solidity contract, but I'm not 100% certain because much of the library code is complete magic to me.

I'm concerned about how we will maintain this code. As far as I can tell we need to be downstream from the Solidity contract, but I don't understand the process by which that happens.

There doesn't seem to be an answer to this. Presumably we just have to trust that we'll be notified if something needs to be fixed. It's not ideal TBH, but I suppose it will have to do.

tests/runner/Polynetwork/message_1.json

jjcnn

OK, I've checked Polynetwork.scillib and the testcases against the Solidity code, and there seems to be some sort of equivalence.

The only two outstanding issues are a naming issue, which is not essential, and a documentation issue, which is an issue for the documentation repo rather than for this PR.

jjcnn · 2021-01-07T07:38:20Z

src/stdlib/Polynetwork.scillib

+    let header_hash =
+      fun (header : ByStr) =>
+        let h1 = builtin sha256hash header in
+        let h2 = builtin sha256hash h1 in


In the Solidity contract there's a call to abi.encodePacked between the two hash calls. Is that an artefact from the Solidity implementation?

Yep, abi.encodePacked just keeps appending.

jjcnn · 2021-01-07T07:52:40Z

src/stdlib/Polynetwork.scillib

+          let rs_bs = builtin substr rs_v pos len in
+          let rs_bs64_opt = builtin to_bystr64 rs_bs in
+          let exnobj_bystr64 = 0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 in
+          let sommer_bystr64 = @some_exn ByStr64 in


This whole bit is an implementation of slice, which is written in EVM assembly in the Solidity contract. I'm giving up there... :-(

Or is it? Is slice just a substring computation?

slice is just substring computation, yes.

So this computation here is separating the signature into two (r,s) components, and then the last byte is the v component. We don't do the + 27 though because that is an EVM specific thing. Our ecrecover doesn't need it.

I had to do a lot of trial-and-error to get this right. Finally this helped: https://ethereum.stackexchange.com/questions/13778/get-public-key-of-any-ethereum-account

I added this reference into the code now.

src/stdlib/Polynetwork.scillib

jjcnn · 2021-01-07T08:06:39Z

tests/contracts/Polynetwork.scilla

+    let nil_msg = Nil {Message} in
+    Cons {Message} msg nil_msg
+
+contract Polynetwork(thisChainID : Uint64)


There doesn't seem to be an answer to this. Presumably we just have to trust that we'll be notified if something needs to be fixed. It's not ideal TBH, but I suppose it will have to do.

Add header deserialization and utilities for it

dd93b89

anton-trunov reviewed Nov 19, 2020

View reviewed changes

src/stdlib/Conversions.scillib Outdated Show resolved Hide resolved

src/stdlib/Conversions.scillib Show resolved Hide resolved

vaivaswatha added 3 commits November 20, 2020 13:37

Merge branch 'master' into polynetwork

7f8e87e

Merge branch 'master' into polynetwork

c0fdad3

Make all integer conversions little-endian

4627dc5

vaivaswatha force-pushed the polynetwork branch from 7643bbe to 4627dc5 Compare November 20, 2020 11:02

vaivaswatha and others added 24 commits November 20, 2020 22:13

Provide an option version of substr

a52a191

Fix order of fields in Header and add test to testsuite

4bd3e9a

Deserialize TxParam and ToMerkleValue

990b3f4

test: add multiple tests for library function next_var_uint

844b5fd

test: add test cases for library function extract_bystr

9cba52a

chore: add testing file into ml file

7d47ef6

Merge branch 'master' into polynetwork

b21117f

Merge branch 'polynetwork' of github.com:Zilliqa/scilla into polynetwork

8fbcaa1

Merge branch 'master' into polynetwork

2234dbd

VarUint serialization

f0c45f7

Merge branch 'master' into polynetwork

a3dd75b

Add serialization of variable length byte strings

d911fbf

Provide serialization to uint16

a24969d

Add compress_pubkey function

7ece7d7

add verifyPubkey and getBookKeeper functions

bfc9fc0

Merge branch 'master' into polynetwork

c681469

Rename few functions to not have confliction names

2e03949

add initGenesisBlock transition

189d3d7

Add merkle proof function

d456444

Add verifyHeaderAndExecuteTx transition

8ad57bf

Add changeBookKeeper transition

49c451e

Add transition crossChain

96c2a77

Add getBookKeeper test

3713163

Fix typo / bug in Polynetwork.lib

717cdc2

vaivaswatha added 5 commits December 8, 2020 19:16

Test for VerifyHeaderAndExecuteTxEvent

4081f48

Improve VerifyHeaderAndExecuteTxEvent

5f5904a

Merge branch 'master' into polynetwork

64b791c

use to_ascii instead of to_string for method_name

d2c639d

Add more tests, including expected fail tests for Polynetwork

10c6d4a

vaivaswatha force-pushed the polynetwork branch from 01ecf0f to 10c6d4a Compare December 9, 2020 08:41

vaivaswatha added 2 commits December 20, 2020 09:52

Merge branch 'master' into polynetwork

bc55b73

Incorporate _origin and fix all tests

88aa15a

vaivaswatha marked this pull request as ready for review December 20, 2020 05:09

vaivaswatha requested a review from jjcnn as a code owner December 20, 2020 05:09

vaivaswatha requested review from renlulu and AmritKumar December 20, 2020 05:13

anton-trunov reviewed Dec 20, 2020

View reviewed changes

Fix bugs in tests in creating div by zero error

cc3dcb2

jjcnn reviewed Dec 21, 2020

View reviewed changes

vaivaswatha mentioned this pull request Dec 22, 2020

Jacob's suggestion in PR #908 #922

Closed

Rename osubstr to substr_safe based on review comment

bd62317

jjcnn reviewed Jan 7, 2021

View reviewed changes

jjcnn approved these changes Jan 7, 2021

View reviewed changes

Add a reference for ecrecover

5f46817

vaivaswatha merged commit 4ef0333 into master Jan 8, 2021

vaivaswatha deleted the polynetwork branch January 8, 2021 07:58

vaivaswatha mentioned this pull request May 18, 2021

Document Conversions and Polynetwork libraries Zilliqa/scilla-docs#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polynetwork support #908

Polynetwork support #908

vaivaswatha commented Nov 19, 2020 •

edited

Loading

anton-trunov left a comment

vaivaswatha commented Dec 20, 2020

renlulu commented Dec 20, 2020 •

edited

Loading

anton-trunov left a comment

vaivaswatha commented Dec 20, 2020

anton-trunov commented Dec 20, 2020

vaivaswatha commented Dec 20, 2020

vaivaswatha commented Dec 20, 2020

jjcnn left a comment

jjcnn Dec 21, 2020

vaivaswatha Dec 22, 2020

anton-trunov Dec 27, 2020

vaivaswatha commented Jan 5, 2021

jjcnn left a comment

jjcnn Jan 6, 2021

jjcnn Jan 6, 2021

vaivaswatha Jan 7, 2021

jjcnn Jan 7, 2021

jjcnn Jan 7, 2021

jjcnn Jan 7, 2021

jjcnn left a comment

jjcnn Jan 7, 2021

vaivaswatha Jan 7, 2021

jjcnn Jan 7, 2021

jjcnn Jan 7, 2021

vaivaswatha Jan 7, 2021

vaivaswatha Jan 7, 2021

vaivaswatha Jan 7, 2021

vaivaswatha Jan 7, 2021

jjcnn Jan 7, 2021

	let osubstr : ByStr -> Uint32 -> Uint32 -> Option ByStr =
	let substr_safe : ByStr -> Uint32 -> Uint32 -> Option ByStr =

Polynetwork support #908

Polynetwork support #908

Conversation

vaivaswatha commented Nov 19, 2020 • edited Loading

anton-trunov left a comment

Choose a reason for hiding this comment

vaivaswatha commented Dec 20, 2020

renlulu commented Dec 20, 2020 • edited Loading

anton-trunov left a comment

Choose a reason for hiding this comment

vaivaswatha commented Dec 20, 2020

anton-trunov commented Dec 20, 2020

vaivaswatha commented Dec 20, 2020

vaivaswatha commented Dec 20, 2020

jjcnn 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

vaivaswatha commented Jan 5, 2021

jjcnn 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

Choose a reason for hiding this comment

jjcnn 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

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

vaivaswatha commented Nov 19, 2020 •

edited

Loading

renlulu commented Dec 20, 2020 •

edited

Loading