diff --git a/core-processor/src/ext.rs b/core-processor/src/ext.rs index 7b822e339ee..c55d696b291 100644 --- a/core-processor/src/ext.rs +++ b/core-processor/src/ext.rs @@ -38,8 +38,8 @@ use gear_core::{ AllocError, AllocationsContext, GrowHandler, Memory, MemoryError, MemoryInterval, PageBuf, }, message::{ - ContextOutcomeDrain, ContextStore, Dispatch, GasLimit, HandlePacket, InitPacket, - MessageContext, Packet, ReplyPacket, + ContextOutcomeDrain, ContextStore, Dispatch, DispatchKind, GasLimit, HandlePacket, + InitPacket, MessageContext, Packet, ReplyPacket, }, pages::{ numerated::{interval::Interval, tree::IntervalsTree}, @@ -422,66 +422,6 @@ impl<'a, LP: LazyPagesInterface> ExtMutator<'a, LP> { Ok(()) } - // It's temporary fn, used to solve `core-audit/issue#22`. - fn safe_gasfull_sends( - &mut self, - packet: &T, - delay: u32, - ) -> Result<(), FallibleExtError> { - // In case of delayed sending from origin message we keep some gas - // for it while processing outgoing sending notes so gas for - // previously gasless sends should appear to prevent their - // invasion for gas for storing delayed message. - match (packet.gas_limit(), delay != 0) { - // Zero gasfull instant. - // - // In this case there is nothing to do. - (Some(0), false) => Ok(()), - - // Any non-zero gasfull or zero gasfull with delay. - // - // In case of zero gasfull with delay it's pretty similar to - // gasless with delay case. - // - // In case of any non-zero gasfull we prevent stealing for any - // previous gasless-es's thresholds from gas supposed to be - // sent with this `packet`. - (Some(_), _) => { - let prev_gasless_fee = self - .outgoing_gasless - .saturating_mul(self.ext.context.mailbox_threshold); - self.reduce_gas(prev_gasless_fee)?; - self.outgoing_gasless = 0; - Ok(()) - } - - // Gasless with delay. - // - // In this case we must give threshold for each uncovered gasless-es - // sent, otherwise they will steal gas from this `packet` that was - // supposed to pay for delay. - // - // It doesn't guarantee threshold for itself. - (None, true) => { - let prev_gasless_fee = self - .outgoing_gasless - .saturating_mul(self.ext.context.mailbox_threshold); - self.reduce_gas(prev_gasless_fee)?; - self.outgoing_gasless = 1; - Ok(()) - } - - // Gasless instant. - // - // In this case there is no need to give any thresholds for previous - // gasless-es: only counter should be increased. - (None, false) => { - self.outgoing_gasless = self.outgoing_gasless.saturating_add(1); - Ok(()) - } - } - } - fn mark_reservation_used( &mut self, reservation_id: ReservationId, @@ -506,21 +446,47 @@ impl<'a, LP: LazyPagesInterface> ExtMutator<'a, LP> { Ok(()) } - fn charge_expiring_resources( - &mut self, - packet: &T, - check_gas_limit: bool, - ) -> Result<(), FallibleExtError> { - let gas_limit = if check_gas_limit { - self.check_gas_limit(packet.gas_limit())? - } else { - packet.gas_limit().unwrap_or(0) - }; + fn charge_expiring_resources(&mut self, packet: &T) -> Result<(), FallibleExtError> { + let reducing_gas_limit = self.get_reducing_gas_limit(packet)?; - self.reduce_gas(gas_limit)?; + self.reduce_gas(reducing_gas_limit)?; self.charge_message_value(packet.value()) } + fn get_reducing_gas_limit(&self, packet: &T) -> Result { + match T::kind() { + DispatchKind::Handle => { + // Any "handle" gasless and gasful *non zero* message must + // cover mailbox threshold. That's because destination + // of the message is unknown, so it could be a user, + // and if gasless message is sent, there must be a + // guaranteed gas to cover mailbox. + let mailbox_threshold = self.context.mailbox_threshold; + let gas_limit = packet.gas_limit().unwrap_or(mailbox_threshold); + + // Zero gasful message is a special case. + if gas_limit != 0 && gas_limit < mailbox_threshold { + return Err(MessageError::InsufficientGasLimit.into()); + } + + Ok(gas_limit) + } + DispatchKind::Init | DispatchKind::Reply => { + // Init and reply messages never go to mailbox. + // + // For init case, even if there's no code with a provided + // code id, the init message still goes to queue and then is handled as non + // executable, as there is no code for the destination actor. + // + // Also no reply to user messages go to mailbox, they all are emitted + // within events. + + Ok(packet.gas_limit().unwrap_or(0)) + } + DispatchKind::Signal => unreachable!("Signals can't be sent as a syscall"), + } + } + fn charge_sending_fee(&mut self, delay: u32) -> Result<(), ChargeError> { if delay == 0 { self.charge_gas_if_enough(self.context.message_context.settings().sending_fee) @@ -720,18 +686,6 @@ impl Ext { Ok(result) } - fn check_gas_limit(&self, gas_limit: Option) -> Result { - let mailbox_threshold = self.context.mailbox_threshold; - let gas_limit = gas_limit.unwrap_or(0); - - // Sending gas should apply the range {0} ∪ [mailbox_threshold; +inf) - if gas_limit < mailbox_threshold && gas_limit != 0 { - Err(MessageError::InsufficientGasLimit.into()) - } else { - Ok(gas_limit) - } - } - /// Checking that reservation could be charged for /// dispatch stash with given delay. fn check_reservation_gas_limit_for_delayed_sending( @@ -978,8 +932,7 @@ impl Externalities for Ext { ) -> Result { self.with_changes(|mutator| { mutator.check_forbidden_destination(msg.destination())?; - mutator.safe_gasfull_sends(&msg, delay)?; - mutator.charge_expiring_resources(&msg, true)?; + mutator.charge_expiring_resources(&msg)?; mutator.charge_sending_fee(delay)?; mutator.charge_for_dispatch_stash_hold(delay)?; @@ -1029,8 +982,7 @@ impl Externalities for Ext { self.with_changes(|mutator| { mutator .check_forbidden_destination(mutator.context.message_context.reply_destination())?; - mutator.safe_gasfull_sends(&msg, 0)?; - mutator.charge_expiring_resources(&msg, false)?; + mutator.charge_expiring_resources(&msg)?; mutator.charge_sending_fee(0)?; mutator @@ -1360,9 +1312,10 @@ impl Externalities for Ext { ) -> Result<(MessageId, ProgramId), Self::FallibleError> { let ed = self.context.existential_deposit; self.with_changes(|mutator| { - // We don't check for forbidden destination here, since dest is always unique and almost impossible to match SYSTEM_ID - mutator.safe_gasfull_sends(&packet, delay)?; - mutator.charge_expiring_resources(&packet, true)?; + // We don't check for forbidden destination here, since dest is always unique + // and almost impossible to match SYSTEM_ID + + mutator.charge_expiring_resources(&packet)?; mutator.charge_sending_fee(delay)?; mutator.charge_for_dispatch_stash_hold(delay)?; @@ -1501,12 +1454,6 @@ mod tests { self } - fn with_mailbox_threshold(mut self, mailbox_threshold: u64) -> Self { - self.0.mailbox_threshold = mailbox_threshold; - - self - } - fn with_existential_deposit(mut self, ed: u128) -> Self { self.0.existential_deposit = ed; @@ -2109,73 +2056,6 @@ mod tests { )); } - #[test] - fn test_gasful_after_gasless() { - let gas = 1_000_000_000; - let mut ext = Ext::new( - ProcessorContextBuilder::new() - .with_message_context( - MessageContextBuilder::new() - .with_outgoing_limit(u32::MAX) - .build(), - ) - .with_gas(GasCounter::new(gas)) - .with_allowance(GasAllowanceCounter::new(gas)) - .with_mailbox_threshold(1000) - .build(), - ); - - // Sending some gasless messages - let gasless_packet = HandlePacket::new(ProgramId::zero(), Default::default(), 0); - assert!(ext.send(gasless_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 1); - assert!(ext.send(gasless_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 2); - assert!(ext.send(gasless_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 3); - - // Sending fee is zero - assert_eq!(ext.current_counter_value(), gas); - - // Now sending gasful message - let msg_gas = 1000; - let gasful_packet = - HandlePacket::new_with_gas(ProgramId::zero(), Default::default(), msg_gas, 0); - assert!(ext.send(gasful_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 0); - assert_eq!( - ext.current_counter_value(), - // reducing gas for the sent message and for mailbox threshold for each gasless - gas - msg_gas - 3 * ext.context.mailbox_threshold - ); - - // Sending another gasful - let gas = ext.current_counter_value(); - assert!(ext.send(gasful_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 0); - assert_eq!( - ext.current_counter_value(), - // reducing gas the sent message only - gas - msg_gas - ); - - // Sending some more gasless - assert!(ext.send(gasless_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 1); - assert!(ext.send(gasless_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 2); - - // And another gasful - let gas = ext.current_counter_value(); - assert!(ext.send(gasful_packet.clone(), 0).is_ok()); - assert_eq!(ext.outgoing_gasless, 0); - assert_eq!( - ext.current_counter_value(), - // reducing gas for the sent message and for mailbox threshold for each gasless - gas - msg_gas - 2 * ext.context.mailbox_threshold - ); - } - #[test] // This function tests: // diff --git a/core/src/message/handle.rs b/core/src/message/handle.rs index d2991a0673f..30f15aba964 100644 --- a/core/src/message/handle.rs +++ b/core/src/message/handle.rs @@ -196,4 +196,8 @@ impl Packet for HandlePacket { fn value(&self) -> Value { self.value } + + fn kind() -> DispatchKind { + DispatchKind::Handle + } } diff --git a/core/src/message/init.rs b/core/src/message/init.rs index ae2362bdcfa..d2483b52a6f 100644 --- a/core/src/message/init.rs +++ b/core/src/message/init.rs @@ -200,4 +200,8 @@ impl Packet for InitPacket { fn value(&self) -> Value { self.value } + + fn kind() -> DispatchKind { + DispatchKind::Init + } } diff --git a/core/src/message/mod.rs b/core/src/message/mod.rs index 476d84c80a6..f80399ad123 100644 --- a/core/src/message/mod.rs +++ b/core/src/message/mod.rs @@ -229,6 +229,9 @@ pub trait Packet { /// Packet value. fn value(&self) -> Value; + + /// A dispatch kind the will be generated from the packet. + fn kind() -> DispatchKind; } /// The struct contains results of read only send message RPC call. diff --git a/core/src/message/reply.rs b/core/src/message/reply.rs index 40896669946..2ee91b8723c 100644 --- a/core/src/message/reply.rs +++ b/core/src/message/reply.rs @@ -253,4 +253,8 @@ impl Packet for ReplyPacket { fn value(&self) -> Value { self.value } + + fn kind() -> DispatchKind { + DispatchKind::Reply + } } diff --git a/examples/constructor/src/builder.rs b/examples/constructor/src/builder.rs index fe1d1ffd078..cd2b2539ec7 100644 --- a/examples/constructor/src/builder.rs +++ b/examples/constructor/src/builder.rs @@ -239,10 +239,30 @@ impl Calls { } pub fn reply(self, payload: impl Into>>) -> Self { - self.add_call(Call::Reply(payload.into(), None, 0.into())) + self.reply_value(payload, 0) + } + + pub fn reply_value( + self, + payload: impl Into>>, + value: impl Into>, + ) -> Self { + self.add_call(Call::Reply(payload.into(), None, value.into())) } pub fn reply_wgas>(self, payload: impl Into>>, gas_limit: T) -> Self + where + T::Error: Debug, + { + self.reply_value_wgas(payload, gas_limit, 0) + } + + pub fn reply_value_wgas>( + self, + payload: impl Into>>, + gas_limit: T, + value: impl Into>, + ) -> Self where T::Error: Debug, { @@ -252,7 +272,7 @@ impl Calls { self.add_call(Call::Reply( payload.into(), Some(gas_limit.into()), - 0.into(), + value.into(), )) } diff --git a/pallets/gear/src/internal.rs b/pallets/gear/src/internal.rs index 5b2d1a8a66c..5927d2dc15d 100644 --- a/pallets/gear/src/internal.rs +++ b/pallets/gear/src/internal.rs @@ -23,7 +23,7 @@ use crate::{ GasBalanceOf, GasHandlerOf, GasNodeIdOf, GearBank, MailboxOf, Pallet, QueueOf, SchedulingCostOf, TaskPoolOf, WaitlistOf, }; -use alloc::collections::BTreeSet; +use alloc::{collections::BTreeSet, format}; use common::{ event::{ MessageWaitedReason, MessageWaitedRuntimeReason::*, MessageWokenReason, Reason::*, @@ -364,10 +364,26 @@ where // Validating duration. if hold.expected_duration().is_zero() { - // TODO: Replace with unreachable call after: - // - `HoldBound` safety usage stabilized; - log::error!("Failed to figure out correct wait hold bound"); - return; + let gas_limit = GasHandlerOf::::get_limit(dispatch.id()).unwrap_or_else(|e| { + let err_msg = format!( + "wait_dispatch: failed getting message gas limit. Message id - {message_id}. \ + Got error - {e:?}", + message_id = dispatch.id() + ); + + log::error!("{err_msg}"); + unreachable!("{err_msg}"); + }); + + let err_msg = format!( + "wait_dispatch: message got zero duration hold bound for waitlist. \ + Requested duration - {duration:?}, gas limit - {gas_limit}, \ + wait reason - {reason:?}, message id - {}.", + dispatch.id(), + ); + + log::error!("{err_msg}"); + unreachable!("{err_msg}"); } // Locking funds for holding. diff --git a/pallets/gear/src/manager/journal.rs b/pallets/gear/src/manager/journal.rs index 41d0df4cfb6..b9c7fd8a2a3 100644 --- a/pallets/gear/src/manager/journal.rs +++ b/pallets/gear/src/manager/journal.rs @@ -407,6 +407,14 @@ where "No referencing code with code hash {:?} for candidate programs", code_id ); + // SAFETY: + // Do not remove insertion into programs map as it gives guarantee + // that init message for destination with no code won't enter + // the mailbox (so no possible uncovered gas charges which leads to panic). + // Such message will be inserted into the queue and later processed as + // non executable. + // + // Test for it - `test_create_program_no_code_hash`. for (_, candidate) in candidates { self.programs.insert(candidate); } diff --git a/pallets/gear/src/tests.rs b/pallets/gear/src/tests.rs index c7472b50fc7..f4da02207a3 100644 --- a/pallets/gear/src/tests.rs +++ b/pallets/gear/src/tests.rs @@ -385,7 +385,9 @@ fn cascading_delayed_gasless_send_work() { ) .expect("calculate_gas_info failed"); - // Case when one of two goes into mailbox. + // Case when trying to send one of them to mailbox. + // Fails as any gasless message sending costs reducing + // mailbox threshold from the gas counter assert_ok!(Gear::send_message( RuntimeOrigin::signed(USER_1), pid, @@ -397,16 +399,12 @@ fn cascading_delayed_gasless_send_work() { let mid = get_last_message_id(); - let first_outgoing = MessageId::generate_outgoing(mid, 0); - let second_outgoing = MessageId::generate_outgoing(mid, 1); - run_to_next_block(None); - assert_succeed(mid); - - run_for_blocks(DELAY as u64, None); - assert!(MailboxOf::::contains(&USER_1, &first_outgoing)); - assert!(!MailboxOf::::contains(&USER_1, &second_outgoing)); + assert_failed( + mid, + ActorExecutionErrorReplyReason::Trap(TrapExplanation::GasLimitExceeded), + ); // Similar case when two of two goes into mailbox. assert_ok!(Gear::send_message( @@ -1202,7 +1200,6 @@ fn reply_deposit_to_user_auto_reply() { init_logger(); let checker = USER_1; - // To user case. new_test_ext().execute_with(|| { let (_init_mid, constructor) = init_constructor(demo_reply_deposit::scheme( @@ -1221,6 +1218,7 @@ fn reply_deposit_to_user_auto_reply() { )); run_to_next_block(None); + // 1 init + 1 handle + 1 auto reply assert_total_dequeued(3); assert!(!MailboxOf::::is_empty(&checker)); @@ -1253,6 +1251,7 @@ fn reply_deposit_panic_in_handle_reply() { )); run_to_next_block(None); + // 1 init + 1 handle + 1 auto reply assert_total_dequeued(3); assert!(MailboxOf::::is_empty(&checker)); @@ -3024,69 +3023,57 @@ fn mailbox_sending_instant_transfer() { new_test_ext().execute_with(|| { let (_init_mid, sender) = init_constructor_with_value(Scheme::empty(), 10_000); - // Message doesn't add to mailbox. - // - // For both cases value moves to destination user instantly. - let cases = [ - // Zero gas for gasful sending. - (Some(0), 1_000), - // Gasless message. - (None, 3_000), - ]; + // Message with 0 gas limit is not added to mailbox. + let gas_limit = 0; + let value = 1_000; - for (gas_limit, value) in cases { - let user_1_balance = Balances::free_balance(USER_1); - assert_eq!(GearBank::::account_total(&USER_1), 0); + let user_1_balance = Balances::free_balance(USER_1); + assert_eq!(GearBank::::account_total(&USER_1), 0); - let user_2_balance = Balances::free_balance(USER_2); - assert_eq!(GearBank::::account_total(&USER_2), 0); + let user_2_balance = Balances::free_balance(USER_2); + assert_eq!(GearBank::::account_total(&USER_2), 0); - let prog_balance = Balances::free_balance(sender.cast::()); - assert_eq!(GearBank::::account_total(&sender.cast()), 0); + let prog_balance = Balances::free_balance(sender.cast::()); + assert_eq!(GearBank::::account_total(&sender.cast()), 0); - let payload = if let Some(gas_limit) = gas_limit { - TestData::gasful(gas_limit, value) - } else { - TestData::gasless(value, ::MailboxThreshold::get()) - }; + let payload = TestData::gasful(gas_limit, value); - // Used like that, because calculate gas info always provides - // message into mailbox while sending without gas. - let gas_info = Gear::calculate_gas_info( - USER_1.into_origin(), - HandleKind::Handle(sender), - payload.request(USER_2.into_origin()).encode(), - 0, - true, - true, - ) - .expect("calculate_gas_info failed"); + // Used like that, because calculate gas info always provides + // message into mailbox while sending without gas. + let gas_info = Gear::calculate_gas_info( + USER_1.into_origin(), + HandleKind::Handle(sender), + payload.request(USER_2.into_origin()).encode(), + 0, + true, + true, + ) + .expect("calculate_gas_info failed"); - assert_ok!(Gear::send_message( - RuntimeOrigin::signed(USER_1), - sender, - payload.request(USER_2.into_origin()).encode(), - gas_info.burned + gas_limit.unwrap_or_default(), - 0, - false, - )); + assert_ok!(Gear::send_message( + RuntimeOrigin::signed(USER_1), + sender, + payload.request(USER_2.into_origin()).encode(), + gas_info.burned + gas_limit, + 0, + false, + )); - utils::assert_balance( - USER_1, - user_1_balance - gas_price(gas_info.burned + gas_limit.unwrap_or_default()), - gas_price(gas_info.burned + gas_limit.unwrap_or_default()), - ); - utils::assert_balance(USER_2, user_2_balance, 0u128); - utils::assert_balance(sender, prog_balance, 0u128); - assert!(MailboxOf::::is_empty(&USER_2)); + utils::assert_balance( + USER_1, + user_1_balance - gas_price(gas_info.burned + gas_limit), + gas_price(gas_info.burned + gas_limit), + ); + utils::assert_balance(USER_2, user_2_balance, 0u128); + utils::assert_balance(sender, prog_balance, 0u128); + assert!(MailboxOf::::is_empty(&USER_2)); - run_to_next_block(None); + run_to_next_block(None); - utils::assert_balance(USER_1, user_1_balance - gas_price(gas_info.burned), 0u128); - utils::assert_balance(USER_2, user_2_balance + value, 0u128); - utils::assert_balance(sender, prog_balance - value, 0u128); - assert!(MailboxOf::::is_empty(&USER_2)); - } + utils::assert_balance(USER_1, user_1_balance - gas_price(gas_info.burned), 0u128); + utils::assert_balance(USER_2, user_2_balance + value, 0u128); + utils::assert_balance(sender, prog_balance - value, 0u128); + assert!(MailboxOf::::is_empty(&USER_2)); }); } @@ -15580,6 +15567,63 @@ fn dust_in_message_to_user_handled_ok() { }); } +#[test] +fn test_gasless_steal_gas_for_wait() { + init_logger(); + new_test_ext().execute_with(|| { + use demo_constructor::{Arg, Calls, Scheme}; + + let wait_duration = 10; + let handle = Calls::builder() + .source("source_store") + .send("source_store", Arg::new("msg1".encode())) + .send("source_store", Arg::new("msg2".encode())) + .send("source_store", Arg::new("msg3".encode())) + .send("source_store", Arg::new("msg4".encode())) + .send("source_store", Arg::new("msg5".encode())) + .wait_for(wait_duration); + let scheme = Scheme::predefined( + Calls::builder().noop(), + handle, + Calls::builder().noop(), + Calls::builder().noop(), + ); + + let (_, pid) = init_constructor(scheme); + let GasInfo { min_limit, .. } = Gear::calculate_gas_info( + USER_1.into_origin(), + HandleKind::Handle(pid), + Default::default(), + 0, + true, + true, + ) + .expect("calculate_gas_info failed"); + let waiting_bound = HoldBoundBuilder::::new(StorageType::Waitlist) + .duration(wait_duration.unique_saturated_into()); + + assert_ok!(Gear::send_message( + RuntimeOrigin::signed(USER_1), + pid, + Default::default(), + min_limit - waiting_bound.lock_amount(), + 0, + true + )); + let mid = get_last_message_id(); + + run_to_next_block(None); + + assert_failed( + mid, + ActorExecutionErrorReplyReason::Trap(TrapExplanation::UnrecoverableExt( + UnrecoverableExtError::Execution(UnrecoverableExecutionError::NotEnoughGas), + )), + ); + assert_eq!(MailboxOf::::len(&USER_1), 0); + }) +} + pub(crate) mod utils { #![allow(unused)]