Add support for V1 transactions#538
Conversation
| let total = message.size(); | ||
| let mut buffer: Vec<u8> = Vec::with_capacity(1 + total); | ||
| // SAFETY: buffer has sufficient capacity for serialization. | ||
| unsafe { | ||
| let ptr = buffer.as_mut_ptr(); | ||
| ptr.write(MESSAGE_VERSION_PREFIX | 1); | ||
| v1::serialize_into(message, ptr.add(1)); | ||
| buffer.set_len(1 + total); | ||
| } | ||
| buffer |
There was a problem hiding this comment.
nit: we might want to also make this logic available in v1::Message, outside this function, in case someone wants to serialize a message without bincode. It also avoids the copied code with the wincode trait implementation
There was a problem hiding this comment.
There is public serialize helper:
solana-sdk/message/src/versions/v1/message.rs
Lines 350 to 362 in a41d3e8
It does not write the version, since the Message object is not "versioned".
| if serializer.is_human_readable() { | ||
| // JSON: encode as (0x81, { ...message fields... }) | ||
| // Note that this format does not match the wire format per SIMD-0385. | ||
| let mut seq = serializer.serialize_tuple(2)?; | ||
| seq.serialize_element(&(MESSAGE_VERSION_PREFIX | 1))?; | ||
| seq.serialize_element(message)?; | ||
| seq.end() | ||
| } else { | ||
| // Messages in V1 format cannot be binary serialized via bincode | ||
| // because the wire format (per SIMD-0385) is incompatible with bincode's | ||
| // data model. Use `VersionedMessage::serialize()` to get the correct | ||
| // wire bytes. | ||
| Err(serde::ser::Error::custom( | ||
| "V1 messages cannot be serialized via bincode. Use VersionedMessage::serialize() for wire bytes.", | ||
| )) | ||
| } |
There was a problem hiding this comment.
This should have nothing to do with bincode, we just need to hand-write the implementation of serde::Serialize to put the fields in the correct order
There was a problem hiding this comment.
I tried, but the format is not really compatible with the way bincode/serde works. There are "fields" that don't appear on the serialization based on values of other fields.
@apfitzge mentioned that bincode is not being used so maybe we should remove the bincode support altogether – I was not sure whether the json encoding is used or not.
| if self.human_readable { | ||
| Ok(VersionedMessage::V1( | ||
| seq.next_element()? | ||
| .ok_or_else(|| de::Error::invalid_length(1, &self))?, | ||
| )) | ||
| } else { | ||
| // V1 messages cannot be deserialized via bincode because the wire format (per SIMD-0385) | ||
| // is incompatible. Use `v1::Message::deserialize()` to parse wire-format bytes. | ||
| Err(de::Error::custom( | ||
| "V1 messages cannot be serialized via bincode. Use `v1::Message::deserialize()` to parse wire-format bytes.", | ||
| )) | ||
| } |
There was a problem hiding this comment.
Same with this, we should just need to hand-write the Deserialize implementation if the auto-generated one isn't correct
| #[test] | ||
| fn test_v1_message_raw_bytes_roundtrip() { |
There was a problem hiding this comment.
nit: we could eventually make these into proptests, or use some randomization to make sure the roundtrip always works
981e01b to
4a1a8ab
Compare
| /// client code, users should construct messages using `try_compile` or | ||
| /// `try_compile_with_config`. | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct MessageBuilder { |
There was a problem hiding this comment.
This type is not super useful as a public type since it would require passing CompileInstruction. Probably the try_compile* ones cover the construction of the message.
| solana-presigner = { workspace = true } | ||
| solana-pubkey = { workspace = true, features = ["rand"] } | ||
| solana-sha256-hasher = { workspace = true } | ||
| solana-sha256-hasher = { workspace = true, features = ["sha2"] } |
There was a problem hiding this comment.
Had to enable this for a test that uses the hash.
| #[cfg_attr( | ||
| feature = "frozen-abi", | ||
| frozen_abi(digest = "Hndd1SDxQ5qNZvzHo77dpW6uD5c1DJNVjtg8tE6hc432"), | ||
| frozen_abi(digest = "6CoVPUxkUvDrAvAkfyVXwVDHCSf77aufm7DEZy5mBVeX"), |
There was a problem hiding this comment.
This was needed since we remove the short-vec serde attribute.
apfitzge
left a comment
There was a problem hiding this comment.
only minor comments left
| .any(|&key| key == bpf_loader_upgradeable::id()) | ||
| } | ||
|
|
||
| /// Returns true if the account at the specified index was requested as writable. |
There was a problem hiding this comment.
This makes it seem the same as is_writable_index? We should add that this includes demotion
Honestly the naming of these 2 functions seems very odd to me.
is_writable_index is the one that is maybe not true.
is_maybe_writable is the one that actually includes demotion, and for non-v0 transactions will always return the truth.
There was a problem hiding this comment.
Looked at the v0 implementation. is_writable_index is meant to be private, so I changed to pub(crate) since there are some tests for it. Also updated the comments for is_maybe_writable. I did not change the name of these functions since they are the same as v0, but happy to do that if needed.
There was a problem hiding this comment.
that's fine if it's not public! It would be super confusing if public.
Should we change is_writable_index to something more like is_requested_writable_index on all 3 variants if it's a private fn? (separate PR if we do)
| .iter() | ||
| .enumerate() | ||
| .map(|(i, _key)| { | ||
| message.is_writable_index(i) |
There was a problem hiding this comment.
Do we not have a function that returns actual writability properly?
There was a problem hiding this comment.
it seems like is_maybe_writable should (confusing as fuck, but consistent with previous namings)
| // Generate between 12 and 64 accounts since we need at least the | ||
| // amount of `required_signatures`. | ||
| accounts in prop::collection::vec(any::<[u8; 32]>(), 12..=64), |
There was a problem hiding this comment.
We could generate accounts based on a random number between required_signatures and 64 instead.
There was a problem hiding this comment.
The issue that I found is that both appear on the same argument list, so they can't reference each other. And we can't move any of them to the second argument list.
There was a problem hiding this comment.
It should be possible to do it using the Just terminology. Here's an example of generating a number and a number in between in the stake pool program: https://github.com/solana-program/stake-pool/blob/8b4ddc69f782b7d3274bc0955aba6cd9c906b40d/program/src/state.rs#L1248
But anyway, I don't want to block this PR on that, we can go forward
There was a problem hiding this comment.
Hmmm, but they are in separate argument lists, right?
fn total_stake_and_rewards()(total_lamports in 1..u64::MAX)(
total_lamports in Just(total_lamports),
rewards in 0..=total_lamports,
) -> (u64, u64) {
(total_lamports - rewards, rewards)
}total_lamports is defined in the first one and then reused in the second.
The tricky thing that we have here is that both accounts and required_signatures need to be defined in the first one, since they are reused in the second.
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me! Just one last question, but nothing needs to block this going forward.
Future work from this PR:
- update
is_maybe_writable->is_writable - update
demote_program_id-> some better name
Is that it?
| // Generate between 12 and 64 accounts since we need at least the | ||
| // amount of `required_signatures`. | ||
| accounts in prop::collection::vec(any::<[u8; 32]>(), 12..=64), |
There was a problem hiding this comment.
It should be possible to do it using the Just terminology. Here's an example of generating a number and a number in between in the stake pool program: https://github.com/solana-program/stake-pool/blob/8b4ddc69f782b7d3274bc0955aba6cd9c906b40d/program/src/state.rs#L1248
But anyway, I don't want to block this PR on that, we can go forward
joncinque
left a comment
There was a problem hiding this comment.
Oops sorry, I'll also approve, in case my questions aren't valid 😄
apfitzge
left a comment
There was a problem hiding this comment.
small comment on having shared impl for a couple functions - the indexing is tricky and it seems better if we can share the tried and true impl instead of writing it in a different way.
Add V1 transaction format
Reference: SIMD-0296 & SIMD-0385
V1 transactions increase the maximum transaction size from 1232 bytes to 4096 bytes and embed compute budget configuration directly in the message header, eliminating the need for separate ComputeBudget program instructions.
Changes:
versions/v1module to message implementing V1 message format.V1variant toVersionedMessageenum.VersionedTransactionsupporting V1 transaction format.Co-authored-by: @grod220