wormhole-attester: Add a previous attestation timestamp field #488

drozdziak1 · 2023-01-13T15:22:15Z

This change bumps price batch format to v3.1 with a new backwards compatible field - prev_attestation_time. This is the last time we've successfully attested the price. If no prior record exists, the current time is used (the same as attestation_time).

The new field is backed by a new PDA for the attester contract, called 'attestation state'. In this PDA, we store a Pubkey -> Metadata hashmap for every price. Currently, the metadata stores just the latest successful attestation timestamp for use with the new field.

guibescos · 2023-01-13T17:26:48Z

wormhole-attester/program/src/attest.rs

@@ -240,6 +267,51 @@ pub fn attest(ctx: &ExecutionContext, accs: &mut Attest, data: AttestData) -> So

    trace!("Attestations successfully created");

+    // Serialize the state to calculate rent/account size adjustments
+    let serialized = accs.attestation_state.1.try_to_vec()?;


I think I would make one PDA per price feed to avoid the complexity of resizing the account

Also potentially might congest the network because write access from many transactions to one account?

about congestion, i had the same doubt, but Wormhole Sequence Account is already a bottleneck. So it won't be different.

Ok then it's just about the extra complexity of having a variable size account

I think this is fine. We're probably not going to exceed the size of the one account, and PDAs have their own issues.

Honestly either way would probably work, but I don't think there's a strong reason to rewrite this to use PDAs.

My main motivation for using a single account is a nasty edge case on Solana. In general, you're allowed to transfer SOL to any non-executable account, PDA or otherwise. For uninitialized accounts, such transfer instantiates the account and assigns ownership to the system program. When a program encounters such auto-initialized account as its own PDA, the program cannot change the contents, because it does not own it. This potentially creates a DoS condition for any known uninitialized PDA. This is completely permissionless and can happen on accident, which means that even Pythnet is not 100% immune. In fact, an accident is how I learned, some time ago while debugging a program I thought I was making a rent mistake so I tried pre-loading an account with SOL, locking the program out of it.

The attack surface in this situation is dependent on how often new PDAs need to be created and how easy (or not) it is to use a different PDA for the desired transaction. If we resort to a per-symbol PDA scheme, each new oracle symbol can potentially be vulnerable. What is even worse, if one symbol is denied its PDA, the only possible fix is to change the seeds inside program logic, losing all on-chain state for all the other accounts.

The proposed single-PDA solution collapses the attack surface into just a single race condition in the whole lifetime of this logic, along with relatively rare upgrades of the attestation state (we version this PDA's format in its seeds). Any lockout condition can be worked around by just altering its seeds.

As for storage space, the account size on Solana is limited, but the current 10 MiB limit seems sufficient. E.g. for 10k symbols we could give a bit more than 1KiB space per symbol. IMO this is more symbols than we'll have in forseeable future, and more space than we'll need in forseeable future

guibescos · 2023-01-13T17:31:58Z

wormhole-attester/program/src/attest.rs

@@ -60,9 +65,10 @@ pub const P2W_MAX_BATCH_SIZE: u16 = 5;
 #[derive(FromAccounts)]
 pub struct Attest<'b> {


The Attest interface has changed. How do plan on achieving 0 downtime for attester cranks?

yeah... what's the upgrade process here?

can you make the attestationstatemap optional so that if it's missing, it just sets the time intervals to 0?

One possible way to handle it smoothly is making the account optional but idk whether that's possible or not.

The other way is that we run a new attester with the new config that does not crash on failure and keep another one that works with the previous one. The new one should not crash on k8s as there is an exponential backoff on restarting the crashed pod.

Another approach is that attester can handle both types at the same time but i'm not sure that be easy to implement.

I was thinking precisely about what Ali said. We run a new attester in parallel with the old one, before deploying new contract version. This new attester will keep crashing intentionally. Then, we perform the program upgrade. This should cause the old attester to start crashing, and the new one to pick up the ball immediately.

Everything else should be relatively happy to utilize the new format. the wire format is extended in a backwards compatible way. Any parser that obeys the format should understand that there's trailing bytes it does not need to care for.

guibescos · 2023-01-13T17:34:06Z

wormhole-attester/program/src/message.rs

@@ -44,7 +44,7 @@ impl<'a> Seeded<&P2WMessageDrvData> for P2WMessage<'a> {
            // See the note at 2022-09-05 above.
            // Change the version in the literal whenever you change the
            // price attestation data.
-            "p2w-message-v1".as_bytes().to_vec(),


Are you sure you need to update this? Doesn't having batch_size as a seed already solve the problem?

The format had changed and payload size had changed. The batch size refers to the number of symbols being attested, not the total payload size in bytes.

Hey Stan, since this will create new accounts, can you make sure our accounts will remain topped up by a lot of pgas when we do the deployment on Pythnet?

guibescos

I think I disagree with not using PDAs for each price feed AttestationState

jayantk

i think we need to be careful about exactly what timestamp we use here -- see inline comment. That shouldn't be too hard to fix, but i do want to make sure it is fixed before approving.

Aside from that, looks good to me.

jayantk · 2023-01-13T19:15:32Z

third_party/pyth/p2w-sdk/rust/src/lib.rs

    #[serde(serialize_with = "use_to_string")]
-    pub prev_conf:          u64,
+    pub prev_conf:             u64,
+    pub prev_attestation_time: UnixTimestamp,


I think we need to use a timestamp derived from the publish time not the attestation time for consistency. All the downstream SDKs use the publish time as the timestamp of the price update.

This will probably get confusing since there's already a field called prev_publish_time. However, I think this field should be something like previously_attested_publish_time which is set to either publish_time or prev_publish_time depending on whether the current status is trading or not (?).

I think that ensures that there's at most one update per time interval, and that each interval ends with a usable price. (If the feed goes offline, I think you get a bunch of price updates where the current update timestamp is equal to the previously_attested_publish_time. When the feed comes back online, you get an update that spans the entire interval with the next available price.)

Please do check that logic and validate that it has the right properties. It's a little tricky.

I like to suggest removing prev_* fields entirely and handling their logic directly here. We can do it in a way that it's not breaking (just pass 0 for those fields and replace one field with the field that we want)

I understood some more context and I used publish_time as proposed

The name for the timestamp stored on chain is "last_attested_publish_time" to distinguish from existing PriceAttestation fields.

Please don't change the prev_ fields right now. It's too much to change at once.

jayantk · 2023-01-13T19:21:44Z

wormhole-attester/program/src/attest.rs

@@ -60,9 +65,10 @@ pub const P2W_MAX_BATCH_SIZE: u16 = 5;
 #[derive(FromAccounts)]
 pub struct Attest<'b> {


yeah... what's the upgrade process here?

can you make the attestationstatemap optional so that if it's missing, it just sets the time intervals to 0?

jayantk · 2023-01-13T19:25:40Z

wormhole-attester/program/src/attest.rs

+    if accs.attestation_state.is_initialized() {
+        accs.attestation_state
+            .info()
+            .realloc(serialized.len(), false)?;


is there a performance consequence or something of doing this every time? Maybe check if you have enough space before reallocating?

No as far as I can tell. Solana v1.10.31 AccountInfo implementation:

pub fn realloc(&self, new_len: usize, zero_init: bool) -> Result<(), ProgramError> { let mut data = self.try_borrow_mut_data()?; let old_len = data.len(); // Return early if length hasn't changed if new_len == old_len { return Ok(()); } // Return early if the length increase from the original serialized data // length is too large and would result in an out of bounds allocation. let original_data_len = unsafe { self.original_data_len() }; if new_len.saturating_sub(original_data_len) > MAX_PERMITTED_DATA_INCREASE { return Err(ProgramError::InvalidRealloc); } // realloc unsafe { let data_ptr = data.as_mut_ptr(); // First set new length in the serialized data *(data_ptr.offset(-8) as *mut u64) = new_len as u64; // Then recreate the local slice with the new length *data = from_raw_parts_mut(data_ptr, new_len) } if zero_init { let len_increase = new_len.saturating_sub(old_len); if len_increase > 0 { sol_memset(&mut data[old_len..], 0, len_increase); } } Ok(()) }

Re: only do increases in size. This is not that easy because borsh gets upset if it doesn't consume a whole buffer for deserialization.

ali-behjati · 2023-01-16T09:04:53Z

wormhole-attester/program/src/attestation_state.rs

+/// Top-level state gathering all known AttestationState values, keyed by price address.
+#[derive(BorshSerialize, BorshDeserialize, Default)]
+pub struct AttestationStateMap {
+    pub entries: BTreeMap<Pubkey, AttestationState>,


question for myself, will adding a BTreeMap here result in a big code size increase?

Not necessarily, BTreeMap is a common data structure. It is likely already in use in our dependencies, thus no significant code size increase beyond the function calls to its methods. As for serialization payload size, it is probably using the borsh-specific key-value type, which depends on borsh traits for construction.

vercel · 2023-01-16T15:00:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
xc-admin-frontend	⬜️ Ignored (Inspect)			Jan 18, 2023 at 6:01PM (UTC)

This change bumps price batch format to v3.1 with a new backwards compatible field - prev_attestation_time. This is the last time we've successfully attested the price. If no prior record exists, the current time is used (the same as attestation_time). The new field is backed by a new PDA for the attester contract, called 'attestation state'. In this PDA, we store a Pubkey -> Metadata hashmap for every price. Currently, the metadata stores just the latest successful attestation timestamp for use with the new field.

drozdziak1 requested review from jayantk and ali-behjati January 13, 2023 15:22

guibescos reviewed Jan 13, 2023

View reviewed changes

jayantk requested changes Jan 13, 2023

View reviewed changes

ali-behjati reviewed Jan 16, 2023

View reviewed changes

vercel bot had a problem deploying to Preview January 16, 2023 15:00 Failure

drozdziak1 added 2 commits January 18, 2023 15:54

wormhole-attester: Use publish_time instead of attestation_time

c0d8cb0

drozdziak1 force-pushed the drozdziak1/wormhole-attester-prev-timestamp-field branch from 600161f to c0d8cb0 Compare January 18, 2023 14:54

wormhole_attester: use prev_publish_time for non-trading prices

cb72921

jayantk approved these changes Jan 18, 2023

View reviewed changes

ali-behjati approved these changes Jan 19, 2023

View reviewed changes

drozdziak1 merged commit 7202b93 into main Jan 19, 2023

drozdziak1 deleted the drozdziak1/wormhole-attester-prev-timestamp-field branch January 19, 2023 13:38

		@@ -60,9 +65,10 @@ pub const P2W_MAX_BATCH_SIZE: u16 = 5;
		#[derive(FromAccounts)]
		pub struct Attest<'b> {

wormhole-attester: Add a previous attestation timestamp field #488

wormhole-attester: Add a previous attestation timestamp field #488

Uh oh!

Conversation

drozdziak1 commented Jan 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos left a comment

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

drozdziak1 Jan 16, 2023 •

edited

Loading

vercel bot commented Jan 16, 2023 •

edited

Loading