ActiveValidatorIndices helper function returns wrong indices

[peterbitfly]

The ActiveValidatorIndices function is supposed to return the indices of the currently active validators.
But when appending to the indices array it appends the index of the validator in the array returned by activeIndicesCache.ActiveIndicesInEpoch(epoch) and not the actual index of the validator.

See:

prysm/beacon-chain/core/helpers/validators.go

Lines 82 to 86 in 0326be8

	for i, v := range state.Validators {
	if IsActiveValidator(v, epoch) {
	indices = append(indices, uint64(i))
	}
	}

That is why a validator with an index greater than the number of active validators will never get any slot proposal or attestation assignments.

Unfortunately the type v1alpha1.Validator that is returned by activeIndicesCache.ActiveIndicesInEpoch(epoch) also does not contain the validator index.

[handelaar2]

validator log as requested by terence

validator.log

[metanull-operator]

validator.log

[terencechain]

Looking at this today

[prestonvanloon]

Could this be why we experience behavior where we don't see any proposers with an index higher than ~500? Those validators are still selected to attest, but never propose.

[prestonvanloon]

Bumping priority because I think this is a P0 bug if the above is true.

[prestonvanloon]

Looking into a regression test for this

[prestonvanloon]

There is no unit test for helpers.ActiveValidatorIndices. Working on a fix now.
I am not sure how this is going to affect syncing the existing chain, it might be invalid and require a restart.

[prestonvanloon]

Confirmed with regression test for ActiveValidatorIndices. This is a bug, I'll try to sync a client with the fix and see if it's runtime breaking.

[prestonvanloon]

Spoke too soon. I cannot confirm this as a bug, even with tests. The i value is the validator's index

[terencechain]

Right I wasn't sure if this was bug, given cache returns empty list if miss and we would have experienced it earlier. Lack of test coverage forsure and we should delete the old cache

[terencechain]

Have a regression test with existing DB set up, will run it through to get a better picture

[peterbitfly]

But state.Validators is an array and not a map hence i will always be the array index. I think it might only return the correct validator indices if it returns all validators (active & inactive ones)?

[prestonvanloon]

This cache doesn't support fork conditions though. When adding feature flag for EnableActiveIndicesCache, this fails with different state Validators. Maybe we need epoch + finalized checkpoint to be confident about a cache hit?

It should be noted that most clients aren't running with this cache on so it would miss 100% of the time, unless they are using experimental caching flags, but even then this seems OK.

[terencechain]

That cache is old and we were suppose to delete a long time ago. It's not supposed to be used. See #3631

[terencechain]

There's only one cache for run time and that's the shuffled committee cache

[prestonvanloon]

Added test here: #4263

I think this is a non-issue. state.Validators returns all validators that ever existed in the system, including active and inactive validators. I am concerned about these caches under forking conditions and finality reversion scenarios, but we'll need to revisit that in a different issue.

[prestonvanloon]

I'll submit fix to delete old active index cache. If it's not used then should be safe to delete and avoid confusion.

[terencechain]

Sounds good. Check if it's not done in #3631. Don't wanna duplicate work :)

[prestonvanloon]

It's probably done in that PR but it looks rotten and does more than delete active index cache, also deletes shuffled indexes.

[prestonvanloon]

Removing this unused cache entirely in #4264. @ppratscher do you think this is resolved?

[terencechain]

I am not sure if it solved it. The bug ultimately lies in archival service retrieving proposer and attesters in old epochs. That's what I'm seeing with my tests. With that said, this is a good fix

[peterbitfly]

I think the bug might be in the ComputeProposerIndex function.

From what I understand it should return indices[candidateIndex] and not just candidateIndex as the return value of ComputeShuffledIndex in this case cannot be greater than len(indices). That would also explain why only validators with an index < len(activeIndices) receive a block proposal assignment.

[prestonvanloon]

Bumping priority based on #4259 (comment). This is another helper with no unit test, I'll try to write a regression test for this scenario.

[terencechain]

Working on this now

[prestonvanloon]

This will be resolved in #4265. Waiting to see if this is consensus breaking on current testnet or not.

If it is, we'll likely merge it into 0.9.2 branch and try to expedite the testnet restart.

[prestonvanloon]

Confirmed this is consensus breaking and will require a testnet restart. I am out of office until Sunday night, I'll pick this up as my top priority as soon as I can get back online.

[terencechain]

@prestonvanloon what are the required actions from this? can delegate

[prestonvanloon]

Thanks @terencechain. Need to get a green PR on #4265.

Following that, we need to focus on 0.9.2 features (#4154) such that we can start testing this at runtime.

[terencechain]

Roger that. On the same page with you. Focusing on 0.9.2 features this week

[rauljordan]

Resolved by #4265