Skip to content

Make validTimePeriod configurable #249

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

Merged
merged 10 commits into from
Aug 22, 2022

Conversation

ali-behjati
Copy link
Collaborator

I will not deploy this change to testnet until we know the proper value, and that mean we can measure time from a pyth published to a client subscribed to the price service getting that price.

The ideal value is derived like this:

2*price_update_frequency + e2e(price published, a price service client receives it) + 2*target_network_latency + target_network_block_time

In solana we have a ~12 seconds validity time threshold which was set for the congestion periods. We might also consider setting in acceptable minimum here, I can change the contract to instead of being initialized with 0, be initialized with that value.

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

change is generally fine. I think the configured validity times are likely too low though (see inline)

@@ -5,3 +5,5 @@ MIGRATIONS_NETWORK=aurora
WORMHOLE_BRIDGE_ADDRESS=0xa321448d90d4e5b0A732867c18eA198e75CAC48E
PYTH_TO_WORMHOLE_CHAIN_ID=0x1
PYTH_TO_WORMHOLE_EMITTER=0x6bb14509a612f01fbbc4cffeebd4bbfb492a86df717ebe92eb6df432a3f00a25

VALID_TIME_PERIOD_SECONDS=15
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set the initial time periods to err on the high side? I think some of these thresholds are likely too aggressive -- would suggest doubling all of them as a baseline, and using 60 seconds as a minimum. (Note that your calculation also doesn't account for clock drift between the chains, which could be substantial)

I suggest this because we are currently not in a position to measure how many failures occur because the threshold is too aggressive. Without that measurement, we could end up in a situation where Pyth is extremely flaky on a target chain, and we wouldn't know it. We can optimize these thresholds later once our measurement infrastructure is in place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are my initial estimates based on the block time of the given chain. But we can measure it once we deploy attester in Pythnet.

I shared my opinion about clock drift in the above thread too. I don't think it be an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I applied your suggestion.

@jayantk
Copy link
Contributor

jayantk commented Aug 11, 2022

Also, no need to overcomplicate this by putting in a minimum validity threshold. We can guard against that in code that's easier for us to change, especially since we don't know the right minimum.

@ali-behjati ali-behjati requested a review from jayantk August 22, 2022 12:10
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

not sure about this trading status change, but LGTM aside from that

@ali-behjati ali-behjati merged commit f09c46b into main Aug 22, 2022
@ali-behjati ali-behjati deleted the abehjati/eth-make-valid-time-configurable branch August 22, 2022 18:13
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.

2 participants