simplify eth2_network error handling #5765
Conversation
This PR gets rid of a bunch of redundant exception handling through async raises guarantees. More can be removed once libp2p gets properly annotated. Includes nim-eth bump for more accurate timeout handling
| raise exc | ||
| except CatchableError as exc: | ||
| # switch.disconnect shouldn't raise | ||
| warn "Unexpected error while disconnecting peer", |
There was a problem hiding this comment.
Don't we use warning for more actionable information - we are telling the users that there is something wrong with their setup or that we have observed something wrong happening with the network which may explain poor performance, etc. Here, we seem to be telling them about a potential bug in our implementation or alternatively about a very benign networking issue that should not affect their node in any way.
Previously, this was a trace-level message, perhaps DEBUG is justified, but warning seems too much.
There was a problem hiding this comment.
all these warnings would start showing if libp2p starts raising new unexpected errors - it's a transition measure while libp2p upgrades to chronosv4
There was a problem hiding this comment.
There is no benefit in using warn vs debug for these kind of messages. They create a risk that we'll run into an unintended increase in log volume (to potentially dangerous levels depending on the particular issue that triggers the increase).
There was a problem hiding this comment.
#5822 - warn would increase chances that we notice it - swallowed exceptions have historically been a source of several security issues - anyway, hopefully we can be rid of them completely soon.
| raise exc | ||
| except CatchableError as exc: | ||
| # TODO remove once libp2p supports `raises` | ||
| warn "Unknown error when opening stream", exc = exc.msg |
There was a problem hiding this comment.
Ditto, using warn to report implementation issues.
| try: | ||
| await noCancel stream.close() | ||
| except CatchableError as exc: | ||
| warn "Unexpected error while closing notification stream", |
| except CancelledError as exc: | ||
| raise exc | ||
| except CatchableError as exc: | ||
| warn "Unexpected error", exc = exc.msg |
| except CancelledError as exc: | ||
| raise exc | ||
| except CatchableError as exc: | ||
| warn "Unexpected error", exc = exc.msg |
| `postSerialization` | ||
| `tracing` | ||
| let `msgBytes` = getOutput(`outputStream`) | ||
| let `msgBytes` = |
There was a problem hiding this comment.
This improvement can be applied to the other copies of the p2pProtocol DSL.
There was a problem hiding this comment.
it is only useful if the DSL is also upgraded to track exceptions, which is a much larger task involving significant changes to rlpx itself - if/when that is done, it would probably make sense to port this together with other changes
|
|
||
| debug "Request manager blob tick", | ||
| blobs_count = len(fetches), | ||
| succeed = succeed, |
There was a problem hiding this comment.
Why were these properties not considered useful?
There was a problem hiding this comment.
cc @etan-status these changes are part of a blob syncing bugfix
There was a problem hiding this comment.
If you are referring to #5766 , I did not touch this file at all.
| req: SyncRequest): Future[BeaconBlocksRes] {.async.} = | ||
| proc getBlocks[A, B](man: SyncManager[A, B], peer: A, | ||
| req: SyncRequest): Future[BeaconBlocksRes] {. | ||
| async: (raises: [CancelledError], raw: true).} = | ||
| mixin getScore, `==` |
There was a problem hiding this comment.
This mixin statement doesn't seem to serve any purpose here.
There was a problem hiding this comment.
indeed, probably reasonable to clean it up in the next round of fixes (mixin wasn't part of the scope of this PR)
| req: SyncRequest | ||
| ): Future[BlobSidecarsRes] {.async.} = | ||
| req: SyncRequest): Future[BlobSidecarsRes] | ||
| {.async: (raises: [CancelledError], raw: true).} = | ||
| mixin getScore, `==` |
| request = req | ||
| return | ||
|
|
||
| func combine(acc: seq[Slot], cur: Slot): seq[Slot] = |
There was a problem hiding this comment.
I would look deeper into this. The code seems to use a very inefficient quadratic algorithm with a ton of unnecessary sequence copies and memory allocations. Leave a TODO comment perhaps.
There was a problem hiding this comment.
#5794 might be an opportunity for this - that said, this PR just removed the unnecessary exception handling, the existing logic wasn't looked at
* unrevert rest of #5765 * rm stray e2store docs changes * reduce diff * fix indent --------- Co-authored-by: Jacek Sieka <[email protected]>
This PR gets rid of a bunch of redundant exception handling through async raises guarantees.
More can be removed once libp2p gets properly annotated.
Includes nim-eth bump for more accurate timeout handling