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.

Sign up for GitHub

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

Jump to bottom

Fix erroneously double incremented and decremented consumers #2037

Open
liamaharon opened this issue Oct 26, 2023 · 16 comments
Open

Fix erroneously double incremented and decremented consumers #2037

liamaharon opened this issue Oct 26, 2023 · 16 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@liamaharon
Copy link
Contributor

liamaharon commented Oct 26, 2023 •
edited
Loading

Needs to be implemented once #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 #1970
@liamaharon liamaharon added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 26, 2023
@liamaharon liamaharon mentioned this issue Oct 26, 2023
Merged
@liamaharon liamaharon self-assigned this Oct 26, 2023
@xlc
Copy link
Contributor

xlc commented Oct 26, 2023

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

liamaharon added a commit that referenced this issue Oct 30, 2023
@liamaharon
Stop Balances pallet erroneously double incrementing and decrementi…
0aeab38 
…ng consumers (#1976)

Closes #1970

Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
#2037
girazoki pushed a commit to moondance-labs/polkadot-sdk that referenced this issue Dec 1, 2023
@liamaharon @girazoki
Stop Balances pallet erroneously double incrementing and decrementi…
d93a55a 
…ng consumers (paritytech#1976)

Closes paritytech#1970

Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
paritytech#2037
@liamaharon liamaharon removed their assignment Mar 18, 2024
@liamaharon liamaharon added the C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. label Mar 18, 2024
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
@liamaharon
Stop Balances pallet erroneously double incrementing and decrementi…
9e7a4a2 
…ng consumers (paritytech#1976)

Closes paritytech#1970

Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
paritytech#2037
bkchr pushed a commit that referenced this issue Apr 10, 2024
@serban300 @bkchr
Define RangeInclusiveExt (#2037)
b8f4086 
* Define RangeInclusiveExt

* Use RangeInclusiveExt

* Add docs
@Ank4n Ank4n mentioned this issue May 13, 2024
Closed
@Ank4n
Copy link
Contributor

Ank4n commented May 13, 2024

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

@ggwpez
Copy link
Member

ggwpez commented May 13, 2024

I think that it would help to have #4398 deployed before we start starting migrations for the references...

@dastansam
Copy link
Contributor

can I take this issue? @ggwpez @liamaharon

@ggwpez
Copy link
Member

ggwpez commented May 14, 2024

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

@liamaharon
Copy link
Contributor Author

I think the way forward is to scrape bad update_locks invocations (#1970), and write a migration to fix consumers based on that. A bit fiddly, but should be feasible I think?

@ggwpez
Copy link
Member

ggwpez commented May 15, 2024

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
Copy link
Contributor Author

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.

@Ank4n Ank4n mentioned this issue May 17, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue May 17, 2024
@Ank4n
Allow pool to be destroyed with an extra (erroneous) consumer referen…
2e36f57 
…ce on the pool account (#4503)

addresses #4440 (will
close once we have this in prod runtimes).
related: #2037.

An extra consumer reference is preventing pools to be destroyed. When a
pool is ready to be destroyed, we
can safely clear the consumer references if any. Notably, I only check
for one extra consumer reference since that is a known bug. Anything
more indicates possibly another issue and we probably don't want to
silently absorb those errors as well.

After this change, pools with extra consumer reference should be able to
destroy normally.
Ank4n added a commit that referenced this issue May 20, 2024
@Ank4n
Allow pool to be destroyed with an extra (erroneous) consumer referen…
3f5b01b 
…ce on the pool account (#4503)

addresses #4440 (will
close once we have this in prod runtimes).
related: #2037.

An extra consumer reference is preventing pools to be destroyed. When a
pool is ready to be destroyed, we
can safely clear the consumer references if any. Notably, I only check
for one extra consumer reference since that is a known bug. Anything
more indicates possibly another issue and we probably don't want to
silently absorb those errors as well.

After this change, pools with extra consumer reference should be able to
destroy normally.
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this issue Jun 5, 2024
@Ank4n @hitchhooker
Allow pool to be destroyed with an extra (erroneous) consumer referen…
5398b62 
…ce on the pool account (paritytech#4503)

addresses paritytech#4440 (will
close once we have this in prod runtimes).
related: paritytech#2037.

An extra consumer reference is preventing pools to be destroyed. When a
pool is ready to be destroyed, we
can safely clear the consumer references if any. Notably, I only check
for one extra consumer reference since that is a known bug. Anything
more indicates possibly another issue and we probably don't want to
silently absorb those errors as well.

After this change, pools with extra consumer reference should be able to
destroy normally.
@Juanma0x
Copy link

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

@ggwpez
Copy link
Member

ggwpez commented Jun 10, 2024

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

#4503 i suppose. Should be in the SDK 1.13 release.

@Juanma0x
Copy link

Juanma0x commented Jul 5, 2024

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

Example: 14E8ZGhchDwhB3igyEudWJ5j3gPGKwT5ki3yStUy3XbgdkwX

@ggwpez
Copy link
Member

ggwpez commented Jul 5, 2024

There was a fix for pools: #4503 but the general issue is not addressed by this.

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
@Ank4n @TarekkMA
Allow pool to be destroyed with an extra (erroneous) consumer referen…
2274513 
…ce on the pool account (paritytech#4503)

addresses paritytech#4440 (will
close once we have this in prod runtimes).
related: paritytech#2037.

An extra consumer reference is preventing pools to be destroyed. When a
pool is ready to be destroyed, we
can safely clear the consumer references if any. Notably, I only check
for one extra consumer reference since that is a known bug. Anything
more indicates possibly another issue and we probably don't want to
silently absorb those errors as well.

After this change, pools with extra consumer reference should be able to
destroy normally.
@Juanma0x
Copy link

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
Copy link
Member

ggwpez commented Oct 21, 2024

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
Copy link
Contributor

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@xlc @kianenigma @Ank4n @ggwpez @liamaharon @dastansam @Juanma0x