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

ethcore sync decodes rlp less often #9264

Merged
merged 9 commits into from
Aug 8, 2018
Merged

ethcore sync decodes rlp less often #9264

merged 9 commits into from
Aug 8, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Aug 1, 2018

should be merged after #9252

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 1, 2018
@dvdplm
Copy link
Collaborator

dvdplm commented Aug 2, 2018

@debris can you merge #9252 to make it easier to review the changes here?

@andresilva
Copy link
Contributor

Please rebase/merge master (which already includes #9252).

hashes.push(head);
self.head = Some(head);
} else {
self.blocks.insert(head, block);
Copy link
Contributor

Choose a reason for hiding this comment

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

So previously this would have removed an already downloaded header? So adding it back is just preventing it from being downloaded again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so the logic has not changed here. previously we called self.blocks.get(...) (line 281) and then self.blocks.remove(...) line 303. Now we are instantly removing self.blocks.remove(...) (line 361), but we need to readd blocks that shouldn't be removed (this line)

Copy link
Collaborator Author

@debris debris Aug 7, 2018

Choose a reason for hiding this comment

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

it lets us avoid unnecessary searches in the hashmap, and unnecessary copying of the blocks (few lines below)

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

@ascjones ascjones added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 7, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM. This was definitely in need of a cleanup 👍.

BlockDownloaderImportError::Invalid
})?;
let number = BlockNumber::from(info.number());
let info = SyncHeader::from_rlp(r.at(i)?.as_raw().to_vec())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's important to keep the logging message? Or will this be logged elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's already logged upstream :)

continue;
}

if self.highest_block.as_ref().map_or(true, |n| number > *n) {
self.highest_block = Some(number);
}
let hash = info.hash();
let hdr = r.at(i).map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

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

We were decoding this twice in the same function just because... 😱

@debris debris merged commit 78a38e9 into master Aug 8, 2018
@debris debris deleted the ethcore-sync-once branch August 8, 2018 08:56
ordian added a commit to ordian/parity that referenced this pull request Aug 10, 2018
…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
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.

5 participants