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.

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

sigverify to identify and mark simple vote transaction #20021

Merged
merged 4 commits into from
Sep 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions perf/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ solana-budget-program = { path = "../programs/budget", version = "=1.6.27" }
solana-logger = { path = "../logger", version = "=1.6.27" }
solana-measure = { path = "../measure", version = "=1.6.27" }
solana-metrics = { path = "../metrics", version = "=1.6.27" }
solana-vote-program = { path = "../programs/vote", version = "=1.6.27" }
jstarry marked this conversation as resolved.
Show resolved Hide resolved
curve25519-dalek = { version = "2" }

[lib]
Expand Down
5 changes: 3 additions & 2 deletions perf/benches/sigverify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ fn bench_get_offsets(bencher: &mut Bencher) {
let tx = test_tx();

// generate packet vector
let batches = to_packets_chunked(&std::iter::repeat(tx).take(1024).collect::<Vec<_>>(), 1024);
let mut batches =
to_packets_chunked(&std::iter::repeat(tx).take(1024).collect::<Vec<_>>(), 1024);

let recycler = Recycler::new_without_limit("");
// verify packets
bencher.iter(|| {
let _ans = sigverify::generate_offsets(&batches, &recycler);
let _ans = sigverify::generate_offsets(&mut batches, &recycler);
})
}
190 changes: 173 additions & 17 deletions perf/src/sigverify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::recycler::Recycler;
use rayon::ThreadPool;
use solana_metrics::inc_new_counter_debug;
use solana_rayon_threadlimit::get_thread_count;
use solana_sdk::hash::Hash;
use solana_sdk::message::MESSAGE_HEADER_LENGTH;
use solana_sdk::pubkey::Pubkey;
use solana_sdk::short_vec::decode_shortu16_len;
Expand Down Expand Up @@ -45,15 +46,23 @@ struct PacketOffsets {
pub sig_start: u32,
pub msg_start: u32,
pub pubkey_start: u32,
pub pubkey_len: u32,
}

impl PacketOffsets {
pub fn new(sig_len: u32, sig_start: u32, msg_start: u32, pubkey_start: u32) -> Self {
pub fn new(
sig_len: u32,
sig_start: u32,
msg_start: u32,
pubkey_start: u32,
pubkey_len: u32,
) -> Self {
Self {
sig_len,
sig_start,
msg_start,
pubkey_start,
pubkey_len,
}
}
}
Expand All @@ -66,6 +75,8 @@ pub enum PacketError {
InvalidSignatureLen,
MismatchSignatureLen,
PayerNotWritable,
InvalidProgramIdIndex,
InvalidProgramLen,
}

impl std::convert::From<std::boxed::Box<bincode::ErrorKind>> for PacketError {
Expand Down Expand Up @@ -234,20 +245,88 @@ fn do_get_packet_offsets(
u32::try_from(sig_start)?,
u32::try_from(msg_start)?,
u32::try_from(pubkey_start)?,
u32::try_from(pubkey_len)?,
))
}

fn get_packet_offsets(packet: &Packet, current_offset: usize) -> PacketOffsets {
fn get_packet_offsets(packet: &mut Packet, current_offset: usize) -> PacketOffsets {
let unsanitized_packet_offsets = do_get_packet_offsets(packet, current_offset);
if let Ok(offsets) = unsanitized_packet_offsets {
check_for_simple_vote_transaction(packet, &offsets, current_offset).ok();
offsets
} else {
// force sigverify to fail by returning zeros
PacketOffsets::new(0, 0, 0, 0)
PacketOffsets::new(0, 0, 0, 0, 0)
}
}

pub fn generate_offsets(batches: &[Packets], recycler: &Recycler<TxOffset>) -> TxOffsets {
fn check_for_simple_vote_transaction(
tao-stones marked this conversation as resolved.
Show resolved Hide resolved
packet: &mut Packet,
packet_offsets: &PacketOffsets,
current_offset: usize,
) -> Result<(), PacketError> {
if packet_offsets.sig_len != 1 {
Copy link
Member

Choose a reason for hiding this comment

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

FYI (+ @sakridge) there are at least two validators on mainnet that use 2 sig votes:

I think it would be better to remove this restriction

Copy link
Member

Choose a reason for hiding this comment

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

Yea, let's allow for 1 or 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feels like checking number of sigs is a quick way to rule out vote, if it is not 100%, might as well remove this check. Unless sig_len==1 || sig_len == 2 is all there is.
BTW, how do I see the 2 sigs vote in via above links?

Copy link
Member

Choose a reason for hiding this comment

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

Yea. sig_len == 1 || sig_len == 2 should be fine, more than 2 doesn't make sense. Could be sig_len <= 2 because I think we exclude sig_len == 0 already in another place.

Copy link
Member

Choose a reason for hiding this comment

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

feels like checking number of sigs is a quick way to rule out vote, if it is not 100%, might as well remove this check. Unless sig_len==1 || sig_len == 2 is all there is.
BTW, how do I see the 2 sigs vote in via above links?

Any of those account's transactions have two account signers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, putting in a new PR for this.

return Err(PacketError::InvalidSignatureLen);
}

let pubkey_start = (packet_offsets.pubkey_start as usize)
.checked_sub(current_offset)
.ok_or(PacketError::InvalidLen)?;

let instructions_len_offset = (packet_offsets.pubkey_len as usize)
.checked_mul(size_of::<Pubkey>())
.and_then(|v| v.checked_add(pubkey_start))
.and_then(|v| v.checked_add(size_of::<Hash>()))
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for instructions.len
let _ = instructions_len_offset
.checked_add(1usize)
.filter(|v| *v <= packet.meta.size)
.ok_or(PacketError::InvalidLen)?;

let (instruction_len, instruction_len_size) =
decode_shortu16_len(&packet.data[instructions_len_offset..])
.map_err(|_| PacketError::InvalidLen)?;

// skip if has more than 1 instruction
if instruction_len != 1 {
return Err(PacketError::InvalidProgramLen);
}

let instruction_start = instructions_len_offset
.checked_add(instruction_len_size)
.ok_or(PacketError::InvalidLen)?;

// Packet should have at least 1 more byte for one instructions_program_id
let _ = instruction_start
.checked_add(1usize)
.filter(|v| *v <= packet.meta.size)
.ok_or(PacketError::InvalidLen)?;

let instruction_program_id_index: usize = usize::from(packet.data[instruction_start]);

if instruction_program_id_index >= packet_offsets.pubkey_len as usize {
return Err(PacketError::InvalidProgramIdIndex);
}

let instruction_program_id_start = instruction_program_id_index
.checked_mul(size_of::<Pubkey>())
.and_then(|v| v.checked_add(pubkey_start))
.ok_or(PacketError::InvalidLen)?;
let instruction_program_id_end = instruction_program_id_start
.checked_add(size_of::<Pubkey>())
.ok_or(PacketError::InvalidLen)?;

if &packet.data[instruction_program_id_start..instruction_program_id_end]
== solana_sdk::vote::program::id().as_ref()
{
packet.meta.is_simple_vote_tx = true;
}
Ok(())
}

pub fn generate_offsets(batches: &mut [Packets], recycler: &Recycler<TxOffset>) -> TxOffsets {
debug!("allocating..");
let mut signature_offsets: PinnedVec<_> = recycler.allocate().unwrap();
signature_offsets.set_pinnable();
Expand All @@ -259,9 +338,9 @@ pub fn generate_offsets(batches: &[Packets], recycler: &Recycler<TxOffset>) -> T
msg_sizes.set_pinnable();
let mut current_offset: usize = 0;
let mut v_sig_lens = Vec::new();
batches.iter().for_each(|p| {
batches.iter_mut().for_each(|p| {
let mut sig_lens = Vec::new();
p.packets.iter().for_each(|packet| {
p.packets.iter_mut().for_each(|packet| {
let packet_offsets = get_packet_offsets(packet, current_offset);

sig_lens.push(packet_offsets.sig_len);
Expand Down Expand Up @@ -471,11 +550,11 @@ mod tests {
use crate::packet::{Packet, Packets};
use crate::sigverify;
use crate::sigverify::PacketOffsets;
use crate::test_tx::{test_multisig_tx, test_tx};
use crate::test_tx::{test_multisig_tx, test_tx, vote_tx};
use bincode::{deserialize, serialize};
use solana_sdk::hash::Hash;
use solana_sdk::instruction::CompiledInstruction;
use solana_sdk::message::{Message, MessageHeader};
use solana_sdk::signature::Signature;
use solana_sdk::signature::{Keypair, Signature};
use solana_sdk::transaction::Transaction;

const SIG_OFFSET: usize = 1;
Expand Down Expand Up @@ -516,9 +595,9 @@ mod tests {
let tx = test_tx();
let tx_bytes = serialize(&tx).unwrap();
let message_data = tx.message_data();
let packet = sigverify::make_packet_from_transaction(tx.clone());
let mut packet = sigverify::make_packet_from_transaction(tx.clone());

let packet_offsets = sigverify::get_packet_offsets(&packet, 0);
let packet_offsets = sigverify::get_packet_offsets(&mut packet, 0);

assert_eq!(
memfind(&tx_bytes, &tx.signatures[0].as_ref()),
Expand Down Expand Up @@ -742,37 +821,38 @@ mod tests {

// Just like get_packet_offsets, but not returning redundant information.
fn get_packet_offsets_from_tx(tx: Transaction, current_offset: u32) -> PacketOffsets {
let packet = sigverify::make_packet_from_transaction(tx);
let packet_offsets = sigverify::get_packet_offsets(&packet, current_offset as usize);
let mut packet = sigverify::make_packet_from_transaction(tx);
let packet_offsets = sigverify::get_packet_offsets(&mut packet, current_offset as usize);
PacketOffsets::new(
packet_offsets.sig_len,
packet_offsets.sig_start - current_offset,
packet_offsets.msg_start - packet_offsets.sig_start,
packet_offsets.pubkey_start - packet_offsets.msg_start,
packet_offsets.pubkey_len,
)
}

#[test]
fn test_get_packet_offsets() {
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 0),
PacketOffsets::new(1, 1, 64, 4)
PacketOffsets::new(1, 1, 64, 4, 2)
);
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 100),
PacketOffsets::new(1, 1, 64, 4)
PacketOffsets::new(1, 1, 64, 4, 2)
);

// Ensure we're not indexing packet by the `current_offset` parameter.
assert_eq!(
get_packet_offsets_from_tx(test_tx(), 1_000_000),
PacketOffsets::new(1, 1, 64, 4)
PacketOffsets::new(1, 1, 64, 4, 2)
);

// Ensure we're returning sig_len, not sig_size.
assert_eq!(
get_packet_offsets_from_tx(test_multisig_tx(), 0),
PacketOffsets::new(2, 1, 128, 4)
PacketOffsets::new(2, 1, 128, 4, 4)
);
}

Expand Down Expand Up @@ -1024,4 +1104,80 @@ mod tests {
failed_g.load(Ordering::Relaxed)
);
}

#[test]
fn test_is_simple_vote_transaction() {
solana_logger::setup();

// tansfer tx is not
{
let mut tx = test_tx();
tx.message.instructions[0].data = vec![1, 2, 3];
let mut packet = sigverify::make_packet_from_transaction(tx);
let packet_offsets = do_get_packet_offsets(&packet, 0).unwrap();
check_for_simple_vote_transaction(&mut packet, &packet_offsets, 0).ok();
assert!(!packet.meta.is_simple_vote_tx);
}

// single vote tx is
{
let mut tx = vote_tx();
tx.message.instructions[0].data = vec![1, 2, 3];
let mut packet = sigverify::make_packet_from_transaction(tx);
let packet_offsets = do_get_packet_offsets(&packet, 0).unwrap();
check_for_simple_vote_transaction(&mut packet, &packet_offsets, 0).ok();
assert!(packet.meta.is_simple_vote_tx);
}

// multiple mixed tx is not
{
let key = Keypair::new();
let key1 = Pubkey::new_unique();
let key2 = Pubkey::new_unique();
let tx = Transaction::new_with_compiled_instructions(
&[&key],
&[key1, key2],
Hash::default(),
vec![solana_vote_program::id(), Pubkey::new_unique()],
vec![
CompiledInstruction::new(3, &(), vec![0, 1]),
CompiledInstruction::new(4, &(), vec![0, 2]),
],
);
let mut packet = sigverify::make_packet_from_transaction(tx);
let packet_offsets = do_get_packet_offsets(&packet, 0).unwrap();
check_for_simple_vote_transaction(&mut packet, &packet_offsets, 0).ok();
assert!(!packet.meta.is_simple_vote_tx);
}
}

#[test]
fn test_is_simple_vote_transaction_with_offsets() {
solana_logger::setup();

let mut current_offset = 0usize;
let mut batch = Packets::default();
batch
.packets
.push(sigverify::make_packet_from_transaction(test_tx()));
batch
.packets
.push(sigverify::make_packet_from_transaction(vote_tx()));
batch
.packets
.iter_mut()
.enumerate()
.for_each(|(index, mut packet)| {
let packet_offsets = do_get_packet_offsets(&packet, current_offset).unwrap();
check_for_simple_vote_transaction(&mut packet, &packet_offsets, current_offset)
.ok();
if index == 1 {
assert!(packet.meta.is_simple_vote_tx);
} else {
assert!(!packet.meta.is_simple_vote_tx);
}

current_offset = current_offset.saturating_add(size_of::<Packet>());
});
}
}
14 changes: 14 additions & 0 deletions perf/src/test_tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use solana_sdk::system_instruction::SystemInstruction;
use solana_sdk::system_program;
use solana_sdk::system_transaction;
use solana_sdk::transaction::Transaction;
use solana_vote_program::vote_transaction;

pub fn test_tx() -> Transaction {
let keypair1 = Keypair::new();
Expand Down Expand Up @@ -38,3 +39,16 @@ pub fn test_multisig_tx() -> Transaction {
instructions,
)
}

pub fn vote_tx() -> Transaction {
let keypair = Keypair::new();
vote_transaction::new_vote_transaction(
vec![2],
Hash::default(),
Hash::default(),
&keypair,
&keypair,
&keypair,
None,
)
}
1 change: 1 addition & 0 deletions sdk/src/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub struct Meta {
pub seed: [u8; 32],
pub slot: Slot,
pub is_tracer_tx: bool,
pub is_simple_vote_tx: bool,
}

#[derive(Clone)]
Expand Down