Fill transaction hash on ethGetLog of light client. by cheme · Pull Request #9938 · openethereum/parity-ethereum

cheme · 2018-11-19T14:21:25Z

This is highly inefficient, yet required to respect standard eth rpc.
Closes #9929 .

It exposes a new rpc method to keep old behavior : parity_getLogsNoTransactionHash

but we keep align with spec.

parity-cla-bot · 2018-11-19T14:21:28Z

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2018-11-23T09:50:29Z

+						.and_then(move |matches| {
+							let mut blocks = BTreeMap::new();
+							let mut result: Vec<Log> = matches.into_iter().map(|(_, v)| {
+								{


indentation is off here

niklasad1 · 2018-11-23T10:05:17Z

+							// future get blocks (unordered it)
+							stream::futures_unordered(blocks.into_iter().map(|(_,v)|v)).collect().map(move |blocks| {
+								let mut tr_per_bl = BTreeMap::new();
+								for enc_bl in blocks.iter() {


I don't understand the name enc_block can you rephrase?

niklasad1

Looks generally, mostly some formatting and naming questions to address

sorpaas

Mostly LGTM. A few additional grumbles.

Maybe good to add a flag that enables/disables this, but I think this extra cost is fine (we added one more request for all involved logs block, and normal light client will not try to fetch large amount of logs). So enabling it on default might not hurt anyway.

sorpaas · 2018-11-25T16:59:21Z

+								v
+							}).collect();
+							// future get blocks (unordered it)
+							stream::futures_unordered(blocks.into_iter().map(|(_,v)|v)).collect().map(move |blocks| {


nit: missing whitespace: .map(|(_, v)| v)

sorpaas · 2018-11-25T17:00:42Z

+							let mut result: Vec<Log> = matches.into_iter().map(|(_, v)| {
+								{
+									let block_hash = v.block_hash.as_ref().expect("Previously initialized with value; qed");
+									blocks.entry(block_hash.clone()).or_insert_with(|| {


block_hash is copy, so we don't need clone.

Actually, I think this is rpc::v1::types::hash which is not Copy but we should refactor that to be consistent in the codebase!

niklasad1 · 2018-11-30T10:48:33Z

Yeah, could be good to provide the old behavior too but I don't think a flag for eth namespace is a good idea instead I would prefer to provide the old behavior in the parity namespace or somewhere else instead! Thoughts?

cheme · 2018-11-30T11:01:02Z

At first I wanted to do it, but then I got lazy :) The truth is that I started wondering if it would not be an additional unused rpc or flag.
You are right a flag is not a good idea.
I am more in favor of a undocumented parity ns rpc. Undocumented means not in wiki because it is very specific use.
So before putting it in place I jut want to check again that it will be useful (depending on use case blocks may be download and put in cache anyway due to other calls). /cc @Tbaut @amaurymartiny

Tbaut · 2018-11-30T11:41:04Z

My 2cts

I am more in favor of a undocumented parity ns rpc. Undocumented means not in wiki because it is very specific use.

If it's available, it should be documented, no matter if it's used or not by the outside world, if we developed it (for Fether) it means it can be valuable for others, be it transparently in light.js. I think it's nice to keep the original one in a parity_ rpc, but as any other RPC, to document it (it's part of the CI now anyways).

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

cheme · 2018-12-11T14:16:52Z

So the method with old behavior can be call by switching :

curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getLogs","params":[ {"fromBlock":"0x5a6da7","toBlock":"0x5a6da7"} ],"id":74}' localhost:8545

to

curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"parity_getLogsLight","params":[ {"fromBlock":"0x5a6da7","toBlock":"0x5a6da7"} ],"id":74}' localhost:8545

(new name is 'parity_getLogsLight').

I think I did address grumbles, please point out the remaining ones if any.

ordian

Looks good. It would be nice to have a test, but we need to write some testing infrastructure for light client's rpc first.
Edit: also, do we (or someone else) rely on the returned logs being ordered?

niklasad1 · 2018-12-14T21:18:50Z

 			})
+
+	}
+


extra whitespace

niklasad1 · 2018-12-14T21:20:52Z

-	/// Get transaction logs
-	pub fn logs(&self, filter: EthcoreFilter) -> impl Future<Item = Vec<Log>, Error = Error> + Send {
-		use std::collections::BTreeMap;
+	fn logs_common(&self, filter: EthcoreFilter) -> impl Future<Item = BTreeMap<(u64,usize),Log>, Error = Error> + Send {


missing spaces

Suggested change

fn logs_common(&self, filter: EthcoreFilter) -> impl Future<Item = BTreeMap<(u64,usize),Log>, Error = Error> + Send {

fn logs_common(&self, filter: EthcoreFilter) -> impl Future<Item = BTreeMap<(u64, usize), Log>, Error = Error> + Send {

niklasad1 · 2018-12-14T21:21:11Z

 			_ => panic!(WRONG_RESPONSE_AMOUNT_TYPE_PROOF),
 		}))
+
+


extra whitespace

niklasad1 · 2018-12-14T21:22:58Z

+		self.logs_common(filter)
+		// retrieve transaction hash.
+			.and_then(move |matches| {
+			let mut blocks = BTreeMap::new();


indentation off

niklasad1 · 2018-12-14T21:23:47Z

+				v
+			}).collect();
+			// future get blocks (unordered it)
+			stream::futures_unordered(blocks.into_iter().map(|(_,v)|v)).collect().map(move |blocks| {


Suggested change

stream::futures_unordered(blocks.into_iter().map(|(_,v)|v)).collect().map(move |blocks| {

stream::futures_unordered(blocks.into_iter().map(|(_, v)| v)).collect().map(move |blocks| {

niklasad1 · 2018-12-14T21:49:25Z

+
+		/// Returns logs matching given filter object.
+		/// Skip filling transaction hash for faster query.
+		#[rpc(name = "parity_getLogsLight")]


I have no strong opinions on the naming but getLogsLight is not self-explanatory as long the documentation explains this I'm satisfied.

Otherwise, I propose parity_getLogsNoTransactionHash

Fix indent and extra white lines.

cheme · 2018-12-17T09:58:31Z

@niklasad1 I did switch rpc naming as suggested.

* 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

* 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

Fill transaction hash on ethGetLog of light client. This is enifficient

49e54b7

but we keep align with spec.

cheme added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 19, 2018

niklasad1 reviewed Nov 23, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

niklasad1 reviewed Nov 23, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

niklasad1 reviewed Nov 23, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

niklasad1 reviewed Nov 23, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

niklasad1 suggested changes Nov 23, 2018

View reviewed changes

niklasad1 added the M6-rpcapi 📣 RPC API. label Nov 23, 2018

Using better variables names.

f25fefa

sorpaas reviewed Nov 25, 2018

View reviewed changes

sorpaas added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 25, 2018

5chdn added this to the 2.3 milestone Nov 25, 2018

5chdn added the B1-patch-beta 🕷🕷 label Dec 3, 2018

cheme added 2 commits December 3, 2018 19:13

Expose old fast light get log as parity_getLogsLight.

564559d

Merge branch 'master' into light-complete-check-query

9ce5396

ordian reviewed Dec 5, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

ordian and others added 2 commits December 5, 2018 10:45

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

dac8cad

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

Factor common code between light_logs and logs.

0cbb9a7

ordian approved these changes Dec 13, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

Remove useless check

3af6385

niklasad1 reviewed Dec 14, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs

})

}

Copy link
Copy Markdown

Collaborator

niklasad1 Dec 14, 2018

Choose a reason for hiding this comment

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

extra whitespace

niklasad1 reviewed Dec 14, 2018

View reviewed changes

niklasad1 approved these changes Dec 14, 2018

View reviewed changes

niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 14, 2018

Rename parity light logs to 'parity_getLogsNoTransactionHash'.

92c6d05

Fix indent and extra white lines.

Use vec instead of tree map to avoid inner function.

c386faf

ordian reviewed Dec 17, 2018

View reviewed changes

Comment thread rpc/src/v1/helpers/light_fetch.rs Outdated

better loop

ad26801

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 17, 2018

ordian merged commit 789bb9c into openethereum:master Dec 17, 2018

5chdn mentioned this pull request Dec 28, 2018

beta backports v2.2.6 #10113

Merged

15 tasks

	fn logs_common(&self, filter: EthcoreFilter) -> impl Future<Item = BTreeMap<(u64,usize),Log>, Error = Error> + Send {
	fn logs_common(&self, filter: EthcoreFilter) -> impl Future<Item = BTreeMap<(u64, usize), Log>, Error = Error> + Send {

	stream::futures_unordered(blocks.into_iter().map(\|(_,v)\|v)).collect().map(move \|blocks\| {
	stream::futures_unordered(blocks.into_iter().map(\|(_, v)\| v)).collect().map(move \|blocks\| {

Conversation

cheme commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Nov 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Nov 30, 2018

Uh oh!

cheme commented Nov 30, 2018

Uh oh!

Tbaut commented Nov 30, 2018

Uh oh!

Uh oh!

Uh oh!

cheme commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ordian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheme commented Dec 17, 2018

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cheme commented Nov 19, 2018 •

edited

Loading

cheme commented Dec 11, 2018 •

edited

Loading

ordian left a comment •

edited

Loading