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

RPC status call gives wrong height when syncing #2261

Closed
webmaster128 opened this Issue Jul 28, 2018 · 6 comments

Comments

4 participants
@webmaster128
Contributor

webmaster128 commented Jul 28, 2018

Expected behavior

The RPC status call returns the same height as the HTTP during sync

Actual behavior

The RPC status call returns a fixed height and broadhash for a very ling time, even if the node's height increases during sync.

In this example, I called the RPC status every 2 seconds on my node between 20:12:01 and 20:36:40 UTC. The returned status increases only a handful of times and I get the same height for a very long period of time

height number of status responses
5030757 57
5038050 149
5039766 51
5041515 405
5044650 78

Steps to reproduce

Repeatedly run the RPC status call on a given node that is syncing

Which version(s) does this affect? (Environment, OS, etc...)

Testnet, 1.0.0-rc.1

@MaciejBaj

This comment has been minimized.

Show comment
Hide comment
@MaciejBaj

MaciejBaj Jul 30, 2018

Member

HTTP GET api/node/status response returns the height from modules.blocks.lastBlock.get().height - the height is always top.

RPC status returns the height out of Peer headers stored in System module. During the sync process headers are not updated after every block.

Headers will be updated only when blocks.processBlocks will be called with broadcast = true, which happens when:

  1. Block is being applied after receiving through broadcast,
  2. Block is being generated after successful forging.

Headers are also updated directly after the whole sync process finished -
https://github.com/LiskHQ/lisk/blob/1.0.0-rc.2/modules/loader.js#L940.

It will happen un-deterministically after node encounters an error during sync 5 times. That's why your height gathered through rpc.status was updated on particular cliffs - only when your sync loop stopped.

I wouldn't consider the current behaviour as a wrong one - during sync Node cannot be considered as a valid Peer to sync with for others. There is no point to broadcast headers during sync loop multiple times each second after every block application. RPC should provide the valuable messages for P2P connections purposes only. HTTP public API should be considered as a source of information for a user.
However, we know that the sync process overall can be improved and we are keeping it in mind while preparing the future development plans.

Member

MaciejBaj commented Jul 30, 2018

HTTP GET api/node/status response returns the height from modules.blocks.lastBlock.get().height - the height is always top.

RPC status returns the height out of Peer headers stored in System module. During the sync process headers are not updated after every block.

Headers will be updated only when blocks.processBlocks will be called with broadcast = true, which happens when:

  1. Block is being applied after receiving through broadcast,
  2. Block is being generated after successful forging.

Headers are also updated directly after the whole sync process finished -
https://github.com/LiskHQ/lisk/blob/1.0.0-rc.2/modules/loader.js#L940.

It will happen un-deterministically after node encounters an error during sync 5 times. That's why your height gathered through rpc.status was updated on particular cliffs - only when your sync loop stopped.

I wouldn't consider the current behaviour as a wrong one - during sync Node cannot be considered as a valid Peer to sync with for others. There is no point to broadcast headers during sync loop multiple times each second after every block application. RPC should provide the valuable messages for P2P connections purposes only. HTTP public API should be considered as a source of information for a user.
However, we know that the sync process overall can be improved and we are keeping it in mind while preparing the future development plans.

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Jul 30, 2018

Contributor

Thanks for the elaborate reply. I am happy that this behavior is not the result of a bug but expected.

Contributor

webmaster128 commented Jul 30, 2018

Thanks for the elaborate reply. I am happy that this behavior is not the result of a bug but expected.

@nazarhussain

This comment has been minimized.

Show comment
Hide comment
@nazarhussain

nazarhussain Jul 31, 2018

Contributor

@MaciejBaj I believe during sync process:

  1. We should keep updating own headers after every bock
  2. So rpc.status can have valid height, and let on other peer to decide either sync from this peer or not
  3. We should broadcast headers after sync process finished

Any information published through any interface (HTTP/WebSocket) must be valid and accurate information. So if a node is on particular height, both interfaces should respond with accurate and same height. HTTP returning one height and WebSocket returning other height is absolutely acceptable behavior in my perspective.

Contributor

nazarhussain commented Jul 31, 2018

@MaciejBaj I believe during sync process:

  1. We should keep updating own headers after every bock
  2. So rpc.status can have valid height, and let on other peer to decide either sync from this peer or not
  3. We should broadcast headers after sync process finished

Any information published through any interface (HTTP/WebSocket) must be valid and accurate information. So if a node is on particular height, both interfaces should respond with accurate and same height. HTTP returning one height and WebSocket returning other height is absolutely acceptable behavior in my perspective.

@MaciejBaj

This comment has been minimized.

Show comment
Hide comment
@MaciejBaj

MaciejBaj Aug 2, 2018

Member

@nazarhussain OK.
The issue is then to update the Node headers after every block application during sync, regardless of broadcast = false flag passed to 'blocks.processBlocks' function:
https://github.com/LiskHQ/lisk/blob/development/modules/blocks/verify.js#L839

Member

MaciejBaj commented Aug 2, 2018

@nazarhussain OK.
The issue is then to update the Node headers after every block application during sync, regardless of broadcast = false flag passed to 'blocks.processBlocks' function:
https://github.com/LiskHQ/lisk/blob/development/modules/blocks/verify.js#L839

@MaciejBaj MaciejBaj added *easy and removed discussion labels Aug 2, 2018

@MaciejBaj MaciejBaj added this to New Issues in Version 1.2.0 via automation Aug 2, 2018

@MaciejBaj MaciejBaj added this to the Version 1.2.0 milestone Aug 2, 2018

@nazarhussain

This comment has been minimized.

Show comment
Hide comment
@nazarhussain

nazarhussain Aug 3, 2018

Contributor

@MaciejBaj Yes that's exactly I suggest we should do. System itself would have latest information and yeah sure to broadcast that information it can wait for certain event in the system e.g. finish syncing.

Contributor

nazarhussain commented Aug 3, 2018

@MaciejBaj Yes that's exactly I suggest we should do. System itself would have latest information and yeah sure to broadcast that information it can wait for certain event in the system e.g. finish syncing.

@MaciejBaj MaciejBaj closed this in 92c2c0e Aug 13, 2018

Version 1.2.0 automation moved this from New Issues to Closed Issues Aug 13, 2018

@yatki yatki reopened this Aug 13, 2018

@diego-G diego-G moved this from Closed Issues to New Issues in Version 1.2.0 Aug 13, 2018

yatki added a commit that referenced this issue Aug 14, 2018

Merge pull request #2313 from LiskHQ/2261-update-system-headers
Disable updating system headers while snapshotting - Closes #2261
@yatki

This comment has been minimized.

Show comment
Hide comment
@yatki

yatki Aug 14, 2018

Member

closed by #2313

Member

yatki commented Aug 14, 2018

closed by #2313

@yatki yatki closed this Aug 14, 2018

Version 1.2.0 automation moved this from New Issues to Closed Issues Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment