Potential fix for softfork guard issue #491
Conversation
There was a problem hiding this comment.
My understanding is that a softfork guard with operator set 3 is either a failure (in mempool mode) or a no-op (in consensus mode). I don't see this logic. Is it just outside of the patch?
I would have expected this case to be handled in connection with OperatorSet::Unknown
| @@ -186,10 +186,10 @@ impl Dialect for ChiaDialect { | |||
| match ext { | |||
| // The BLS extensions (and coinid) operators were brought into the | |||
| // main operator set as part of the hard fork | |||
| 0 => OperatorSet::BLS, | |||
| 1 if (self.flags & ENABLE_KECCAK) != 0 => OperatorSet::Keccak, | |||
| 0 | 1 => OperatorSet::Default, | |||
There was a problem hiding this comment.
When I reviewed your first PR, I (for some reason) was thinking that the BLS softfork had id 0, so 1 was the next one. I realize I was wrong. Can you think of ways to prevent this mistake in the future?
There should probably be a comment here explaining what operator set 1 is (or was, prior to hard fork).
Maybe it even makes sense to preserve the OperatorSet::BLS enum value.
| Keccak, // keccak256 operator | ||
| Unknown, |
There was a problem hiding this comment.
I think it would make sense to, briefly, describe the issue we just introduced and how Default and Unknown differ and solve the problem.
| @@ -336,7 +336,7 @@ impl<'a, D: Dialect> RunProgramContext<'a, D> { | |||
| let extension = | |||
| self.dialect | |||
| .softfork_extension(uint_atom::<4>(self.allocator, extension, "softfork")? as u32); | |||
| if extension == OperatorSet::Default { | |||
| if extension == OperatorSet::Unknown { | |||
There was a problem hiding this comment.
this failure must depend on whether we're in mempool or not, right? Do we ignore the "unknown softfork extension" error we issue below, in the caller, if we're in consensus mode?
|
I do not think this is an actual issue, 0 is the BLS extension, not 1 |
Moves keccak256 into softfork extension 2, to prevent conflicts with the old extension 1. It should be treated as a no-op until the fork activates, and the old extension should continue to work in mempool mode. This is a bug fix that allows that.