Possible consensus bug with proposer slashings

[JustinDrake]

The process_proposer_slashings function has two key steps, namely verifying the proposer slashings in parallel and then calling slash_validator once per proposer_slashing. The parallel proposer slashing verification seems to assume proposer slashings can be verified independently but there is some state dependency in assert is_slashable_validator(proposer, get_current_epoch(state)) (see spec here).

Specifically, if the same (unslashed) validator is put several times in process_proposer_slashings then the second proposer slashing should throw an error. As far as I can tell slash_validator in Lighthouse will not throw an error if the validator is already slashed and process_proposer_slashings will not throw an error if the same validator is slashed several times.

[protolambda]

The spec has no explicit concurrency, Lighthouse may have optimized it wrong, and the edge case deserves a test. After first slashing, validator.slashed = True, and is-slashable will be False and abort the 2nd slashing.

[protolambda]

Can confirm the bug;

lighthouse/eth2/state_processing/src/per_block_processing.rs

Line 288 in 32074f0

.par_iter()

The operations are verified with a parallel iterator, only reading the state. Rust doesn't complain here. And then the slashings are applied, and the two proposer slashings that were valid on the pre-state, but not the specced intermediate state, would be confirmed.

Maybe it's a good idea to adjust slash_validator in the spec to explicitly check is_slashable_validator, so this can never happen again?

[paulhauner]

Thanks @JustinDrake, this is a great pickup.

I think this comes from back when I was working on serenity-benches, so I'll take responsibility for this.

We do this type of parallel execution for several block-operations processing functions because we saw a significant gain from doing BLS verification on multiple cores. Now we're doing batch BLS verification and we never* actually verify signatures during per_block_processing/per_state_processing I think we can realistically remove all these par_iter. @michaelsproul keen to hear your thoughts, as always.

*: we still verify signatures during deposits processing. Perhaps keeping only concurrent signature verification here is sensible.

[michaelsproul]

I had a look at attestation processing, and it looks like it's safe because it only verifies the indexed attestations in parallel, and does the actual slashing in series, checking the non-empty slashing condition here:

lighthouse/eth2/state_processing/src/per_block_processing/verify_attester_slashing.rs

Line 97 in 26bdc29

verify!(!slashable_indices.is_empty(), Invalid::NoSlashableIndices);

For proposer slashings, I agree we could remove the parallel iterator entirely. And I guess we could defensively remove all parallel iterators from state processing?

---

Oh damn, and I just checked the op pool, which I thought handled this correctly, but it looks like it doesn't :\ It takes great care to avoid including duplicate attester slashings, but just piles in all the proposer slashings that are compatible with a single state:

lighthouse/eth2/operation_pool/src/lib.rs

Lines 237 to 246 in 15a3af8

	let proposer_slashings = filter_limit_operations(
	self.proposer_slashings.read().values(),
	|slashing| {
	state
	.validators
	.get(slashing.signed_header_1.message.proposer_index as usize)
	.map_or(false, |validator| !validator.slashed)
	},
	T::MaxProposerSlashings::to_usize(),
	);

[JustinDrake]

I think we can realistically remove all these par_iter

In this particular instance it may be wise to remove the par_iter especially if the performance gains are small. Having said that, as more of a meta comment for the codebase as a whole, I quite like that you guys are going "beyond" the spec to squeeze out performance :)

[JustinDrake]

As a meta question for @zedt3ster, why didn't the fuzzing infrastructure catch this bug or the recent consensus bug in Prysm regarding reward calculations?

[protolambda]

@JustinDrake Prysm introduced the bug just a few days before testnet launch with a last-minute v0.11 update. Can't catch something with fuzzing when the fuzzer doesn't run the code. But I'm sure it could have, if it were up to date.
For lighthouse, it's more questionable. Maybe the double proposer slashing is too specific to find with the fuzzer. The fuzzer would need to figure out to duplicate the exact slashing operation, not impossible though.

[zedt3ster]

So our beacon fuzz fuzzers were targeting v0.10 up until a few days ago. As mentioned by @protolambda, the code that was instrumented didn't include the bug in question. We've also ran into multiple issues with the Prysm build (that are now resolved) which prevented us from integrating them into our continuous fuzzing.

We're currently revamping the differential fuzzer completely to include new mutation engines and leveraging structural fuzzing. In fact, we just identified an SSZ decoding/processing bug in Prysm. The differential part of the fuzzers is currently "on hold", we'll be sharing a new design this week - this type of bug could definitely be picked up in the future.

[michaelsproul]

I'm going to fix this