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

Prevent blockchain & miner racing when accessing pending block. #9310

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Prevent blockchain & miner racing when accessing pending block. #9310

merged 2 commits into from
Aug 10, 2018

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Aug 8, 2018 •
edited
Loading

The issue is that client might have imported a new block already, but on_chain_new_blocks notification has not been run yet (so new pending block was not yet constructed). This is even more visible now, when pending block construction may take significant amount of time (hundreds millis).
So the miner has a pending block with the same number as the best_block returned from client.

We could either:

  1. Return a bit stale data (implemented in this PR)
  2. Force miner to create a new pending block (I would avoid that, we don't really want to create pending blocks from RPC threads, since it's quite heavy)
  3. Wait for the new pending block (if we know we run with --force-sealing or sth). This would required to have some kind of lock on the pending data. (I would also avoid that to prevent locking down the RPC cpupool).

Since the inherent nature of pending is that it's ephemeral (and the data might actually be invalid by the time you receive a response) I believe that returning stale data is the least evil approach.

Should address #9288

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Aug 8, 2018
dvdplm
dvdplm approved these changes Aug 8, 2018
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If I understand this correctly, by "stale" you mean that the RPC call could include a block in the "pending" list that is actually not pending any more? If yes then that sounds like a reasonable trade-off (but should perhaps be documented in the API docs?).

@tomusdrw tomusdrw mentioned this pull request Aug 8, 2018
Closed
@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 8, 2018
@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Aug 8, 2018

I've noticed another possible issue that causes incorrect behaviour and will attempt to fix it now.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Aug 8, 2018

Logs in #9288 indicated on another reason why it may be failing.
During reseal the pending block was actually popped of the queue, so it was no longer available. Added a fix for this, changing the semantics of using_queue a bit:

  1. pop_if is replaced with get_pending_if (we always clone)
  2. push was renamed to set_pending

So now we can push to an old block, but it's previous pending version is still available for the queries.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 8, 2018
@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Aug 8, 2018

@dvdplm yup, I mean exactly this, where we already imported a block with number N to the blockchain, but we have a pending block N that might be a little bit different though (but it's the same block that has been returned for all previous queries).

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 10, 2018
@debris debris merged commit 30e4007 into master Aug 10, 2018
@debris debris deleted the td-pendingblock branch August 10, 2018 09:01
@debris
Copy link
Collaborator

debris commented Aug 10, 2018

does it need to backported?

ordian added a commit to ordian/parity that referenced this pull request Aug 10, 2018
@ordian
Merge branch 'master' of https://github.com/paritytech/parity into wa…
cfd6152 
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  Allow setting the panic hook with parity-clib (openethereum#9292)
  Prevent blockchain & miner racing when accessing pending block. (openethereum#9310)
  Docker alpine: use multi-stage concept (openethereum#9269)
  Update `log` -> 0.4, `env_logger` -> 0.5. (openethereum#9294)
  Update tobalaba.json (openethereum#9313)
  Allow tx pool to be Send (openethereum#9315)
  Fix codecov.io badge in README (openethereum#9327)
  Move ethereum-specific H256FastMap type to own crate (openethereum#9307)
  ethcore sync decodes rlp less often (openethereum#9264)
@5chdn 5chdn added this to the 2.1 milestone Aug 21, 2018
@5chdn 5chdn mentioned this pull request Aug 23, 2018
Merged
28 tasks
andresilva pushed a commit that referenced this pull request Aug 27, 2018
@tomusdrw @andresilva
Prevent blockchain & miner racing when accessing pending block. (#9310)
acfad41 
* Prevent blockchain & miner racing when accessing pending block.

* Fix unavailability of pending block during reseal.
5chdn added a commit that referenced this pull request Aug 31, 2018
@5chdn
beta backports for 2.0.3 (#9229)
a0a2bed 
* parity-version: bump beta to 2.0.2

* remove ssl from dockerfiles, closes #8880 (#9195)

* snap: remove ssl dependencies from snapcraft definition (#9222)

* parity-version: bump beta to 2.0.3

* Remove all dapp permissions related settings (#9120)

* Completely remove all dapps struct from rpc

* Remove unused pub use

* Remove dapp policy/permission func in ethcore

* Remove all dapps settings from rpc

* Fix rpc tests

* Use both origin and user_agent

* Address grumbles

* Address grumbles

* Fix tests

* Check if synced when using eth_getWork (#9193) (#9210)

* Check if synced when using eth_getWork (#9193)

* Don't use fn syncing

* Fix identation

* Fix typo

* Don't check for warping

* rpc: avoid calling queue_info twice on eth_getWork

* Fix potential as_usize overflow when casting from U256 in miner (#9221)

* Allow old blocks from peers with lower difficulty (#9226)

Previously we only allow downloading of old blocks if the peer
difficulty was greater than our syncing difficulty. This change allows
downloading of blocks from peers where the difficulty is greater then
the last downloaded old block.

* Update Dockerfile (#9242)

* Update Dockerfile

fix Docker build

* fix dockerfile paths: parity -> parity-ethereum (#9248)

* Propagate transactions for next 4 blocks. (#9265)

Closes #9255 

This PR also removes the limit of max 64 transactions per packet, currently we only attempt to prevent the packet size to go over 8MB. This will only be the case for super-large transactions or high-block-gas-limit chains.

Patching this is important only for chains that have blocks that can fit more than 4k transactions (over 86M block gas limit)

For mainnet, we should actually see a tiny bit faster propagation since instead of computing 4k pending set, we only need `4 * 8M / 21k = 1523` transactions.

Running some tests on `dekompile` node right now, to check how it performs in the wild.

* Update tobalaba.json (#9313)

* Fix load share (#9321)

* fix(light_sync): calculate `load_share` properly

* refactor(api.rs): extract `light_params` fn, add test

* style(api.rs): add trailing commas

* ethcore: fix pow difficulty validation (#9328)

* ethcore: fix pow difficulty validation

* ethcore: validate difficulty is not zero

* ethcore: add issue link to regression test

* ethcore: fix tests

* ethcore: move difficulty_to_boundary to ethash crate

* ethcore: reuse difficulty_to_boundary and boundary_to_difficulty

* ethcore: fix grumbles in difficulty_to_boundary_aux

* Light client `Provide default nonce in transactions when it´s missing` (#9370)

* Provide `default_nonce` in tx`s when it´s missing

When `nonce` is missing in a `EthTransaction` will cause it to fall in
these cases provide `default_nonce` value instead!

* Changed http:// to https:// on Yasm link (#9369)

Changed http:// to https:// on Yasm link in README.md

* Provide `default_nonce` in tx`s when it´s missing

When `nonce` is missing in a `EthTransaction` will cause it to fall in
these cases provide `default_nonce` value instead!

* Address grumbles

* ethcore: kovan: delay activation of strict score validation (#9406)

* Better support for eth_getLogs in light mode (#9186)

* Light client on-demand request for headers range.

* Cache headers in HeaderWithAncestors response.

Also fulfills request locally if all headers are in cache.

* LightFetch::logs fetches missing headers on demand.

* LightFetch::logs limit the number of headers requested at a time.

* LightFetch::logs refactor header fetching logic.

* Enforce limit on header range length in light client logs request.

* Fix light request tests after struct change.

* Respond to review comments.

* Add update docs script to CI (#9219)

* Add update docs script to CI

Added a script to CI that will use the jsonrpc tool to update rpc
documentation then commit and push those to the wiki repo.

* fix gitlab ci lint

* Only apply jsonrpc docs update on tags

* Update gitlab-rpc-docs.sh

* Copy correct parity repo to jsonrpc folder

Copy correct parity repo to jsonrpc folder before attempting to build docs since the CI runner clones the repo as parity and not parity-ethereum.

* Fix JSONRPC docs CI job

Update remote config in wiki repo before pushing changes using a github
token for authentication. Add message to wiki tag when pushing changes.
Use project directory to correctly copy parity code base into the
jsonrpc repo for doc generation.

* Fix set_remote_wiki function call in CI

* Prevent blockchain & miner racing when accessing pending block. (#9310)

* Prevent blockchain & miner racing when accessing pending block.

* Fix unavailability of pending block during reseal.

* Prevent sync restart if import queue full (#9381)

* Add POA Networks: Core and Sokol (#9413)

* ethcore: add poa network and sokol chainspecs

* rpc: simplify chain spec docs

* cli: rearrange networks by main/test and size/range

* parity: don't blacklist 0x00a328 on sokol testnet

* parity: add sokol and poanet to params and clean up a bit, add tests

* ethcore: add the poa networks and clean up a bit

* ethcore: fix path to poacore chain spec

* parity: rename poa networks to poacore and poasokol

* parity: fix configuration tests

* parity: fix parameter tests

* ethcore: rename POA Core and POA Sokol

* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@debris debris debris approved these changes

@dvdplm dvdplm dvdplm approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Milestone
2.1 Whatever
Development

Successfully merging this pull request may close these issues.
4 participants
@tomusdrw @debris @dvdplm @5chdn