Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve pallet asset implementation to avoid potential reentrency issue. #10432

Closed
gui1117 opened this issue Dec 7, 2021 · 3 comments · Fixed by #10473
Closed

Improve pallet asset implementation to avoid potential reentrency issue. #10432

gui1117 opened this issue Dec 7, 2021 · 3 comments · Fixed by #10473
Assignees
Labels
J0-enhancement An additional feature request. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.

Comments

@gui1117
Copy link
Contributor

gui1117 commented Dec 7, 2021

Currently the method FrozenBalance::on_died is called while there is some pending change in storage.
If the implementation of on_died call into the pallet asset again, there can be some change which are discarded when we finally commit the pending change above.

This is/will be documented with #10431 but ideally it should be improved. It is quite error prone.

Sidenote: similar issue in the past #7605

@gui1117 gui1117 added J0-enhancement An additional feature request. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Dec 7, 2021
@ggwpez
Copy link
Member

ggwpez commented Dec 8, 2021

I think the issue runs deeper since the implementor of died does not know which mutate closures are currently on the call-stack.
Lets say pallet-A::do() calls pallet-B::do() in a mutate; now any callbacks that pallet-B::do() invokes should not modify the storage of pallet-A either, right?
So writing to the storage of any pallet could potentially result in lost updates.
@thiolliere

@gui1117
Copy link
Contributor Author

gui1117 commented Dec 9, 2021

I think the issue runs deeper since the implementor of died does not know which mutate closures are currently on the call-stack. Lets say pallet-A::do() calls pallet-B::do() in a mutate; now any callbacks that pallet-B::do() invokes should not modify the storage of pallet-A either, right? So writing to the storage of any pallet could potentially result in lost updates. @thiolliere

the implementor of died knows that storage of pallet asset have pending change that will override any other change written to its storage.

Lets say pallet-A::do() calls pallet-B::do() in a mutate; now any callbacks that pallet-B::do() invokes should not modify the storage of pallet-A either, right? So writing to the storage of any pallet could potentially result in lost updates.

Yes, but asset function must not be called inside a mutate, otherwise indeed, the died implementation can break original pallets storage.
You should never call another pallet function while having some pending change which will override any other change, except when it is explicitly written that it won't call into your original pallet.

Here the API is error prone, because people can easily miss the constraint in the doc.

@ggwpez
Copy link
Member

ggwpez commented Dec 10, 2021

Thanks for the explanation. Then I would like to pick up this issue if you don't mind.
@thiolliere

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants