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

Para Owner cannot unlock and deregister a Parachain when downgraded to Parathread #4565

Open
2 tasks done
chrisdcosta opened this issue May 24, 2024 · 3 comments
Open
2 tasks done
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@chrisdcosta
Copy link

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

After a parachain has been downgraded to a parathread, the para owner (manager) should be able to deregister the parathread if so desired. The code hints that this should be possible however in practice there is a logic trap:

  • A lock is applied to the parachain when it starts to produce blocks that is not lifted when the chain is downgraded.
  • A parathread cannot unlock itself because it no longer produces blocks.
  • The para owner is not allowed to remove the lock.
  • A lock can therefore only be removed by Root.
  • There is no appropriate governance track to unlock a parathread.

This appears to be a bug following this question on stack exchange.

The affected file is found at polkadot-sdk/polkadot/runtime/common/src/paras_registrar/mod.rs

The context is that the para owner should not be able to remove a lock from a running parachain, because that is not decentralised. I agree with this.

However I would also argue that when a parachain is down-graded it may need to be unlocked to either perform a parachain swap or to de-register - either of those case involve some centralised decision making and therefore the para owner / manager must be given the power to unlock the parathread.

Steps to reproduce

This can be performed on any non-production relaychain:

  1. Register a parachain ID with an owner.

  2. Add the PVF.

  3. Upgrade to parachain for 1 lease period.

  4. When the parachain automatically downgrades to a parathread following the end of the lease, attempt to de-register.

The Parathread cannot be de-registered due to a lock

  1. Attempt to remove the lock by the para owner

The Para owner will get a BadOrigin error.

@chrisdcosta chrisdcosta added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels May 24, 2024
@xlc
Copy link
Contributor

xlc commented May 24, 2024

It will be a centralization risk if parathread/on-demand parachain owner can just remove it without any restrictions. I will say this is intended behaviour and there isn't much we can do to improve without introducing new risks.

@bkchr
Copy link
Member

bkchr commented May 24, 2024

@chrisdcosta you beat me to create an issue for this ;) It was on my list!

Generally, we should not just allow the parachain owner to deregister them as they like, as @xlc said. However, currently there is no way at all to unregister your chain if you are downgraded to a parathread, because we don't support this right now. In the future when we have on-demand, this will now be such a big problem, as the chain will always be able to buy one more block and deregister itself. In the future there will also not be anymore any distinguishing between on-demand or parachain on this level. All of them are parachains, just having a different way of acquiring blockspace.

chrisdcosta added a commit to chrisdcosta/polkadot-sdk that referenced this issue May 24, 2024
The `remove_lock()` function did not allow a para manager to unlock the chain in the circumstance where it was a parathread as described in the issue paritytech#4565

This code fixes that logic trap, and implements some additional improvements:

* a new event `Unlocked`
* new error type `ParaNotOwner`
* a function to determine only three allowed callers, returning a caller type or error
* implemented more robust test in `para_lock_works()`
* added getter functions to storage
@chrisdcosta
Copy link
Author

chrisdcosta commented May 24, 2024

@xlc I agree with your comment in the current implementation, because it provides no further checks and no distinction is made between the states. This could be improved in order to provide more specific conditions.

There is another issue however - I think a PVF swap cannot be made either once a parachain has been downgraded, and this does require some form of central coordination between parties wishing to perform this - or indeed to address @bkchr point as new functionality becomes available, downgraded chains would also be unable to upgrade their own PVF to implement any necessary changes.

Lastly, it could be viewed that funds are unreasonably trapped by this situation and as there is no governance process to address it I think the best way in the interim is to provide code with specific conditions and a robust test.

I have created a pull request #4568 for review. It contains some minor improvement noted in text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

No branches or pull requests

3 participants