fix: double deposit bug #3108
fix: double deposit bug #3108
Conversation
| @@ -397,6 +397,7 @@ pub mod pallet { | |||
| T::EnsureWitnessedAtCurrentEpoch::ensure_origin(origin)?; | |||
| for (intent_id, address) in addresses { | |||
| IntentActions::<T, I>::remove(&address); | |||
| FetchParamDetails::<T, I>::remove(intent_id); | |||
There was a problem hiding this comment.
We should also add this statement above in on_initialize where we check if the ingress intents have expired and we stop witnessing if they have (remove the FetchParamDetails for that intent)
There was a problem hiding this comment.
Broke some tests 😭
There was a problem hiding this comment.
I think there is still an edge case where it could panic -> there is a race condition between calling finalize_ingress which would disable witnessing, and potential second ingress on the same address (it can happen before the StopWitnessing is emitted by finalize_ingress). It would still panic in that case.
In general I think we should make it explicit to the relayers that they should, in their UI/interface, design safeguards so that people cant ingress twice on the same address
There is an open issue for something similar: #3106 Also, we recently decided that we'll keep the ingress channel open, meaning we no longer want to close it when we witness an ingress, so this should fix the possibility of a panic.
In general, we can't rely on the UI for anything that can make us panic. |
We should not clean up the FetchParamDetails until the ingress is finalised.
dandanlen
force-pushed
the
fix/double-deposit-bug
branch
from
52bbb4f to
4c3bb60
Compare
|
The author of this PR, dandanlen, is not an activated member of this organization on Codecov. |
…m-egress * origin/main: chore: ensure ceremony failure tag is always used (#3141) Feat: Signing Ceremony with Multiple Keys (#3123) refactor: add epoch index to rotation state (#3138) chore: update pr template with mention of Linear. (#3135) fix: double deposit bug (#3108) hotfix: failing ci chore: add dockerfiles (#3134) Update issue templates (#3133) Debugging for localnets (#3132) fix: polkadot key lookup: key status is irrelevant (#3130) fix: add commit_hash (#3131) fix: add missing `/` 🤫 (#3127) Move db out of multisig (#3112) # Conflicts: # state-chain/pallets/cf-ingress-egress/src/mock.rs # state-chain/pallets/cf-ingress-egress/src/tests.rs
We should not clean up the FetchParamDetails until the ingress is finalised.
Fixes #3099
However note that this was partly borne out of confusion about how ingresses should operate. Right now the ingress expires after it's first use. So the test script will likely still return an error (but it won't panic!).
There is a pending PR #3032 which might address some of this but I'll raise a separate issue to make sure it doesn't fall through the cracks.