From 060efa930f8005c6b950ec47872fb9598b7b18d4 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 25 Jan 2024 01:08:42 +0100 Subject: [PATCH 1/4] check diffs --- .../namada/src/ledger/native_vp/multitoken.rs | 116 +++++++++++++++--- 1 file changed, 98 insertions(+), 18 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/multitoken.rs b/crates/namada/src/ledger/native_vp/multitoken.rs index 5537ca7541..adc7e6bbe8 100644 --- a/crates/namada/src/ledger/native_vp/multitoken.rs +++ b/crates/namada/src/ledger/native_vp/multitoken.rs @@ -11,7 +11,7 @@ use crate::token::storage_key::{ is_any_minted_balance_key, is_any_minter_key, is_any_token_balance_key, minter_key, }; -use crate::token::{Amount, Change}; +use crate::token::Amount; use crate::types::address::{Address, InternalAddress}; use crate::types::storage::{Key, KeySeg}; use crate::vm::WasmCacheAccess; @@ -51,26 +51,87 @@ where keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result { - let mut changes = HashMap::new(); - let mut mints = HashMap::new(); + let mut inc_changes: HashMap = HashMap::new(); + let mut dec_changes: HashMap = HashMap::new(); + let mut inc_mints: HashMap = HashMap::new(); + let mut dec_mints: HashMap = HashMap::new(); for key in keys_changed { if let Some([token, _]) = is_any_token_balance_key(key) { let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); - let diff = post.change() - pre.change(); - match changes.get_mut(token) { - Some(change) => *change += diff, - None => _ = changes.insert(token, diff), + match post.checked_sub(pre) { + Some(diff) => match inc_changes.get_mut(token) { + Some(change) => { + change.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + inc_changes.insert(token.clone(), diff); + } + }, + None => { + let diff = pre + .checked_sub(post) + .expect("Underflow shouldn't happen here"); + match dec_changes.get_mut(token) { + Some(change) => { + change.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + dec_changes.insert(token.clone(), diff); + } + } + } } } else if let Some(token) = is_any_minted_balance_key(key) { let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); - let diff = post.change() - pre.change(); - match mints.get_mut(token) { - Some(mint) => *mint += diff, - None => _ = mints.insert(token, diff), + match post.checked_sub(pre) { + Some(diff) => match inc_mints.get_mut(token) { + Some(mint) => { + mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + inc_mints.insert(token.clone(), diff); + } + }, + None => { + let diff = pre + .checked_sub(post) + .expect("Underflow shouldn't happen here"); + match dec_mints.get_mut(token) { + Some(mint) => { + mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + dec_mints.insert(token.clone(), diff); + } + } + } } - // Check if the minter is set if !self.is_valid_minter(token, verifiers)? { return Ok(false); @@ -90,12 +151,31 @@ where } } - Ok(changes.iter().all(|(token, change)| { - let mint = match mints.get(token) { - Some(mint) => *mint, - None => Change::zero(), - }; - *change == mint + let mut all_tokens = BTreeSet::new(); + let _ = inc_changes.keys().map(|k| all_tokens.insert(k.clone())); + let _ = dec_changes.keys().map(|k| all_tokens.insert(k.clone())); + let _ = inc_mints.keys().map(|k| all_tokens.insert(k.clone())); + let _ = dec_mints.keys().map(|k| all_tokens.insert(k.clone())); + + Ok(all_tokens.iter().all(|token| { + let inc_change = + inc_changes.get(token).cloned().unwrap_or_default(); + let dec_change = + dec_changes.get(token).cloned().unwrap_or_default(); + let inc_mint = inc_mints.get(token).cloned().unwrap_or_default(); + let dec_mint = dec_mints.get(token).cloned().unwrap_or_default(); + + if inc_change >= dec_change && inc_mint >= dec_mint { + inc_change.checked_sub(dec_change) + == inc_mint.checked_sub(dec_mint) + } else if (inc_change < dec_change && inc_mint >= dec_mint) + || (inc_change >= dec_change && inc_mint < dec_mint) + { + false + } else { + dec_change.checked_sub(inc_change) + == dec_mint.checked_sub(inc_mint) + } })) } } From db5702b14e38868f39ecd1daf1ba5f677fece591 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 25 Jan 2024 12:05:26 +0100 Subject: [PATCH 2/4] fix all tokens --- crates/namada/src/ledger/native_vp/multitoken.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/multitoken.rs b/crates/namada/src/ledger/native_vp/multitoken.rs index adc7e6bbe8..afb96c5f81 100644 --- a/crates/namada/src/ledger/native_vp/multitoken.rs +++ b/crates/namada/src/ledger/native_vp/multitoken.rs @@ -152,10 +152,18 @@ where } let mut all_tokens = BTreeSet::new(); - let _ = inc_changes.keys().map(|k| all_tokens.insert(k.clone())); - let _ = dec_changes.keys().map(|k| all_tokens.insert(k.clone())); - let _ = inc_mints.keys().map(|k| all_tokens.insert(k.clone())); - let _ = dec_mints.keys().map(|k| all_tokens.insert(k.clone())); + inc_changes.keys().for_each(|k| { + all_tokens.insert(k.clone()); + }); + dec_changes.keys().for_each(|k| { + all_tokens.insert(k.clone()); + }); + inc_mints.keys().for_each(|k| { + all_tokens.insert(k.clone()); + }); + dec_mints.keys().for_each(|k| { + all_tokens.insert(k.clone()); + }); Ok(all_tokens.iter().all(|token| { let inc_change = From 5845bc13789acf9bb3119454d0649e9ebde8bd76 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 25 Jan 2024 14:25:04 +0100 Subject: [PATCH 3/4] add changelog --- .changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md diff --git a/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md b/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md new file mode 100644 index 0000000000..e9ec9aab31 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md @@ -0,0 +1,2 @@ +- Avoid diff overflow in Multitoken VP + ([\#2443](https://github.com/anoma/namada/issues/2443)) \ No newline at end of file From 7af076945581ed6bc32c02ef29873c6ff5d65579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 25 Jan 2024 14:49:16 +0000 Subject: [PATCH 4/4] vp/multitoken: refactor maps and sets usage --- .../namada/src/ledger/native_vp/multitoken.rs | 98 +++++++------------ 1 file changed, 37 insertions(+), 61 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/multitoken.rs b/crates/namada/src/ledger/native_vp/multitoken.rs index afb96c5f81..9ad76a5598 100644 --- a/crates/namada/src/ledger/native_vp/multitoken.rs +++ b/crates/namada/src/ledger/native_vp/multitoken.rs @@ -60,8 +60,10 @@ where let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); match post.checked_sub(pre) { - Some(diff) => match inc_changes.get_mut(token) { - Some(change) => { + Some(diff) => { + let change = + inc_changes.entry(token.clone()).or_default(); + *change = change.checked_add(diff).ok_or_else(|| { Error::NativeVpError( native_vp::Error::SimpleMessage( @@ -69,67 +71,49 @@ where ), ) })?; - } - None => { - inc_changes.insert(token.clone(), diff); - } - }, + } None => { let diff = pre .checked_sub(post) .expect("Underflow shouldn't happen here"); - match dec_changes.get_mut(token) { - Some(change) => { - change.checked_add(diff).ok_or_else(|| { - Error::NativeVpError( - native_vp::Error::SimpleMessage( - "Overflowed in balance check", - ), - ) - })?; - } - None => { - dec_changes.insert(token.clone(), diff); - } - } - } - } - } else if let Some(token) = is_any_minted_balance_key(key) { - let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); - let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); - match post.checked_sub(pre) { - Some(diff) => match inc_mints.get_mut(token) { - Some(mint) => { - mint.checked_add(diff).ok_or_else(|| { + let change = + dec_changes.entry(token.clone()).or_default(); + *change = + change.checked_add(diff).ok_or_else(|| { Error::NativeVpError( native_vp::Error::SimpleMessage( "Overflowed in balance check", ), ) })?; - } - None => { - inc_mints.insert(token.clone(), diff); - } - }, + } + } + } else if let Some(token) = is_any_minted_balance_key(key) { + let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); + let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); + match post.checked_sub(pre) { + Some(diff) => { + let mint = inc_mints.entry(token.clone()).or_default(); + *mint = mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } None => { let diff = pre .checked_sub(post) .expect("Underflow shouldn't happen here"); - match dec_mints.get_mut(token) { - Some(mint) => { - mint.checked_add(diff).ok_or_else(|| { - Error::NativeVpError( - native_vp::Error::SimpleMessage( - "Overflowed in balance check", - ), - ) - })?; - } - None => { - dec_mints.insert(token.clone(), diff); - } - } + let mint = dec_mints.entry(token.clone()).or_default(); + *mint = mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; } } // Check if the minter is set @@ -152,18 +136,10 @@ where } let mut all_tokens = BTreeSet::new(); - inc_changes.keys().for_each(|k| { - all_tokens.insert(k.clone()); - }); - dec_changes.keys().for_each(|k| { - all_tokens.insert(k.clone()); - }); - inc_mints.keys().for_each(|k| { - all_tokens.insert(k.clone()); - }); - dec_mints.keys().for_each(|k| { - all_tokens.insert(k.clone()); - }); + all_tokens.extend(inc_changes.keys().cloned()); + all_tokens.extend(dec_changes.keys().cloned()); + all_tokens.extend(inc_mints.keys().cloned()); + all_tokens.extend(dec_mints.keys().cloned()); Ok(all_tokens.iter().all(|token| { let inc_change =