Skip to content

Conversation

@martintomazic
Copy link
Contributor

@martintomazic martintomazic commented Jul 15, 2025

@netlify
Copy link

netlify bot commented Jul 15, 2025

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 66596ab
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-oasis-core/deploys/68acd47718a6c500078a0041

@martintomazic martintomazic force-pushed the martin/feature/show-checkpoints branch from 627efc8 to 747cdd3 Compare July 15, 2025 08:28
@martintomazic martintomazic force-pushed the martin/feature/show-checkpoints branch from 747cdd3 to 0587cae Compare July 15, 2025 08:41
@martintomazic martintomazic force-pushed the martin/feature/show-checkpoints branch from 0587cae to 1bcd02f Compare July 28, 2025 23:40
@martintomazic martintomazic marked this pull request as ready for review July 30, 2025 21:14
@martintomazic martintomazic changed the title go/control: Show local checkpoints height and block hash go/control: Show local checkpoint heights Jul 30, 2025
@martintomazic martintomazic force-pushed the martin/feature/show-checkpoints branch 2 times, most recently from 970e3aa to 2222a19 Compare July 31, 2025 15:37
Comment on lines 844 to 845

status.Checkpoint = n.fetchCheckpointStatus(ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you previously had checkpointer enabled (and you have some checkpoints), then you restart a node and disable checkpoints, you would still show old checkpoints. Could this confuse node operators?

Copy link
Member

Choose a reason for hiding this comment

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

You could also show information on whether the checkpointer is currently enabled or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could also show information on whether the checkpointer is currently enabled or not.

You mean to only show it if the checkpointer is enabled? I think this would be good.

Alternative is to show the status of the checkpointer directly:
consensus.checkpointer.enabled/heights or possibly consensus.state.checkpointer.enabled/heights?

I notice for the consensus we tend to display "domain view" (current solution), whereas for the runtimes we tend to show status of independent components (alternative solution).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have kept is simple and only show it if checkpointer is enabled.

@codecov
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

❌ Patch coverage is 52.38095% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.36%. Comparing base (5d1c3ea) to head (66596ab).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
go/consensus/cometbft/full/common.go 52.38% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6265      +/-   ##
==========================================
- Coverage   64.37%   64.36%   -0.02%     
==========================================
  Files         697      697              
  Lines       67836    67854      +18     
==========================================
+ Hits        43670    43674       +4     
- Misses      19155    19171      +16     
+ Partials     5011     5009       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@martintomazic martintomazic force-pushed the martin/feature/show-checkpoints branch from 1bc6907 to e4e7863 Compare August 25, 2025 21:20
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.

control status: Show checkpoint height(s)

4 participants