From 1751c860082870d69159c6431d37d8cd5d2b1ac2 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sat, 27 Jan 2024 18:26:02 +0200 Subject: [PATCH 01/16] feat: implement a capped hashmap colelction --- src/capped_hashmap.rs | 44 +++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + 2 files changed, 45 insertions(+) create mode 100644 src/capped_hashmap.rs diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs new file mode 100644 index 0000000..730e0ea --- /dev/null +++ b/src/capped_hashmap.rs @@ -0,0 +1,44 @@ +use std::{ + hash::Hash, cmp::Eq, + collections::{HashMap, VecDeque}, +}; + +pub struct CappedHashMap +where + K: Hash + Eq + Copy + Clone +{ + inner: HashMap, + last_tasks: VecDeque, +} + +impl CappedHashMap +where + K: Hash + Eq + Copy + Clone, +{ + const MAX_LEN: usize = 100; + + pub fn new() -> Self { + Self { + inner: HashMap::with_capacity(Self::MAX_LEN), + last_tasks: VecDeque::with_capacity(Self::MAX_LEN), + } + } + + /// Inserts an new item to the collection. Return Some(key) where key is the + /// key that was removed when we reach the max capacity. Otherwise returns None. + pub fn insert(&mut self, k: K, v: V) -> Option { + self.last_tasks.push_front(k); + + if self.last_tasks.len() == Self::MAX_LEN { + // remove the oldest item. We an safely unwrap because we know the vec is no empty at this point + let key = self.last_tasks.pop_back().unwrap(); + self.inner.remove(&key); + + return Some(key) + } + + self.inner.insert(k, v); + + None + } +} diff --git a/src/lib.rs b/src/lib.rs index 366acb7..241997c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,6 +21,7 @@ pub mod bob_request; /// 3. The zkBitcoin committee produce a collaborative schnorr signature to unlock the funds for Bob. pub mod mpc_sign_tx; +pub mod capped_hashmap; // // Helpers // From d44929c920ca8167d3839156f740c9217cce5d73 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sat, 27 Jan 2024 18:33:38 +0200 Subject: [PATCH 02/16] feat: implemebt CappedHashMap::remove --- src/capped_hashmap.rs | 17 ++++++++++++++++- src/committee/node.rs | 10 ++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 730e0ea..5fbdb0a 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -32,7 +32,7 @@ where if self.last_tasks.len() == Self::MAX_LEN { // remove the oldest item. We an safely unwrap because we know the vec is no empty at this point let key = self.last_tasks.pop_back().unwrap(); - self.inner.remove(&key); + self.remove(&key); return Some(key) } @@ -41,4 +41,19 @@ where None } + + /// Removes a key from the map, returning the value at the key if the key was previously in the map. + pub fn remove(&mut self, k: &K) -> Option { + let Some(v) = self.inner.remove(k) else { + return None + }; + + self.last_tasks = self.last_tasks + .iter() + .filter(|key| *key != k) + .map(|key| *key) + .collect::>(); + + Some(v) + } } diff --git a/src/committee/node.rs b/src/committee/node.rs index 3360815..bbeb58d 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -1,5 +1,5 @@ use std::{ - collections::{BTreeMap, HashMap}, + collections::BTreeMap, net::SocketAddr, sync::{Arc, RwLock}, }; @@ -17,9 +17,7 @@ use rand::thread_rng; use serde::{Deserialize, Serialize}; use crate::{ - bob_request::{BobRequest, SmartContract}, - frost, - mpc_sign_tx::get_digest_to_hash, + bob_request::{BobRequest, SmartContract}, capped_hashmap::CappedHashMap, frost, mpc_sign_tx::get_digest_to_hash }; // @@ -35,7 +33,7 @@ pub struct NodeState { pub pubkey_package: frost::PublicKeyPackage, // TODO: ensure that this cannot grow like crazy? prune old tasks? - pub signing_tasks: RwLock>, + pub signing_tasks: RwLock>, } #[derive(Clone)] @@ -233,7 +231,7 @@ pub async fn run_server( let ctx = NodeState { key_package, pubkey_package, - signing_tasks: RwLock::new(HashMap::new()), + signing_tasks: RwLock::new(CappedHashMap::new()), }; let server = Server::builder() From 8929636be2cfdb8f6c7c758526133fc5bff25502 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sat, 27 Jan 2024 18:40:11 +0200 Subject: [PATCH 03/16] chore: use 4 space tabs --- src/capped_hashmap.rs | 84 +++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 5fbdb0a..2ddc48b 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -1,59 +1,59 @@ use std::{ - hash::Hash, cmp::Eq, - collections::{HashMap, VecDeque}, + hash::Hash, cmp::Eq, + collections::{HashMap, VecDeque}, }; pub struct CappedHashMap where - K: Hash + Eq + Copy + Clone + K: Hash + Eq + Copy + Clone { - inner: HashMap, - last_tasks: VecDeque, + inner: HashMap, + last_tasks: VecDeque, } impl CappedHashMap where - K: Hash + Eq + Copy + Clone, + K: Hash + Eq + Copy + Clone, { - const MAX_LEN: usize = 100; + const MAX_LEN: usize = 100; - pub fn new() -> Self { - Self { - inner: HashMap::with_capacity(Self::MAX_LEN), - last_tasks: VecDeque::with_capacity(Self::MAX_LEN), + pub fn new() -> Self { + Self { + inner: HashMap::with_capacity(Self::MAX_LEN), + last_tasks: VecDeque::with_capacity(Self::MAX_LEN), + } } - } - /// Inserts an new item to the collection. Return Some(key) where key is the - /// key that was removed when we reach the max capacity. Otherwise returns None. - pub fn insert(&mut self, k: K, v: V) -> Option { - self.last_tasks.push_front(k); - - if self.last_tasks.len() == Self::MAX_LEN { - // remove the oldest item. We an safely unwrap because we know the vec is no empty at this point - let key = self.last_tasks.pop_back().unwrap(); - self.remove(&key); + /// Inserts an new item to the collection. Return Some(key) where key is the + /// key that was removed when we reach the max capacity. Otherwise returns None. + pub fn insert(&mut self, k: K, v: V) -> Option { + self.last_tasks.push_front(k); + + if self.last_tasks.len() == Self::MAX_LEN { + // remove the oldest item. We an safely unwrap because we know the vec is no empty at this point + let key = self.last_tasks.pop_back().unwrap(); + self.remove(&key); + + return Some(key) + } + + self.inner.insert(k, v); + + None + } - return Some(key) + /// Removes a key from the map, returning the value at the key if the key was previously in the map. + pub fn remove(&mut self, k: &K) -> Option { + let Some(v) = self.inner.remove(k) else { + return None + }; + + self.last_tasks = self.last_tasks + .iter() + .filter(|key| *key != k) + .map(|key| *key) + .collect::>(); + + Some(v) } - - self.inner.insert(k, v); - - None - } - - /// Removes a key from the map, returning the value at the key if the key was previously in the map. - pub fn remove(&mut self, k: &K) -> Option { - let Some(v) = self.inner.remove(k) else { - return None - }; - - self.last_tasks = self.last_tasks - .iter() - .filter(|key| *key != k) - .map(|key| *key) - .collect::>(); - - Some(v) - } } From bab9e7aaf19fb559dc152c82d3d515446a9a3732 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sat, 27 Jan 2024 18:41:02 +0200 Subject: [PATCH 04/16] chore: fi typo --- src/capped_hashmap.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 2ddc48b..f2df036 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -30,7 +30,7 @@ where self.last_tasks.push_front(k); if self.last_tasks.len() == Self::MAX_LEN { - // remove the oldest item. We an safely unwrap because we know the vec is no empty at this point + // remove the oldest item. We an safely unwrap because we know the last_tasks is not empty at this point let key = self.last_tasks.pop_back().unwrap(); self.remove(&key); From fe21a4783502e49f4f0074dc9fc757b013f784e3 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sun, 28 Jan 2024 11:32:41 +0200 Subject: [PATCH 05/16] chore: run cargo fmt --- src/capped_hashmap.rs | 30 ++++++++++++++++-------------- src/committee/node.rs | 5 ++++- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index f2df036..a2931d6 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -1,30 +1,31 @@ use std::{ - hash::Hash, cmp::Eq, + cmp::Eq, collections::{HashMap, VecDeque}, + hash::Hash, }; pub struct CappedHashMap where - K: Hash + Eq + Copy + Clone + K: Hash + Eq + Copy + Clone, { inner: HashMap, last_tasks: VecDeque, } -impl CappedHashMap +impl CappedHashMap where K: Hash + Eq + Copy + Clone, { const MAX_LEN: usize = 100; pub fn new() -> Self { - Self { - inner: HashMap::with_capacity(Self::MAX_LEN), - last_tasks: VecDeque::with_capacity(Self::MAX_LEN), - } + Self { + inner: HashMap::with_capacity(Self::MAX_LEN), + last_tasks: VecDeque::with_capacity(Self::MAX_LEN), + } } - /// Inserts an new item to the collection. Return Some(key) where key is the + /// Inserts an new item to the collection. Return Some(key) where key is the /// key that was removed when we reach the max capacity. Otherwise returns None. pub fn insert(&mut self, k: K, v: V) -> Option { self.last_tasks.push_front(k); @@ -34,21 +35,22 @@ where let key = self.last_tasks.pop_back().unwrap(); self.remove(&key); - return Some(key) + return Some(key); } - + self.inner.insert(k, v); - + None } /// Removes a key from the map, returning the value at the key if the key was previously in the map. pub fn remove(&mut self, k: &K) -> Option { let Some(v) = self.inner.remove(k) else { - return None + return None; }; - - self.last_tasks = self.last_tasks + + self.last_tasks = self + .last_tasks .iter() .filter(|key| *key != k) .map(|key| *key) diff --git a/src/committee/node.rs b/src/committee/node.rs index bbeb58d..5f81358 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -17,7 +17,10 @@ use rand::thread_rng; use serde::{Deserialize, Serialize}; use crate::{ - bob_request::{BobRequest, SmartContract}, capped_hashmap::CappedHashMap, frost, mpc_sign_tx::get_digest_to_hash + bob_request::{BobRequest, SmartContract}, + capped_hashmap::CappedHashMap, + frost, + mpc_sign_tx::get_digest_to_hash, }; // From 6f8294af67bac4b875c386b36b6ace911ad133d4 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sun, 28 Jan 2024 12:06:05 +0200 Subject: [PATCH 06/16] feat: add get fns to capped hashmap --- docs/mpc.md | 2 +- src/capped_hashmap.rs | 44 +++++++++++++++++++++++++++++++++++++------ src/committee/node.rs | 2 +- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/docs/mpc.md b/docs/mpc.md index c9e8ce4..b9276b0 100644 --- a/docs/mpc.md +++ b/docs/mpc.md @@ -68,7 +68,7 @@ pub struct LocalSigningTask { Members keep track of such signing tasks in a local hashmap: ```rust -signing_tasks: RwLock> +signing_tasks: RwLock> ``` The commitments created at this point are sent back to the orchestrator: diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index a2931d6..899708e 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -8,6 +8,7 @@ pub struct CappedHashMap where K: Hash + Eq + Copy + Clone, { + capacity: usize, inner: HashMap, last_tasks: VecDeque, } @@ -16,12 +17,11 @@ impl CappedHashMap where K: Hash + Eq + Copy + Clone, { - const MAX_LEN: usize = 100; - - pub fn new() -> Self { + pub fn new(capacity: usize) -> Self { Self { - inner: HashMap::with_capacity(Self::MAX_LEN), - last_tasks: VecDeque::with_capacity(Self::MAX_LEN), + capacity, + inner: HashMap::with_capacity(capacity), + last_tasks: VecDeque::with_capacity(capacity), } } @@ -30,7 +30,7 @@ where pub fn insert(&mut self, k: K, v: V) -> Option { self.last_tasks.push_front(k); - if self.last_tasks.len() == Self::MAX_LEN { + if self.last_tasks.len() == self.capacity { // remove the oldest item. We an safely unwrap because we know the last_tasks is not empty at this point let key = self.last_tasks.pop_back().unwrap(); self.remove(&key); @@ -58,4 +58,36 @@ where Some(v) } + + /// Returns a reference to the value corresponding to the key. + pub fn get(&self, k: &K) -> Option<&V> { + self.inner.get(k) + } + + /// Returns a mutable reference to the value corresponding to the key. + pub fn get_mut(&mut self, k: &K) -> Option<&mut V> { + self.inner.get_mut(k) + } + + /// Returns the number of elements in the collection + pub fn size(&self) -> usize { + self.inner.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_insert_into_non_full_collection() { + let mut col: CappedHashMap<&str, u8> = CappedHashMap::new(10); + col.insert("key_1", 1); + col.insert("key_2", 2); + col.insert("key_3", 3); + + assert_eq!(*col.get(&"key_1").unwrap(), 1); + assert_eq!(*col.get(&"key_2").unwrap(), 2); + assert_eq!(*col.get(&"key_3").unwrap(), 3); + } } diff --git a/src/committee/node.rs b/src/committee/node.rs index 5f81358..428e314 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -234,7 +234,7 @@ pub async fn run_server( let ctx = NodeState { key_package, pubkey_package, - signing_tasks: RwLock::new(CappedHashMap::new()), + signing_tasks: RwLock::new(CappedHashMap::new(100)), }; let server = Server::builder() From f061aaddf3de716916dbb7be09f3129ec0450d18 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sun, 28 Jan 2024 12:32:00 +0200 Subject: [PATCH 07/16] test: add test_insert_should_return_removed_key --- src/capped_hashmap.rs | 48 +++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 899708e..ee6e177 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -28,19 +28,20 @@ where /// Inserts an new item to the collection. Return Some(key) where key is the /// key that was removed when we reach the max capacity. Otherwise returns None. pub fn insert(&mut self, k: K, v: V) -> Option { - self.last_tasks.push_front(k); + let mut ret = None; if self.last_tasks.len() == self.capacity { // remove the oldest item. We an safely unwrap because we know the last_tasks is not empty at this point let key = self.last_tasks.pop_back().unwrap(); - self.remove(&key); + assert!(self.remove(&key).is_some()); - return Some(key); + ret = Some(key); } + self.last_tasks.push_front(k); self.inner.insert(k, v); - None + ret } /// Removes a key from the map, returning the value at the key if the key was previously in the map. @@ -61,14 +62,14 @@ where /// Returns a reference to the value corresponding to the key. pub fn get(&self, k: &K) -> Option<&V> { - self.inner.get(k) + self.inner.get(k) } /// Returns a mutable reference to the value corresponding to the key. pub fn get_mut(&mut self, k: &K) -> Option<&mut V> { - self.inner.get_mut(k) + self.inner.get_mut(k) } - + /// Returns the number of elements in the collection pub fn size(&self) -> usize { self.inner.len() @@ -81,13 +82,30 @@ mod tests { #[test] fn test_insert_into_non_full_collection() { - let mut col: CappedHashMap<&str, u8> = CappedHashMap::new(10); - col.insert("key_1", 1); - col.insert("key_2", 2); - col.insert("key_3", 3); - - assert_eq!(*col.get(&"key_1").unwrap(), 1); - assert_eq!(*col.get(&"key_2").unwrap(), 2); - assert_eq!(*col.get(&"key_3").unwrap(), 3); + let mut col: CappedHashMap = CappedHashMap::new(10); + col.insert(1, 1); + col.insert(2, 2); + col.insert(3, 3); + + assert_eq!(*col.get(&1).unwrap(), 1); + assert_eq!(*col.get(&2).unwrap(), 2); + assert_eq!(*col.get(&3).unwrap(), 3); + } + + #[test] + fn test_insert_should_return_removed_key() { + let mut col: CappedHashMap = CappedHashMap::new(10); + + for i in 0..10 { + col.insert(i, i); + } + + for i in 10..30 { + // the nth oldest key will be removed + let key_removed = col.insert(i, i); + assert!(key_removed.is_some()); + assert_eq!(key_removed.unwrap(), i - 10); + assert_eq!(col.size(), 10); + } } } From 332966006287b91dc7601a8fec01dfb3cfe6b6a7 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sun, 28 Jan 2024 12:36:09 +0200 Subject: [PATCH 08/16] test: add test_remove --- src/capped_hashmap.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index ee6e177..bbb3abe 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -108,4 +108,24 @@ mod tests { assert_eq!(col.size(), 10); } } + + #[test] + fn test_remove() { + let mut col: CappedHashMap = CappedHashMap::new(10); + + for i in 0..10 { + col.insert(i, i); + } + + for i in 0..10 { + let v = col.remove(&i); + assert!(v.is_some()); + assert_eq!(v.unwrap(), i); + assert_eq!(col.size() as u8, 10 - i - 1); + } + + // the collection is empty so the next remove should return None + let v = col.remove(&0); + assert!(v.is_none()); + } } From c636aae75e8240b99d77e749ac21f9fc7c37f98b Mon Sep 17 00:00:00 2001 From: ppoliani Date: Sun, 28 Jan 2024 12:58:33 +0200 Subject: [PATCH 09/16] test: test insert duplicate --- src/capped_hashmap.rs | 105 +++++++++++++++++++++++++++++------------- 1 file changed, 73 insertions(+), 32 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index bbb3abe..a94ad74 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -10,7 +10,7 @@ where { capacity: usize, inner: HashMap, - last_tasks: VecDeque, + last_items: VecDeque, } impl CappedHashMap @@ -19,9 +19,11 @@ where { pub fn new(capacity: usize) -> Self { Self { - capacity, + // We add a little bit margin to the capacity, because we always insert first and then check + // if collection is full. So we don't want the hashmap to double it's capacity because of that. + capacity: capacity + 1, inner: HashMap::with_capacity(capacity), - last_tasks: VecDeque::with_capacity(capacity), + last_items: VecDeque::with_capacity(capacity), } } @@ -30,17 +32,19 @@ where pub fn insert(&mut self, k: K, v: V) -> Option { let mut ret = None; - if self.last_tasks.len() == self.capacity { - // remove the oldest item. We an safely unwrap because we know the last_tasks is not empty at this point - let key = self.last_tasks.pop_back().unwrap(); + // replacing a value should not push any new items to last_items + if let None = self.inner.insert(k, v) { + self.last_items.push_front(k); + } + + if self.last_items.len() > self.capacity - 1 { + // remove the oldest item. We an safely unwrap because we know the last_items is not empty at this point + let key = self.last_items.pop_back().unwrap(); assert!(self.remove(&key).is_some()); ret = Some(key); } - self.last_tasks.push_front(k); - self.inner.insert(k, v); - ret } @@ -50,8 +54,8 @@ where return None; }; - self.last_tasks = self - .last_tasks + self.last_items = self + .last_items .iter() .filter(|key| *key != k) .map(|key| *key) @@ -101,31 +105,68 @@ mod tests { } for i in 10..30 { - // the nth oldest key will be removed - let key_removed = col.insert(i, i); - assert!(key_removed.is_some()); - assert_eq!(key_removed.unwrap(), i - 10); - assert_eq!(col.size(), 10); + // the nth oldest key will be removed + let key_removed = col.insert(i, i); + assert!(key_removed.is_some()); + assert_eq!(key_removed.unwrap(), i - 10); + assert_eq!(col.size(), 10); } + + // Not that we should have the last 10 keys in the collection i.e. 20-30. All the previous + // were replaced by these new ones + for i in 0..20 { + assert!(col.get(&i).is_none()); + } + + // after cyclic inserts we still have a full capacity collection. We can remove one item... + assert!(col.remove(&20).is_some()); + assert_eq!(col.size(), 9); + + // ... and now inserting a new item will not replace any existing one + assert!(col.insert(31, 31).is_none()); + } + + #[test] + fn test_insert_duplicate() { + let mut col: CappedHashMap = CappedHashMap::new(10); + + for i in 0..10 { + col.insert(i, i); + } + + assert_eq!(*col.get(&0).unwrap(), 0); + assert_eq!(col.size(), 10); + + // replacing should simply replace the value and not affect the size. + // so altough our col is full capacity, replacing an existing should not remove the oldest item + assert!(col.insert(0, 2).is_none()); + assert_eq!(*col.get(&0).unwrap(), 2); + assert_eq!(col.size(), 10); + + // but inserting a new one should + let key_removed = col.insert(10, 10); + assert!(key_removed.is_some()); + assert_eq!(key_removed.unwrap(), 0); + assert_eq!(col.size(), 10); } #[test] fn test_remove() { - let mut col: CappedHashMap = CappedHashMap::new(10); - - for i in 0..10 { - col.insert(i, i); - } - - for i in 0..10 { - let v = col.remove(&i); - assert!(v.is_some()); - assert_eq!(v.unwrap(), i); - assert_eq!(col.size() as u8, 10 - i - 1); - } - - // the collection is empty so the next remove should return None - let v = col.remove(&0); - assert!(v.is_none()); + let mut col: CappedHashMap = CappedHashMap::new(10); + + for i in 0..10 { + col.insert(i, i); + } + + for i in 0..10 { + let v = col.remove(&i); + assert!(v.is_some()); + assert_eq!(v.unwrap(), i); + assert_eq!(col.size() as u8, 10 - i - 1); + } + + // the collection is empty so the next remove should return None + let v = col.remove(&0); + assert!(v.is_none()); } } From c60603d2a185d4e9d229803d7dde54dd2a42f6e8 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 11:38:57 +0200 Subject: [PATCH 10/16] fix: PR comments --- .vscode/launch.json | 26 ++++++++++++++++++++++++++ src/capped_hashmap.rs | 21 ++++++++++++--------- src/committee/node.rs | 10 ++++------ src/constants.rs | 2 ++ src/lib.rs | 2 +- 5 files changed, 45 insertions(+), 16 deletions(-) create mode 100644 .vscode/launch.json diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 0000000..8681c85 --- /dev/null +++ b/.vscode/launch.json @@ -0,0 +1,26 @@ +{ + // Use IntelliSense to learn about possible attributes. + // Hover to view descriptions of existing attributes. + // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 + "version": "0.2.0", + "configurations": [ + { + "type": "lldb", + "request": "launch", + "name": "Debug zkbtc", + "cargo": { + "args": [ + "build", + "--bin=zkbtc", + "--package=zkbitcoin" + ], + "filter": { + "name": "zkbtc", + "kind": "bin" + } + }, + "args": [], + "cwd": "${workspaceFolder}" + }, + ] +} diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index a94ad74..0ab1c79 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -19,11 +19,11 @@ where { pub fn new(capacity: usize) -> Self { Self { + capacity, // We add a little bit margin to the capacity, because we always insert first and then check // if collection is full. So we don't want the hashmap to double it's capacity because of that. - capacity: capacity + 1, - inner: HashMap::with_capacity(capacity), - last_items: VecDeque::with_capacity(capacity), + inner: HashMap::with_capacity(capacity + 1), + last_items: VecDeque::with_capacity(capacity + 1), } } @@ -33,11 +33,11 @@ where let mut ret = None; // replacing a value should not push any new items to last_items - if let None = self.inner.insert(k, v) { + if self.inner.insert(k, v).is_none() { self.last_items.push_front(k); } - if self.last_items.len() > self.capacity - 1 { + if self.last_items.len() > self.capacity { // remove the oldest item. We an safely unwrap because we know the last_items is not empty at this point let key = self.last_items.pop_back().unwrap(); assert!(self.remove(&key).is_some()); @@ -54,12 +54,11 @@ where return None; }; - self.last_items = self + self .last_items .iter() - .filter(|key| *key != k) - .map(|key| *key) - .collect::>(); + .position(|key| key == k) + .and_then(|pos| self.last_items.remove(pos)); Some(v) } @@ -98,6 +97,7 @@ mod tests { #[test] fn test_insert_should_return_removed_key() { + // The real capacity will be 14. Read here for how this is calculated https://stackoverflow.com/a/76114888/512783 let mut col: CappedHashMap = CappedHashMap::new(10); for i in 0..10 { @@ -107,6 +107,9 @@ mod tests { for i in 10..30 { // the nth oldest key will be removed let key_removed = col.insert(i, i); + // our hashmap and vecqueue should never grow i.e. capacity doesn't change + assert_eq!(col.last_items.capacity(), 11); + assert!(key_removed.is_some()); assert_eq!(key_removed.unwrap(), i - 10); assert_eq!(col.size(), 10); diff --git a/src/committee/node.rs b/src/committee/node.rs index 428e314..b0d4782 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -17,10 +17,8 @@ use rand::thread_rng; use serde::{Deserialize, Serialize}; use crate::{ - bob_request::{BobRequest, SmartContract}, - capped_hashmap::CappedHashMap, - frost, - mpc_sign_tx::get_digest_to_hash, + bob_request::{BobRequest, SmartContract}, capped_hashmap::CappedHashMap, + constants::MAX_SIGNING_TASK, frost, mpc_sign_tx::get_digest_to_hash, }; // @@ -35,7 +33,7 @@ pub struct NodeState { /// The public key stuff they need. pub pubkey_package: frost::PublicKeyPackage, - // TODO: ensure that this cannot grow like crazy? prune old tasks? + /// The current pending signing tasks pub signing_tasks: RwLock>, } @@ -234,7 +232,7 @@ pub async fn run_server( let ctx = NodeState { key_package, pubkey_package, - signing_tasks: RwLock::new(CappedHashMap::new(100)), + signing_tasks: RwLock::new(CappedHashMap::new(MAX_SIGNING_TASK)), }; let server = Server::builder() diff --git a/src/constants.rs b/src/constants.rs index d66468f..108091f 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -43,3 +43,5 @@ pub const KEEPALIVE_WAIT_SECONDS: u64 = 5; /// The total number of fibonacci backoff retries before considering an MPC node offline pub const KEEPALIVE_MAX_RETRIES: u8 = 10; + +pub const MAX_SIGNING_TASK: usize = 100; diff --git a/src/lib.rs b/src/lib.rs index 241997c..f4d2cff 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,6 +3,7 @@ use anyhow::Context; use secp256k1::hashes::Hash; +pub mod capped_hashmap; pub mod committee; pub mod constants; pub mod frost; @@ -21,7 +22,6 @@ pub mod bob_request; /// 3. The zkBitcoin committee produce a collaborative schnorr signature to unlock the funds for Bob. pub mod mpc_sign_tx; -pub mod capped_hashmap; // // Helpers // From 3eda4cf94b5d4f48521378d4312781c58fe0210e Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 12:55:14 +0200 Subject: [PATCH 11/16] feat: check if key exists before adding a new one --- src/capped_hashmap.rs | 24 +++++++++++------------- src/committee/node.rs | 7 +++++-- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 0ab1c79..f2430f4 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -20,10 +20,8 @@ where pub fn new(capacity: usize) -> Self { Self { capacity, - // We add a little bit margin to the capacity, because we always insert first and then check - // if collection is full. So we don't want the hashmap to double it's capacity because of that. - inner: HashMap::with_capacity(capacity + 1), - last_items: VecDeque::with_capacity(capacity + 1), + inner: HashMap::with_capacity(capacity), + last_items: VecDeque::with_capacity(capacity), } } @@ -31,13 +29,9 @@ where /// key that was removed when we reach the max capacity. Otherwise returns None. pub fn insert(&mut self, k: K, v: V) -> Option { let mut ret = None; + let new_key = !self.inner.contains_key(&k); - // replacing a value should not push any new items to last_items - if self.inner.insert(k, v).is_none() { - self.last_items.push_front(k); - } - - if self.last_items.len() > self.capacity { + if new_key && self.last_items.len() >= self.capacity { // remove the oldest item. We an safely unwrap because we know the last_items is not empty at this point let key = self.last_items.pop_back().unwrap(); assert!(self.remove(&key).is_some()); @@ -45,6 +39,11 @@ where ret = Some(key); } + if new_key { + self.inner.insert(k, v); + self.last_items.push_front(k); + } + ret } @@ -54,8 +53,7 @@ where return None; }; - self - .last_items + self.last_items .iter() .position(|key| key == k) .and_then(|pos| self.last_items.remove(pos)); @@ -108,7 +106,7 @@ mod tests { // the nth oldest key will be removed let key_removed = col.insert(i, i); // our hashmap and vecqueue should never grow i.e. capacity doesn't change - assert_eq!(col.last_items.capacity(), 11); + assert_eq!(col.last_items.capacity(), 10); assert!(key_removed.is_some()); assert_eq!(key_removed.unwrap(), i - 10); diff --git a/src/committee/node.rs b/src/committee/node.rs index b0d4782..5186c2d 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -17,8 +17,11 @@ use rand::thread_rng; use serde::{Deserialize, Serialize}; use crate::{ - bob_request::{BobRequest, SmartContract}, capped_hashmap::CappedHashMap, - constants::MAX_SIGNING_TASK, frost, mpc_sign_tx::get_digest_to_hash, + bob_request::{BobRequest, SmartContract}, + capped_hashmap::CappedHashMap, + constants::MAX_SIGNING_TASK, + frost, + mpc_sign_tx::get_digest_to_hash, }; // From 181a3ad27b59aeb0d8153a115e93c8c3371d362e Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 12:58:00 +0200 Subject: [PATCH 12/16] feat: allow user replace the code --- src/capped_hashmap.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index f2430f4..d16c0f8 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -8,7 +8,6 @@ pub struct CappedHashMap where K: Hash + Eq + Copy + Clone, { - capacity: usize, inner: HashMap, last_items: VecDeque, } @@ -19,7 +18,6 @@ where { pub fn new(capacity: usize) -> Self { Self { - capacity, inner: HashMap::with_capacity(capacity), last_items: VecDeque::with_capacity(capacity), } @@ -31,7 +29,7 @@ where let mut ret = None; let new_key = !self.inner.contains_key(&k); - if new_key && self.last_items.len() >= self.capacity { + if new_key && self.last_items.len() >= self.last_items.capacity() { // remove the oldest item. We an safely unwrap because we know the last_items is not empty at this point let key = self.last_items.pop_back().unwrap(); assert!(self.remove(&key).is_some()); @@ -39,8 +37,8 @@ where ret = Some(key); } - if new_key { - self.inner.insert(k, v); + // replacing a value should not push any new items to last_items + if self.inner.insert(k, v).is_none() { self.last_items.push_front(k); } From 81f616ca1e2ae78121bbad78659bf070b35c7b2d Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 13:05:08 +0200 Subject: [PATCH 13/16] refactor: rename insert to add_entry --- src/capped_hashmap.rs | 24 ++++++++++++------------ src/committee/node.rs | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index d16c0f8..0a4650d 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -23,9 +23,9 @@ where } } - /// Inserts an new item to the collection. Return Some(key) where key is the + /// Inserts an new key-value pair to the collection. Return Some(key) where key is the /// key that was removed when we reach the max capacity. Otherwise returns None. - pub fn insert(&mut self, k: K, v: V) -> Option { + pub fn add_entry(&mut self, k: K, v: V) -> Option { let mut ret = None; let new_key = !self.inner.contains_key(&k); @@ -82,9 +82,9 @@ mod tests { #[test] fn test_insert_into_non_full_collection() { let mut col: CappedHashMap = CappedHashMap::new(10); - col.insert(1, 1); - col.insert(2, 2); - col.insert(3, 3); + col.add_entry(1, 1); + col.add_entry(2, 2); + col.add_entry(3, 3); assert_eq!(*col.get(&1).unwrap(), 1); assert_eq!(*col.get(&2).unwrap(), 2); @@ -97,12 +97,12 @@ mod tests { let mut col: CappedHashMap = CappedHashMap::new(10); for i in 0..10 { - col.insert(i, i); + col.add_entry(i, i); } for i in 10..30 { // the nth oldest key will be removed - let key_removed = col.insert(i, i); + let key_removed = col.add_entry(i, i); // our hashmap and vecqueue should never grow i.e. capacity doesn't change assert_eq!(col.last_items.capacity(), 10); @@ -122,7 +122,7 @@ mod tests { assert_eq!(col.size(), 9); // ... and now inserting a new item will not replace any existing one - assert!(col.insert(31, 31).is_none()); + assert!(col.add_entry(31, 31).is_none()); } #[test] @@ -130,7 +130,7 @@ mod tests { let mut col: CappedHashMap = CappedHashMap::new(10); for i in 0..10 { - col.insert(i, i); + col.add_entry(i, i); } assert_eq!(*col.get(&0).unwrap(), 0); @@ -138,12 +138,12 @@ mod tests { // replacing should simply replace the value and not affect the size. // so altough our col is full capacity, replacing an existing should not remove the oldest item - assert!(col.insert(0, 2).is_none()); + assert!(col.add_entry(0, 2).is_none()); assert_eq!(*col.get(&0).unwrap(), 2); assert_eq!(col.size(), 10); // but inserting a new one should - let key_removed = col.insert(10, 10); + let key_removed = col.add_entry(10, 10); assert!(key_removed.is_some()); assert_eq!(key_removed.unwrap(), 0); assert_eq!(col.size(), 10); @@ -154,7 +154,7 @@ mod tests { let mut col: CappedHashMap = CappedHashMap::new(10); for i in 0..10 { - col.insert(i, i); + col.add_entry(i, i); } for i in 0..10 { diff --git a/src/committee/node.rs b/src/committee/node.rs index 5186c2d..a4dc692 100644 --- a/src/committee/node.rs +++ b/src/committee/node.rs @@ -100,7 +100,7 @@ async fn round_1_signing( // store it locally { let mut signing_tasks = context.signing_tasks.write().unwrap(); - signing_tasks.insert( + signing_tasks.add_entry( txid, LocalSigningTask { proof_hash: bob_request.proof.hash(), From 6f39a59beae14ec1d8e9928d4e2db7179dd3ebe2 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 13:38:06 +0200 Subject: [PATCH 14/16] feat: use max_size field --- src/capped_hashmap.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index 0a4650d..edc7d2a 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -8,6 +8,7 @@ pub struct CappedHashMap where K: Hash + Eq + Copy + Clone, { + max_size: usize, inner: HashMap, last_items: VecDeque, } @@ -16,10 +17,11 @@ impl CappedHashMap where K: Hash + Eq + Copy + Clone, { - pub fn new(capacity: usize) -> Self { + pub fn new(max_size: usize) -> Self { Self { - inner: HashMap::with_capacity(capacity), - last_items: VecDeque::with_capacity(capacity), + max_size, + inner: HashMap::with_capacity(max_size), + last_items: VecDeque::with_capacity(max_size), } } @@ -29,7 +31,7 @@ where let mut ret = None; let new_key = !self.inner.contains_key(&k); - if new_key && self.last_items.len() >= self.last_items.capacity() { + if new_key && self.last_items.len() >= self.max_size { // remove the oldest item. We an safely unwrap because we know the last_items is not empty at this point let key = self.last_items.pop_back().unwrap(); assert!(self.remove(&key).is_some()); From a0adf49747f958569e895d7efc8ed791f0e4ab02 Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 13:44:56 +0200 Subject: [PATCH 15/16] feat: add helper log function --- src/capped_hashmap.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index edc7d2a..f1ab254 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -4,6 +4,8 @@ use std::{ hash::Hash, }; +use log::info; + pub struct CappedHashMap where K: Hash + Eq + Copy + Clone, @@ -25,6 +27,18 @@ where } } + fn log(&self) { + let count = self.last_items.len(); + + if count >= self.max_size * 90 / 100 { + info!("Over 90% full"); + } else if count >= self.max_size / 2 { + info!("Over 50% full"); + } else if count >= self.max_size / 4 { + info!("Over 25% full"); + } + } + /// Inserts an new key-value pair to the collection. Return Some(key) where key is the /// key that was removed when we reach the max capacity. Otherwise returns None. pub fn add_entry(&mut self, k: K, v: V) -> Option { @@ -44,6 +58,7 @@ where self.last_items.push_front(k); } + self.log(); ret } From 8b7ab845a4945653d07db3df12a06374f87d8a9c Mon Sep 17 00:00:00 2001 From: ppoliani Date: Mon, 29 Jan 2024 13:45:15 +0200 Subject: [PATCH 16/16] chore: code format --- src/capped_hashmap.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/capped_hashmap.rs b/src/capped_hashmap.rs index f1ab254..a9efd2f 100644 --- a/src/capped_hashmap.rs +++ b/src/capped_hashmap.rs @@ -28,15 +28,15 @@ where } fn log(&self) { - let count = self.last_items.len(); - - if count >= self.max_size * 90 / 100 { - info!("Over 90% full"); - } else if count >= self.max_size / 2 { - info!("Over 50% full"); - } else if count >= self.max_size / 4 { - info!("Over 25% full"); - } + let count = self.last_items.len(); + + if count >= self.max_size * 90 / 100 { + info!("Over 90% full"); + } else if count >= self.max_size / 2 { + info!("Over 50% full"); + } else if count >= self.max_size / 4 { + info!("Over 25% full"); + } } /// Inserts an new key-value pair to the collection. Return Some(key) where key is the