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.

Sign up for GitHub

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

go/control: Show last consensus height seen by block history #6013

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

go/control: Show last consensus height seen by block history #6013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .changelog/5998.feature.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Show the progress of the history reindex in oasis control status
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually start changelog and git messages with a prefix, in this case go/oasis_node/cmd or go/oasis_node would be fine.
After the title we could explain which field was added and what it shows. See previous logs for examples.

3 changes: 3 additions & 0 deletions go/control/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ type RuntimeStatus struct {
// Storage contains the storage worker status in case this node is a storage node.
Storage *storageWorker.Status `json:"storage,omitempty"`

// BlockHistoryLastConsensusHeight is last consensus height seen by block history.
BlockHistoryLastConsensusHeight int64 `json:"block_history_last_consensus_height"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite long field name. I would try to find a shorter name or move it under history.
Should this field be grouped with LastRetainedRound?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should shorten it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think the cleanest way would probably be to show last round and height as seen by the block history right below storage field. I am thinking about exposing this status from the roothash/api/BlockHistory interface (see):

      "latest_round": 10160924,
      "latest_hash": "85867c19bd6ad43521432e187962e3bb19a9731c79cd64f195821bfd90bd7b24",
      "latest_time": "2025-01-31T11:08:05+01:00",
      "latest_state_root": {
        "ns": "000000000000000000000000000000000000000000000000a6d1e3ebf60dff6c",
        "version": 10160924,
        "root_type": 1,
        "hash": "a914f8c8baf05a47757c1d7f7caecebdf614c8b3d1e20aa33d6855ec96a50ba1"
      },
      "genesis_round": 2995927,
      "genesis_hash": "c9f3ca654531b775d944c85e1f00e76944aaf5de1902acfa082ff76e852dba5e",
      "last_retained_round": 10145927,
      "last_retained_hash": "fad686c7eb077ec1a1a5a1ba1c27027a3788b003c94130e78c7dfb25a1d0e8cd",
      // other fields
      "storage": {
        "status": "syncing rounds",
        "last_finalized_round": 10160923
      },
      "history": {
        "last_round": 10160923,
        "last_height": 25058099
      },

Should this field be grouped with LastRetainedRound?

  1. LastRetainedRound is actually earliest (not latest) runtime block that is also synced to the storage.
    i. You may still get an error and show the latest retained round from the block history even if you don't have it synced to the storage... See. Is this a bug?
  2. History reindex is actually happening and finishes before storage round syncing so ideally this should be separate grouping.

I am thinking could we move last_retained_round and last_retained_hash to storage? This is breaking obviously. More importantly the storage status is received from the storage committee worker, whilst LastRetainedRound and hash are technically overwritten, directly from the runtime storage (see, described in 1.i). As from the docstring the former is synced + finalized, whilst if reading from the storage directly it is only guaranteed to be synced? Not sure what is the difference (if any)?


martintomazic marked this conversation as resolved.
Show resolved Hide resolved
// Provisioner is the name of the runtime provisioner.
Provisioner string `json:"provisioner,omitempty"`
}
Expand Down
11 changes: 11 additions & 0 deletions go/oasis-node/cmd/node/node_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,17 @@
}
}

// Fetch last consensus height seen by block history.
height, err := rt.History().LastConsensusHeight()
Comment on lines +346 to +347
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be useful for oasis-node to report the current reindex round

Notice I report consensus height instead of runtime round. I would argue this is also more appropriate, since reindex iterates over the block height...

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we show the last height and the last round?
Would percent of indexed blocks help the 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.

I agree, all of these would be useful.

if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err == nil {
if err != nil {

n.logger.Error("failed to retrieve last consensus height seen by block history",
"err", err,
"id", rt.ID(),
)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I'm not a big fan of if/else, as it makes code less readable and the reader normally expects only if part which handles the error. To be in-line with the code above, you could assign directly to the status field (even if there is an error, assuming that the returned value is zero value) or use switch like it is done above. I would use the former, just to be in-line with code above.

Maybe this code could be moved up, where we handle history.

status.BlockHistoryLastConsensusHeight = height

Check warning on line 354 in go/oasis-node/cmd/node/node_control.go

View check run for this annotation

Codecov / codecov/patch

go/oasis-node/cmd/node/node_control.go#L354 

Added line #L354 was not covered by tests
}

// Fetch provisioner type.
status.Provisioner = "none"
if provisioner := rt.HostProvisioner(); provisioner != nil {
Expand Down
Loading