EthBridge storage should only be modifiable by protocol transactions #1094

james-chf · 2022-05-11T08:17:31Z

Relates to anoma/namada#114 - Depends on #1066 - this branch is based off of james/1059/eth-bridge-vp and this PR is targeting that branch for the time being to get a better diff.

add a new type of protocol transaction - EthereumBridgeUpdate(Tx) - and allow the wrapped Tx to be executed
add tx_enqueue_eth_transfer.wasm which modifies the /queue key under the Ethereum bridge account's storage - currently it just writes the data of the transaction directly to it, but in a future update it will manipulate the queue structure there properly
make the Ethereum bridge validity predicate reject any transaction which does not have its data signed with an active validator's protocol key
add an end-to-end test exercising a successful protocol transaction
some end-to-end test helpers (may be moved into a separate PR)

Still to come after this PR

protocol transactions themselves will be verified against active validator's protocol keys rather than a bundled public key as is currently the case
transactions that modify Ethereum bridge account storage (and perhaps all protocol transactions in general) may be restricted to have to be signed by the block proposer rather than any active validator

Testing

To run end-to-end tests, assuming you already have wasm/s built except for the new one(s) added in this PR, run:

export RUST_BACKTRACE=short
export ANOMA_E2E_KEEP_TEMP=true
export ANOMA_E2E_DEBUG=true
make -C wasm/wasm_source tx_enqueue_eth_transfer
make checksum-wasm

using ABCI

cargo test \
  eth_bridge -- \
    --test-threads=1 \
    --nocapture

using ABCI++ (currently the Tendermint client fails when trying to submit a transaction with an error like "Internal error: timed out waiting for tx to be included in a block (code: -32603)")

# ensure tendermint++ is built from this commit - https://github.com/tendermint/tendermint/commit/f40884911
export TENDERMINT=${HOME}/bin/tendermint++

cargo test \
 --no-default-features \
 --features "ABCI-plus-plus" \
 eth_bridge -- \
     --test-threads=1 \
     --nocapture

james-chf · 2022-05-11T12:00:25Z

pls update wasm

james-chf · 2022-05-12T12:14:19Z

pls update wasm

james-chf · 2022-05-13T09:14:44Z

pls update wasm

james-chf · 2022-05-18T09:00:23Z

pls update wasm

james-chf

I have a helper utility for submitting protocol transactions I can publish in another repo

james-chf · 2022-05-18T20:57:23Z

apps/src/lib/node/ledger/protocol/mod.rs

@@ -61,7 +62,8 @@ pub type Result<T> = std::result::Result<T, Error>;

 /// Apply a given transaction
 ///
-/// The only Tx Types that should be input here are `Decrypted` and `Wrapper`
+/// The only Tx Types that should be input here are `Decrypted`, `Wrapper` and


I'm not sure about this docstring as we have a catchall case (_ =>) which would do things for other TxTypes. I'd say we should remove this caveat from the docstring and replace the _ => with an explicit match arm for each TxType, then we should also get a compile error to make us update this fn if we introduced a new TxType.

james-chf · 2022-05-18T20:59:00Z

shared/src/types/transaction/util.rs

+
+/// Represents a write to a storage key
+#[derive(BorshDeserialize, BorshSerialize, Debug)]
+pub struct WriteOp {


This isn't a great location, not sure where to put it. It's a struct used in the tx_write_storage_key test wasm, and also in the e2e tests which constructs the serialized tx.data for a tx_write_storage_key transaction.

james-chf · 2022-05-18T20:59:33Z

tests/src/e2e/helpers/wasms.rs

+    use super::*;
+
+    #[test]
+    fn test_get_abs_path() {


This test runs with cargo test get_abs_path but isn't currently running in CI

james-chf · 2022-05-18T21:06:52Z

shared/src/ledger/eth_bridge/mod.rs

            "Validity predicate triggered",
        );
+        let signed: Signed<Vec<u8>> =
+            match Signed::<Vec<u8>>::try_from_slice(tx_data) {


If submitting a protocol transaction, tx_data ends up being the tx.data field of the innermost Tx (i.e. the one wrapped by EthereumBridgeUpdate(Tx)), which should be a Signed { data, sig } if the protocol transaction is constructed correctly.

In the case of submitting a regular transaction, it seems to end up being a SignedTxData, which I think must be the tx.data field of the outermost Tx that actually gets submitted to Tendermint RPC.

I haven't looked closely yet at why there is this difference, it could be just to do with the way this branch is coded currently, but the VP will reject as it can't deserialize that to a Signed, if it gets that far.

james-chf · 2022-05-18T21:11:48Z

wasm/wasm_source/src/tx_enqueue_eth_transfer.rs

+fn log(msg: &str) {
+    log_string(format!("[{}] {}", TX_NAME, msg))
+}
+
+fn fatal(msg: &str, err: impl std::error::Error) -> ! {
+    log(&format!("ERROR: {} - {:?}", msg, err));
+    panic!()
+}
+
+fn fatal_msg(msg: &str) -> ! {
+    log(msg);
+    panic!()
+}


I had to put a fair bit of logging while developing and unsure how much to remove, I guess it would add to gas cost

james-chf · 2022-05-19T12:16:49Z

pls update wasm

They're executed in a similar way to regular decrypted transactions

For now, this wasm attempts to write the transaction data to #EthBridge/queue

In the 'anoma_apps' crate, we need to make `tendermint_websocket_client` public and also re-export our tendermint crates

This is for reading "real" wasms from `wasm/` which may have their hash in their filename

Needed for working with protocol transactions (rebuilt test wasms not committed)

Needed for running our async fns in e2e tests (rebuilt test wasms not committed)

…m data Also some logging changes

Also update the existing e2e test to assert that the #EthBridge/queue storage key was not updated

james-chf · 2022-05-23T09:56:33Z

pls update wasm

[ci]: update wasm checksums

james-chf · 2022-06-09T13:52:14Z

I will pick what's useful from this branch to other branch(es) since transactions to modify the Ethereum bridge may be applied internally rather than via Tendermint

james-chf force-pushed the james/1059/eth-bridge-vp branch from 9bbd931 to ea88075 Compare May 11, 2022 09:35

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch from ac8821a to 978ee02 Compare May 11, 2022 09:35

james-chf changed the title ~~Eth bridge VP storage should be modifiable only by protocol transactions~~ EthBridge VP storage should be modifiable only by protocol transactions May 11, 2022

james-chf force-pushed the james/1059/eth-bridge-vp branch from ea88075 to c417cf9 Compare May 12, 2022 16:09

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch from 6e71c65 to f7ffd43 Compare May 12, 2022 16:11

james-chf force-pushed the james/1059/eth-bridge-vp branch from c417cf9 to a48bd79 Compare May 13, 2022 08:03

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch from f7ffd43 to 5419ba1 Compare May 13, 2022 08:10

james-chf force-pushed the james/1059/eth-bridge-vp branch from 54ecc2d to 134b8b5 Compare May 13, 2022 12:40

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch 2 times, most recently from e967eff to 93d1b34 Compare May 16, 2022 08:58

james-chf mentioned this pull request Jul 7, 2022

Add end-to-end test asserting protocol transactions are rejected when submitted via Tendermint anoma/namada#113

Closed

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch 2 times, most recently from 8d09daa to d718136 Compare May 17, 2022 13:26

james-chf force-pushed the james/1059/eth-bridge-vp branch from 134b8b5 to d46240e Compare May 17, 2022 13:27

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch 4 times, most recently from 75c91bf to e8ae85d Compare May 18, 2022 08:44

james-chf changed the title ~~EthBridge VP storage should be modifiable only by protocol transactions~~ EthBridge VP should only accept protocol transactions May 18, 2022

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch 2 times, most recently from de28c1b to d19f526 Compare May 18, 2022 20:41

james-chf commented May 18, 2022

View reviewed changes

james-chf added 3 commits May 19, 2022 14:14

Add ProtocolTxType::EthereumBridgeUpdate

53f7cff

node/ledger: permit EthereumBridgeUpdate transactions to be executed

b806076

They're executed in a similar way to regular decrypted transactions

wasm: add tx_enqueue_eth_transfer

8f52bc0

For now, this wasm attempts to write the transaction data to #EthBridge/queue

james-chf added 11 commits May 19, 2022 14:16

EthBridge VP: verify signature against active validators

8bf289a

e2e/helpers: expose TendermintWebsocketClient from anoma_apps

0321483

In the 'anoma_apps' crate, we need to make `tendermint_websocket_client` public and also re-export our tendermint crates

e2e/helpers: add a helper for reading wasms

67f8bc3

This is for reading "real" wasms from `wasm/` which may have their hash in their filename

tests/Cargo.toml: enable anoma/ferveo-tpke feature by default

b7aa667

Needed for working with protocol transactions (rebuilt test wasms not committed)

tests/Cargo.toml: add tokio dependency

742d131

Needed for running our async fns in e2e tests (rebuilt test wasms not committed)

test/Cargo.toml: add rand

398022c

tests/Cargo.toml: add toml

e39f785

wasm_for_tests: update tx_write_storage_key to be able to write custo…

e278cd0

…m data Also some logging changes

e2e/setup.rs: add get_validator_protocol_key helper

20845f2

e2e/helpers: add random_secret_key helper

e8b9395

eth_bridge_tests.rs: add an e2e test for a valid protocol transaction

0385d23

Also update the existing e2e test to assert that the #EthBridge/queue storage key was not updated

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch from 0e7d200 to e52a85d Compare May 19, 2022 13:35

[ci]: update wasm checksums

4ed21e4

[ci]: update wasm checksums

james-chf force-pushed the james/1059/eth-bridge-allow-protocol-txs branch from bbed273 to 4ed21e4 Compare May 23, 2022 11:54

james-chf added the ethereum-bridge label Jun 1, 2022

james-chf changed the title ~~EthBridge VP should only accept protocol transactions~~ EthBridge storage should only be modifiable by protocol transactions Jun 1, 2022

james-chf mentioned this pull request Jun 1, 2022

Implement the Ethereum bridge validity predicate anoma/namada#114

Closed

5 tasks

james-chf self-assigned this Jun 8, 2022

james-chf closed this Jun 9, 2022

james-chf deleted the james/1059/eth-bridge-allow-protocol-txs branch June 14, 2022 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EthBridge storage should only be modifiable by protocol transactions #1094

EthBridge storage should only be modifiable by protocol transactions #1094

james-chf commented May 11, 2022 •

edited

Loading

james-chf commented May 11, 2022

james-chf commented May 12, 2022

james-chf commented May 13, 2022

james-chf commented May 18, 2022

james-chf left a comment

james-chf May 18, 2022

james-chf May 18, 2022

james-chf May 18, 2022

james-chf May 18, 2022

james-chf May 18, 2022

james-chf commented May 19, 2022

james-chf commented May 23, 2022

james-chf commented Jun 9, 2022 •

edited

Loading

EthBridge storage should only be modifiable by protocol transactions #1094

EthBridge storage should only be modifiable by protocol transactions #1094

Conversation

james-chf commented May 11, 2022 • edited Loading

Testing

james-chf commented May 11, 2022

james-chf commented May 12, 2022

james-chf commented May 13, 2022

james-chf commented May 18, 2022

james-chf left a comment

Choose a reason for hiding this comment

james-chf May 18, 2022

Choose a reason for hiding this comment

james-chf May 18, 2022

Choose a reason for hiding this comment

james-chf May 18, 2022

Choose a reason for hiding this comment

james-chf May 18, 2022

Choose a reason for hiding this comment

james-chf May 18, 2022

Choose a reason for hiding this comment

james-chf commented May 19, 2022

james-chf commented May 23, 2022

james-chf commented Jun 9, 2022 • edited Loading

james-chf commented May 11, 2022 •

edited

Loading

james-chf commented Jun 9, 2022 •

edited

Loading