PIP-34 Fix MaxChains Paramater (FMP)

Proposals Improvements
1

Attributes

Summary

When POKT launched, mainnet had a parameter called “MaxChains”. This parameter was designed so the network could have power over deciding how many chains Servicers could serve.

However, unbeknownst to the designers, it had a flaw, where any changes to MaxChains would only effect new node, and not effect existing stakes. This means that this parameter can not be changed without created two classes of nodes within POKT (new nodes and old nodes), each with different reward possibilities depending on their MaxChains.

This proposal (and the attached code) is simply a fix to the MaxChains Parameter so that it operates as intended for the POKT DAO.

Motivation

This is about fixing MaxChains so POKT isn’t perpetually locked with the current setting. The motive is to allow the parameter to be what it was intended to be.

Specification

MaxChains is fixed by adding a MaxChains check at the beginning of a node’s session. This is when the protocol checks to make sure a node qualifies to be sent relays and generate reward.

These are some of the checks POKT already does before each session to see if a node qualifies for a session:

  1. Does this node have at least 15k POKT staked?
  2. Is the node currently jailed?

To fix MaxChains, this proposal adds one more:

  1. Does this node’s MaxChains equal to or less than protocol parameter?

With this check, only nodes who are in compliance with the MaxChains parameter (equal to or less than the MaxChains) will be able to be a part of sessions to serve relays. Any that are not in compliance will not be given new sessions.

There is no other negative effect (like a slash) if a node has more chains than the MaxChain parameter… they just simply won’t be given sessions to generate reward.

@h5law is a protocol developer and has submitted a PR to pocketcore. It’s ready for review and testing by the protocol team and others participating in the next hard fork release. @bulutcambazi currently has two proposals being considered by the DAO to initiate a hard fork upgrade, and the MaxChains fix would simply be included in the next release.

Rationale

MaxChains should work the way MaxChains was intended to work. This is a matter of patching an existing bug in its implementation.

No parameter changes are being proposed in this proposal, this is simply a fix to the protocol.

Dissenting Opinions

MaxChains shouldn’t be fixed because I don’t want MaxChains to ever be changed

This isn’t about deciding on what the MaxChains parameter should be, it’s about fixing the protocol so that MaxChains works how it was intended to work. This is a simple protocol fix.

If the POKT network is to have a MaxChains parameter, it should be in a workable condition. This proposal is not setting MaxChains to a new value, it is simply fixing the protocol to make that parameter usable.

Any parameter value changes to MaxChains must go through a PUP, and this is not a PUP.

Adding the MaxChains check will require a fork, which it so much work.

Two other PIPs are currently up to vote which are consensus breaking changes, requiring a fork. The MaxChains fix would simply be added to the next release with the other features, whenever that may be.

Is this the GANDALF proposal?

No. The GANDALF proposal is economic research proposal on the potential benefits of different MaxChain settings, but this proposal is simply about fixing MaxChains. In the GANDALF discussion, we all realized that the MaxChains parameter doesn’t work as expected, therefore this proposal is about providing a solution.

Any parameter value changes to MaxChains (like GANDALF) must go through a PUP, and this is not a PUP.

Viability

This PR was submitted by @h5law who is a protocol developer. After seeing a discussion in Telegram about MaxChains, he jumped into the code, saw it was a quick fix, and submitted a PR in a matter of minutes. It is a very simple PR, that can be easily tested with the other coming upgrades.

Implementation/Audit

Once approved by the DAO the PR will be allowed to be included in the next RC release for testnet testing. It will be a part of the standard process for testing/auditing a new RC release.

Copyright

Copyright and related rights waived via CC0.

6 Likes
2

This seems pretty straightforward. I don’t see anything to either agree or disagree with. Unless a bug is found in the pull request, this PIP has my vote.

4 Likes
3

Updated title to PIP-34 as C0D3R have a proposal for PIP33 currently. Otherwise, this is a minor change (although consensus breaking) and I am behind it.

5 Likes
4

This seems like a sensible bug fix. Rolled up with another consensus-breaking mechanism as suggested will hopefully cause minimal disturbance.

As long as no one spots an error in the code, this looks pretty straightforward and I support it.

5 Likes
5

Not much to say on this, just following the due process to include a minor change in the next release.
This changes nothing and just adds a security check, easy approve.

3 Likes
6

Fixing the MaxChains parameter is now up for vote :point_down:

https://gov.pokt.network/#/proposal/0xba18a09245a6fbe34f07da80fc43f11d4e889f86acb9167ebe94909163e46e44

2 Likes
7

Glad to see we’re fixing MaxChain param. But I think the approach currently being taken in PR 1582 touches way more files than necessary, and unnecessarily breaks consensus when alternate approaches are available that don’t break consensus and touch less code. Please see my comment on PR 1582 on Github for suggested alternate approaches.

One other item of note regarding enforcing MaxChain at the level of rewards, if that becomes the final approach that is implemented: This can create a persistent, non-negligible gap between the total relays serviced and the subset of this total that is rewarded. Currently RTTM is calculated based on total relays under the assumption that this is synonymous with the number of rewarded relays. If a gap opens between total relays and rewarded relays, then daily mint could be persistently lower than expected based on RTTM. This is not a negative or a problem. Just bringing it up so all are aware of the implications at the system level of enforcing MaxChains at rewards vs enforcing upstream (eg at session selection).

1 Like
8

I think this needs some clarification. I was a little hard for me to follow. Please correct me if I’m wrong.

For anyone wondering, the link to the PR comment is this:

This comment refers to a change proposed by @msa6867 , not to the actual PR that was voted.
The proposed change was to move the verification from the session construction to the rewards minting. This would enable the nodes with more than maxChains to keep on servicing but they would only receive rewards for up to maxChains chains.

The warning is only relevant if the change proposed by @msa6867 is implemented. It is not relevant to the PR voted and approved here. The RTTM modulation mechanism will still be valid.

1 Like
9

@RawthiL , this is not correct. Can you check the PR again… I am referencing the current PR 1582 commits, in which @h5law transitioned original concept to a reward-based enforcement based on suggestions from @msmania. I threw out an alternate method to accomplish a reward-based enforcement that doesn’t break consensus and touches less code just in case the dev team felt it was important (e.g., for transition stability) to enforce at reward rather than upstream. That being said, on my github comment I was mainly suggesting to enforce (again in a non-consensus-breaking manner) at session selection rather than at reward. If PR-1582 gets merged as is, this comment will apply.

Also, just to clarify, PIP-34 authorizes the fix via a PR, but the code changes in progress in PR 1582 at the time of vote does not get locked in stone … The dev team has leeway to refine the code used to patch the bug, and in fact has been doing so based on input from @msmania, etc.

1 Like
10

Thanks for the clarification, this should not have happened.
The vote was on a feature that worked using the check on the session generation. Modifying the mechanism after the vote should not be allowed.

The commit 998f378ba9feaf73966aa2e41bc01e80e092cb38 is from 2023-11-06, and the voting started on 2023-11-01 and ended on 2023-11-08.

The new method, if implemented, does not only changes the essence of whats was voted, but also generates other dependencies, like the RTTM calculation change thata @msa6867 pointed out.

I’m against this new mechanic, it creates a problem on calculation of the RTTM and enables a mad-man (or slooply-man) attack where a node staked for more chains than maxChains reduces the rewards on nodes that are only staked for the right amount of chains by starving the nodes.

Enabling an starvation attack and adding complexity to the RTTM calculation is not the right call. Enforcing on the session generation is the right way to go, the chances that no node runner will stake on a chain are really low. Why would this happen if the node runner already has the working blockchain node? Also, we have community chains to quickly fix this, since these abandoned chains will mean constant and uncontested POKT for the first node runner that stakes.

1 Like
11

This approach is very bloated and touches way more files than would be needed in other approaches.

I would specify to the devs what files you believe should or should not be changed. As it stands now, your comment doesn’t actual provided actionable steps, it just suggests that they are doing bad work. I would suggest you point out to the devs which parts of their code you see as unnecessary, or provide your own PR with the code to address their shortcomings.

A much simpler approach would be the following. Pass the current MaxChain parameter to session selection code and update the session selection code so as to effectively ignore any chain ID in a node’s list of chain IDs beyond the first MaxChain entries. The only downside is that enforcement would be done at the level of portal rather than protocol, so in a multi-portal environment, you would need to make sure each portal enforces this.

To this part of your github comment :point_up: I believe the discussion should happen here and not on Github. Github is for talking about the code specially, not alternative solutions.

Below :point_down: is my comment and further discussion should happen here IMO.

@msa6867 You suggest that enforcement of MaxChains should be on the gateways themselves, but that is much harder to enforce. Systems would have to be created to audit sessions and gateway traffic to ensure that gateways are enforcing a network wide rule. The network already has a consensus layer, to ensure that rules are being followed, so it makes way more sense to utilize it for MaxChains, as was it’s intended design.

I personally felt that the network’s own consensus layer made more sense then creating an off-chain mechanism of enforcement (which is essentially an off-chain consensus mechanism). This is why I did not pursue the gateway enforcement route and believe in this current route. The DAO has already voted on a consensus level change, so I would suggest you take alternative suggestions to the DAO/forum first. This PR is specifically focused on a consensus level fix so any discussion on an alternative gateway level mechanism should happen over there.

1 Like
12

Perhaps I am not stating it correctly, and I might be entirely wrong about being able to accomplish the fix without breaking consensus. I think I was suggesting going back more to @h5law original vision (enforce at session)… but rather than doing an enforcement check as to whether a node is in compliance, simply ignore any entry in the node’s chain ID list beyond the first MaxChain entries when selecting nodes for a session.

1 Like
13

I’ve gone back and forth in my mind about this point. You’re probably right about this being a bad thing, but I can also see the other side of the equation, which is that if a whole volunteer army wants to altruistically provide network service without pay, do we really care? Service is being provided and at less mint cost to the protocol… if starving of paid network providers really becomes a problem, it would be pretty easy to change the RTTM calculation from using total relays to using total paid relays…

1 Like
14

The problem is not that, if I recall correctly, if a node is not staked in a chain at the time of the proof, it will lose the rewards from the claim. So, enforcing this at the level of claim will remove the problem of the RTTM calculation, since we only count claim+proof relays. These “altruist” relays will not be seen anywhere.
Yes, the apps will get free relays, the problem is that the protocol wont see the benefits and the honest node runners could get starved. No proof mean no relay counts which means no burning.
A big bad actor could starve a chain and coordinately exit after the honest node runners leave. We need to account for all relays in the protocol.

Edit: The option of enforcing only at minting level will create some weird entries in the blocks where a node is awarded relays that it is not allowed to process. This has not only the problem of RTTM but also data consistency in the blocks.

1 Like
15

I could see that being a viable option. If the MaxChain is set to 5, then the protocol would only assign sessions to the first 5 chains a node is staked to. If a node is staked to 15 nodes, then it won’t receive sessions for the other 10 chains. They are essentially rendered useless. If that is what you suggesting… then that makes sense. A dev would need to apply the code to validate it would work as expected (I’m not sure an “ignore” mechanism is used in this way on the protocol).

Your original comment though was negative in nature towards other’s work, and focused on other points like gateway enforcement and not breaking consensus… neither of which applies since consensus still needs to be changed for your suggest fix and it is still happening on the protocol level (not on the gateway level).

I’d suggest rephrasing your comment so that what you are actually suggesting can be considered.

1 Like
16

@msa6867 @shane @RawthiL I have left a comment in the PR that explains some of the comments made. I think any discussion of rules of enforcement regarding a change to the parameter probably better go through another vote if/when the parameter changes. The first n approach could be an idea but was not what was voted on.

On that note we voted on a change to session joining, however this approach opens the door to a potential issue of chains being unservicable during the change over period if the parameter changes, negatively affecting QoS which would not be ideal. Enforcement at the claim level makes a lot of sense here. Chains will still be serviced just claims will not be valid which incentivised node runners to update their chains staked. My inital approach at the session level makes the most sense at first glance but if the event of a negative impact is possible (no matter how unlikely) it should be avoided.

1 Like
17

@h5law, the DAO voted to fix MaxChains at the protocol level, not hard locked into any specific code to do that. An initial PR was given, but that doesn’t mean code can’t be changed as the the development progresses. The DAO agreed that a fix is desired, so now the protocol and testing teams can implement in the best method. Past proposals have had code changes after a vote was passed, so this is no different.

The PR acts not as the final code, but an initial implementation for the protocol team to consider if the DAO desires the feature.

@msa6867 modified his position above and provided a simple suggestion of instead of using “allow” or “deny” method, the protocol could just only assign session to the first number of chains. So if MaxChains was set to 5, and a node was staked on 15 chains, then the protocol would simply only assign sessions to the first 5 chains in the node’s stake string. This way nodes aren’t penalized when the param is changed, the network will only assign sessions to the first slots up until MaxChains is reached.

Is that a possible implementation worth considering?

1 Like
18

@shane this idea makes sense, instead of enforcing n max chains only consider the first n staked for? At first glance it seems like the current code would be minimal changes to this idea. This check would still be able to go into the validate claims method as well, a claim would only be valid if it was one of the first n chains you are staked for, where n=MaxChains. @msa6867 wdyt?

1 Like
19

yes, that is what I am suggesting… preferably back a t the session level

My bad. Will do

1 Like
20

That makes sense to me. I think it was a good suggestion by @msa6867 and will make the transition easier for node runners, since there nodes aren’t completely disqualified from the param change.

I think it would be best suited at the session level though, so that nodes only get good sessions that they will rewards from. Would going to the session level be possible?

2 Likes