Skip to content
This repository has been 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
merged 10 commits into from
Sep 27, 2016

Conversation

rphmeier
Copy link
Contributor

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

Coverage Status

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

@rphmeier
Copy link
Contributor Author

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

Coverage Status

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

@rphmeier
Copy link
Contributor Author

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
Contributor

looks fairly reasonable to me.

@arkpar could you take a look?

@rphmeier
Copy link
Contributor Author

rphmeier commented Sep 21, 2016

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
Contributor Author

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

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

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
Collaborator

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.

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
Collaborator

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

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
* 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.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants