MVP Orchestrator #257

evan-forbes · 2022-03-30T04:19:43Z

Description

This PR introduces the minimum viable orchestrator for the quantum gravity bridge. Still needs to tested with the most recent refactors, along with some documentation additions, but we can start review process.

closes: part 2/3 #223

codecov-commenter · 2022-03-30T04:23:10Z

Codecov Report

❗ No coverage uploaded for pull request base (qgb-integration@2f3686c). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##             qgb-integration     #257   +/-   ##
==================================================
  Coverage                   ?   13.62%           
==================================================
  Files                      ?       47           
  Lines                      ?     9571           
  Branches                   ?        0           
==================================================
  Hits                       ?     1304           
  Misses                     ?     8177           
  Partials                   ?       90

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f3686c...8a2f8d1. Read the comment docs.

evan-forbes · 2022-03-30T04:25:33Z

I planned on having an additional command that queries for the necessary data and deploys the smart contracts, but ran into a really weird vs code bug when importing the go bindings that crippled my ide/go modules. ~~We will likely want something similar to that command before merging to better test both the orchestrator and relayer.~~

rach-id

Great job 👍 Left some basic comments

proto/qgb/msgs.proto

proto/qgb/query.proto

x/qgb/keeper/query_valset_confirm.go

x/qgb/orchestrator/orchestrator.go

x/qgb/types/datacommitmentconfirm.go

proto/qgb/genesis.proto

proto/qgb/types.proto

adlerjohn · 2022-03-30T15:20:06Z

x/qgb/orchestrator/client.go

+	if vParam == byte(0) {
+		vParam = byte(27)
+	} else if vParam == byte(1) {
+		vParam = byte(28)
+	}


Is it guaranteed vParam is either zero or one? If not, can we check here?

I just copied this over from here https://github.com/celestiaorg/quantum-gravity-bridge/blob/6b6efce45c4ba67cf277d5e34c11c33e15084b25/orchestrator/ethereum/peggy/peggy_contract.go#L90-L104

I don't think it is guaranteed, but then we just want to use whatever is without changing it afaict. Again, I don't actually know why we have to replace these bytes. I tried to find some docs on the encoding of ethereum signatures, but didn't find any within a short period of time. I can look into this before merging if we want, or we can write an issue to investigate further.

https://ethereum.stackexchange.com/a/113505

so is this behavior correct as is? do we need to investigate this further?

If block.number >= FORK_BLKNUM and CHAIN_ID is available, then when computing the hash of a transaction for the purposes of signing, instead of hashing only six rlp encoded elements (nonce, gasprice, startgas, to, value, data), you SHOULD hash nine rlp encoded elements (nonce, gasprice, startgas, to, value, data, chainid, 0, 0). If you do, then the v of the signature MUST be set to {0,1} + CHAIN_ID * 2 + 35 where {0,1} is the parity of the y value of the curve point for which r is the x-value in the secp256k1 signing process. If you choose to only hash 6 values, then v continues to be set to {0,1} + 27 as previously. If block.number >= FORK_BLKNUM and v = CHAIN_ID * 2 + 35 or v = CHAIN_ID * 2 + 36, then when computing the hash of a transaction for purposes of recovering, instead of hashing six rlp encoded elements (nonce, gasprice, startgas, to, value, data), hash nine rlp encoded elements (nonce, gasprice, startgas, to, value, data, chainid, 0, 0). The currently existing signature scheme using v = 27 and v = 28 remains valid and continues to operate under the same rules as it did previously.

From EIP-155.

IMO, we add an else branch where the bridge panics if it hits it (so we know if we're having other values than 0 and 1. Some threads say that 2 and 3 are also possible. More on this below).

Also, we need to check which fields we're signing, so that in case we're signing 9 fields, then we replace 27 and 28 with 37 and 38.

On v value: it is used for recovery, and ECDSA allows having 4 points referring to the same signature here. Thus, it's possible for v to be in {0, 1, 2, 3}. However, AFAIK, to solve ECDSA signatures malleability, Ethereum removed half the field. Thus, only two values for v should be valid.

I'm surprised there's no module in ethermint/some EVM compatibility layer that the Cosmos ecosystem provides to handle this properly.

Note that 2 and 3 are rare:

27 = lower X even Y. 28 = lower X odd Y. 29 = higher X even Y. 30 = higher X odd Y. Note that 29 and 30 are exceedingly rarely, and will in practice only ever be seen in specifically generated examples. There are only two possible X values if r is between 1 and (p mod n), which has a chance of about 0.000000000000000000000000000000000000373 % to happen randomly – Pieter Wuille

They're rare in Bitcoin. However, in Ethereum, they're considered invalid IIUR.

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures.

and,

Allowing transactions with any s value with 0 < s < secp256k1n, as is currently the case, opens a transaction malleability concern, as one can take any transaction, flip the s value from s to secp256k1n - s, flip the v value (27 -> 28, 28 -> 27), and the resulting signature would still be valid. This is not a serious security flaw, especially since Ethereum uses addresses and not transaction hashes as the input to an ether value transfer or other transaction, but it nevertheless creates a UI inconvenience as an attacker can cause the transaction that gets confirmed in a block to have a different hash from the transaction that any user sends, interfering with user interfaces that use transaction hashes as tracking IDs. Preventing high s values removes this problem.

from here.

Thus, we shouldn't have 2 or 3 values... But, I guess adding an else branch is safer and will let us know in case we encounter something similar.

Doesn't hurt to catch that case, yeah.

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

…rg/lazyledger-app into evan/orchestrator-orchestrator

* adds overview and valset init ADRs * cleanup * adds Data commitment ADR init * improves the valset adr * cosmetics * adds more details and removes unnecessary stuff * cosmetics * proto format + ADR updates * updates proto files + ADR * adds endblocker doc to adr * typo

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

evan-forbes · 2022-03-30T23:23:24Z

created an issue to refactor to make unit testing easier #264

This PR still needs these unit tests, we could merge and add the tests later, or we can wait. It doesn't matter to me.

…rg/lazyledger-app into evan/orchestrator-orchestrator

evan-forbes · 2022-03-30T23:25:45Z

the rest of the linting errors are caused by code that will be used in the relayer PR

rach-id · 2022-03-31T10:03:12Z

x/qgb/orchestrator/client.go

+	if vParam == byte(0) {
+		vParam = byte(27)
+	} else if vParam == byte(1) {
+		vParam = byte(28)
+	}


If block.number >= FORK_BLKNUM and CHAIN_ID is available, then when computing the hash of a transaction for the purposes of signing, instead of hashing only six rlp encoded elements (nonce, gasprice, startgas, to, value, data), you SHOULD hash nine rlp encoded elements (nonce, gasprice, startgas, to, value, data, chainid, 0, 0). If you do, then the v of the signature MUST be set to {0,1} + CHAIN_ID * 2 + 35 where {0,1} is the parity of the y value of the curve point for which r is the x-value in the secp256k1 signing process. If you choose to only hash 6 values, then v continues to be set to {0,1} + 27 as previously. If block.number >= FORK_BLKNUM and v = CHAIN_ID * 2 + 35 or v = CHAIN_ID * 2 + 36, then when computing the hash of a transaction for purposes of recovering, instead of hashing six rlp encoded elements (nonce, gasprice, startgas, to, value, data), hash nine rlp encoded elements (nonce, gasprice, startgas, to, value, data, chainid, 0, 0). The currently existing signature scheme using v = 27 and v = 28 remains valid and continues to operate under the same rules as it did previously.

From EIP-155.

IMO, we add an else branch where the bridge panics if it hits it (so we know if we're having other values than 0 and 1. Some threads say that 2 and 3 are also possible. More on this below).

Also, we need to check which fields we're signing, so that in case we're signing 9 fields, then we replace 27 and 28 with 37 and 38.

On v value: it is used for recovery, and ECDSA allows having 4 points referring to the same signature here. Thus, it's possible for v to be in {0, 1, 2, 3}. However, AFAIK, to solve ECDSA signatures malleability, Ethereum removed half the field. Thus, only two values for v should be valid.

x/qgb/types/abi_consts.go

x/qgb/orchestrator/orchestrator.go

evan-forbes · 2022-04-10T15:45:12Z

closing in favor of #276

evan-forbes added 7 commits March 28, 2022 14:01

initial orchestrator client

260c66b

format proto and add necessary queries

0c01c30

add orchestrator logic

7bdc719

and encoding of data commitments and validator set changes

feafc20

add orchestrator command

8f6b680

Merge branch 'qgb-integration' into evan/orchestrator-orchestrator

6e35820

add the orchestrator command

6da95d2

evan-forbes requested a review from rach-id March 30, 2022 04:19

evan-forbes self-assigned this Mar 30, 2022

evan-forbes requested review from adlerjohn and liamsi March 30, 2022 04:20

evan-forbes mentioned this pull request Mar 30, 2022

orchestrator mvp #234

Closed

proto formatting

53a1eba

rach-id reviewed Mar 30, 2022

View reviewed changes

adlerjohn reviewed Mar 30, 2022

View reviewed changes

evan-forbes and others added 7 commits March 30, 2022 13:22

use correct name

c2b653a

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

use correct path to endpoint

f4acc94

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

get rid of GravityNonces struct

0120edf

use correct name

f95e18b

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

Merge branch 'evan/orchestrator-orchestrator' of github.com:celestiao…

71ddf95

…rg/lazyledger-app into evan/orchestrator-orchestrator

regenerate proto

715723d

rach-id mentioned this pull request Mar 30, 2022

Move the QGB module constants to a specific consts package #262

Closed

evan-forbes and others added 2 commits March 30, 2022 17:02

go mod tidy

31793a0

use correct name

fc03805

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

evan-forbes added 2 commits March 30, 2022 18:24

handle error

654eb51

Merge branch 'evan/orchestrator-orchestrator' of github.com:celestiao…

8a2f8d1

…rg/lazyledger-app into evan/orchestrator-orchestrator

evan-forbes mentioned this pull request Mar 31, 2022

MVP Relayer #265

Closed

move constant

adf2ea7

rach-id reviewed Mar 31, 2022

View reviewed changes

x/qgb/orchestrator/orchestrator.go Show resolved Hide resolved

x/qgb/orchestrator/orchestrator.go Show resolved Hide resolved

evan-forbes closed this Apr 10, 2022

rach-id deleted the evan/orchestrator-orchestrator branch April 14, 2022 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVP Orchestrator #257

MVP Orchestrator #257

evan-forbes commented Mar 30, 2022

codecov-commenter commented Mar 30, 2022 •

edited

Loading

evan-forbes commented Mar 30, 2022 •

edited

Loading

rach-id left a comment

adlerjohn Mar 30, 2022

evan-forbes Mar 30, 2022

adlerjohn Mar 30, 2022

evan-forbes Mar 30, 2022

rach-id Mar 31, 2022

adlerjohn Apr 1, 2022

adlerjohn Apr 1, 2022

rach-id Apr 1, 2022

adlerjohn Apr 1, 2022

evan-forbes commented Mar 30, 2022 •

edited

Loading

evan-forbes commented Mar 30, 2022

rach-id Mar 31, 2022

evan-forbes commented Apr 10, 2022

MVP Orchestrator #257

MVP Orchestrator #257

Conversation

evan-forbes commented Mar 30, 2022

Description

codecov-commenter commented Mar 30, 2022 • edited Loading

Codecov Report

evan-forbes commented Mar 30, 2022 • edited Loading

rach-id 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

evan-forbes commented Mar 30, 2022 • edited Loading

evan-forbes commented Mar 30, 2022

Choose a reason for hiding this comment

evan-forbes commented Apr 10, 2022

codecov-commenter commented Mar 30, 2022 •

edited

Loading

evan-forbes commented Mar 30, 2022 •

edited

Loading

evan-forbes commented Mar 30, 2022 •

edited

Loading