Skip to content

Conversation

paulhauner
Copy link
Member

Issue Addressed

NA

Proposed Changes

Apply two changes to code introduced in #4179:

  1. Remove the ERRO log for when we error on proposer_has_been_observed(). We were seeing a lot of this in our logs for finalized blocks and it's a bit noisy.
  2. Use false rather than true for proposal_already_known when there is an error. If a block raises an error in proposer_has_been_observed() then the block must be invalid, so we should process (and reject) it now rather than queuing it.

For reference, here is one of the offending ERRO logs:

ERRO Failed to check observed proposers block_root: 0x5845…878e, source: rpc, error: FinalizedBlock { slot: Slot(5410983), finalized_slot: Slot(5411232) }

Additional Info

NA

@paulhauner paulhauner added ready-for-review The code is ready for review v4.1.0 Post-Capella minor release labels Apr 14, 2023
// Both of these blocks will be rejected, so reject them now rather
// than re-queuing them.
Err(ObserveError::FinalizedBlock { .. })
| Err(ObserveError::ValidatorIndexTooHigh { .. }) => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, I really failed to spot the infinite re-queuing behaviour that returning true here would have!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed it too, during review!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 14, 2023
@paulhauner
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Apr 14, 2023
## Issue Addressed

NA

## Proposed Changes

Apply two changes to code introduced in #4179:

1. Remove the `ERRO` log for when we error on `proposer_has_been_observed()`. We were seeing a lot of this in our logs for finalized blocks and it's a bit noisy.
1. Use `false` rather than `true` for `proposal_already_known` when there is an error. If a block raises an error in `proposer_has_been_observed()` then the block must be invalid, so we should process (and reject) it now rather than queuing it.

For reference, here is one of the offending `ERRO` logs:

```
ERRO Failed to check observed proposers block_root: 0x5845…878e, source: rpc, error: FinalizedBlock { slot: Slot(5410983), finalized_slot: Slot(5411232) }
```

## Additional Info

NA
@bors
Copy link

bors bot commented Apr 14, 2023

Build failed:

@michaelsproul
Copy link
Member

Looks like a spurious failure from CRIT Unable to listen on libp2p address. There are some other nasty CRITs in the simulator like CRIT Failed to update execution head error: ExecutionLayerMissing, but I suspect they've been there for a while.

@michaelsproul
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Apr 14, 2023
## Issue Addressed

NA

## Proposed Changes

Apply two changes to code introduced in #4179:

1. Remove the `ERRO` log for when we error on `proposer_has_been_observed()`. We were seeing a lot of this in our logs for finalized blocks and it's a bit noisy.
1. Use `false` rather than `true` for `proposal_already_known` when there is an error. If a block raises an error in `proposer_has_been_observed()` then the block must be invalid, so we should process (and reject) it now rather than queuing it.

For reference, here is one of the offending `ERRO` logs:

```
ERRO Failed to check observed proposers block_root: 0x5845…878e, source: rpc, error: FinalizedBlock { slot: Slot(5410983), finalized_slot: Slot(5411232) }
```

## Additional Info

NA
@bors bors bot changed the title Address observed proposers behaviour [Merged by Bors] - Address observed proposers behaviour Apr 14, 2023
@bors bors bot closed this Apr 14, 2023
bors bot pushed a commit that referenced this pull request Apr 19, 2023
## Issue Addressed

NA

## Proposed Changes

Avoids reprocessing loops introduced in #4179. (Also somewhat related to #4192).

Breaks the re-queue loop by only re-queuing when an RPC block is received before the attestation creation deadline.

I've put `proposal_is_known` behind a closure to avoid interacting with the `observed_proposers` lock unnecessarily. 

## Additional Info

NA
xenowits pushed a commit to xenowits/lighthouse that referenced this pull request May 16, 2023
## Issue Addressed

NA

## Proposed Changes

Avoids reprocessing loops introduced in sigp#4179. (Also somewhat related to sigp#4192).

Breaks the re-queue loop by only re-queuing when an RPC block is received before the attestation creation deadline.

I've put `proposal_is_known` behind a closure to avoid interacting with the `observed_proposers` lock unnecessarily. 

## Additional Info

NA
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

NA

## Proposed Changes

Apply two changes to code introduced in sigp#4179:

1. Remove the `ERRO` log for when we error on `proposer_has_been_observed()`. We were seeing a lot of this in our logs for finalized blocks and it's a bit noisy.
1. Use `false` rather than `true` for `proposal_already_known` when there is an error. If a block raises an error in `proposer_has_been_observed()` then the block must be invalid, so we should process (and reject) it now rather than queuing it.

For reference, here is one of the offending `ERRO` logs:

```
ERRO Failed to check observed proposers block_root: 0x5845…878e, source: rpc, error: FinalizedBlock { slot: Slot(5410983), finalized_slot: Slot(5411232) }
```

## Additional Info

NA
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

NA

## Proposed Changes

Avoids reprocessing loops introduced in sigp#4179. (Also somewhat related to sigp#4192).

Breaks the re-queue loop by only re-queuing when an RPC block is received before the attestation creation deadline.

I've put `proposal_is_known` behind a closure to avoid interacting with the `observed_proposers` lock unnecessarily. 

## Additional Info

NA
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
NA

Apply two changes to code introduced in sigp#4179:

1. Remove the `ERRO` log for when we error on `proposer_has_been_observed()`. We were seeing a lot of this in our logs for finalized blocks and it's a bit noisy.
1. Use `false` rather than `true` for `proposal_already_known` when there is an error. If a block raises an error in `proposer_has_been_observed()` then the block must be invalid, so we should process (and reject) it now rather than queuing it.

For reference, here is one of the offending `ERRO` logs:

```
ERRO Failed to check observed proposers block_root: 0x5845…878e, source: rpc, error: FinalizedBlock { slot: Slot(5410983), finalized_slot: Slot(5411232) }
```

NA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v4.1.0 Post-Capella minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants