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

make block queue into a more generic verification queue and fix block heap size calculation#2095

Merged
gavofyork merged 10 commits into
masterfrom
header_queue
Sep 27, 2016
Merged

make block queue into a more generic verification queue and fix block heap size calculation#2095
gavofyork merged 10 commits into
masterfrom
header_queue

Conversation

@rphmeier
Copy link
Copy Markdown
Contributor

@rphmeier rphmeier commented Sep 14, 2016

in preparation for header-only sync. verification pipelines (currently assumed to be 3-stage) are defined in the kinds module.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 14, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 14, 2016

Coverage Status

Coverage decreased (-0.03%) to 84.912% when pulling 30952a5 on header_queue into 2ba4968 on master.

@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Sep 16, 2016

The latest push implements a header queue, but it will give warnings that the Headers struct is never used. This will be remedied in the next PR.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 16, 2016

Coverage Status

Coverage increased (+0.3%) to 85.242% when pulling 417f371 on header_queue into 2ba4968 on master.

@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Sep 19, 2016

This also fixes calculation of heap size for blocks in the blocks queue (previously hard-coded to be zero, which was wrong)

@rphmeier rphmeier changed the title make block queue into a more generic verification queue make block queue into a more generic verification queue and fix block heap size calculation Sep 20, 2016
@gavofyork
Copy link
Copy Markdown
Contributor

gavofyork commented Sep 21, 2016

looks fairly reasonable to me.

@arkpar could you take a look?

@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Sep 21, 2016 •
edited
Loading

Doing a full sync with this right now, strangely it seems to have broken the limits on the queue size (or exposed an existing issue) -- I saw 100MiB of blocks (48k) queued at once with the default maximums: 50MiB and 30k blocks. I haven't changed the is_full logic, so I'm not sure why it seems to be importing too many blocks.

It might just be that it sees that the block queue isn't quite full and then imports all the blocks it has, bumping it over the limit.

@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Sep 21, 2016

On the plus side, it's definitely queuing way fewer blocks (while syncing ~700k it has typically 15-20k at any given time) while keeping the usual queue size slightly over 50MiB. So the footprint is definitely more in line with the specified options now.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 22, 2016

Coverage Status

Coverage decreased (-0.03%) to 85.164% when pulling ea534a1 on header_queue into 862feb7 on master.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 26, 2016

Coverage Status

Coverage increased (+0.007%) to 85.181% when pulling f0bb1fb on header_queue into 92451ef on master.

arkpar
arkpar reviewed Sep 27, 2016
View reviewed changes
Comment thread ethcore/src/verification/queue/kind.rs Outdated
match verify_block_unordered(un.header, un.bytes, engine) {
Ok(verified) => Ok(verified),
Err(e) => {
warn!(target: "client", "Stage 2 block verification failed for {}\n Error: {:?}", hash, e);
Copy link
Copy Markdown
Collaborator

@arkpar arkpar Sep 27, 2016

Choose a reason for hiding this comment

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

Word "Error" should be removed from this message. It is confusing for users.

Comment thread ethcore/src/verification/queue/kind.rs Outdated
match verify_block_basic(&input.header, &input.bytes, engine) {
Ok(()) => Ok(input),
Err(e) => {
warn!(target: "client", "Stage 1 block verification failed for {:?}", input.hash());
Copy link
Copy Markdown
Collaborator

@arkpar arkpar Sep 27, 2016

Choose a reason for hiding this comment

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

Please add error info to the message

@arkpar arkpar added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Sep 27, 2016
@rphmeier rphmeier removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Sep 27, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 27, 2016

Coverage Status

Changes Unknown when pulling 6abadd6 on header_queue into * on master*.

@arkpar arkpar added the A8-looksgood 🦄 Pull request is reviewed well. label Sep 27, 2016
@arkpar arkpar removed the A0-pleasereview 🤓 Pull request needs code review. label Sep 27, 2016
@gavofyork gavofyork merged commit 9d4bee4 into master Sep 27, 2016
@gavofyork gavofyork deleted the header_queue branch September 27, 2016 14:50
jacogr added a commit that referenced this pull request Sep 29, 2016
@jacogr
Merge branch 'master' into js
50adb8a 
* master:
  Fixing Delegate Call in JIT (#2378)
  Prioritizing re-imported transactions (#2372)
  Revert #2172, pretty much. (#2387)
  correct sync memory usage calculation (#2385)
  Update gitlab-ci
  Fix the traceAddress field in transaction traces. (#2373)
  Removing extras data from retracted blocks. (#2375)
  fixed #2263, geth keys with ciphertext shorter than 32 bytes (#2318)
  Expanse compatibility (#2369)
  Specify column cache sizes explicitly; default fallback of 2MB (#2358)
  Canonical state cache (master) (#2311)
  make block queue into a more generic verification queue and fix block heap size calculation (#2095)
  Hash Content RPC method (#2355)
  Reorder transaction_by_hash to favour canon search (#2332)
  DIV optimization (#2327)
  Error when deserializing invalid hex (#2339)
  Changed http:// to https:// on some links (#2349)
  add a test
  fix migration system, better errors

# Conflicts:
#	.gitlab-ci.yml
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@arkpar arkpar arkpar left review comments

@gavofyork gavofyork gavofyork approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rphmeier @coveralls @gavofyork @arkpar