search

pokt-network / pocket-core

Official implementation of the Pocket Network Protocol
http://www.pokt.network
MIT License
210 stars 103 forks source link

Update default session sync allowance #1560

Closed nodiesBlade closed 1 year ago

nodiesBlade commented 1 year ago

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: https://github.com/pokt-network/pocket-core/pull/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.

Olshansk commented 1 year ago

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

nodiesBlade commented 1 year ago

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

nodiesBlade commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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!