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

Make sure parent block is not in importing queue when importing ancient blocks #10138

Merged
merged 9 commits into from Jan 9, 2019
Merged

Make sure parent block is not in importing queue when importing ancient blocks #10138

merged 9 commits into from Jan 9, 2019

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Jan 6, 2019 •
edited

rel #10127. Partially fixes it.

  • When queuing ancient blocks, we first check to make sure that the parent block is not unknown. However, this can also mean that the block is in the import queue. If this happens, ancient block importing would fail because it cannot find the parent block's total difficulty. This PR fixes the issue by only checking whether the parent block has been completely imported.
  • in case an ancient import fails, all further imports wouldn't have parents, thus fetching parent total difficulty would panic. This PR fixes the issue by clearing the ancient queue if error happens.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 6, 2019
@sorpaas sorpaas added this to the 2.3 milestone Jan 6, 2019
ascjones
ascjones suggested changes Jan 6, 2019
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

I think this is checking the wrong queue.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Jan 6, 2019

I updated the PR with another case that would cause panic: in case an ancient import fails, all further imports wouldn't have parents, thus fetching parent total difficulty would panic.

@ascjones
Copy link
Contributor

ascjones commented Jan 6, 2019

Yes, and indeed this looks more likely to be the cause in #10127, just before the panic we get: 2019-01-03 12:15:29 Error importing ancient block: Block error (Seal element out of bounds: Expected 0xe0d0…bc7a, found 0x900b…b42c)

ascjones
ascjones approved these changes Jan 6, 2019
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM.

ethcore/src/client/client.rs Show resolved Hide resolved
ethcore/src/client/client.rs Outdated Show resolved Hide resolved
@sorpaas sorpaas added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 7, 2019
@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 7, 2019
ascjones
ascjones approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

👍

@ascjones ascjones added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 7, 2019
@5chdn 5chdn mentioned this pull request Jan 7, 2019
Merged
15 tasks
@ordian ordian merged commit 010cfb7 into master Jan 9, 2019
@ordian ordian deleted the sp-ancient-fix branch January 9, 2019 13:47
ordian pushed a commit that referenced this pull request Jan 9, 2019
@sorpaas @ordian
Make sure parent block is not in importing queue when importing ancie…
cf65627 
…nt blocks (#10138)

* Make sure parent block is not in importing queue when importing ancient blocks

* Clear queue when an ancient import fails

* Lock only once in clear

* Add comments why queued check is needed

* Should push the value back to the queue

* Directly check in chain.read()

* Remove extra empty line

* Revert unused verification change
5chdn added a commit that referenced this pull request Jan 9, 2019
@5chdn
beta backports v2.2.6 (#10113)
e9396e1 
* version: bump beta to v2.2.6

* Update pwasm-utils to 0.6.1 (#10134)

* version: mark upgrade critical on kovan

* Identity fix (#10128)

* fix #10125

fix service transaction version detection if --identity is enabled, change test to match how --identity actually works

* fix wrong var

* get the index of  v, not /

* idx, not idx.len()

* Update ethcore/sync/src/chain/propagator.rs

Co-Authored-By: joshua-mir <43032097+joshua-mir@users.noreply.github.com>

* Update ethcore/sync/src/chain/propagator.rs

Co-Authored-By: joshua-mir <43032097+joshua-mir@users.noreply.github.com>

* change version prefix to a const

* space

Co-Authored-By: joshua-mir <43032097+joshua-mir@users.noreply.github.com>

* [Hit return to continue]

* ethcore: update hardcoded headers for foundation

* ethcore: update hardcoded headers for ropsten

* ethcore: update hardcoded headers for kovan

* ethcore: use consistant formatting

* ethcore: restore spaces after colon in chain spec

* ethcore: fix bootnodes in chain specs

* ethcore: fix bootnodes in chain specs

* ethcore: enforce newline at the end of chainspecs

* Follow-up to #10105 (#10107)

* HF in POA Sokol (2019-01-04) (#10077)

* HF in POA Sokol (2019-01-04)

poanetwork/poa-chain-spec#91

* Update POA Core bootnodes

* Autogen docs for the "Configuring Parity Ethereum" wiki page. (#10067)

* publish docs changes for autogen config docs

* Update publish-docs.sh

adding an environment variable so js knows not to download git master and just grab the local repo

* Update publish-docs.sh

made some changes making this unnecessary

* fix env variable

env variable passes to node properly now

* use yarn

* test pipeline, revert me

* fix test pipeline, revert me

* change runner tag

* change runner tag 2

* change runner tag

* global git config

* supress upload_files output

* Update .gitlab-ci.yml

reverting testing changes

* Replace tag if exists

Very unlikely to be important/useful

* Autogen docs for the "Configuring Parity Ethereum" wiki page. (#10067)

* publish docs changes for autogen config docs

* Update publish-docs.sh

adding an environment variable so js knows not to download git master and just grab the local repo

* Update publish-docs.sh

made some changes making this unnecessary

* fix env variable

env variable passes to node properly now

* use yarn

* test pipeline, revert me

* fix test pipeline, revert me

* change runner tag

* change runner tag 2

* change runner tag

* global git config

* supress upload_files output

* Update .gitlab-ci.yml

reverting testing changes

* Replace tag if exists

Very unlikely to be important/useful

* finality: dont require chain head to be in the chain (#10054)

* Fill transaction hash on ethGetLog of light client. (#9938)

* Fill transaction hash on ethGetLog of light client. This is enifficient
but we keep align with spec.

* Using better variables names.

* Expose old fast light get log as `parity_getLogsLight`.

* Update rpc/src/v1/helpers/light_fetch.rs

Fix indent.

Co-Authored-By: cheme <emericchevalier.pro@gmail.com>

* Factor common code between light_logs and logs.

* Remove useless check

* Rename parity light logs to 'parity_getLogsNoTransactionHash'.
Fix indent and extra white lines.

* Use vec instead of tree map to avoid inner function.

* better loop

* fix pubsub new_blocks notifications to include all blocks (#9987)

Fix: new blocks notifications sometimes missing in pubsub RPC

Implement new struct to pass to `new_blocks()` with extra parameter - `has_more_blocks_to_import`, which was previously used to determine whether the notification should be sent. Now it's up to each implementation to decide what to do.

Updated all implementations to behave as before, except `eth_pubsub`, which will send notification even when the queue is not empty.

Update tests.

* Revert part of 70a6db7

* HF in POA Core (2019-01-18) - Constantinople (#10155)

poanetwork/poa-chain-spec#100

* ci: re-enable snap publishing (#10142)

* ci: enable snap publishing~

* ci: add publish snap script

* ci: add snapcraft skeleton

* ci: group export statements

* ci: enable snaps on pr branch

* ci: enable snaps on pr branch

* ci: set default BUILD_ARCH

* ci: enable snaps on pr branch

* ci: enable snaps on pr branch

* ci: add libdb to snap

* ci: reinitiate gitlabci

* ci: reinitiate publish-snap script

* ci: fix yaml syntax

* cargo/gitlab env vars

* debug, revert me

* version?

* debug vars

* vars

* vars fix

* vars fix

* revert

* Update scripts/gitlab/publish-snap.sh

Co-Authored-By: 5chdn <5chdn@users.noreply.github.com>

* ci: read track from cargo toml

* Make sure parent block is not in importing queue when importing ancient blocks (#10138)

* Make sure parent block is not in importing queue when importing ancient blocks

* Clear queue when an ancient import fails

* Lock only once in clear

* Add comments why queued check is needed

* Should push the value back to the queue

* Directly check in chain.read()

* Remove extra empty line

* Revert unused verification change
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ascjones ascjones ascjones approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sorpaas @ascjones @ordian @5chdn