diff --git a/shared/src/ledger/eth_bridge/bridge_pool_vp.rs b/shared/src/ledger/eth_bridge/bridge_pool_vp.rs index 0624a83ae1..3588d76734 100644 --- a/shared/src/ledger/eth_bridge/bridge_pool_vp.rs +++ b/shared/src/ledger/eth_bridge/bridge_pool_vp.rs @@ -9,13 +9,13 @@ //! This VP checks that additions to the pool are handled //! correctly. This means that the appropriate data is //! added to the pool and gas fees are submitted appropriately. -use std::collections::{BTreeSet, HashSet}; +use std::collections::BTreeSet; use borsh::BorshDeserialize; use eyre::eyre; use crate::ledger::eth_bridge::storage::bridge_pool::{ - get_pending_key, is_protected_storage, BRIDGE_POOL_ADDRESS, + get_pending_key, is_bridge_pool_key, BRIDGE_POOL_ADDRESS, }; use crate::ledger::native_vp::{Ctx, NativeVp, StorageReader}; use crate::ledger::storage::traits::StorageHasher; @@ -115,43 +115,33 @@ where } }; - // check that only the pending_key value is changed - if keys_changed.iter().any(is_protected_storage) { - tracing::debug!( - "Rejecting transaction as it is attempting to change the \ - bridge pool storage other than the pending transaction pool" - ); - return Ok(false); + let pending_key = get_pending_key(&transfer); + for key in keys_changed.iter().filter(|k| is_bridge_pool_key(k)) { + if *key != pending_key { + tracing::debug!( + "Rejecting transaction as it is attempting to change an \ + incorrect key in the pending transaction pool: {}.\n \ + Expected key: {}", + key, + pending_key + ); + return Ok(false); + } } - // check that the pending transfer (and only that) was added to the pool - // TODO: This will change slightly when we merkelize the pool, - // but that will be a separate PR. - let pending_key = get_pending_key(); - let pending_pre: HashSet = - (&self.ctx).read_pre_value(&pending_key)?.ok_or(eyre!( - "The bridge pool transfers are missing from storage" - ))?; - let pending_post: HashSet = + let pending: PendingTransfer = (&self.ctx).read_post_value(&pending_key)?.ok_or(eyre!( - "The bridge pool transfers are missing from storage" - ))?; - if !pending_post.contains(&transfer) { - tracing::debug!( "Rejecting transaction as the transfer wasn't added to the \ pending transfers" + ))?; + if pending != transfer { + tracing::debug!( + "An incorrect transfer was added to the pool: {:?}.\n \ + Expected: {:?}", + transfer, + pending ); return Ok(false); } - for item in pending_pre.symmetric_difference(&pending_post) { - if item != &transfer { - tracing::debug!( - ?item, - "Rejecting transaction as an unrecognized item was added \ - to the pending transfers" - ); - return Ok(false); - } - } // check that gas fees were put into escrow @@ -232,8 +222,8 @@ mod test_bridge_pool_vp { } /// The bridge pool at the beginning of all tests - fn initial_pool() -> HashSet { - let transfer = PendingTransfer { + fn initial_pool() -> PendingTransfer { + PendingTransfer { transfer: TransferToEthereum { asset: EthAddress([0; 20]), recipient: EthAddress([0; 20]), @@ -244,9 +234,7 @@ mod test_bridge_pool_vp { amount: 0.into(), payer: bertha_address(), }, - }; - - HashSet::::from([transfer]) + } } /// Create a new storage @@ -256,9 +244,9 @@ mod test_bridge_pool_vp { writelog .write(&get_signed_root_key(), Hash([0; 32]).try_to_vec().unwrap()) .unwrap(); - + let transfer = initial_pool(); writelog - .write(&get_pending_key(), initial_pool().try_to_vec().unwrap()) + .write(&get_pending_key(&transfer), transfer.try_to_vec().unwrap()) .unwrap(); let escrow_key = balance_key(&xan(), &BRIDGE_POOL_ADDRESS); let amount: Amount = ESCROWED_AMOUNT.into(); @@ -291,19 +279,21 @@ mod test_bridge_pool_vp { ) } + enum Expect { + True, + False, + Error, + } + /// Helper function that tests various ways gas can be escrowed, /// either correctly or incorrectly, is handled appropriately fn assert_bridge_pool( payer_delta: SignedAmount, escrow_delta: SignedAmount, insert_transfer: F, - keys_changed: BTreeSet, - expect: bool, + expect: Expect, ) where - F: FnOnce( - PendingTransfer, - HashSet, - ) -> HashSet, + F: FnOnce(PendingTransfer, &mut WriteLog) -> BTreeSet, { // setup let mut write_log = new_writelog(); @@ -359,10 +349,7 @@ mod test_bridge_pool_vp { .expect("Test failed"); // add transfer to pool - let pool = insert_transfer(transfer.clone(), initial_pool()); - write_log - .write(&get_pending_key(), pool.try_to_vec().expect("Test failed")) - .expect("Test failed"); + let keys_changed = insert_transfer(transfer.clone(), &mut write_log); // create the data to be given to the vp let vp = BridgePoolVp { @@ -379,10 +366,12 @@ mod test_bridge_pool_vp { .expect("Test failed"); let verifiers = BTreeSet::default(); - let res = vp - .validate_tx(&signed, &keys_changed, &verifiers) - .expect("Test failed"); - assert_eq!(res, expect); + let res = vp.validate_tx(&signed, &keys_changed, &verifiers); + match expect { + Expect::True => assert!(res.expect("Test failed")), + Expect::False => assert!(!res.expect("Test failed")), + Expect::Error => assert!(res.is_err()), + } } /// Test adding a transfer to the pool and escrowing gas passes vp @@ -391,13 +380,15 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - true, + Expect::True, ); } @@ -408,13 +399,15 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Negative(10.into()), SignedAmount::Positive(GAS_FEE.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - false, + Expect::False, ); } @@ -425,13 +418,15 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Positive(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - false, + Expect::False, ); } @@ -442,13 +437,15 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(10.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - false, + Expect::False, ); } @@ -459,58 +456,83 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Negative(GAS_FEE.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - false, + Expect::False, ); } - /// Test that if a transaction is removed from - /// the pool, the tx is rejected. + /// Test that if the transfer was not added to the + /// pool, the vp rejects #[test] - fn test_remove_transfer_rejected() { + fn test_not_adding_transfer_rejected() { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |transfer, _pool| HashSet::from([transfer]), - BTreeSet::default(), - false, + |transfer, _| BTreeSet::from([get_pending_key(&transfer)]), + Expect::Error, ); } - /// Test that if the transfer was not added to the - /// pool, the vp rejects + /// Test that if the wrong transaction was added + /// to the pool, it is rejected. #[test] - fn test_not_adding_transfer_rejected() { + fn test_add_wrong_transfer() { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |_transfer, pool| pool, - BTreeSet::default(), - false, + |transfer, log| { + let t = PendingTransfer { + transfer: TransferToEthereum { + asset: EthAddress([0; 20]), + recipient: EthAddress([1; 20]), + amount: 100.into(), + nonce: 10u64.into(), + }, + gas_fee: GasFee { + amount: GAS_FEE.into(), + payer: bertha_address(), + }, + }; + log.write(&get_pending_key(&transfer), t.try_to_vec().unwrap()) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) + }, + Expect::False, ); } /// Test that if the wrong transaction was added /// to the pool, it is rejected. #[test] - fn test_add_wrong_transfer() { + fn test_add_wrong_key() { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |_transfer, pool| { - let mut pool = pool; - let wrong_transfer = - initial_pool().into_iter().next().expect("Test failed"); - pool.insert(wrong_transfer); - pool + |transfer, log| { + let t = PendingTransfer { + transfer: TransferToEthereum { + asset: EthAddress([0; 20]), + recipient: EthAddress([1; 20]), + amount: 100.into(), + nonce: 10u64.into(), + }, + gas_fee: GasFee { + amount: GAS_FEE.into(), + payer: bertha_address(), + }, + }; + log.write(&get_pending_key(&t), transfer.try_to_vec().unwrap()) + .unwrap(); + BTreeSet::from([get_pending_key(&transfer)]) }, - BTreeSet::default(), - false, + Expect::Error, ); } @@ -521,13 +543,18 @@ mod test_bridge_pool_vp { assert_bridge_pool( SignedAmount::Negative(GAS_FEE.into()), SignedAmount::Positive(GAS_FEE.into()), - |transfer, pool| { - let mut pool = pool; - pool.insert(transfer); - pool + |transfer, log| { + log.write( + &get_pending_key(&transfer), + transfer.try_to_vec().unwrap(), + ) + .unwrap(); + BTreeSet::from([ + get_pending_key(&transfer), + get_signed_root_key(), + ]) }, - BTreeSet::from([get_signed_root_key()]), - false, + Expect::False, ); } @@ -558,11 +585,11 @@ mod test_bridge_pool_vp { }, }; - // add transfer to pool - let mut pool = initial_pool(); - pool.insert(transfer.clone()); write_log - .write(&get_pending_key(), pool.try_to_vec().expect("Test failed")) + .write( + &get_pending_key(&transfer), + transfer.try_to_vec().expect("Test failed"), + ) .expect("Test failed"); // create the data to be given to the vp diff --git a/shared/src/ledger/eth_bridge/storage/bridge_pool.rs b/shared/src/ledger/eth_bridge/storage/bridge_pool.rs index 0e7d67eae6..865c4a42ec 100644 --- a/shared/src/ledger/eth_bridge/storage/bridge_pool.rs +++ b/shared/src/ledger/eth_bridge/storage/bridge_pool.rs @@ -11,13 +11,11 @@ use crate::types::eth_bridge_pool::PendingTransfer; use crate::types::hash::Hash; use crate::types::keccak::encode::Encode; use crate::types::keccak::{keccak_hash, KeccakHash}; -use crate::types::storage::{DbKeySeg, Key}; +use crate::types::storage::{DbKeySeg, Key, KeySeg}; /// The main address of the Ethereum bridge pool pub const BRIDGE_POOL_ADDRESS: Address = Address::Internal(InternalAddress::EthBridgePool); -/// Sub-segmnet for getting the contents of the pool -const PENDING_TRANSFERS_SEG: &str = "pending_transfers"; /// Sub-segment for getting the latest signed const SIGNED_ROOT_SEG: &str = "signed_root"; @@ -27,11 +25,11 @@ const SIGNED_ROOT_SEG: &str = "signed_root"; pub struct Error(#[from] eyre::Error); /// Get the storage key for the transfers in the pool -pub fn get_pending_key() -> Key { +pub fn get_pending_key(transfer: &PendingTransfer) -> Key { Key { segments: vec![ DbKeySeg::AddressSeg(BRIDGE_POOL_ADDRESS), - DbKeySeg::StringSeg(PENDING_TRANSFERS_SEG.into()), + transfer.keccak256().to_db_key(), ], } } @@ -52,13 +50,6 @@ pub fn is_bridge_pool_key(key: &Key) -> bool { matches!(&key.segments[0], DbKeySeg::AddressSeg(addr) if addr == &BRIDGE_POOL_ADDRESS) } -/// Check if a key belongs to the bridge pool but is not -/// the key for the pending transaction pool. Such keys -/// may not be modified via transactions. -pub fn is_protected_storage(key: &Key) -> bool { - is_bridge_pool_key(key) && *key != get_pending_key() -} - /// A simple Merkle tree for the Ethereum bridge pool /// /// Note that an empty tree has root [0u8; 20] by definition. @@ -128,7 +119,7 @@ impl BridgePoolTree { } } - /// Return the root as a [`struct@Hash`] type. + /// Return the root as a [struct@`Hash`] type. pub fn root(&self) -> Hash { self.root.clone().into() } diff --git a/shared/src/types/storage.rs b/shared/src/types/storage.rs index be15012cfa..eee9b5b847 100644 --- a/shared/src/types/storage.rs +++ b/shared/src/types/storage.rs @@ -20,6 +20,7 @@ use crate::ledger::storage::IBC_KEY_LIMIT; use crate::types::address::{self, Address}; use crate::types::eth_bridge_pool::PendingTransfer; use crate::types::hash::Hash; +use crate::types::keccak::{KeccakHash, TryFromError}; use crate::types::time::DateTimeUtc; #[allow(missing_docs)] @@ -640,6 +641,21 @@ impl KeySeg for Hash { } } +impl KeySeg for KeccakHash { + fn parse(seg: String) -> Result { + seg.try_into() + .map_err(|e: TryFromError| Error::ParseError(e.to_string())) + } + + fn raw(&self) -> String { + self.to_string() + } + + fn to_db_key(&self) -> DbKeySeg { + DbKeySeg::StringSeg(self.raw()) + } +} + /// Epoch identifier. Epochs are identified by consecutive numbers. #[derive( Clone, diff --git a/wasm/wasm_source/src/tx_bridge_pool.rs b/wasm/wasm_source/src/tx_bridge_pool.rs index bed8e3820e..0c945d6925 100644 --- a/wasm/wasm_source/src/tx_bridge_pool.rs +++ b/wasm/wasm_source/src/tx_bridge_pool.rs @@ -20,8 +20,6 @@ fn apply_tx(tx_data: Vec) { } = transfer.gas_fees; token::transfer(payer, &BRIDGE_POOL_ADDRESS, &address::xan(), amount); // add transfer into the pool - let pending_key = bridge_pool::get_pending_key(); - let mut pending: HashSet = read(&pending_key).unwrap(); - pending.insert(transfer); - write(pending_key, pending.try_to_vec().unwrap()); + let pending_key = bridge_pool::get_pending_key(&transfer); + write(pending_key, transfer.try_to_vec().unwrap()); } \ No newline at end of file