Solidity contract that accepts unverified substrate headers #65

svyatonik · 2020-04-13T15:09:46Z

part of #50

This PR introduces:

ethereum-contract-builtin crate, which will be used by bridge contract to import headers and verify finality proofs. Right now it has fn parse_substrate_header(), which is (almost) completed and fn verify_substrate_finality_proof() which is unimplemented. This combination allows contract to import Substrate headers, but not to import finality proofs;
solidity contract that is able to import unverified Substrate headers and (in theory) import and verify finality proofs;
sub-to-eth subcommand for the relay to be able to test that headers sync. The pipeline is almost the same as in eth-to-sub, but we have no extra data to be submitted along with header (like transactions receipts in eth-to-sub). Also just realized that there are some parts of future AsyncExtra data sync (which is finality proof in our case) - can remove on demand, just wanted to make separate PRs for headers import and finality proof import;
eth-deploy-contract subcommand for the relay which deploys bridge contract on eth-like chain. We may throw it later, or make a separate binary, but right now I find it much more convenient to have it as a subcommand in relay.

Some details:

in the first version of contract, it has been able to import only finalized headers - i.e. it had function importHeaders(bytes[] calldata rawHeaders, bytes calldata rawFinalityProof) instead of two functions (function importHeader() and function importFinalityProof()) now. But it might be impossible to fit this data in single transaction in some cases (like when GRANDA validators were unable to finalize headers for some long time);
there is no pruning in bridge contract now, mostly for simplicity - we may add support later. But we need to take Prune old headers with separate transaction #62 into account;
relay code is mostly the same, though there are some new RPC calls for both Substrate and Ethereum nodes. So most new lines are utility lines describing sub-to-eth sync, code reorgs and the contract itself. I was thinking of introducing tests for the contract, but then realized that I won't be able to do that without mocking builtins. And I do not know any way to mock builtins. So the contract comes without any unit tests - please advice if you know any way to solve it.

tomusdrw

Looks good overall, I'd like to see contract metadata file fixed in this PR and the small grumbles, but major things (like RPC caling refactor, or constant extraction) can be done in separate PRs. I will turn them into issues.

modules/ethereum-contract/builtin/src/lib.rs

tomusdrw · 2020-04-21T11:55:18Z

modules/ethereum-contract/contracts/substrate-bridge.sol

+	}
+
+	/// Initializes bridge contract.
+	/// @param rawInitialHeader Raw finalized header that will be ancestor of all importing headers.


Suggested change

/// @param rawInitialHeader Raw finalized header that will be ancestor of all importing headers.

/// @param rawInitialHeader Raw finalized header that will be ancestor of all imported headers.

modules/ethereum-contract/contracts/substrate-bridge.sol

tomusdrw · 2020-04-21T12:05:10Z

modules/ethereum-contract/contracts/substrate-bridge.sol

+	) public {
+		// check that header that we're going to finalize is already imported
+		require(
+			headerByHash[finalityTargetHash].number == finalityTargetNumber,


It think we should make sure that finalityTargetNumber is not 0 too:

Suggested change

headerByHash[finalityTargetHash].number == finalityTargetNumber,

headerByHash[finalityTargetHash].number == finalityTargetNumber && finalityTargetNumber != 0,

[SOLIDITY] I'm not against that (will consider that when preparing final version of the contract), but I see nothing criminal in re-finalizing finalized block (genesis in this case) - it just should be a noop. Do you have any other concerns to mention?

Since 0 is used for initialized storage as well, I'd prefer to keep it on the safe side here. i.e. with finalityTargetNumber ==0 this check would pass with arbitrarty finalityTargetHash (i.e. non-existent).
So nothing very specific for now, but just my internal warning side lightning up :)

tomusdrw · 2020-04-21T12:07:02Z

modules/ethereum-contract/contracts/substrate-bridge.sol

+		require(
+			!headerByHash[header.hash].isKnown,
+			"Header is already known"
+		);


Let's add an extra check that we are not importing a header with hash=0 or number=0. We can optimize this later if it turns out not needed.
Just to be on the safe side :)

[SOLIDITY] This is checked by the parentHeader.isKnown && parentHeader.number == header.number - 1, later. So unless we'll have 2^32 headers (which will break contract assumptions anyway), we won't allow to import such headers.

relays/ethereum/src/substrate_client.rs

relays/ethereum/src/substrate_sync_loop.rs

relays/ethereum/src/utils.rs

tomusdrw · 2020-04-28T14:29:36Z

relays/ethereum/res/substrate-bridge-metadata.txt

+Solc version: 0.6.6+commit.6c089d02
+Source hash (keccak256): 0xdc46aff04e37129265223e507d17f1407a70cb1ecea3230e1eaa77a17586724d
+Source gist: https://gist.github.com/svyatonik/876b388f9507a8de242cb2db9547c4f0
+Compiler flags used (command to produce the file): `docker run -i ethereum/solc:0.6.6 --optimize --bin - < substrate-bridge.sol`


🙇‍♂️

tomusdrw · 2020-04-28T14:34:18Z

modules/ethereum-contract/contracts/substrate-bridge.sol

+	) public {
+		// check that header that we're going to finalize is already imported
+		require(
+			headerByHash[finalityTargetHash].number == finalityTargetNumber,


Since 0 is used for initialized storage as well, I'd prefer to keep it on the safe side here. i.e. with finalityTargetNumber ==0 this check would pass with arbitrarty finalityTargetHash (i.e. non-existent).
So nothing very specific for now, but just my internal warning side lightning up :)

tomusdrw

Looks good for me. There are couple of rename suggestions by @HCastano - I don't really have a strong opinion, cause the plural sounds ok to me, but I'm not a native speaker.

HCastano · 2020-04-28T14:55:19Z

relays/ethereum/res/substrate-bridge-metadata.txt

@@ -0,0 +1,5 @@
+Last Change Date: 2020-04-28


Is this date necessary? Could we not get this from the commit info?

Commits sometimes get lost (when you move to a new repo) or we might move stuff around. I'd prefer to keep it explicitly.

relays/ethereum/src/ethereum_sync_loop.rs

relays/ethereum/src/substrate_sync_loop.rs

HCastano · 2020-04-28T15:10:35Z

Looks good for me. There are couple of rename suggestions by @HCastano - I don't really have a strong opinion, cause the plural sounds ok to me, but I'm not a native speaker.

If the structs were some sort of collection I'd be okay with the plurals. Since they're not the naming is a bit awkward. While I would prefer to see them changed, I also don't have a strong enough opinion that I'm going to block this over the naming, lol

svyatonik · 2020-04-28T15:25:57Z

relays/ethereum/Cargo.toml

+ethabi = "12.0"
+ethabi-contract = "11.0"
+ethabi-derive = "11.0"
+ethereum-tx-sign = { git = "https://github.com/svyatonik/ethereum-tx-sign.git", branch = "up-ethereum-types" }


The biggest change of latest merge-with-master - PR is already opened in original repo, but let's refer to my branch for now

Otherwise the first positional argument is just swallowed Fixes #62 Forked at: cd1eb37 Parent branch: origin/master

…ch#65) * solidity contract * continue * upd * cargo update * fixes * ehtereum_headers -> headers * extracted some common stuff * ethereum_sync.rs -> sync.rs * make sync generic * continue extracting * continue * add eth-contract argument * continue * some fixes * contract v2 * continue * more fixes * more fixes * deal with duplicated params * removed multiple call_rpc variants * bail_on_error!() * fn submit_ethereum_transaction * more fixes * cargo fmt --all * fix * bail_on_arg_error!() * fix * fix * remove async_extra stuff * substrate-bridge.json -> substrate-bridge-abi.json * get rid of substrate transactions hashes * get rid of ethereum transactions hashes * extracted contract bytecode to separate file * cargo fmt --all * avoid duplicate import in contracts * removed Default::default() * swapped configurations for sub2eth && eth2sub * fix compilation * do not double gas limit when submitting Substrate headers * cargo fmt --all * solidity contract removed * consts * extracted solc compilation details to separate file * removed (obsolete in future Vec<u8> justification) * fixed cli option description * fix typos * fix grumble * extracted constants * log decoded header * cargo fmt --all * comment

svyatonik added 30 commits April 3, 2020 16:59

solidity contract

dbd7f0e

continue

a9d58a0

upd

6995aac

cargo update

7a55f48

fixes

f7e9768

ehtereum_headers -> headers

3a13d1f

extracted some common stuff

39555d0

ethereum_sync.rs -> sync.rs

5831990

make sync generic

4dfffd8

continue extracting

33e9e31

Merge branch 'master' into solidity-contract

e132276

continue

4ec3f6f

add eth-contract argument

bf77451

continue

1312d6e

some fixes

cfc9235

contract v2

b73aa54

continue

f29b2e5

more fixes

d90dff3

Merge branch 'master' into solidity-contract

eb07871

more fixes

ac393da

deal with duplicated params

abebf1a

removed multiple call_rpc variants

8168278

bail_on_error!()

fd02e2f

fn submit_ethereum_transaction

e15bd1f

more fixes

4fd2cff

cargo fmt --all

ea6cc8c

fix

dbda454

bail_on_arg_error!()

74cd275

fix

84443a1

fix

8118db6

tomusdrw suggested changes Apr 24, 2020

View reviewed changes

This was referenced Apr 24, 2020

Gas Pricing helper tool #73

Closed

Extract low-level RPC calls to a separate module #74

Closed

svyatonik added 9 commits April 28, 2020 08:55

extracted solc compilation details to separate file

16a06f8

removed (obsolete in future Vec<u8> justification)

0911921

fixed cli option description

da6f9b1

fix typos

427ef9b

fix grumble

bbdad9e

extracted constants

d1f8bb0

Merge branch 'master' into solidity-contract

4615fc4

log decoded header

042d8fe

cargo fmt --all

5396377

tomusdrw reviewed Apr 28, 2020

View reviewed changes

tomusdrw approved these changes Apr 28, 2020

View reviewed changes

HCastano reviewed Apr 28, 2020

View reviewed changes

relays/ethereum/src/ethereum_sync_loop.rs Outdated Show resolved Hide resolved

HCastano reviewed Apr 28, 2020

View reviewed changes

relays/ethereum/src/substrate_sync_loop.rs Outdated Show resolved Hide resolved

HCastano reviewed Apr 28, 2020

View reviewed changes

relays/ethereum/src/substrate_sync_loop.rs Show resolved Hide resolved

comment

0f432c3

Merge branch 'master' into solidity-contract

a100ebd

svyatonik commented Apr 28, 2020

View reviewed changes

svyatonik merged commit 9993873 into master Apr 29, 2020

HCastano deleted the solidity-contract branch April 29, 2020 15:42

svyatonik mentioned this pull request May 19, 2020

Solidity contract that'll accept Substrate headers #50

Closed

This was referenced Jul 27, 2021

Use pallet-xdns to update the Gateway TTL on finality proof submission #1072

Closed

Use pallet-xdns to update the Gateway TTL on finality proof submission MaciejBaj/parity-bridges-common#6

Merged

svyatonik pushed a commit that referenced this pull request Jul 17, 2023

Fix: relay chain arguments need a first argument "command" (#65)

e13511b

Otherwise the first positional argument is just swallowed Fixes #62 Forked at: cd1eb37 Parent branch: origin/master

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solidity contract that accepts unverified substrate headers #65

Solidity contract that accepts unverified substrate headers #65

svyatonik commented Apr 13, 2020

tomusdrw left a comment

tomusdrw Apr 21, 2020

tomusdrw Apr 21, 2020

svyatonik Apr 28, 2020

tomusdrw Apr 28, 2020

tomusdrw Apr 21, 2020

svyatonik Apr 28, 2020

tomusdrw Apr 28, 2020

tomusdrw Apr 28, 2020

tomusdrw left a comment

HCastano Apr 28, 2020

tomusdrw Apr 28, 2020

HCastano commented Apr 28, 2020

svyatonik Apr 28, 2020

	/// @param rawInitialHeader Raw finalized header that will be ancestor of all importing headers.
	/// @param rawInitialHeader Raw finalized header that will be ancestor of all imported headers.

	headerByHash[finalityTargetHash].number == finalityTargetNumber,
	headerByHash[finalityTargetHash].number == finalityTargetNumber && finalityTargetNumber != 0,

Solidity contract that accepts unverified substrate headers #65

Solidity contract that accepts unverified substrate headers #65

Conversation

svyatonik commented Apr 13, 2020

tomusdrw 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

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano commented Apr 28, 2020

Choose a reason for hiding this comment