paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 695 forks source link

Fix erroneously double incremented and decremented consumers #2037

Open liamaharon opened 1 year ago

liamaharon commented 1 year ago

Needs to be implemented once https://github.com/paritytech/polkadot-sdk/pull/1976 is merged and released.

Two options have been floated:

  1. write a script that scrapes extrinsics for accounts that had their consumers erroneously incremented and generates a migration to decrement them
  2. introduce fn fix_consumers(origin, who: AccountId) in the System pallet, with an additional frame_system::Config::CountConsumers trait with a function fn count_consumers(who: &AccountId) -> u32, which could possibly be written on a per-pallet basis and composed in a tuple and used by AllPalletsWithSystem`.

also see https://github.com/paritytech/polkadot-sdk/issues/1970

xlc commented 1 year ago

2 will be useful and we can easily convert it into a try_runtime invariant to ensure the consistency in future

Ank4n commented 6 months ago

Has there been any progress/update to this? We have recently found pools are failing to be destroyed because of this bug.

ggwpez commented 6 months ago

I think that it would help to have https://github.com/paritytech/polkadot-sdk/pull/4398 deployed before we start starting migrations for the references...

dastansam commented 6 months ago

can I take this issue? @ggwpez @liamaharon

ggwpez commented 6 months ago

I think there is no clear forward yet. I guess the count_consumer approach could work, but its a bit complicated in general.

liamaharon commented 6 months ago

I think the way forward is to scrape bad update_locks invocations (https://github.com/paritytech/polkadot-sdk/issues/1970), and write a migration to fix consumers based on that. A bit fiddly, but should be feasible I think?

ggwpez commented 6 months ago

Could work, but we have this issue not just on Polkadot - but presumably on all chains...
What you mean is the go through an archive and record and erroneous double-decrements?

liamaharon commented 6 months ago

Yes, write a script that replays transactions and checks after every lock/unlock whether there was an erroneous increment/decrement. Then it crafts a migration which can be inserted into the runtime to fix them.

Juanma0x commented 5 months ago

Is there any news about allowing currently 'destroying' pools to be destroyed?

ggwpez commented 5 months ago

Is there any news about allowing currently 'destroying' pools to be destroyed?

https://github.com/paritytech/polkadot-sdk/pull/4503 i suppose. Should be in the SDK 1.13 release.

Juanma0x commented 4 months ago

Pools can be destroyed now. I believe this issue can be closed.

Example: 14E8ZGhchDwhB3igyEudWJ5j3gPGKwT5ki3yStUy3XbgdkwX

ggwpez commented 4 months ago

There was a fix for pools: https://github.com/paritytech/polkadot-sdk/pull/4503 but the general issue is not addressed by this.

Juanma0x commented 4 weeks ago

I'm curious to know how progress on this issue is coming along. We've had several users reach out about being unable to empty their accounts fully, and I'd like to provide them with an update. Any information would be helpful!

ggwpez commented 3 weeks ago

We've had several users reach out about being unable to empty their accounts fully

The only thing remaining is the ED? There is no straight forward fix for this historic issue. Fixing this properly would require quite some effort - i think more than we currently have available. Maybe @kianenigma has some ideas.

kianenigma commented 3 weeks ago

I'm curious to know how progress on this issue is coming along. We've had several users reach out about being unable to empty their accounts fully, and I'd like to provide them with an update. Any information would be helpful!

Can you give us an idea of how many users will be affected by this, and what is the total amount of DOTs that will be "locked" while this is unfixed?

You can probably do this by writing a script that scans all accounts, and checks those that cannot be killed now.

This will help us prioritize this properly.

Juanma0x commented 2 weeks ago

I'm more than happy to communicate the solution to affected users, but locating every affected account may not fall entirely within my scope. But let me know if I can help in any other way.