Bugfix: Invalid blocks were produced in the presence of invalid deposits #3639
Conversation
| @@ -296,7 +296,7 @@ proc process_deposit*(cfg: RuntimeConfig, | |||
| else: | |||
| # Verify the deposit signature (proof of possession) which is not checked | |||
| # by the deposit contract | |||
| if skipBlsValidation in flags or verify_deposit_signature(cfg, deposit.data): | |||
| if verify_deposit_signature(cfg, deposit.data): | |||
There was a problem hiding this comment.
we have tests that don't create signatures for deposits but expect them to succeed
| @@ -296,7 +296,7 @@ proc process_deposit*(cfg: RuntimeConfig, | |||
| else: | |||
| # Verify the deposit signature (proof of possession) which is not checked | |||
| # by the deposit contract | |||
| if skipBlsValidation in flags or verify_deposit_signature(cfg, deposit.data): | |||
| if verify_deposit_signature(cfg, deposit.data): | |||
There was a problem hiding this comment.
we have tests that don't create signatures for deposits but expect them to succeed
|
good find 🎉 |
zah
force-pushed
the
invalid-deposits-min-fix
branch
from
c80bf84 to
74c7d4d
Compare
Since we were not verifying BLS signature in blocks that we produce, we were failing to notice that some deposits need to be ignored (due to having an invalid signature). Processing these deposits resulted in a different ending state after the state transition which caused our blocks to be rejected by the network.
zah
force-pushed
the
invalid-deposits-min-fix
branch
from
74c7d4d to
2a9dd4f
Compare
| @@ -124,9 +124,11 @@ proc addTestBlock*( | |||
| sync_aggregate, | |||
| default(ExecutionPayload), | |||
| noRollback, | |||
| cache) | |||
| cache, | |||
| verificationFlags = {skipBlsValidation}) | |||
There was a problem hiding this comment.
ping @zah this should be flags
There was a problem hiding this comment.
I think this should be removed, but there wasn't enough time before the release to investigate how the tests can be fixed. Otherwise, feel free to rename it although the flags name would be a bit misleading in a function called makeBeaconBlock (the flags don't affect how the block is created, but rather how it's executed (or verified), hence my name).
There was a problem hiding this comment.
we use flags as a convention for this variable throughout the spec code which makes the code easier to read - it would be nice if this code adhered to it as well
Since we were not verifying BLS signature in blocks that we produce,
we were failing to notice that some deposits need to be ignored (due
to having an invalid signature). Processing these deposits resulted
in a different ending state after the state transition which caused
our blocks to be rejected by the network.