Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENR Parser and Monitor Fixes #111

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Conversation

praetoriansentry
Copy link
Contributor

@praetoriansentry praetoriansentry commented Aug 9, 2023

Description

This PR adds the function polycli enr which will parse an Ethereum ENR and convert it to enode for simpler processing.

This PR also includes fixes for some panics in polycli monitor. Additonally, we're swapping out the uncle graph for a pending transaction graph which won't be aligned with the block history, but will be more interesting to look at. This PR also includes a fix to stop polycli monitor from trying to fetch blocks forever

Jira / Linear Tickets

Testing

  • Test ENR with output from polycli p2p crawl
  • Long scroll test against https://polygon-rpc.com
  • Monitor test against local geth
  • Monitor test against empty chain (the cause of the panic)

@praetoriansentry praetoriansentry marked this pull request as ready for review August 9, 2023 23:11
Copy link
Contributor

@gatsbyz gatsbyz left a comment

Choose a reason for hiding this comment

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

🔥

if !currentlyFetchingHistoryLock.TryLock() {
return fmt.Errorf("the function is currently locked")
}
defer currentlyFetchingHistoryLock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

lock 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

so previously there were multiple fetches going on..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I believe so
fetchBlocks is called in a loop forever and by default it was always calling appendOlderBlocks. That was it's own issue. But it seemed possible that fetchBlocks -> appendOlderBlocks could be called in parallel with renderMonitorUI -> redraw -> appendOlderBlocks

I was also considering running appendOlderBlocks in a different go routined in order to avoid blocking the UI while we wait for blocks to load. If we do that then it's possible that appendOlderBlocks could be called multiple times while pressing the down arrow.

I didn't end up implementing that, but if we do it in the future it should make appendOlderBlocks work safely (I hope)

@@ -338,7 +404,7 @@ func setUISkeleton() (blockTable *widgets.List, grid *ui.Grid, blockGrid *ui.Gri
termUi.sl3 = widgets.NewSparkline()
termUi.sl3.LineColor = ui.ColorBlue
slg3 := widgets.NewSparklineGroup(termUi.sl3)
slg3.Title = "Uncles"
slg3.Title = "Pending Tx"
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -145,7 +178,9 @@ func appendOlderBlocks(ctx context.Context, ms *monitorStatus, rpc *ethrpc.Clien
err := ms.getBlockRange(ctx, from, to, rpc)
if err != nil {
log.Error().Err(err).Msg("There was an issue fetching the block range")
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

🆗

@praetoriansentry praetoriansentry merged commit 3dfe6bd into main Aug 9, 2023
5 checks passed
@praetoriansentry praetoriansentry deleted the jhilliard/monitor-panic-fix branch August 9, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants