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

Remove unfreeze feature #3072

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Remove unfreeze feature #3072

merged 2 commits into from
Sep 30, 2024

Conversation

Bushstar
Copy link
Member

@Bushstar Bushstar commented Sep 27, 2024

Summary

  • The unfreeze feature is very unlikely to be approved, this PR will remove the feature from the code base.

Implications

  • Storage

    • Database reindex required
    • Database reindex optional
    • Database reindex not required
    • None
  • Consensus

    • Network upgrade required
    • Includes backward compatible changes
    • Includes consensus workarounds
    • Includes consensus refactors
    • None

@prasannavl
Copy link
Member

Let's leave this in to as the template PR to be merged in later, for the next upgrade or once 4,420,000 is reached based on the results.

@prasannavl prasannavl added protocol Has network protocol changes v/next-network-upgrade Items ready or targeted for upcoming network upgrade release(s) e2 Easier effort // fast one for those familiar with codebase labels Sep 27, 2024
@bernd-mack
Copy link
Contributor

Things that affect our core functionalities in particular must not be left in the release without consideration. Especially when there is currently not even a majority for it and the capacities for testing should not be wasted on functions that are not desired by the master nodes and the community.

If someone wants to, they can add the functions themselves, but this has no place in the release for the general public and should be removed immediately.

@kuegi
Copy link
Contributor

kuegi commented Sep 27, 2024

I agree with Bernd. The potential risk of this being in is massive, with no upside whatsoever.
Testing this would also mean that the removal of it later leads to needed rollbacks on the testnet (which we agreed should not happen). On the other hand, getting untested code into production is also a No-Go.

IMHO the only solution is to remove it before release.

@skutcher
Copy link

i see it the same, just too critical to have it merged as a "backdoor" as long as there is no real need for it

@prasannavl
Copy link
Member

prasannavl commented Sep 27, 2024

We already have other things that are not activated / never activated on chain - this is not new. Let's please not turn this political. This is not untested code. We're testing it (and all the more reason for it to be in changi, and testnet).

It can just be later removed for the next release if there if it's never activated, community decides if it doesn't want the code. For now, this is transparently brought about as proposal. And then proposal is not something without insignificant votes. So, the decision is for the MNs. The capability is there either way - that's it.

We're also moving governance to GitHub, and setgovheight timelock based mechanisms only (with ahead of time, before any set), so there's no backdoor here. All this sudden conversation about this being new risk is unsubstantiated, as we already have many things like these on code, just not activated.

Software engineers shouldn't be playing arbiters of this. That defeats the whole point of governance - that role is assigned to network validators and governance.

This PR is purely there as something to go in later, so it's easy to merge after and be removed once the proposal is resolved.

@kuegi
Copy link
Contributor

kuegi commented Sep 27, 2024

Can you please provide a full list of all people who will have theoretical access to foundation keys after the fork. So who might be able to activate this?

The difference to other features is, that this feature can produce massive damage to the whole system within blocks, if activated accidentally.

@prasannavl
Copy link
Member

We can defer the testing of this on testnet if that's a concern, if / when in a hypothetical case be needed or voted on to make that easier.

@kuegi
Copy link
Contributor

kuegi commented Sep 27, 2024

We can defer the testing of this on testnet if that's a concern, if / when in a hypothetical case be needed or voted on to make that easier.

so you want to put untested code onto mainnet?

@prasannavl
Copy link
Member

so you want to put untested code onto mainnet?

what do you mean untested? we are, and if it wasn't deemed sufficiently safe, we would not have even considered merging in. Just providing another option to avoid a potential testnet rollback if you prefer it to be removed later.

This is turning unproductively contentious, again (from Discord). We're trying to make sure all options are available, including the rationale for this PR - so it's available for the community to merge at a later time, and remove.

@bernd-mack
Copy link
Contributor

I hope that wasn't a serious argument because we used to accept it in the past and we have to continue to do so. In the end, it has never been ok to implement things without a completed DFIP. It was accepted - maybe now is the time to change that. Because who tells me that the current team will even create the next release and then it will stay in there.

So my view remains that it has to go out immediately so that we can take care of the important things, successfully voted DFIPs and improvements. Because all of this burns resources unnecessarily and it starts with the fact that someone has released the implementation instead of concentrating on the other things.

@skutcher
Copy link

totally agree with @bernd-mack

@prasannavl
Copy link
Member

prasannavl commented Sep 27, 2024

Sorry, I disagree with this assessment. Anyone is free to implement any DFIP as long as the proposal is in place and it has non insignificant votes, regardless of where it leans. The only predicament to reject PR would be if it's undeniably spam, has an actual security concern, critical bugs (edit: or performance concerns, indexing data requirements that's detriment to the network, etc that's unresolved).

Otherwise, software engineers are playing arbiters and choosing to play additional role of soft governance, making actual governance pointless. It's upto governance to decide.

hope that wasn't a serious argument because we used to accept it in the past and we have to continue to do so

There's no way to know how things will even work if there's no implementation. Flagged implementation is how both experiments and well modelling of a potential feature will have to work. Not by playing trial and error on economy without implementation. Flagged features are how features move up. Here's it's one potential implementation that's flagged. Without it there's no way to test or visualize. You and I can disagree with it and chose not to be a part of enabling it. But not reject the PR when it's cleanly flagged and left upto the community governance to make the decision.

Let's focus on testing the more important things than this discussion that's heated unnecessarily. When the proposal ends, this can be removed for the next fork if so desired. If it get's further contentious, I'd have no choice but to close the PR to avoid this wasting more time on heated unproductive conversations.

@skutcher
Copy link

Sorry, I disagree with this assessment. Anyone is free to implement any DFIP as long as the proposal is in place and it has non insignificant votes, regardless of where it leans. The only predicament to reject PR would be if it's undeniably spam, has an actual security concern or critical bugs.

Seems u answered it yourself, it should not go in there cause of security concerns great

@prasannavl
Copy link
Member

I realize this in unpopular. But it's upto the governance to decide. Please continue to advocate on governance voting.

If there's an actual bug or security concern in the implementation, please feel free to point out. That would be a valid reason to consider revert if a fix is not viable.

@prasannavl
Copy link
Member

Seems u answered it yourself, it should not go in there cause of security concerns great

The apparent security concern here is completely unsubstantiated. We have many feature flags in the code (that btw has potentially has lot more intrusive abilities than just unfreezing nodes). But it's all based on governance and we're also clamping down on governance execution and slowly deprecating the old governance model by moving everything to timelocked setgovheight only -- which adds safety on anything that's ever activated or deactivated related to these flags.

Not only will they have to go through the governance repo, but also sit through transparent timelock, which can be cleared if anything that hasn't seen due process is detected. (these are all the changes that's in the current governance changes).

So, this security concern is entirely unsubstantiated, because of disagreement. Please take up disagreements through governance and not here. If there's not an actual security concern, will ask not to waste everyone's time here. Thanks.

@bernd-mack

This comment was marked as abuse.

@DeFiCh DeFiCh locked as too heated and limited conversation to collaborators Sep 27, 2024
@prasannavl
Copy link
Member

[Hiding last comment as it's unproductive and inciting a revolt. Stated many times that disagreements be dealt with governance and voting. There is an established mechanism to resolve disputes. Inciting revolts here isn't the way.]

Conversation locked. Please advocate through governance and votes.

@Bushstar Bushstar merged commit 7b0665e into master Sep 30, 2024
24 of 32 checks passed
@Bushstar Bushstar deleted the bush/revert-unfreeze branch September 30, 2024 09:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e2 Easier effort // fast one for those familiar with codebase protocol Has network protocol changes v/next-network-upgrade Items ready or targeted for upcoming network upgrade release(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants