From 8ac8b67c5c72c4336a3f46aeafca7935a04c2d99 Mon Sep 17 00:00:00 2001 From: swimricky Date: Wed, 26 Apr 2023 08:28:57 -0700 Subject: [PATCH 1/2] fix(message-buffer): address PR feedback from 779, update tests Add new unit test for reading with cursor, update test setup --- message_buffer/Cargo.lock | 1 + .../programs/message_buffer/Cargo.toml | 3 + .../src/instructions/create_buffer.rs | 3 + .../src/instructions/delete_buffer.rs | 3 +- .../message_buffer/src/instructions/mod.rs | 41 +--- .../src/instructions/put_all.rs | 3 +- .../src/instructions/resize_buffer.rs | 8 +- .../src/state/message_buffer.rs | 177 ++++++++++++++---- 8 files changed, 157 insertions(+), 82 deletions(-) diff --git a/message_buffer/Cargo.lock b/message_buffer/Cargo.lock index 5631a2392e..e8804e3ef3 100644 --- a/message_buffer/Cargo.lock +++ b/message_buffer/Cargo.lock @@ -891,6 +891,7 @@ version = "0.1.0" dependencies = [ "anchor-lang", "bytemuck", + "byteorder", ] [[package]] diff --git a/message_buffer/programs/message_buffer/Cargo.toml b/message_buffer/programs/message_buffer/Cargo.toml index 3ea49f8b2f..b2a2b3adf4 100644 --- a/message_buffer/programs/message_buffer/Cargo.toml +++ b/message_buffer/programs/message_buffer/Cargo.toml @@ -19,3 +19,6 @@ default = [] anchor-lang = { version = "0.27.0" } # needed for the new #[account(zero_copy)] in anchor 0.27.0 bytemuck = { version = "1.4.0", features = ["derive", "min_const_generics"]} + +[dev-dependencies] +byteorder = "1.4.3" diff --git a/message_buffer/programs/message_buffer/src/instructions/create_buffer.rs b/message_buffer/programs/message_buffer/src/instructions/create_buffer.rs index d16c46e8b2..28c73fd9f4 100644 --- a/message_buffer/programs/message_buffer/src/instructions/create_buffer.rs +++ b/message_buffer/programs/message_buffer/src/instructions/create_buffer.rs @@ -75,6 +75,9 @@ pub fn create_buffer<'info>( *message_buffer = MessageBuffer::new(bump); } loader.exit(&crate::ID)?; + } else { + // FIXME: change this to be emit!(Event) + msg!("Buffer account already initialized"); } Ok(()) diff --git a/message_buffer/programs/message_buffer/src/instructions/delete_buffer.rs b/message_buffer/programs/message_buffer/src/instructions/delete_buffer.rs index c32eedc6fd..e873a7812f 100644 --- a/message_buffer/programs/message_buffer/src/instructions/delete_buffer.rs +++ b/message_buffer/programs/message_buffer/src/instructions/delete_buffer.rs @@ -1,6 +1,5 @@ use { crate::{ - instructions::verify_message_buffer, state::*, MessageBufferError, MESSAGE, @@ -23,7 +22,7 @@ pub fn delete_buffer<'info>( .whitelist .is_allowed_program_auth(&allowed_program_auth)?; - verify_message_buffer(message_buffer_account_info)?; + MessageBuffer::check_discriminator(message_buffer_account_info)?; let expected_key = Pubkey::create_program_address( &[ diff --git a/message_buffer/programs/message_buffer/src/instructions/mod.rs b/message_buffer/programs/message_buffer/src/instructions/mod.rs index 30bd8e4ba5..f40ee00efb 100644 --- a/message_buffer/programs/message_buffer/src/instructions/mod.rs +++ b/message_buffer/programs/message_buffer/src/instructions/mod.rs @@ -1,13 +1,6 @@ -use { - crate::{ - state::MessageBuffer, - MessageBufferError, - }, - anchor_lang::{ - prelude::*, - system_program, - Discriminator, - }, +use anchor_lang::{ + prelude::*, + system_program, }; pub use { create_buffer::*, @@ -23,34 +16,12 @@ mod put_all; mod resize_buffer; // String constants for deriving PDAs. +// // An authorized program's message buffer will have PDA seeds [authorized_program_pda, MESSAGE, base_account_key], -// where authorized_program_pda is the +// where authorized_program_pda is the where `allowed_program_auth` +// is the whitelisted pubkey who authorized this call. pub const MESSAGE: &str = "message"; -pub const FUND: &str = "fund"; - pub fn is_uninitialized_account(ai: &AccountInfo) -> bool { ai.data_is_empty() && ai.owner == &system_program::ID } - -/// Verify message buffer account is initialized and has the correct discriminator. -/// -/// Note: manually checking because using anchor's `AccountLoader.load()` -/// will panic since the `AccountInfo.data_len()` will not match the -/// size of the `MessageBuffer` since the `MessageBuffer` struct does not -/// include the messages. -pub fn verify_message_buffer(message_buffer_account_info: &AccountInfo) -> Result<()> { - if is_uninitialized_account(message_buffer_account_info) { - return err!(MessageBufferError::MessageBufferUninitialized); - } - let data = message_buffer_account_info.try_borrow_data()?; - if data.len() < MessageBuffer::discriminator().len() { - return Err(ErrorCode::AccountDiscriminatorNotFound.into()); - } - - let disc_bytes = &data[0..8]; - if disc_bytes != &MessageBuffer::discriminator() { - return Err(ErrorCode::AccountDiscriminatorMismatch.into()); - } - Ok(()) -} diff --git a/message_buffer/programs/message_buffer/src/instructions/put_all.rs b/message_buffer/programs/message_buffer/src/instructions/put_all.rs index c24bec9aa1..1ea70af834 100644 --- a/message_buffer/programs/message_buffer/src/instructions/put_all.rs +++ b/message_buffer/programs/message_buffer/src/instructions/put_all.rs @@ -1,6 +1,5 @@ use { crate::{ - instructions::verify_message_buffer, state::*, MessageBufferError, }, @@ -20,7 +19,7 @@ pub fn put_all<'info>( .first() .ok_or(MessageBufferError::MessageBufferNotProvided)?; - verify_message_buffer(message_buffer_account_info)?; + MessageBuffer::check_discriminator(message_buffer_account_info)?; let account_data = &mut message_buffer_account_info.try_borrow_mut_data()?; let header_end_index = mem::size_of::() + 8; diff --git a/message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs b/message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs index ff009ac152..cd34a102c9 100644 --- a/message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs +++ b/message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs @@ -1,6 +1,5 @@ use { crate::{ - instructions::verify_message_buffer, state::*, MessageBufferError, MESSAGE, @@ -30,7 +29,7 @@ pub fn resize_buffer<'info>( ctx.accounts .whitelist .is_allowed_program_auth(&allowed_program_auth)?; - verify_message_buffer(message_buffer_account_info)?; + MessageBuffer::check_discriminator(message_buffer_account_info)?; require_gte!( target_size, @@ -62,7 +61,10 @@ pub fn resize_buffer<'info>( MessageBufferError::InvalidPDA ); - if target_size_delta > 0 { + // allow for delta == 0 in case Rent requirements have changed + // and additional lamports need to be transferred. + // the realloc step will be a no-op in this case. + if target_size_delta >= 0 { let target_rent = Rent::get()?.minimum_balance(target_size); if message_buffer_account_info.lamports() < target_rent { system_program::transfer( diff --git a/message_buffer/programs/message_buffer/src/state/message_buffer.rs b/message_buffer/programs/message_buffer/src/state/message_buffer.rs index 5c4e6846e8..e5feb97180 100644 --- a/message_buffer/programs/message_buffer/src/state/message_buffer.rs +++ b/message_buffer/programs/message_buffer/src/state/message_buffer.rs @@ -1,9 +1,13 @@ use { crate::{ accumulator_input_seeds, + instructions, MessageBufferError, }, - anchor_lang::prelude::*, + anchor_lang::{ + prelude::*, + Discriminator, + }, }; /// A MessageBuffer will have the following structure @@ -121,6 +125,28 @@ impl MessageBuffer { require_keys_eq!(expected_key, key); Ok(()) } + + /// Verify message buffer account is initialized and has the correct discriminator. + /// + /// Note: manually checking because using anchor's `AccountLoader.load()` + /// will panic since the `AccountInfo.data_len()` will not match the + /// size of the `MessageBuffer` since the `MessageBuffer` struct does not + /// include the messages. + pub fn check_discriminator(message_buffer_account_info: &AccountInfo) -> Result<()> { + if instructions::is_uninitialized_account(message_buffer_account_info) { + return err!(MessageBufferError::MessageBufferUninitialized); + } + let data = message_buffer_account_info.try_borrow_data()?; + if data.len() < MessageBuffer::discriminator().len() { + return Err(ErrorCode::AccountDiscriminatorNotFound.into()); + } + + let disc_bytes = &data[0..8]; + if disc_bytes != &MessageBuffer::discriminator() { + return Err(ErrorCode::AccountDiscriminatorMismatch.into()); + } + Ok(()) + } } #[cfg(test)] @@ -171,23 +197,27 @@ mod test { let message_buffer = &mut MessageBuffer::new(0); let header_len = message_buffer.header_len as usize; - let message_buffer_bytes = bytes_of_mut(message_buffer); - // assuming account_info.data.len() == 10KB - let messages = &mut vec![0u8; 10_240 - header_len]; let account_info_data = &mut vec![]; let discriminator = &mut sighash("accounts", "MessageBuffer"); + let destination = &mut vec![0u8; 10_240 - header_len]; + + account_info_data.write_all(discriminator).unwrap(); - account_info_data.write_all(message_buffer_bytes).unwrap(); account_info_data - .write_all(messages.as_mut_slice()) + .write_all(bytes_of_mut(message_buffer)) .unwrap(); + account_info_data.write_all(destination).unwrap(); + + + let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); + let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); - let _account_data_len = account_info_data.len(); + let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(body_bytes, &data_bytes); - let destination = &mut account_info_data[(message_buffer.header_len as usize)..]; - let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(destination, &data_bytes); + let message_buffer: &MessageBuffer = + bytemuck::from_bytes(&account_info_data.as_slice()[8..header_len]); assert_eq!(num_msgs, 2); assert_eq!(num_bytes, 5); @@ -197,26 +227,22 @@ mod test { assert_eq!(message_buffer.end_offsets[1], 5); - // let account_data = bytes_of(accumulator_input); - - - // // The header_len field represents the size of all data prior to the message bytes. - // // This includes the account discriminator, which is not part of the header struct. - // // Subtract the size of the discriminator (8 bytes) to compensate - // let header_len = message_buffer.header_len as usize - 8; - let header_len = message_buffer.header_len as usize; - - let iter = message_buffer.end_offsets.iter().take_while(|x| **x != 0); let mut start = header_len; let mut data_iter = data_bytes.iter(); + let read_data = &mut vec![]; for offset in iter { let end_offset = header_len + *offset as usize; let message_buffer_data = &account_info_data[start..end_offset]; - let expected_data = data_iter.next().unwrap(); - assert_eq!(message_buffer_data, expected_data.as_slice()); + read_data.push(message_buffer_data); start = end_offset; } + println!("read_data: {:?}", read_data); + assert_eq!(read_data.len(), num_msgs); + for d in read_data.iter() { + let expected_data = data_iter.next().unwrap(); + assert_eq!(d, &expected_data.as_slice()); + } } #[test] @@ -227,23 +253,27 @@ mod test { let message_buffer = &mut MessageBuffer::new(0); let header_len = message_buffer.header_len as usize; - let message_buffer_bytes = bytes_of_mut(message_buffer); - // assuming account_info.data.len() == 10KB - let messages = &mut vec![0u8; 10_240 - header_len]; let account_info_data = &mut vec![]; let discriminator = &mut sighash("accounts", "MessageBuffer"); + let destination = &mut vec![0u8; 10_240 - header_len]; + + account_info_data.write_all(discriminator).unwrap(); - account_info_data.write_all(message_buffer_bytes).unwrap(); account_info_data - .write_all(messages.as_mut_slice()) + .write_all(bytes_of_mut(message_buffer)) .unwrap(); + account_info_data.write_all(destination).unwrap(); + - let _account_data_len = account_info_data.len(); + let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); + let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); - let destination = &mut account_info_data[(message_buffer.header_len as usize)..]; + let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(body_bytes, &data_bytes); - let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(destination, &data_bytes); + + let message_buffer: &MessageBuffer = + bytemuck::from_bytes(&account_info_data.as_slice()[8..header_len]); assert_eq!(num_msgs, 2); assert_eq!( @@ -277,29 +307,31 @@ mod test { ]; let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); - // let message_buffer = &mut MessageBufferTemp::new(0); - // let (num_msgs, num_bytes) = message_buffer.put_all(&data_bytes); let message_buffer = &mut MessageBuffer::new(0); let header_len = message_buffer.header_len as usize; - let message_buffer_bytes = bytes_of_mut(message_buffer); - // assuming account_info.data.len() == 10KB - let messages = &mut vec![0u8; 10_240 - header_len]; - let account_info_data = &mut vec![]; let discriminator = &mut sighash("accounts", "MessageBuffer"); + let destination = &mut vec![0u8; 10_240 - header_len]; + + account_info_data.write_all(discriminator).unwrap(); - account_info_data.write_all(message_buffer_bytes).unwrap(); account_info_data - .write_all(messages.as_mut_slice()) + .write_all(bytes_of_mut(message_buffer)) .unwrap(); + account_info_data.write_all(destination).unwrap(); - let _account_data_len = account_info_data.len(); - let destination = &mut account_info_data[(message_buffer.header_len as usize)..]; + let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); + let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); + + let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(body_bytes, &data_bytes); + + + let message_buffer: &MessageBuffer = + bytemuck::from_bytes(&account_info_data.as_slice()[8..header_len]); - let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(destination, &data_bytes); assert_eq!(num_msgs, 3); assert_eq!( @@ -325,4 +357,69 @@ mod test { assert_eq!(message_buffer.end_offsets[3], 0); assert_eq!(message_buffer.end_offsets[4], 0); } + + #[test] + pub fn test_cursor_read() { + use byteorder::{ + LittleEndian, + ReadBytesExt, + }; + + let data = vec![vec![12, 34], vec![56, 78, 90]]; + let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); + + let message_buffer = &mut MessageBuffer::new(0); + let header_len = message_buffer.header_len as usize; + + let account_info_data = &mut vec![]; + let discriminator = &mut sighash("accounts", "MessageBuffer"); + let destination = &mut vec![0u8; 10_240 - header_len]; + + + account_info_data.write_all(discriminator).unwrap(); + account_info_data + .write_all(bytes_of_mut(message_buffer)) + .unwrap(); + account_info_data.write_all(destination).unwrap(); + + + let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); + let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); + + let (num_msgs, num_bytes) = message_buffer.put_all_in_buffer(body_bytes, &data_bytes); + assert_eq!(num_msgs, 2); + assert_eq!(num_bytes, 5); + + + let message_buffer: &MessageBuffer = + bytemuck::from_bytes(&account_info_data.as_slice()[8..header_len]); + + assert_eq!(message_buffer.end_offsets[0], 2); + assert_eq!(message_buffer.end_offsets[1], 5); + + + let mut cursor = std::io::Cursor::new(&account_info_data[10..]); + let header_len = cursor.read_u16::().unwrap(); + println!("header_len: {}", header_len); + let mut header_begin = header_len; + let mut header_end = cursor.read_u16::().unwrap(); + let mut data_iter = data_bytes.iter(); + println!("init header_end: {}", header_end); + let read_data = &mut vec![]; + while header_end != 0 { + let end_offset = header_len + header_end; + let accumulator_input_data = + &account_info_data[header_begin as usize..end_offset as usize]; + header_end = cursor.read_u16::().unwrap(); + header_begin = end_offset; + read_data.push(accumulator_input_data); + } + + println!("read_data: {:?}", read_data); + for d in read_data.iter() { + let expected_data = data_iter.next().unwrap(); + assert_eq!(d, &expected_data.as_slice()); + } + assert_eq!(read_data.len(), 2); + } } From e08f9fecabb1ee57cb8998b0b968767ab985d08a Mon Sep 17 00:00:00 2001 From: swimricky Date: Wed, 26 Apr 2023 14:15:40 -0700 Subject: [PATCH 2/2] refactor(message-buffer): refactor test methods, add big endian doc on MessageBuffer --- .../src/state/message_buffer.rs | 102 ++++++++---------- 1 file changed, 42 insertions(+), 60 deletions(-) diff --git a/message_buffer/programs/message_buffer/src/state/message_buffer.rs b/message_buffer/programs/message_buffer/src/state/message_buffer.rs index e5feb97180..652a51ca76 100644 --- a/message_buffer/programs/message_buffer/src/state/message_buffer.rs +++ b/message_buffer/programs/message_buffer/src/state/message_buffer.rs @@ -26,6 +26,13 @@ use { /// A `MessageBuffer` AccountInfo.data will look like: /// [ , , ] /// (0..8) (8..header_len) (header_len...accountInfo.data.len) +/// +///
+/// +/// NOTE: The defined fields are read as *Little Endian*. The actual messages +/// are read as *Big Endian*. The MessageBuffer fields are only ever read +/// by the Pythnet validator & Hermes so don't need to be in Big Endian +/// for cross-platform compatibility. #[account(zero_copy)] #[derive(InitSpace, Debug)] pub struct MessageBuffer { @@ -180,6 +187,23 @@ mod test { sighash } + fn generate_message_buffer_bytes(_data_bytes: &Vec>) -> Vec { + let message_buffer = &mut MessageBuffer::new(0); + let header_len = message_buffer.header_len as usize; + + let account_info_data = &mut vec![]; + let discriminator = &mut sighash("accounts", "MessageBuffer"); + let destination = &mut vec![0u8; 10_240 - header_len]; + + + account_info_data.write_all(discriminator).unwrap(); + account_info_data + .write_all(bytes_of_mut(message_buffer)) + .unwrap(); + account_info_data.write_all(destination).unwrap(); + account_info_data.to_vec() + } + #[test] fn test_sizes_and_alignments() { @@ -195,19 +219,9 @@ mod test { let data = vec![vec![12, 34], vec![56, 78, 90]]; let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); - let message_buffer = &mut MessageBuffer::new(0); - let header_len = message_buffer.header_len as usize; + let account_info_data = &mut generate_message_buffer_bytes(&data_bytes); - let account_info_data = &mut vec![]; - let discriminator = &mut sighash("accounts", "MessageBuffer"); - let destination = &mut vec![0u8; 10_240 - header_len]; - - - account_info_data.write_all(discriminator).unwrap(); - account_info_data - .write_all(bytes_of_mut(message_buffer)) - .unwrap(); - account_info_data.write_all(destination).unwrap(); + let header_len = MessageBuffer::HEADER_LEN as usize; let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); @@ -251,20 +265,9 @@ mod test { let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); - let message_buffer = &mut MessageBuffer::new(0); - let header_len = message_buffer.header_len as usize; - - let account_info_data = &mut vec![]; - let discriminator = &mut sighash("accounts", "MessageBuffer"); - let destination = &mut vec![0u8; 10_240 - header_len]; - - - account_info_data.write_all(discriminator).unwrap(); - account_info_data - .write_all(bytes_of_mut(message_buffer)) - .unwrap(); - account_info_data.write_all(destination).unwrap(); + let account_info_data = &mut generate_message_buffer_bytes(&data_bytes); + let header_len = MessageBuffer::HEADER_LEN as usize; let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); @@ -295,7 +298,8 @@ mod test { assert_eq!(message_buffer.end_offsets[2], 0); } - // + + #[test] fn test_put_all_long_vec() { let data = vec![ @@ -308,19 +312,9 @@ mod test { let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); - let message_buffer = &mut MessageBuffer::new(0); - let header_len = message_buffer.header_len as usize; + let account_info_data = &mut generate_message_buffer_bytes(&data_bytes); - let account_info_data = &mut vec![]; - let discriminator = &mut sighash("accounts", "MessageBuffer"); - let destination = &mut vec![0u8; 10_240 - header_len]; - - - account_info_data.write_all(discriminator).unwrap(); - account_info_data - .write_all(bytes_of_mut(message_buffer)) - .unwrap(); - account_info_data.write_all(destination).unwrap(); + let header_len = MessageBuffer::HEADER_LEN as usize; let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); @@ -367,21 +361,9 @@ mod test { let data = vec![vec![12, 34], vec![56, 78, 90]]; let data_bytes: Vec> = data.into_iter().map(data_bytes).collect(); + let account_info_data = &mut generate_message_buffer_bytes(&data_bytes); - let message_buffer = &mut MessageBuffer::new(0); - let header_len = message_buffer.header_len as usize; - - let account_info_data = &mut vec![]; - let discriminator = &mut sighash("accounts", "MessageBuffer"); - let destination = &mut vec![0u8; 10_240 - header_len]; - - - account_info_data.write_all(discriminator).unwrap(); - account_info_data - .write_all(bytes_of_mut(message_buffer)) - .unwrap(); - account_info_data.write_all(destination).unwrap(); - + let header_len = MessageBuffer::HEADER_LEN as usize; let (header_bytes, body_bytes) = account_info_data.split_at_mut(header_len); let message_buffer: &mut MessageBuffer = bytemuck::from_bytes_mut(&mut header_bytes[8..]); @@ -401,17 +383,17 @@ mod test { let mut cursor = std::io::Cursor::new(&account_info_data[10..]); let header_len = cursor.read_u16::().unwrap(); println!("header_len: {}", header_len); - let mut header_begin = header_len; - let mut header_end = cursor.read_u16::().unwrap(); + let mut current_msg_start = header_len; + let mut end_offset = cursor.read_u16::().unwrap(); let mut data_iter = data_bytes.iter(); - println!("init header_end: {}", header_end); + println!("init header_end: {}", end_offset); let read_data = &mut vec![]; - while header_end != 0 { - let end_offset = header_len + header_end; + while end_offset != 0 { + let current_msg_end = header_len + end_offset; let accumulator_input_data = - &account_info_data[header_begin as usize..end_offset as usize]; - header_end = cursor.read_u16::().unwrap(); - header_begin = end_offset; + &account_info_data[current_msg_start as usize..current_msg_end as usize]; + end_offset = cursor.read_u16::().unwrap(); + current_msg_start = current_msg_end; read_data.push(accumulator_input_data); }