Add sync_status in status Node API #2966

quentinlesceller · 2019-07-23T15:51:23Z

Fix #2946.
Looks like this:

Awaiting peers:

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "sync_status": "AwaitingPeers(true)"
}

HeaderSync:

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "sync_status": "HeaderSync { current_height: 271043, highest_height: 0 }"
}

BodySync:

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "sync_status": "BodySync { current_height: 271043, highest_height: 271044 }"
}

NoSync:

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271044,
    "last_block_pushed": "000000c2f360a342748b9a7998989b03aa9a907abc1d4d53b2862ba2f4e3f161",
    "prev_block_to_last": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "total_difficulty": 790339316641310
  },
  "sync_status": "NoSync"
}

Not very pretty. Suggestions welcome.

quentinlesceller · 2019-07-23T16:00:17Z

I might implement a specific to_string method for SyncStatus so it looks a bit prettier.
I can also use serde to_json_string but it's not very pretty either.
e.g. :

"sync_status": "\"NoSync\""
"sync_status": "{\"BodySync\":{\"current_height\":271067,\"highest_height\":271069}}"

antiochp

I think we should decouple the status itself from the value in the json completely.
Just have an explicit mapping of status -> api representation. Its more work now but I think it insulates us from breaking changes in the future.
Some sync statuses are purely internal and I think we want to be able to rework these without needing to care about clients of the api.

API clients only really care about syncing/synced or whatever we decide to call them.

antiochp · 2019-07-24T15:53:09Z

api/src/handlers/server_api.rs

 }

 impl StatusHandler {
 	fn get_status(&self) -> Result<Status, Error> {
 		let head = w(&self.chain)?
 			.head()
 			.map_err(|e| ErrorKind::Internal(format!("can't get head: {}", e)))?;
-		let sync_status = w(&self.sync_state)?.status();
+		let sync_status = match w(&self.sync_state)?.status() {
+			SyncStatus::NoSync => "no_sync".to_owned(),


Might be worth getting some feedback around no_sync to see if this makes sense to people.
Maybe it should just be running or something as this is going to be the most common state people see (presumably).

Yeah agree, to be fair the last commit was just that I wanted to save my work before testing the new master :). Will ask for feedback.

antiochp · 2019-07-24T15:55:28Z

api/src/types.rs

@@ -78,15 +78,22 @@ pub struct Status {
 	pub connections: u32,
 	// The state of the current fork Tip
 	pub tip: Tip,
+	// The current sync status
+	pub sync_status: String,


If this is sync vs synced then maybe this is just status and not sync_status?
I'm not sure if we are exposing the actual "sync status" here or exposing a "status" that we are using our internal sync_status to derive from.

The reason I'm being pedantic here is from the original issue raised about this.

"Expose the Basic Status > Current Status: Running field in the Node API"

I'd argue nobody actually cares about the sync status itself - they just care about when the node is finally "running".

Changed to status with two possible status: syncing or running.

nijynot · 2019-08-02T14:35:54Z

I'm thinking something like this:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "chain": {
    "height": 0,
    "last_block_pushed": "...",
    "prev_block_to_last": "...",
    "total_difficulty": 0
  },
  "status": "BodySync"
}

I think it makes sense to make the status prop string only, and then put the relevant data in other separate JSON props.

GHLover · 2019-08-07T10:37:12Z

What has frustrated me using the API is there is no way for me to determine how much has been synced. I think the status should reveal to the client the sync task, but in addition to that the current network height. That way the client knows that it X% of the blockchain has been processed.

Currently there is no way to know this. Another area I would expect to see this is in the peer API. The listing of connected peers should be able to list that peers network height just like it does when you run the grin server TUI.

antiochp · 2019-08-09T15:06:47Z

I think the status should reveal to the client the sync task, but in addition to that the current network height. That way the client knows that it X% of the blockchain has been processed.

The complexity here is Grin/MW is not a straightforward block-based sync. We don't start syncing from height 1 and then sync all blocks in increasing height.

The sync process is something more like -

sync headers (these are sequential from 1 to current height)
sync full state (txhashset.zip from horizon, approx 48 hours previous)
- full set of kernels (full kernel MMR)
- UTXO set and associated data (MMR hashes etc.)
sync full blocks from horizon to current height

Given all this its not trivial to say we are 50% sync'd, for example.
We can show the various steps along the way but there is no clean sequential progress available.

Note: The order and internal impl of these steps is also subject to change. We have some ideas around improving performance of the large "full state sync" middle step. So any assumptions about what constitutes say 75% sync'd may be subject to change over time.

quentinlesceller · 2019-08-21T19:39:21Z

@nijynot @antiochp given the fact that API user wants to see the progress of header sync or txhashset download and have a "stable" structure to parse here is an idea: adding an additional field sync_info along with the sync_status.

For example during body_sync:

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "sync_status": "body_sync"
  "sync_info": {
    "current_height": 271043,
    "highest_height: 271044
    }
}

and no sync (the sync_info field is not required anymore):

$ curl http://127.0.0.1:3413/v1/status
{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 11,
  "tip": {
    "height": 271043,
    "last_block_pushed": "0000040a7ef8e6d52bb6ec2c34bc6595ae76e7724d11838a65266289d06cc6db",
    "prev_block_to_last": "00000d7aa3fe0d1ba382259b9c4ea5577a7b425303a2cf09fd0f0d81a7a3cc8f",
    "total_difficulty": 790336634189653
  },
  "sync_status": "no_sync"
}

Downside is that the sync_info is not a fixed struct but a variable struct which change during "txhashset_download" (for example).

DavidBurkett · 2019-08-21T20:19:54Z

Not sure if this is the best way, but what I do in Grin++ is just have the status API always return header_height, block_height, network_height, state_process_status (percentage of txhashset processed), and sync_status (eg. no_sync, header_sync, waiting_for_peers, etc). That's granular enough for the front-end to calculate and display just about anything it needs, and there's no extra parsing based on what the sync_status is - the structure is always the same.

quentinlesceller · 2019-08-21T20:25:20Z

@DavidBurkett this could do. Not a big fan of having a "state_process_status": 100" all the time but I guess it simplifies the structure quite a bit.

DavidBurkett · 2019-08-21T20:29:42Z

@quentinlesceller Yea, I struggled with it as well. Not quite as consistent and useful as the other fields, but it's a messy world we live in :)

antiochp · 2019-08-22T10:35:09Z

👍 on including both sync_info (variable) and sync_status (fixed). This gives clients the flexibility they need and keeps it extensible.
Say we split the txhashet download out into two separate steps - we'd add a new sync_status value and a corresponding sync_info struct that may contain different fields.

DavidBurkett · 2019-08-22T10:56:07Z

@antiochp I'm good with that, just want to point out that you can add to the sync_info, but you can't really add a new sync_status in a backward compatible way.

hashmap · 2019-08-22T11:19:08Z

api/src/handlers/server_api.rs

@@ -83,3 +86,43 @@ impl Handler for StatusHandler {
 		result_to_response(self.get_status())
 	}
 }
+
+/// Convert a SyncStatus to correspond sync_status API string
+fn sync_status_to_api_string(sync_status: SyncStatus) -> String {


Does it make sense to implement Display trait on SyncStatus to make it more reusable?

quentinlesceller · 2019-08-22T15:48:40Z

Okay here is the latest version:

Awaiting peers:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 9,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "awaiting_peers"
}

Header sync:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 18,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "header_sync",
  "sync_info": {
    "current_height": 302628,
    "highest_height": 314100
  }
}

Txhashset download:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 5,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "txhashset_download",
  "sync_info": {
    "downloaded_size": 4848000,
    "total_size": 350384784
  }
}

Txhashset validation:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 18,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "txhashset_validation",
  "sync_info": {
    "kernels": x,
    "kernels_total": x,
    "rproof": x,
    "rproof_total": x
  }
}

Body sync:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 18,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "body_sync",
  "sync_info": {
    "current_height": 302628,
    "highest_height": 314100
  }
}

No sync:

{
  "protocol_version": 1,
  "user_agent": "MW/Grin 2.0.1-beta.1",
  "connections": 9,
  "tip": {
    "height": 285220,
    "last_block_pushed": "0000004a17933a8c5330b87c88e969a8dc1c9b2495e6600b5cdb06b9bd0fd095",
    "prev_block_to_last": "000001c10ab2c05dbb8a0f356c60c8638f2374766a3d271813ecc41528c46340",
    "total_difficulty": 833895047654262
  },
  "sync_status": "no_sync"
}

Only minor disadvantage that I see is that uint64 are serialised as number and not a string with can be problematic for javascript implementations. However with the current number (blocks, rproof, etc...) we should be fine.

quentinlesceller · 2019-08-22T17:23:26Z

I've removed the internal state from the mapping. Ready for review.

quentinlesceller added 2 commits July 23, 2019 17:48

Add sync_status in status Node API

9fd98bf

Rustfmt

afced3a

quentinlesceller added the enhancement label Jul 23, 2019

quentinlesceller requested a review from antiochp July 23, 2019 15:51

quentinlesceller requested a review from hashmap July 23, 2019 16:01

antiochp reviewed Jul 23, 2019

View reviewed changes

Very basic conversion

e23efe6

quentinlesceller changed the base branch from milestone/2.x.x to master July 24, 2019 15:40

quentinlesceller changed the title ~~[2.x.x] Add sync_status in status Node API~~ Add sync_status in status Node API Jul 24, 2019

antiochp reviewed Jul 24, 2019

View reviewed changes

quentinlesceller added 4 commits August 1, 2019 11:30

Simple status with syncing and running

08aff26

Rustfmt

7edc7c1

Mapping from SyncStatus to String

383734a

Rustfmt

d1b7f31

antiochp mentioned this pull request Aug 21, 2019

Release Planning: Grin v2.1.0 mimblewimble/grin-pm#179

Closed

Merge branch 'master' into status

4e83e76

hashmap reviewed Aug 22, 2019

View reviewed changes

quentinlesceller added 2 commits August 22, 2019 11:35

Add sync info field

db2ba23

Rustfmt

a65b8b3

quentinlesceller added 2 commits August 22, 2019 13:19

Fix typo

46c8995

Remove internal state which are just replaced by 'syncing'

b639565

lehnberg added this to the 2.1.0 milestone Aug 30, 2019

quentinlesceller mentioned this pull request Sep 3, 2019

API improvements for better management #2987

Closed

quentinlesceller merged commit b209244 into mimblewimble:master Sep 17, 2019

quentinlesceller mentioned this pull request Sep 25, 2019

Add missing doc and route for Node API #3059

Merged

quentinlesceller deleted the status branch February 14, 2020 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sync_status in status Node API #2966

Add sync_status in status Node API #2966

quentinlesceller commented Jul 23, 2019 •

edited

Loading

quentinlesceller commented Jul 23, 2019 •

edited

Loading

antiochp left a comment •

edited

Loading

antiochp Jul 24, 2019

quentinlesceller Jul 24, 2019

antiochp Jul 24, 2019

antiochp Jul 24, 2019

quentinlesceller Aug 1, 2019

nijynot commented Aug 2, 2019 •

edited

Loading

GHLover commented Aug 7, 2019

antiochp commented Aug 9, 2019

quentinlesceller commented Aug 21, 2019 •

edited

Loading

DavidBurkett commented Aug 21, 2019

quentinlesceller commented Aug 21, 2019

DavidBurkett commented Aug 21, 2019

antiochp commented Aug 22, 2019

DavidBurkett commented Aug 22, 2019

hashmap Aug 22, 2019

quentinlesceller commented Aug 22, 2019 •

edited

Loading

quentinlesceller commented Aug 22, 2019

Add sync_status in status Node API #2966

Add sync_status in status Node API #2966

Conversation

quentinlesceller commented Jul 23, 2019 • edited Loading

quentinlesceller commented Jul 23, 2019 • edited Loading

antiochp left a comment • edited Loading

Choose a reason for hiding this comment

antiochp Jul 24, 2019

Choose a reason for hiding this comment

quentinlesceller Jul 24, 2019

Choose a reason for hiding this comment

antiochp Jul 24, 2019

Choose a reason for hiding this comment

antiochp Jul 24, 2019

Choose a reason for hiding this comment

quentinlesceller Aug 1, 2019

Choose a reason for hiding this comment

nijynot commented Aug 2, 2019 • edited Loading

GHLover commented Aug 7, 2019

antiochp commented Aug 9, 2019

quentinlesceller commented Aug 21, 2019 • edited Loading

DavidBurkett commented Aug 21, 2019

quentinlesceller commented Aug 21, 2019

DavidBurkett commented Aug 21, 2019

antiochp commented Aug 22, 2019

DavidBurkett commented Aug 22, 2019

hashmap Aug 22, 2019

Choose a reason for hiding this comment

quentinlesceller commented Aug 22, 2019 • edited Loading

quentinlesceller commented Aug 22, 2019

quentinlesceller commented Jul 23, 2019 •

edited

Loading

quentinlesceller commented Jul 23, 2019 •

edited

Loading

antiochp left a comment •

edited

Loading

nijynot commented Aug 2, 2019 •

edited

Loading

quentinlesceller commented Aug 21, 2019 •

edited

Loading

quentinlesceller commented Aug 22, 2019 •

edited

Loading