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

Misc docs and renames …and one less clone#11556

Merged
dvdplm merged 4 commits into
masterfrom
dp/chore/more-sync-misc
Mar 10, 2020
Merged

Misc docs and renames …and one less clone#11556
dvdplm merged 4 commits into
masterfrom
dp/chore/more-sync-misc

Conversation

@dvdplm
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm commented Mar 10, 2020

I had a plan for a more extensive refactoring of ClientMessage::Execute but it didn't pan out; this here is the random bits and pieces that survived. :(

…and one less clone
for i in 0 .. item_count {
let rlp = r.at(i)?;
let rlp = tx_rlp.at(i)?;
let tx = rlp.as_raw().to_vec();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to get rid of this copy here, but I can't make it work.

Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 Mar 10, 2020

Choose a reason for hiding this comment

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

I don't think it is possible because rlpin::Rlp is essentially a wrapper over &[u8], so one way or another you need to allocate to get Vec<u8> in safe Rust.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, my plan was to defer the allocation until we transform the rlp bytes into UnverifiedTransactions. But then the transform to UnverifiedTransaction has to happen outside the ClientIoMessage::Execute closure. And even then, that closure is an Fn and changing that to an FnOnce doesn't work because it can't be called when boxed and at that point I gave up.

@dvdplm dvdplm self-assigned this Mar 10, 2020
@dvdplm dvdplm marked this pull request as ready for review March 10, 2020 10:13
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Mar 10, 2020
Comment thread ethcore/src/test_helpers/test_client.rs Outdated
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 10, 2020
Comment thread ethcore/src/client/client.rs
Comment thread ethcore/src/client/client.rs Outdated
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@dvdplm dvdplm merged commit 5be4924 into master Mar 10, 2020
@dvdplm dvdplm deleted the dp/chore/more-sync-misc branch March 10, 2020 22:58
ordian added a commit that referenced this pull request Mar 24, 2020
* master:
  informant: display I/O stats (#11523)
  [devp2p discovery]: remove `deprecated_echo_hash` (#11564)
  [secretstore] create db_version file when database doesn't exist (#11570)
  Remove Parity's Security Policy (#11565)
  ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets (#11529)
  Misc docs and renames …and one less clone (#11556)
  [secretstore]: don't sign message with only zeroes (#11561)
  [devp2p discovery]: cleanup (#11547)
  Code cleanup in the sync module (#11552)
  initial cleanup (#11542)
  Warn if genesis constructor revert (#11550)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants