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
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
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
Fixing the MaxChains parameter is now up for vote 
2 Likes
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
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
@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
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
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 
I believe the discussion should happen here and not on Github. Github is for talking about the code specially, not alternative solutions.
Below 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
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
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
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
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
@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
@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
@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
yes, that is what I am suggesting… preferably back a t the session level
My bad. Will do
1 Like
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
No, I strongly disagree.
I voted upon an idea and the implementation. The current code does not do what I expect and I would not vote to approve this version.
If we only consider the top N chains then the problem of sessions without nodes can occurs all the same. If a chain is not normally in the top N staked chains it could not have enough nodes all the same.
(This is under the assumption that such thing can happen, something I find unlikely and is easily avoidable with some planning. )
- Enforcing at proof or minting level makes no sense.
- Keeping nodes with more than
maxChainsstaked is just bloat. - We cannot trust lazy node runners to provide high QoS.
I will be as clear as possible here since I feel that we are trying to adapt a simple straightforward change to enable the continuity of a behavior that should not exist after a parameter change.
Keep the protocol simple.
With nodes that can enter sessions that cannot claim-proof:
- We lose visibility of total relays done in the network. Relays go down (apparently).
- We cannot effectively charge the gateways, they get the free relays not the protocol.
- We cannot burn since we cannot prove the relays done.
- We keep lazy node runners on rotations. Lazy node runners are not synonym of good QoS.
- We bloat the sessions with nodes that are staked but are not receiving rewards, why should they keep the staked blockchain alive? The effective number of real nodes by session will go down.
- The number of nodes by chain will not mean anything.
- A bad actor can simply stake 14 chains to small chains just to f**k on the protocol. If these chains are small enough, you will get horrible sessions in all of them. All this because you allow them to stay staked.
- We kick the problem to Shannon upgrade. Wasn’t this all about “slowly update”? are going to code Shannon to allow legacy nodes with 15 chains? To what end?
We must enforce at session level.
The transition period can be handled off-chain, with correct timing. I don’t want to be exotic states just to allow for a one time transition. Exotic states lead to poor understanding of the protocol.
Just some quick ideas:
- Offending nodes can be slashed or force unstaked.
- We can plan the enforcement of the max chain parameter. The same way we do with any other protocol update, we observe the chain data, when we are sure that we have at least 24 nodes on each chain with only one chain staked, we make the change.
- We set the RTTM to zero during the transition period, you will see how fast node runners will get 24 nodes on each chain. Then compensate after the transition by increasing the RTTM.
None of these will have a footprint in the blocks. None of these will require people to be explained how the world was before Shannon and why some nodes have a super power of having 15 chains staked or why we enable them to do relays for free.
2 Likes
