snowfork/snowbridge

A trustless bridge between Polkadot and Ethereum

snowbridge's Issues

Origins for PolkaETH dispatchables need to be verified

• https://github.com/Snowfork/polkadot-ethereum/pull/15/files#r454417710

Relayer should be refactored

The code has become messy and unorganized, especially after I landed the M4 proof-of-concept work.

Current issues:

• Need to make the ethereum and substrate parts match each other in terms of general design
• Application layer concerns have leaked into the bridge level, especially in the substrate portion.
• The Router concept/type used by our chain implementations doesn't really fit IMO, and makes message sending seem more complex than it needs to be.
• Style issues (golint, gofmt, etc)

Automate end-to-end testing

Currently setting up an end-to-end test is tedious and very error-prone.

Relayer: Block polling interval is too long

This should be too long for most substrate chains (~6 seconds block time). Can we maybe add this to the config?

Originally posted by @philipstanislaus in #79 (comment)

Handle Infura connection drops

Infura is known to drop idle websocket connections, and our relayer does not handle these robustly. This means that after a period of idleness, the header & event subscriptions will be terminated silently.

Naming of traits could be more descriptive

For example, rename Broker to BridgeIn. Especially since the trait only covers a subset of the Broker module's behavior.

See review comments:

• #5 (comment)
• #5 (comment)

Add more descriptive alias for H160 in emitted Asset events

See #79 (comment)

Deploy system to infrastructure

Need to deploy all our infrastructure to some kind of infrastructure. AWS for example.

Cumulus Integration

Need to convert our standalone chain into a Rococo-compatible parachain using the Cumulus toolkit.

Our rococo contains the latest code for this endeavour. Due to ongoing development in upstream dependencies, this work will stay in a branch until things have stabilized.

• Update chain for compatibility with Substrate v2.0.0-rc5 / rc6: #124
• Switch Substrate version to rococo-branch
• Integrate cumulus toolkit
• Test integration following the setup in the Cumulus workshop

Relayer should preserve event ordering

See #69 (comment)

Target applications should expect fixed-length payload

Need to address this review comment: #100 (comment)

Could you add a comment why we allow longer payloads? Thanks!

Handling multiple events in a single transaction

Currently, events will be protected from replay protection by checking against the transaction id + block that they originate. This means that two events coming from the same transaction id + block may be considered as the same event, even when there may be two different events in the same transaction.

We need to make sure we can handle this scenario. This should be handled when we migrate to block/transaction processing through light clients, deprecating current dummy verification

Cannot run relayer

Trying to run relayer but it fails:

$ build/artemis-relay run --config /tmp/relay-config.toml
Using config file: /tmp/relay-config.toml
ERRO[0000] Failed to initialize relayer                  error="invalid length, need 256 bits"
Error: invalid length, need 256 bits

Go version

$ go version
go version go1.15.2 linux/amd64

Config file:

 cat /tmp/relay-config.toml
[ethereum]
endpoint = "ws://localhost:9545/"
  [ethereum.bridge]
  address = "0x3629571d48274D3f97fAa41b6E0fa563a1C7618F"
  abi = "/tmp/Bridge-a1a9a522"
[ethereum.apps.eth]
address = "0x6ec61C7B86D1fa37616C5A7a5e0452Fb2Ab0c3E8"
abi = "/tmp/ETHApp-b2823f36"
[ethereum.apps.erc20]
address = "0x103098d4Df85bf4FEF3130fb361c571d57c2271c"
abi = "/tmp/ERC20App-8a2fe332"
[substrate]
endpoint = "ws://127.0.0.1:9944/"

ETHApp contract won't unlock entire amount

@Noc2 got the following error the first time he ran the test suite. the second time it ran fine though. sounds like something fishy worth digging into - @denalimarsh @vgeddes any thoughts? maybe its a timing related thing.

sudo yarn test

yarn run v1.22.5
$ mocha --timeout 35000 --exit


  Bridge
    ETH App
      ✓ should transfer ETH from Ethereum to Substrate (10085ms)
      1) should transfer ETH from Substrate to Ethereum
    ERC20 App
      ✓ should transfer ERC20 tokens from Ethereum to Substrate (10174ms)
      ✓ should transfer ERC20 from Substrate to Ethereum (30078ms)


  3 passing (1m)
  1 failing

  1) Bridge
       ETH App
         should transfer ETH from Substrate to Ethereum:

      AssertionError: expected '0' to equal '10000000000000000'
      + expected - actual

      -0
      +10000000000000000
      
      at Context.<anonymous> (test/test.js:70:71)
      at processTicksAndRejections (internal/process/task_queues.js:93:5)



error Command failed with exit code 1.

Publish netlify sites under a single hostname

Something like:

• Main docs: peb.snowfork.com/
• Substrate UI: peb.snowfork.com/apps
• Rust docs: peb.snowfork.com/rustdocs

Can use this approach, which relies on Netlify's proxy feature: https://community.netlify.com/t/support-guide-can-i-deploy-multiple-repositories-in-a-single-site/179

Update chainspec to include token metadata

  "properties": {
    "ss58Format": 42,
    "tokenDecimals": 12,
    "tokenSymbol": "ROC"
  },

Lightclient Verifier dispatchables need weights assigned

We should calculate the weights for dispatchables in the parachain's lightclient verifier pallet.

cannot start relayer

After I deployed ethereum smart contract using truffle and ran the parachain bridge, I got error when running relayer:

Using config file: /tmp/relay-config.toml

ERRO[0000] Failed to initialize relayer                  
error="json: cannot unmarshal object into Go value of type []struct { Type string; Name string; Inputs []abi.Argument; Outputs []abi.Argument; StateMutability string; Constant bool; Payable bool; Anonymous bool }"

Error: json: cannot unmarshal object into Go value of type []struct { Type string; Name string; Inputs []abi.Argument; Outputs []abi.Argument; StateMutability string; Constant bool; Payable bool; Anonymous bool }

Here is my relay-config.toml:

endpoint = "ws://localhost:9545/"
[ethereum.bridge]
address = "0x722bd05E96b1841cFdfb79E174867541c2AaDEa5"

abi = "/tmp/Bridge.json"
[ethereum.apps.eth]
address = "0xd4b4131bcF86B14B28c4bf2D40A17D88e5fbC2f3"

abi = "/tmp/ETHApp.json"
[ethereum.apps.erc20]
address = "0xb8fDBE6AC4264654B3290108237F7A3C94A70221"

abi = "/tmp/ERC20App.json"
[substrate]
endpoint = "ws://127.0.0.1:9944/"

Is any hint for where is wrong? What is the supported golang version for relayer? Thanks!

Remove unnecessary cmd directory

There doesn't seem to be a good reason for it. The contents of bridgerelayer/cmd/ could just be moved to bridgerelayer

Use more specific naming and types for Channel objects

Can we be more specific with argument names and channels here?

func NewChain(msgsObserved chan<- chain.Message, msgsToSend <-chan chain.Message) (*Chain, error) {

Originally posted by @philipstanislaus in #69 (comment)

Store secrets in AWS secrets

Our Ansible tooling needs to access various secrets in order to generate configuration for our services. We're currently using Ansible Vault to store these secrets in encrypted form in our repo.

This isn't ideal as our repo is public and everyone has access to the ciphertext. So we should try and use AWS Secrets instead.

Ansible Vault is probably still fine for secrets with minor impact such as login usernames, etc.

Integration tests for XCMP transfers

Need to do a live of transfer bridged ETH & ERC20 assets from one parachain to another.

Tasks:

• Deploy our updated parachain to a testnet
• Register it twice with different ParaIds.
• Test XCMP transfers for both ETH and ERC20 transfers.

Adapt ORML xtokens pallet for our own use

open-web3-stack/open-runtime-module-library#297

Consider using host function for keccak_512 in ethereum light client

We currently run our hashing via code in our parachain wasm binary. Ideally polkadot would add this as a host function we can use directly. If they do this, then we should port over to using it.

Comment on our PR: #147 (comment)
Comment on Substrate keccak: paritytech/substrate@510e68b

Decode generic length SCALE-encoded bytes

We've implemented compact uint decoding, allowing us to parse the length of arbitrary SCALE-encoded bytes and use it to properly decode the data. However, we're not currently using the compact uint decoding functionality. As we know the exact length of each variable in the encoded data package submitted to the EthereumApp and ERC20App we're able to decode it according to preset length expectations. While this works for a finite number of registered applications (M4 has just two applications) we'll want to put everything together and decode the SCALE-encoded data in a generic way.

Remove unnecessary indirect dependency

The bridge relayer has indirect dependencies on three different versions/forks of GSRPC.

See #40 (comment)

Add custom withdrawal reason when burning PolkaETH

See https://github.com/Snowfork/polkadot-ethereum/pull/15/files#r454417888

Relayer should be more robust

• Audit our error handling to ensure that errors don't cause unrecoverable failures, segfaults, exits, etc.
• Need some way to keep track of which blocks for which we've relayed or haven't relayed events.
• Improve logging

Error in XCMP specification document

https://github.com/Snowfork/polkadot-ethereum/blob/main/docs/specs/xcmp.md#alice-transfers-21-polkaeth-to-bob-on-our-chain-

The sovereign account of H on D will be credited with 21 PolkaETH. => The sovereign account of H on D will be reduced with 21 PolkaETH

Provision a test network

Need to setup a test network in the cloud with our Rococo-compatible parachain.

• Setup Ansible boilerplate
• Provision AWS resources
• Deploy relay chain
• Deploy parachain
• Deploy relayer daemon for cross-chain transfers
• DNS endpoints and SSL certificates

Use 256-bit BigNum for representing PolkaETH balances

See https://github.com/Snowfork/polkadot-ethereum/pull/15/files#r454413008

Configure block headers to use Keccak256 instead of Blake2b

The runtime-wide hash function is configurable in the frame_system pallet. So to create a standalone substrate chain, or a parachain it is possible to use Keccak256. This will simplify the verification process in Solidity as verifying a blake2b header against a hash is expensive.

See Snowfork/snowbridge-experiments#1 (comment).

Parachain: Verifier should try and keep storage size bounded

Just as a note for the future to keep the storage size bounded:

We could simply have a double map here, where the first key is the block height of the received message. We can the define a maxTailLength, e. g. 200 blocks, and just remove all storage keys where height < currentHeight - 200. At the same time, we decline verification for older blocks.

Originally posted by @philipstanislaus in #79 (comment)

Add custom origins for Bridge and Verifier modules

This is pretty important to ensure that target applications can trust the messages being passed onto them by the verifier.

See https://github.com/Snowfork/polkadot-ethereum/pull/5/files#r453729105

Fix decoding of pallet_scheduler events

Our type definitions in the file below don't match reality, which causes decoding of pallet_scheduler events to fail. This would prevent events from being read for the entire block if the block contains a single pallet_scheduler event:
https://github.com/Snowfork/polkadot-ethereum/blob/v0.2.1-alpha/bridgerelayer/chains/substrate/events.go#L7
https://github.com/Snowfork/polkadot-ethereum/blob/v0.2.1-alpha/bridgerelayer/chains/substrate/streamer.go#L122

The actual fix should go into GSRPC since pallet_scheduler is one of the standard FRAME pallets. So we should move the scheduler event definitions from bridgerelayer/chains/substrate/events.go into GSRPC.

Restrict message size on our parachain

To reduce attack vectors, our parachain should restrict the maximum message size. Not sure what a reasonable limit might be, but I propose that few chains would need more than 2,048 bits (8 * 256 bit words).

Update dependencies for Rococo + XCMP

Figure which upstream branches we're supposed to be using:

Polkadot branch: master (Acala still on gav-xcmp)
Substrate dependency: Not sure (either v2.0.0-rc5 or rococo-branch)
Cumulus branch: Not sure but probably gav-xcmp according to AcalaNetwork/Acala#426

Make use of App IDs to identify applications

Our early design envisioned identifying applications on both chains using opaque 256-bit identifiers. During development however we never diverted from that approach, and used more ad-hoc means of identification in order to get a basic proof of work complete.

We'll need to figure out a final solution for M5.

Update ethashproof merkle roots for Ethereum Lightclient

Our ethash verification relies on having the merkle root of the DAG for the given epoch. We store these roots for the first 512 epochs here: https://github.com/Snowfork/polkadot-ethereum/blob/main/parachain/primitives/ethereum/src/ethashdata.rs. 512 epochs will run out after ~600 days from the date of this issue (very rough estimate based on current block production rate). We should update merkle roots well before that.

The merkle roots can be calculated using https://github.com/talbaneth/ethashproof

Add XCM handler pallet to runtime

https://github.com/paritytech/polkadot/tree/gav-xcmp/xcm/xcm-handler

The pallet uses XcmExecutor and TransactAsset to execute messages.

Address review feedback for relayer & solidity

Need to address review feedback from #58 and #59.

Calculate weights for pallet dispatchables

We'll need to perform benchmarking in order to calculate the appropriate weights.

Encode recipient's Polkadot address in Bank contract

The recipient's Polkadot address needs to have a fixed length. As different valid addresses can be hex-encoded to different lengths, this is a problem. One potential solution is encoding the Polkadot address to base58 on contract or not packing the data before event emission e.g. using abi.encode instead of abi.encodePacked.

Relayer should submit a mortal extrinsic

From Philip:

Immortal extrinsics are an issue indeed: Since nonces are tracked per account, and accounts can be reaped (nonce will be reset), any immortal transaction can be replayed.

Hence, anyone can mess up our bridge by replaying immortal extrinsics after a reap happens (even if accidentally).

Update verification to prevent malleable signatures

See https://www.di.ens.fr/~stern/data/St101.pdf.

To resolve, the Verifier contract should be updated with:

        if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
            revert("ECDSA: invalid signature 's' value");
        }

The Ethereum relayer's router must be tested to ensure compatibility with the updated Verifier.

Dispatching of messages to target applications has high runtime overhead

The current approach is nicely decoupled but will result in high and varying runtime overhead.

See https://github.com/Snowfork/polkadot-ethereum/pull/5/files#r453730843

Add token dealer pallet for XCMP

We need to add a pallet that makes it easy to transfer assets using XCMP. The pallet should have dispatchables that allow users to initiate the following asset transfers:

• Transfer DOT to upwards to the relay chain
• Transfer bridged Ethereum assets to sibling parachains.

Lock in version of Rust ethash

artemis-ethereum and pallet-verifier-lightclient rely on the Rust ethash implementation at https://github.com/rust-ethereum/ethash. The latest published version is 0.4.0 which uses ethereum-types ^0.8 and primitive-types ^0.6. However, we use 0.9 and 0.7 respectively. To avoid messy type conversions, we either need to get a new version of ethash published, or maintain our own fork like Near did (https://github.com/nearprotocol/rust-ethash?branch=upgrade-eth-types). We're currently using the Near fork

Adapt assets pallet to work with XCMP

Implement TransactAsset trait to enable withdraws and deposits from our assets pallet via XCMP

https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/traits.rs#L58

SCALE decoding gas optimization: fill array from back

To save gas, we could just iterate backwards over the array instead of reversing it, right?

Originally posted by @philipstanislaus in #51 (comment)