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

Panic on invalid unsigned phragmen solution #5980

Closed
kianenigma opened this issue May 12, 2020 · 9 comments · Fixed by #6485
Closed

Panic on invalid unsigned phragmen solution #5980

kianenigma opened this issue May 12, 2020 · 9 comments · Fixed by #6485
Labels
I2-security The client fails to follow expected, security-sensitive, behaviour. J0-enhancement An additional feature request.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented May 12, 2020

ensure_none(origin)?;
Self::check_and_replace_solution(
winners,
compact_assignments,
ElectionCompute::Unsigned,
score,
era,
)?
// TODO: instead of returning an error, panic. This makes the entire produced block
// invalid.
// This ensures that block authors will not ever try and submit a solution which is not
// an improvement, since they will lose their authoring points/rewards.

@kianenigma kianenigma added the J0-enhancement An additional feature request. label May 12, 2020
@kianenigma kianenigma added the I2-security The client fails to follow expected, security-sensitive, behaviour. label May 19, 2020
@gui1117
Copy link
Contributor

gui1117 commented May 22, 2020

I'm not sure this is very important in this PR #6106 I see 0.33 weak submission per era.
Moreover weak submission will no longer take weight because it will be refund with #6032

Thus I think it doesn't waste too much.

Also I don't see why there is a security flag here.

@kianenigma
Copy link
Contributor Author

kianenigma commented May 22, 2020

related: #4517 (comment)

Yeah now with proper weight refund perhaps this is less of a security issue. But still, no change allows each block author to submit any potentially a garbage solution with no consequence. Even if we refund, this is taking everyone's time. Moreover, they can submit a garbage solution that will not be refunded, and will be rejected due to an issue at the very end (i.e. bogus score). This allows validators to mess with the chain with no consequence, and again waste everyone's time.

Still, you can argue that if they do this, then they will have less weight to pack other transactions in the block, hence they will gain less profit. But I don't think this is a strong enough guarantee. Panic would disincentivize this because they lose their entire authoring block.

I won't run into this issue, but I think it should be closed long term once we have a stabilised offchain phragmen pipeline and it has been battle tested.

@bkchr
Copy link
Member

bkchr commented May 23, 2020

Panic would disincentivize this because they lose their entire authoring block.

Panic where? In the extrinsic?

@kianenigma
Copy link
Contributor Author

yeah.

@bkchr
Copy link
Member

bkchr commented May 24, 2020

We don't have a mechanism that invalidates a full block on panic of an extrinsic ^^

@kianenigma
Copy link
Contributor Author

We don't have a mechanism that invalidates a full block on panic of an extrinsic ^^

I think what Gav meant was during import: a transaction panics in block import, then this invalidates the whole block no?

@bkchr
Copy link
Member

bkchr commented May 25, 2020

Ahh yes that is correct.

@kianenigma
Copy link
Contributor Author

Once #6173 is merged, if we no longer have invalid unsigned solutions, this will be patched thereafter.

@kianenigma
Copy link
Contributor Author

kianenigma commented Jun 23, 2020

I've been doing more analysis after #6173 and we seem to be doing well. I will soon fix this.

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I2-security The client fails to follow expected, security-sensitive, behaviour. J0-enhancement An additional feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants