From 514563cef057762150f5b2f403dd707c057225ea Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 15 Oct 2022 21:39:06 -0400 Subject: [PATCH] Remove height as a term Unbeknowst to me, height doesn't have a universal definition of the chain length. Bitcoin defines height as the block number, with getblockcount existing for the chain length. Ethereum uses the unambiguous term "block number". Monero defines height as both the block number and the chain length. Instead of arguing about who's right, it's agreed it referring to both isn't productive. While we could provide our own definition, taking a side, moving to the unambiguous block number prevents future hiccups. height is now only a term in the Monero code, where it takes its Monero-specific definition, as documented in the processor. --- coins/monero/src/rpc.rs | 2 +- processor/src/coin/mod.rs | 8 ++-- processor/src/coin/monero.rs | 52 +++++++++++---------- processor/src/tests/mod.rs | 14 +++--- processor/src/wallet.rs | 90 +++++++++++++++++------------------- 5 files changed, 82 insertions(+), 84 deletions(-) diff --git a/coins/monero/src/rpc.rs b/coins/monero/src/rpc.rs index 0967d7cb..d4243e64 100644 --- a/coins/monero/src/rpc.rs +++ b/coins/monero/src/rpc.rs @@ -206,7 +206,7 @@ impl Rpc { self.get_transactions(&[tx]).await.map(|mut txs| txs.swap_remove(0)) } - pub async fn get_transaction_height(&self, tx: &[u8]) -> Result { + pub async fn get_transaction_block_number(&self, tx: &[u8]) -> Result { let txs: TransactionsResponse = self.rpc_call("get_transactions", Some(json!({ "txs_hashes": [hex::encode(tx)] }))).await?; diff --git a/processor/src/coin/mod.rs b/processor/src/coin/mod.rs index 98471d34..d018a7b7 100644 --- a/processor/src/coin/mod.rs +++ b/processor/src/coin/mod.rs @@ -47,8 +47,8 @@ pub trait Coin { // Doesn't have to take self, enables some level of caching which is pleasant fn address(&self, key: ::G) -> Self::Address; - async fn get_height(&self) -> Result; - async fn get_block(&self, height: usize) -> Result; + async fn get_latest_block_number(&self) -> Result; + async fn get_block(&self, number: usize) -> Result; async fn get_outputs( &self, block: &Self::Block, @@ -56,13 +56,13 @@ pub trait Coin { ) -> Result, CoinError>; // TODO: Remove - async fn is_confirmed(&self, tx: &[u8], height: usize) -> Result; + async fn is_confirmed(&self, tx: &[u8]) -> Result; async fn prepare_send( &self, keys: FrostKeys, transcript: RecommendedTranscript, - height: usize, + block_number: usize, inputs: Vec, payments: &[(Self::Address, u64)], fee: Self::Fee, diff --git a/processor/src/coin/monero.rs b/processor/src/coin/monero.rs index b25d42e6..d859a6c2 100644 --- a/processor/src/coin/monero.rs +++ b/processor/src/coin/monero.rs @@ -54,12 +54,13 @@ impl OutputTrait for Output { } #[derive(Debug)] -pub struct SignableTransaction( - FrostKeys, - RecommendedTranscript, - usize, - MSignableTransaction, -); +pub struct SignableTransaction { + keys: FrostKeys, + transcript: RecommendedTranscript, + // Monero height, defined as the length of the chain + height: usize, + actual: MSignableTransaction, +} #[derive(Clone, Debug)] pub struct Monero { @@ -121,12 +122,13 @@ impl Coin for Monero { self.scanner(key).address() } - async fn get_height(&self) -> Result { - self.rpc.get_height().await.map_err(|_| CoinError::ConnectionError) + async fn get_latest_block_number(&self) -> Result { + // Monero defines height as chain length, so subtract 1 for block number + Ok(self.rpc.get_height().await.map_err(|_| CoinError::ConnectionError)? - 1) } - async fn get_block(&self, height: usize) -> Result { - self.rpc.get_block(height).await.map_err(|_| CoinError::ConnectionError) + async fn get_block(&self, number: usize) -> Result { + self.rpc.get_block(number).await.map_err(|_| CoinError::ConnectionError) } async fn get_outputs( @@ -147,27 +149,27 @@ impl Coin for Monero { ) } - async fn is_confirmed(&self, tx: &[u8], height: usize) -> Result { - let tx_height = - self.rpc.get_transaction_height(tx).await.map_err(|_| CoinError::ConnectionError)?; - Ok((height.saturating_sub(tx_height) + 1) >= 10) + async fn is_confirmed(&self, tx: &[u8]) -> Result { + let tx_block_number = + self.rpc.get_transaction_block_number(tx).await.map_err(|_| CoinError::ConnectionError)?; + Ok((self.get_latest_block_number().await?.saturating_sub(tx_block_number) + 1) >= 10) } async fn prepare_send( &self, keys: FrostKeys, transcript: RecommendedTranscript, - height: usize, + block_number: usize, mut inputs: Vec, payments: &[(Address, u64)], fee: Fee, ) -> Result { let spend = keys.group_key(); - Ok(SignableTransaction( + Ok(SignableTransaction { keys, transcript, - height, - MSignableTransaction::new( + height: block_number + 1, + actual: MSignableTransaction::new( self.rpc.get_protocol().await.unwrap(), // TODO: Make this deterministic inputs.drain(..).map(|input| input.0).collect(), payments.to_vec(), @@ -176,7 +178,7 @@ impl Coin for Monero { fee, ) .map_err(|_| CoinError::ConnectionError)?, - )) + }) } async fn attempt_send( @@ -185,13 +187,13 @@ impl Coin for Monero { included: &[u16], ) -> Result { transaction - .3 + .actual .clone() .multisig( &self.rpc, - transaction.0.clone(), - transaction.1.clone(), - transaction.2, + transaction.keys.clone(), + transaction.transcript.clone(), + transaction.height, included.to_vec(), ) .await @@ -231,7 +233,7 @@ impl Coin for Monero { async fn test_send(&self, address: Self::Address) { use rand_core::OsRng; - let height = self.get_height().await.unwrap(); + let new_block = self.get_latest_block_number().await.unwrap() + 1; self.mine_block().await; for _ in 0 .. 7 { @@ -239,7 +241,7 @@ impl Coin for Monero { } let outputs = Self::empty_scanner() - .scan(&self.rpc, &self.rpc.get_block(height).await.unwrap()) + .scan(&self.rpc, &self.rpc.get_block(new_block).await.unwrap()) .await .unwrap() .swap_remove(0) diff --git a/processor/src/tests/mod.rs b/processor/src/tests/mod.rs index c27c373a..c365acf8 100644 --- a/processor/src/tests/mod.rs +++ b/processor/src/tests/mod.rs @@ -59,9 +59,9 @@ impl Network for LocalNetwork { } async fn test_send(coin: C, fee: C::Fee) { - // Mine a block so there's a confirmed height + // Mine blocks so there's a confirmed block coin.mine_block().await; - let height = coin.get_height().await.unwrap(); + let latest = coin.get_latest_block_number().await.unwrap(); let mut keys = frost::tests::key_gen::<_, C::Curve>(&mut OsRng); let threshold = keys[&1].params().t(); @@ -70,13 +70,13 @@ async fn test_send(coin: C, fee: C::Fee) { let mut wallets = vec![]; for i in 1 ..= threshold { let mut wallet = Wallet::new(MemCoinDb::new(), coin.clone()); - wallet.acknowledge_height(0, height); + wallet.acknowledge_block(0, latest); wallet.add_keys(&WalletKeys::new(keys.remove(&i).unwrap(), 0)); wallets.push(wallet); } - // Get the chain to a height where blocks have sufficient confirmations - while (height + C::CONFIRMATIONS) > coin.get_height().await.unwrap() { + // Get the chain to a length where blocks have sufficient confirmations + while (latest + (C::CONFIRMATIONS - 1)) > coin.get_latest_block_number().await.unwrap() { coin.mine_block().await; } @@ -91,8 +91,8 @@ async fn test_send(coin: C, fee: C::Fee) { for (network, wallet) in networks.iter_mut().zip(wallets.iter_mut()) { wallet.poll().await.unwrap(); - let height = coin.get_height().await.unwrap(); - wallet.acknowledge_height(1, height - 10); + let latest = coin.get_latest_block_number().await.unwrap(); + wallet.acknowledge_block(1, latest - (C::CONFIRMATIONS - 1)); let signable = wallet .prepare_sends(1, vec![(wallet.address(), 10000000000)], fee) .await diff --git a/processor/src/wallet.rs b/processor/src/wallet.rs index 4ef2ed2b..b2496e8a 100644 --- a/processor/src/wallet.rs +++ b/processor/src/wallet.rs @@ -18,12 +18,12 @@ use crate::{ pub struct WalletKeys { keys: FrostKeys, - creation_height: usize, + creation_block: usize, } impl WalletKeys { - pub fn new(keys: FrostKeys, creation_height: usize) -> WalletKeys { - WalletKeys { keys, creation_height } + pub fn new(keys: FrostKeys, creation_block: usize) -> WalletKeys { + WalletKeys { keys, creation_block } } // Bind this key to a specific network by applying an additive offset @@ -45,42 +45,42 @@ impl WalletKeys { } pub trait CoinDb { - // Set a height as scanned to - fn scanned_to_height(&mut self, height: usize); - // Acknowledge a given coin height for a canonical height - fn acknowledge_height(&mut self, canonical: usize, height: usize); + // Set a block as scanned to + fn scanned_to_block(&mut self, block: usize); + // Acknowledge a specific block number as part of a canonical block + fn acknowledge_block(&mut self, canonical: usize, block: usize); // Adds an output to the DB. Returns false if the output was already added fn add_output(&mut self, output: &O) -> bool; - // Height this coin has been scanned to - fn scanned_height(&self) -> usize; - // Acknowledged height for a given canonical height - fn acknowledged_height(&self, canonical: usize) -> usize; + // Block this coin has been scanned to (inclusive) + fn scanned_block(&self) -> usize; + // Acknowledged block for a given canonical block + fn acknowledged_block(&self, canonical: usize) -> usize; } pub struct MemCoinDb { // Height this coin has been scanned to - scanned_height: usize, - // Acknowledged height for a given canonical height - acknowledged_heights: HashMap, + scanned_block: usize, + // Acknowledged block for a given canonical block + acknowledged_blocks: HashMap, outputs: HashMap, Vec>, } impl MemCoinDb { pub fn new() -> MemCoinDb { - MemCoinDb { scanned_height: 0, acknowledged_heights: HashMap::new(), outputs: HashMap::new() } + MemCoinDb { scanned_block: 0, acknowledged_blocks: HashMap::new(), outputs: HashMap::new() } } } impl CoinDb for MemCoinDb { - fn scanned_to_height(&mut self, height: usize) { - self.scanned_height = height; + fn scanned_to_block(&mut self, block: usize) { + self.scanned_block = block; } - fn acknowledge_height(&mut self, canonical: usize, height: usize) { - debug_assert!(!self.acknowledged_heights.contains_key(&canonical)); - self.acknowledged_heights.insert(canonical, height); + fn acknowledge_block(&mut self, canonical: usize, block: usize) { + debug_assert!(!self.acknowledged_blocks.contains_key(&canonical)); + self.acknowledged_blocks.insert(canonical, block); } fn add_output(&mut self, output: &O) -> bool { @@ -96,12 +96,12 @@ impl CoinDb for MemCoinDb { true } - fn scanned_height(&self) -> usize { - self.scanned_height + fn scanned_block(&self) -> usize { + self.scanned_block } - fn acknowledged_height(&self, canonical: usize) -> usize { - self.acknowledged_heights[&canonical] + fn acknowledged_block(&self, canonical: usize) -> usize { + self.acknowledged_blocks[&canonical] } } @@ -212,22 +212,18 @@ impl Wallet { Wallet { db, coin, keys: vec![], pending: vec![] } } - pub fn scanned_height(&self) -> usize { - self.db.scanned_height() + pub fn scanned_block(&self) -> usize { + self.db.scanned_block() } - pub fn acknowledge_height(&mut self, canonical: usize, height: usize) { - self.db.acknowledge_height(canonical, height); - if height > self.db.scanned_height() { - self.db.scanned_to_height(height); - } + pub fn acknowledge_block(&mut self, canonical: usize, block: usize) { + self.db.acknowledge_block(canonical, block); } - pub fn acknowledged_height(&self, canonical: usize) -> usize { - self.db.acknowledged_height(canonical) + pub fn acknowledged_block(&self, canonical: usize) -> usize { + self.db.acknowledged_block(canonical) } pub fn add_keys(&mut self, keys: &WalletKeys) { - // Doesn't use +1 as this is height, not block index, and poll moves by block index - self.pending.push((self.acknowledged_height(keys.creation_height), keys.bind(C::ID))); + self.pending.push((self.acknowledged_block(keys.creation_block), keys.bind(C::ID))); } pub fn address(&self) -> C::Address { @@ -236,17 +232,18 @@ impl Wallet { // TODO: Remove pub async fn is_confirmed(&mut self, tx: &[u8]) -> Result { - self.coin.is_confirmed(tx, self.scanned_height() + C::CONFIRMATIONS).await + self.coin.is_confirmed(tx).await } pub async fn poll(&mut self) -> Result<(), CoinError> { - if self.coin.get_height().await? < C::CONFIRMATIONS { + if self.coin.get_latest_block_number().await? < (C::CONFIRMATIONS - 1) { return Ok(()); } - let confirmed_block = self.coin.get_height().await? - C::CONFIRMATIONS; + let confirmed_block = self.coin.get_latest_block_number().await? - (C::CONFIRMATIONS - 1); - for b in self.scanned_height() ..= confirmed_block { - // If any keys activated at this height, shift them over + // Will never scan the genesis block, which shouldn't be an issue + for b in (self.scanned_block() + 1) ..= confirmed_block { + // If any keys activated at this block, shift them over { let mut k = 0; while k < self.pending.len() { @@ -274,8 +271,7 @@ impl Wallet { ); } - // Blocks are zero-indexed while heights aren't - self.db.scanned_to_height(b + 1); + self.db.scanned_to_block(b); } Ok(()) @@ -296,7 +292,7 @@ impl Wallet { return Ok((vec![], vec![])); } - let acknowledged_height = self.acknowledged_height(canonical); + let acknowledged_block = self.acknowledged_block(canonical); // TODO: Log schedule outputs when MAX_OUTPUTS is lower than payments.len() // Payments is the first set of TXs in the schedule @@ -318,16 +314,16 @@ impl Wallet { // Create the transcript for this transaction let mut transcript = RecommendedTranscript::new(b"Serai Processor Wallet Send"); transcript - .append_message(b"canonical_height", &u64::try_from(canonical).unwrap().to_le_bytes()); + .append_message(b"canonical_block", &u64::try_from(canonical).unwrap().to_le_bytes()); transcript.append_message( - b"acknowledged_height", - &u64::try_from(acknowledged_height).unwrap().to_le_bytes(), + b"acknowledged_block", + &u64::try_from(acknowledged_block).unwrap().to_le_bytes(), ); transcript.append_message(b"index", &u64::try_from(txs.len()).unwrap().to_le_bytes()); let tx = self .coin - .prepare_send(keys.clone(), transcript, acknowledged_height, inputs, &outputs, fee) + .prepare_send(keys.clone(), transcript, acknowledged_block, inputs, &outputs, fee) .await?; // self.db.save_tx(tx) // TODO txs.push(tx);