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

add signing prefix for PruneData and verify PruneData with and without signing prefix #1472

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

gregcusack
Copy link

First PR to add a signing prefix for PruneData.
shoutout @ripatel-fd for first identifying this need.

Problem

Need signing domains. Ensure all signed PruneData messages are only used for PruneData and nothing else.

Summary of Changes

  1. add PruneData serializing with prefix (but do not propagate this prefixed, serialized data)
  2. support verifying both serialized PruneData with and without prefix

Once all validators have upgraded, we will

  1. Create a new PR and will switch over to sending out serialized PruneData with the new signing prefix.
  2. Create a new PR removing support for non-prefixed PruneData

Comment on lines +225 to +240
fn signable_data_without_prefix(&self) -> Cow<[u8]> {
#[derive(Serialize)]
struct SignData<'a> {
pubkey: &'a Pubkey,
prunes: &'a [Pubkey],
destination: &'a Pubkey,
wallclock: u64,
}
let data = SignData {
pubkey: &self.pubkey,
prunes: &self.prunes,
destination: &self.destination,
wallclock: self.wallclock,
};
Cow::Owned(serialize(&data).expect("should serialize PruneData"))
}
Copy link
Author

Choose a reason for hiding this comment

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

could just leave this in signable_data() instead of pulling it out into its own function. But wanted to explicitly show the change and the path we're taking. But open to other thoughts!

@@ -129,6 +129,8 @@ pub const MAX_INCREMENTAL_SNAPSHOT_HASHES: usize = 25;
/// Maximum number of origin nodes that a PruneData may contain, such that the
/// serialized size of the PruneMessage stays below PACKET_DATA_SIZE.
const MAX_PRUNE_DATA_NODES: usize = 32;
/// Prune data prefix for PruneMessage
const PRUNE_DATA_PREFIX: &[u8] = "SOLANA_PRUNE_DATA".as_bytes();

Choose a reason for hiding this comment

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

I suggest we prefix this with 0x7f.
The first byte of a transaction is the number of signatures. 127 signatures would imply > 8128 byte transaction.
So therefore, an Ed25519 payload starting with 0x7f is guaranteed not a valid transaction.

cc @t-nelson

Copy link

Choose a reason for hiding this comment

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

this would assume we never go to the trouble of extending the MTU. there's a similar treatment specified for off-chain message signing. afaik the associated runtime support hasn't been added yet, but we should probably use the same value for the sake of future proofing

@ripatel-fd ripatel-fd requested a review from t-nelson May 24, 2024 16:34
behzadnouri
behzadnouri previously approved these changes May 24, 2024
Copy link

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

lgtm

@gregcusack
Copy link
Author

updated to prefix PRUNE_DATA_PREFIX with 0xff. 0xff is "...implicitly illegal as the first byte in a transaction MessageHeader today." link

@gregcusack gregcusack merged commit f8e0822 into anza-xyz:master Jun 11, 2024
42 checks passed
@gregcusack gregcusack deleted the handle-prefix-prune-data branch June 11, 2024 16:16
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
…thout signing prefix (anza-xyz#1472)

* add PruneData serializing with prefix and support verifying both serialized data with and without prefix

* prefix PRUNE_DATA_PREFIX with 0xff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants