Conversation
| /// Create a V1 transaction from a message and existing signatures. | ||
| /// | ||
| /// Returns an error if the signature count doesn't match `num_required_signatures`. | ||
| pub fn from_signatures( |
There was a problem hiding this comment.
try to think if there are use cases for this, is it for tests only?
There was a problem hiding this comment.
Thinking offline signing/multisig workflows, but realizing the current struct has public fields. So this method is sort of optional right now. However, constructing manually gets around the validation check. Leaning toward making these fields private and forcing method construction. What do you think?
There was a problem hiding this comment.
It makes sense to keep fields private and do "raw" construction via this api.
My train-of-thoughts is: V1::Transaction type is temporarily; it will, or can, be merged into Transaction - after all, they have same structure; but managing APIs might be bit messy. That's why I feel don't need to introduce from_signature(), but keep fields public as legacy/v0 Transactions currently are.
There was a problem hiding this comment.
@febo subbing you in if that's ok! 🙏
Totally open for whatever API design changes ya'll find are best.
There was a problem hiding this comment.
@tao-stones Made the fields public and removed from_signatures.
There was a problem hiding this comment.
I think the actual VersionedTransaction needs to be modified. As is, the signatures would still be at the front of the serialized bytes, right?
There was a problem hiding this comment.
in current impl, v1::Transaction::serialize() puts sigs after message. But if we don't introduces V1::Transaction as new type, then (de)ser for v1 can happen elsewhere.
There was a problem hiding this comment.
right but for it to actually work in validator we need VersionedTransaction; or we've got to change over to some new type wrapping the different message types - which imo doesn't make much sense
| /// - Signatures are appended directly with NO length prefix | ||
| /// - Signature count is determined by `num_required_signatures` from the message header | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct Transaction { |
There was a problem hiding this comment.
what do you think about not having V1::Transaction? So from user perspective, using bench-tps as example, it only knows one transaction type that is solana-transaction::Transaction, which can be constructed from V1::Message, then goes through same Transaction -> VersionTransaction -> SanitizedVersionTransaction lifespan. In another words, we only introduce new Message v1, and Transaction (somewhat) automatically supports that. @febo
There was a problem hiding this comment.
Makes sense to me. We would need to make Transaction generic over Message right? Seems like VersinedTransaction and SanitizedVersionedTransaction would be fine since they use enum representations for the message.
There was a problem hiding this comment.
Transaction is the wrong type I think. It only supports Legacy iirc.
VersionedTransaction, SanitizedVersionedTransaction, SanitizedTransaction are what we need to work if we want a drop in replacement in the validator (ignoring users for my perspective only).
There was a problem hiding this comment.
Those ones should support V1 with the changes on this PR. But Transaction is being duplicated at the moment. Perhaps we don't need the duplication after all?
There was a problem hiding this comment.
yea, remove (or "merge") duplicated "Transaction" is what I was thinking.
There was a problem hiding this comment.
Those ones should support V1 with the changes on this PR.
I think the Message types are correctly handled, but VersionedTransaction doesn't have a new variant afaict, so don't see how that could be working given the format difference in signature placement (leading vs trailing)
There was a problem hiding this comment.
VersionedTransaction uses VersionedMessage, which has a variant for V1. So you can create a new one VersionedTransaction::try_new passing a VersionedMessage::V1.
I see what you mean now. That has to change indeed.
There was a problem hiding this comment.
@tao-stones I am reworking the implementation to avoid so much duplication and address Andrew's comment. I will put a separate PR with it and then we can decide which approach works best.
|
Closed in favour of #538 |
4 participants
Reference: SIMD-0296 & SIMD-0385
Add V1 transaction format
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
New module (message/src/versions/v1/):
MessagestructComputeBudgetConfigandComputeBudgetConfigMaskfor inline compute budgetLoadedMessagewrapper with precomputed writability for runtimeNew module (transaction/src/v1.rs):
Transaction: transaction wrapper combiningMessagewith signatures[message bytes][signatures](no signature length prefix)