TxPause&SafeMode for Astar #1501
Conversation
ashutoshvarma
left a comment
ashutoshvarma
left a comment
There was a problem hiding this comment.
looks good!
Do we want to add integration tests for it?
ipapandinas
left a comment
ipapandinas
left a comment
There was a problem hiding this comment.
LGTM, thanks for opening issue #1503
| | RuntimeCall::TechnicalCommittee(_) | ||
| | RuntimeCall::Sudo(_) | ||
| | RuntimeCall::Democracy( | ||
| pallet_democracy::Call::external_propose_majority { .. } |
There was a problem hiding this comment.
I've thought about that but decided to exclude it since it's not a privileged action.
In a recent exploit scenario, the problematic account could have voted to put funds into locked state, which would be problematic for us. This is why it's blocked.
Rest of the whitelisted calls are all privileged ones, that require collective agreement.
There was a problem hiding this comment.
@ermalkaleci I've allowed Vote and added a comprehensive integration test to demonstrate the full flow of how recovery would go.
There was a problem hiding this comment.
For preimage, sure, makes sense if we want to run referendum.
Multisig can also be alloed, since the embedded calls will be filtered again.
For oracle, I'm not sure.
We can allow them, but keep the option to block those calls using tx-pause I guess.
EDIT: added, thanks for the suggestion
|
/bench astar |
|
Benchmark job failed. |
|
/bench astar pallet_inflation |
|
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/16437771685. |
|
Benchmarks have been finished. |
| | b"Council" | ||
| | b"TechnicalCommittee" | ||
| | b"Sudo" | ||
| | b"TxPause" |
There was a problem hiding this comment.
Same for the tx-pause pallet, its own calls are always allowed.
There was a problem hiding this comment.
I know, but wanted to add it here for completeness, irrelevant of the implementation details.
I could remove it.
There was a problem hiding this comment.
Thanks for the explanation, I don't have a strong opinion, we can keep it
| ) | ||
| | RuntimeCall::Proxy(_) | ||
| | RuntimeCall::TxPause(_) | ||
| | RuntimeCall::SafeMode(_) => true, |
There was a problem hiding this comment.
The safe-mode pallet always allows its own calls.
|
|
||
| // Now use main council to extend safe mode | ||
| let safe_mode_extend_call = RuntimeCall::SafeMode(pallet_safe_mode::Call::force_extend {}); | ||
| propose_vote_and_close!(Council, safe_mode_extend_call, 0); |
There was a problem hiding this comment.
Do we want to check the correct extension against the configuration param? This can be done by reading enteredUntil storage value.
There was a problem hiding this comment.
I want to avoid checking pallet's implementation details.
If duration doesn't properly work, it's not something for integration tests to discover IMO.
| type RuntimeCall = RuntimeCall; | ||
| type PauseOrigin = EnsureRootOrHalfTechCommitteeOrTwoThirdCouncil; | ||
| type UnpauseOrigin = EnsureRootOrHalfTechCommitteeOrTwoThirdCouncil; | ||
| type WhitelistedCalls = TxPauseWhitelistedCalls; |
There was a problem hiding this comment.
When I've added tx_pause into Shibuya, I didn't specified whitelisting on purpose, reference: #1388 (comment)
In fact, I've just removed the
TxPauseWhitelistedCallstype and itsContainstrait implementation.tx-pauseis more granular, we should be able to pause any call in an emergency.
But I guess you're adding it now on purpose to avoid being stuck?
There was a problem hiding this comment.
I haven't checked this, but thanks for the reminder.
I've added it since Pierre commented it, and avoiding being stuck just came as an extra reason later.
|
@ermalkaleci @ashutoshvarma can you please also check & think about more possible problematic scenarios? |
Minimum allowed line rate is |
|
Merging it since I got approval from everyone, at least once 🙂 |
Pull Request Summary
Introduces safe mode & tx pause support for Astar.