Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SMO instruction take data ptr as an argument #404

Merged
merged 4 commits into from
Mar 22, 2023
Merged

Make SMO instruction take data ptr as an argument #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0ecfa8
Make SMO instruction take data ptr as an argument
Dentosal Mar 21, 2023
7112d8d
Remove todo comment
Dentosal Mar 21, 2023
18bb8bb
Add tests, fix issues, cleanup
Dentosal Mar 21, 2023
6b295c9
Add a new error type MessageDataTooLong
Dentosal Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion fuel-asm/src/panic_reason.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ pub enum PanicReason {
ContractIdAlreadyDeployed = 0x20,
/// The loaded contract mismatch expectations.
ContractMismatch = 0x21,
/// Attempting to send message data longer than `MAX_MESSAGE_DATA_LENGTH`
MessageDataTooLong = 0x22,
/// The byte can't be mapped to any known `PanicReason`.
UnknownPanicReason = 0x23,
}
Expand Down Expand Up @@ -137,6 +139,7 @@ impl From<u8> for PanicReason {
0x1f => IllegalJump,
0x20 => ContractIdAlreadyDeployed,
0x21 => ContractMismatch,
0x22 => MessageDataTooLong,
_ => UnknownPanicReason,
}
}
Expand All @@ -163,7 +166,7 @@ mod tests {

#[test]
fn test_u8_panic_reason_round_trip() {
const LAST_PANIC_REASON: u8 = 0x22;
const LAST_PANIC_REASON: u8 = 0x23;
for i in 0..LAST_PANIC_REASON {
let reason = PanicReason::from(i);
let i2 = reason as u8;
Expand Down
37 changes: 16 additions & 21 deletions fuel-vm/src/interpreter/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,8 @@ where
fp: fp.as_ref(),
pc,
recipient_mem_address: a,
call_abi_len: b,
message_output_idx: c,
msg_data_ptr: b,
msg_data_len: c,
amount_coins_to_send: d,
};
input.message_output()
Expand Down Expand Up @@ -667,12 +667,6 @@ pub(crate) fn timestamp(

inc_pc(pc)
}

// TODO: Register b is the address of the begin of the data and register c
// is the end of the data(Based on the specification).
// The user doesn't specify the output index for message.
// We need to use the index of the receipt to calculate the `Nonce`.
// Add unit tests for a new usage.
struct MessageOutputCtx<'vm, Tx> {
max_message_data_length: u64,
memory: &'vm mut [u8; MEM_SIZE],
Expand All @@ -685,10 +679,9 @@ struct MessageOutputCtx<'vm, Tx> {
/// A
recipient_mem_address: Word,
/// B
call_abi_len: Word,
msg_data_ptr: Word,
/// C
#[allow(dead_code)]
message_output_idx: Word,
msg_data_len: Word,
/// D
amount_coins_to_send: Word,
}
Expand All @@ -699,14 +692,16 @@ impl<Tx> MessageOutputCtx<'_, Tx> {
Tx: ExecutableTransaction,
{
let recipient_address = CheckedMemValue::<Address>::new::<{ Address::LEN }>(self.recipient_mem_address)?;
let memory_constraint = recipient_address.end() as Word
..(arith::checked_add_word(recipient_address.end() as Word, self.max_message_data_length)?
.min(MEM_MAX_ACCESS_SIZE));
let call_abi = CheckedMemRange::new_with_constraint(
recipient_address.end() as Word,
self.call_abi_len as usize,
memory_constraint,
)?;

if self.msg_data_len > MEM_MAX_ACCESS_SIZE {
return Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow));
}

if self.msg_data_len > self.max_message_data_length {
return Err(RuntimeError::Recoverable(PanicReason::MessageDataTooLong));
}

let msg_data_range = CheckedMemRange::new(self.msg_data_ptr, self.msg_data_len as usize)?;

let recipient = recipient_address.try_from(self.memory)?;

Expand All @@ -716,7 +711,7 @@ impl<Tx> MessageOutputCtx<'_, Tx> {

let sender = CheckedMemConstLen::<{ Address::LEN }>::new(*self.fp)?;
let txid = tx_id(self.memory);
let call_abi = call_abi.read(self.memory).to_vec();
let msg_data = msg_data_range.read(self.memory).to_vec();
let sender = Address::from_bytes_ref(sender.read(self.memory));

let receipt = Receipt::message_out_from_tx_output(
Expand All @@ -725,7 +720,7 @@ impl<Tx> MessageOutputCtx<'_, Tx> {
*sender,
recipient,
self.amount_coins_to_send,
call_abi,
msg_data,
);

append_receipt(
Expand Down
50 changes: 25 additions & 25 deletions fuel-vm/src/interpreter/blockchain/smo_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ struct Input {
/// A
recipient_mem_address: Word,
/// B
call_abi_len: Word,
msg_data_ptr: Word,
/// C
message_output_idx: Word,
msg_data_len: Word,
/// D
amount_coins_to_send: Word,
max_message_data_length: Word,
Expand All @@ -28,8 +28,8 @@ impl Default for Input {
fn default() -> Self {
Self {
recipient_mem_address: Default::default(),
call_abi_len: Default::default(),
message_output_idx: Default::default(),
msg_data_ptr: Default::default(),
msg_data_len: Default::default(),
amount_coins_to_send: Default::default(),
memory: vec![(400, Address::from([1u8; 32]).to_vec())],
max_message_data_length: 100,
Expand All @@ -41,8 +41,8 @@ impl Default for Input {
#[test_case(
Input {
recipient_mem_address: 400,
call_abi_len: 1,
message_output_idx: 0,
msg_data_ptr: 0,
msg_data_len: 1,
amount_coins_to_send: 0,
..Default::default()
} => matches Ok(Output { .. })
Expand All @@ -51,18 +51,18 @@ impl Default for Input {
#[test_case(
Input {
recipient_mem_address: 0,
call_abi_len: 0,
message_output_idx: 0,
msg_data_ptr: 0,
msg_data_len: 0,
amount_coins_to_send: 0,
..Default::default()
} => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow))
; "Call abi can't be zero"
)]
#[test_case(
Input {
recipient_mem_address: Word::MAX,
call_abi_len: 1,
message_output_idx: 0,
recipient_mem_address: 0,
msg_data_ptr: Word::MAX,
msg_data_len: 1,
amount_coins_to_send: 0,
max_message_data_length: Word::MAX,
..Default::default()
Expand All @@ -71,31 +71,31 @@ impl Default for Input {
)]
#[test_case(
Input {
recipient_mem_address: VM_MAX_RAM - 64,
call_abi_len: 100,
message_output_idx: 0,
recipient_mem_address: 0,
msg_data_ptr: VM_MAX_RAM - 64,
msg_data_len: 100,
amount_coins_to_send: 0,
max_message_data_length: Word::MAX,
..Default::default()
} => Err(RuntimeError::Recoverable(PanicReason::ArithmeticOverflow))
} => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow))
; "address + call abi length overflows memory"
)]
#[test_case(
Input {
recipient_mem_address: 400,
call_abi_len: 101,
message_output_idx: 0,
recipient_mem_address: 0,
msg_data_ptr: 0,
msg_data_len: 101,
amount_coins_to_send: 0,
max_message_data_length: 100,
..Default::default()
} => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow))
} => Err(RuntimeError::Recoverable(PanicReason::MessageDataTooLong))
; "call abi length > max message data length"
)]
#[test_case(
Input {
recipient_mem_address: 400,
call_abi_len: 10,
message_output_idx: 0,
msg_data_ptr: 0,
msg_data_len: 10,
amount_coins_to_send: 30,
balance: [(AssetId::zeroed(), 29)].into_iter().collect(),
..Default::default()
Expand All @@ -106,8 +106,8 @@ impl Default for Input {
fn test_smo(
Input {
recipient_mem_address,
call_abi_len,
message_output_idx,
msg_data_len,
msg_data_ptr,
amount_coins_to_send,
memory: mem,
max_message_data_length,
Expand All @@ -133,8 +133,8 @@ fn test_smo(
fp: Reg::new(&fp),
pc: RegMut::new(&mut pc),
recipient_mem_address,
call_abi_len,
message_output_idx,
msg_data_len,
msg_data_ptr,
amount_coins_to_send,
};
input.message_output().map(|_| Output { receipts })
Expand Down
2 changes: 1 addition & 1 deletion fuel-vm/src/interpreter/executors/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ where

Instruction::SMO(smo) => {
let (a, b, c, d) = smo.unpack();
self.dependent_gas_charge(self.gas_costs.smo, r!(b))?;
self.dependent_gas_charge(self.gas_costs.smo, r!(c))?;
self.message_output(r!(a), r!(b), r!(c), r!(d))?;
}

Expand Down
25 changes: 19 additions & 6 deletions fuel-vm/src/tests/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1319,13 +1319,21 @@ fn smo_instruction_works() {
let secret = SecretKey::random(rng);
let sender = rng.gen();

// Two bytes of random data to send as the message
let msg_data = [rng.gen::<u8>(), rng.gen::<u8>()];

#[rustfmt::skip]
let script = vec![
Dentosal marked this conversation as resolved.
Show resolved Hide resolved
op::movi(0x10, 2), // data buffer allocation size
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe with script_with_data_offset! we may do it more readable=D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not script_data, instead I just allocate heap space for the bytes. Do you think I should rather use script_data for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a script transaction, and we can pass as much data as we wish=) With script_with_data_offset!, we can use more than 2 bytes and pass them with fewer opcodes(it will be more readable than putting one byte into hp and another into hp + 1).

Plus, we can easy transform this test into rtest or test_case and make a test case with huge data, later=)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't use script_with_data_offset anymore now that we have op::gtf to find the start of the script data.

op::aloc(0x10), // allocate
op::movi(0x10, msg_data[0].into()), // first message byte
op::sb(RegId::HP, 0x10, 0), // store above to the message buffer
op::movi(0x10, msg_data[1].into()), // second message byte
op::sb(RegId::HP, 0x10, 1), // store above to the message buffer
op::movi(0x10, 0), // set the txid as recipient
op::movi(0x11, 1), // send the whole data buffer
op::movi(0x12, 0), // tx output idx
op::movi(0x12, 2), // one byte of data
op::movi(0x13, message_output_amount as Immediate24), // expected output amount
op::smo(0x10,0x11,0x12,0x13),
op::smo(0x10,RegId::HP,0x12,0x13),
op::ret(RegId::ONE)
];

Expand Down Expand Up @@ -1363,15 +1371,20 @@ fn smo_instruction_works() {
});

let state = client.state_transition().expect("tx was executed");
// TODO: Add check for the `data` field too, but it requires fixing of the `smo` behaviour.
let message_receipt = state.receipts().iter().find_map(|o| match o {
Receipt::MessageOut { recipient, amount, .. } => Some((*recipient, *amount)),
Receipt::MessageOut {
recipient,
amount,
data,
..
} => Some((*recipient, *amount, data.clone())),
_ => None,
});
assert_eq!(message_receipt.is_some(), success);
if let Some((recipient, transferred)) = message_receipt {
if let Some((recipient, transferred, data)) = message_receipt {
assert_eq!(txid.as_ref(), recipient.as_ref());
assert_eq!(message_output_amount, transferred);
assert_eq!(data, msg_data);
}
// get gas used from script result
let gas_used = if let Receipt::ScriptResult { gas_used, .. } = state.receipts().last().unwrap() {
Expand Down