Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Only check warp syncing for eth_getWorks #9484

Merged
merged 2 commits into from
Sep 10, 2018
Merged

Only check warp syncing for eth_getWorks #9484

merged 2 commits into from
Sep 10, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Sep 6, 2018

No description provided.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Sep 6, 2018
@sorpaas sorpaas added this to the 2.1 milestone Sep 6, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 6, 2018

That only affects master 2.1.0+, does it?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 6, 2018

It affects anything after #9210. So I think that means beta and stable.

@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 6, 2018
This was referenced Sep 6, 2018
@lexfrl
Copy link
Contributor

lexfrl commented Sep 6, 2018

I'd like to add my review, but I need to understand some context first.. Why we don't need to check for is_major_importing but check only for is_warp_syncing here?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 7, 2018

@fckt the issue is that using is_major_importing may break this RPC call for some miners. A peer may cause the client to occasionally switch to SyncState::Blocks (considered syncing by is_major_importing), while the client is really close to chain head.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 7, 2018

Copied from chat, and I do appreciate if someone more familiar with our sync crate would take a review on this PR:

  • In sync::chain::ChainSync::on_peer_new_hashes, when we get a newHashes message, we would set sync state to NewBlocks, and call ChainSync::sync_peer.
  • In sync_peer, we go into the NewBlocks match case, and call self.new_blocks.request_block.
  • If we need receipts or block bodies, the requests we received would be Some, and we go into the if statement.
  • Within it, we set state to SyncState::Blocks.
  • That state is considered "major importing" by eth_getWork rpc, and we would return error "still sycning".

Would an attacker / client from another network force a victim to stay in SyncState::Blocks? A peer can report having higher difficulty, and the victim would send block bodies/receipts requests to then and set its state to Blocks. The response can be invalid (from another network, for example) and the victim may choose to disconnect. But in that short period of time eth_getWorks will not be available.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Sep 10, 2018

I'm going to add the blocker tag for this PR, because if this is indeed an issue, then pow mining for some chains may have been broken.

@sorpaas sorpaas added the B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. label Sep 10, 2018
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

There is actually already this method implemented: SyncStatus::is_snapshot_syncing that does what we want here.
https://github.com/paritytech/parity-ethereum/blob/master/ethcore/sync/src/chain/mod.rs#L246L253

@sorpaas sorpaas merged commit ba487ea into master Sep 10, 2018
@sorpaas sorpaas deleted the sp-major-importing branch September 10, 2018 17:53
sorpaas added a commit that referenced this pull request Sep 10, 2018
* Only check warp syncing for eth_getWorks

* Use SyncStatus::is_snapshot_syncing
sorpaas added a commit that referenced this pull request Sep 10, 2018
* Only check warp syncing for eth_getWorks

* Use SyncStatus::is_snapshot_syncing
5chdn added a commit that referenced this pull request Sep 10, 2018
* parity-version: bump stable to 1.11.11

* Update tobalaba.json (#9419)

* Update hardcoded sync (#9421)

- Update foundation hardcoded header to block 6219777 
- Update ropsten hardcoded header to block 3917825 
- Update kovan hardcoded header to block 8511489

* parity: print correct keys path on startup (#9501)

* Only check warp syncing for eth_getWorks (#9484)

* Only check warp syncing for eth_getWorks

* Use SyncStatus::is_snapshot_syncing
5chdn added a commit that referenced this pull request Sep 10, 2018
* parity-version: bump beta to 2.0.4

* [light/jsonrpc] Provide the actual account for `eth_coinbase` RPC and unify error handeling for light and full client (#9383)

* Provide the actual `account` for eth_coinbase

The previous implementation always provided the `zero address` on
`eth_coinbase` RPC. Now, instead the actual address is returned on
success or an error when no account(s) is found!

* full client `eth_coinbase` return err

In the full-client return an error when no account is found instead of
returning the `zero address`

* Remove needless blocks on single import

* Remove needless `static` lifetime on const

* Fix `rpc_eth_author` test

* parity: print correct keys path on startup (#9501)

* aura: don't report skipped primaries when empty steps are enabled (#9435)

* Only check warp syncing for eth_getWorks (#9484)

* Only check warp syncing for eth_getWorks

* Use SyncStatus::is_snapshot_syncing

* Fix Snapshot restoration failure on Windows (#9491)

* Close Blooms DB files before DB restoration

* PR Grumbles I

* PR Grumble

* Grumble
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants