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

Update default session sync allowance #1560

Merged
merged 1 commit into from
Jul 5, 2023
Merged

Conversation

nodiesBlade
Copy link
Contributor

@nodiesBlade nodiesBlade commented Jun 27, 2023

Description

This PR sets the default session sync allowance back to zero, effectively removing the session rollover fix as a default setting. Clients can re-enable as they see fit. Gateways should code logic that accounts for old sessions. Gateways should also always use the latest session when possible.

PNI recently transitioned from AWS to GCP, and their Portal V1 stack to V2 stack. This has resulted in 1.3B to ~600m relays being submitted on the chain. Our fleet of 4K+ nodes has also seen reduced served traffic significantly.

The rationale behind this change is simple: Too many changes all happening at once. This change is simply to prevent additional noise from the ongoing incident. Once PNI's V2 gateway is stable and network traffic is stable, we can set this default back to 1 with a new build given its nonconsensus changing.

NOTE: This is to prevent any more damage with the ongoing incident and cause additional confusion for the gateway and node runners. Since we don't know what the root cause is for the missing traffic, by not including it by default, it removes one less factor for them to consider.

Relevant PR: #1536

Summary generated by Reviewpad on 27 Jun 23 04:41 UTC

This pull request updates the default session sync allowance configuration from 1 to 0, representing a session unit (irrespective of num blocks per session). This ensures that the session sync allowance configuration is more adjustable for user's needs.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Jun 27, 2023
@Olshansk
Copy link
Member

@PoktBlade We didn't have any tests dependent on the default value?

@nodiesBlade
Copy link
Contributor Author

@Olshansk don't remember, i'll rerun the tests and modify them if needed

@nodiesBlade
Copy link
Contributor Author

The tests explicitly set the session tolerance per test case, so changing the default value shouldn't impact the test cases. Good idea/reminder to check though

TC 1
TC 2

@Olshansk Olshansk added this to the Network Cost milestone Jun 28, 2023
@Olshansk
Copy link
Member

Let's hold off on either merging this on or closing it until the latest portal metrics are stable. Will send an announcement out in discord and I added a note at the top offhttps://github.com/pokt-network/pocket-core/releases/edit/RC-0.10.0

@nodiesBlade
Copy link
Contributor Author

nodiesBlade commented Jul 5, 2023

discord messages for visibility

Blade — 07/03/2023 5:21 PM
hey - I don’t think we need to merge it in anymore now that the portal issue is resolved. But if it’s already been done, it’s no big deal

Olshansky — 07/03/2023 6:59 PM
I'm still of the opinion we should merge it in to achieve feature parity with what we had before, and so there's no surprise to node runners who don't read the announcements or release in detail.

Olshansky — 07/03/2023 8:48 PM:
@blade, let me know what you think from the perspective of a node runner.

Blade — 07/03/2023 8:50 PM:
I think that's a fair approach. 👍

Blade — 07/03/2023 9:11 PM:
The more I think about it, the more I agree that setting the default value as zero makes sense. We can always make adjustments later on once we start using it as a gateway!

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Thank you @PoktBlade! Approving it per the results of this discussion

Screenshot 2023-07-05 at 11 00 24 AM

@Olshansk Olshansk merged commit 589869d into staging Jul 5, 2023
5 checks passed
@Olshansk Olshansk deleted the PoktBlade-patch-1 branch July 5, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small Pull request is small
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants