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

Base node block sync RPC service #2348

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Oct 16, 2020

Description

RPC service for faster block sync. Allows streaming of full blocks,
headers and a method for chain split detection. The client-side is next
up.

  • Added BlockchainStateService. This service should eventually be the only service
    which provides a query interface to the blockchain database for other
    services.
  • Added BaseNodeSyncRpcService. This RPC service exposes sync_blocks,
    sync_headers, fetch_header_by_height and find_chain_split. These
    functions will be used in the improved block sync.
  • Added NotFound status to RpcStatus
  • Added RpcStatus::log_internal_error() convenience function to make
    dealing with misc errors in RPC handler functions easier.
  • Added spawn_until_shutdown method to the service initialiser context
    that spawns a task until the shutdown is triggered. This removes the need for
    implementers to worry about how to implement shutdown in initialisers.
  • Cleaned up some test helpers
  • Added unit tests for the Base node sync RPC handlers
  • Added unit tests for the BlockchainStateService
  • Remove unused tokio-test dependency

Motivation and Context

Described here: #2197 (comment)
Ref #2113

How Has This Been Tested?

Extensive unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I ran cargo test successfully before submitting my PR.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@sdbondi sdbondi force-pushed the sb-core-base-node-rpc branch 4 times, most recently from 8b9f92b to 7978ce2 Compare October 19, 2020 07:31
@sdbondi sdbondi added the A-base_node Area - The Tari base node executable and libraries label Oct 19, 2020
@sdbondi sdbondi force-pushed the sb-core-base-node-rpc branch 5 times, most recently from fbed41c to 690286a Compare October 19, 2020 14:46
philipr-za
philipr-za previously approved these changes Oct 19, 2020
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

Not hugely familiar with the context of this PR but LGTM

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Looks good. I think the use of heights instead of hashes could let to unexpected responses though. I would also prefer the FindChainSplit to immediately return headers to increase the cost of spoofing

@sdbondi sdbondi marked this pull request as draft October 20, 2020 13:48
@sdbondi sdbondi force-pushed the sb-core-base-node-rpc branch 6 times, most recently from 78cf393 to 9badeaf Compare October 21, 2020 15:42
@sdbondi sdbondi marked this pull request as ready for review October 22, 2020 10:59
RPC service for faster block sync. Allows streaming of full blocks,
headers and a method for chain split detection. The client-side is next
up.

- Added BlockchainStateService. This service should eventually be the only service
  which provides a query interface to the blockchain database for other
  services.
- Added BaseNodeSyncRpcService. This RPC service exposes `sync_blocks`,
  `sync_headers`, `get_header_by_height` and `find_chain_split`. These
  functions will be used in the improved block sync.
- Added `NotFound` status to RpcStatus
- Added `RpcStatus::log_internal_error()` convenience function to make
  dealing with misc errors in RPC handler functions easier.
- Added `spawn_until_shutdown` method to the service initialiser context
   that spawns a task until the shutdown is triggered. This removes the need for
   implementers to worry about how to implement shutdown in initialisers.
- Cleaned up some test helpers
- Added unit tests for the Base node sync RPC handlers
- Added unit tests for the BlockchainStateService

Ref tari-project#2113
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Looks good. The tests are really nice and easy to read.

@stringhandler stringhandler merged commit 95ceee5 into tari-project:development Oct 26, 2020
@sdbondi sdbondi deleted the sb-core-base-node-rpc branch October 26, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-base_node Area - The Tari base node executable and libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants