-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
sigverify to identify and mark simple vote transaction #20021
Conversation
c5bc874
to
a1889e3
Compare
Codecov Report
@@ Coverage Diff @@
## v1.6 #20021 +/- ##
========================================
Coverage 82.3% 82.3%
========================================
Files 424 425 +1
Lines 118984 119112 +128
========================================
+ Hits 97998 98132 +134
+ Misses 20986 20980 -6 |
packet_offsets: &PacketOffsets, | ||
current_offset: usize, | ||
) -> Result<(), PacketError> { | ||
if packet_offsets.sig_len != 1 { |
There was a problem hiding this comment.
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:
- https://explorer.solana.com/address/p2pUJruVeKTuZ41zbGWcxienmzRFnUa9fjBHigsR3QC
- https://explorer.solana.com/address/DWvDTSh3qfn88UoQTEKRV2JnLt5jtJAVoiCo3ivtMwXP
I think it would be better to remove this restriction
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…0021) * sigverify to identify and mark simple vote transaction * check vote tx at get_packet_offsets to cover both cpu and gpu paths * add pubkey_len to PacketOffsets to reduce the redundant bytes counting
@taozhu-chicago I'm porting this change to master fyi |
Problem
Changes to sigverify to identify simple vote transaction should be standalone PR
Summary of Changes
Pull the relevant changes from #19884
Fixes #