RPC: parity_getBlockReceipts by tomusdrw · Pull Request #9527 · openethereum/parity-ethereum

tomusdrw · 2018-09-11T14:53:07Z

Closes: #9430

Some performance benchmarks:
After: http://gist.github.com/tomusdrw/05242fb0a7d42ed13474218e6a83f517
Before: https://gist.github.com/tomusdrw/f664ff78ce2714fadc2128fa5539cd86

(Not realy meant to compare eth_getTransactionReceipt performance before and after, cause obviously here is obviously a huge variance here, but I think it's clearly visible that getBlockReceipts is way faster and single receipt performance has not degraded)

Script used to check the performance:
https://gist.github.com/tomusdrw/ffd102326522d415ddea1b2aa1938cc9

EDIT: realised it's a bit unfair if we test parity_getBlockReceipts as the second one, cause the first call fills up the cache. Updated the results, but even if getBlockReceipts fetches data from disk it's faster, so it's not really a contributing factor here.

debris

lgtm, just a few questions

debris · 2018-09-12T21:26:14Z

+		/// Get block receipts.
+		/// Allows you to fetch receipts from the entire block at once.
+		#[rpc(name = "parity_getBlockReceipts")]
+		fn block_receipts(&self, Trailing<BlockNumber>) -> BoxFuture<Vec<Receipt>>;


Trailing == latest?

debris · 2018-09-12T21:29:22Z

+	fn block_receipts(&self, number: Trailing<BlockNumber>) -> BoxFuture<Vec<Receipt>> {
+		// Note: Here we treat `Pending` as `Latest`.
+		//       Since light clients don't produce pending blocks
+		//       (they don't have state) we can safely fallback to `Latest`.


redundant extra spaces

debris · 2018-09-12T21:31:08Z

+	receipts.into_iter()
+		.flat_map(|r| {
+			let hash = r.transaction_hash;
+			r.logs.into_iter().map(move |l| (hash.clone(), l))


move and clone() are probably redundant, cause hash implements Copy

debris · 2018-09-12T21:33:35Z

+		let transaction = body.view().localized_transaction_at(&hash, number, address.index)?;
+		let receipt = receipts.pop()?;
+		let gas_used = receipts.last().map_or_else(|| 0.into(), |r| r.gas_used);
+		let no_of_logs = receipts.into_iter().map(|receipt| receipt.logs.len()).sum::<usize>();


I don't understand these 3 lines. why do we pop a receipt and then take a gas_used from the one before it?

So to actually generate a LocalizedReceipt we need 3 things:

Actual receipt

Gas used by that transaction

Number of logs generate prior to this transaction (to calculate log_index_in_block).

So the logic is as follows:

Fetch all receipts from 0 to index (inclusive)

Take the last one (receipts.pop(); point 1.)

Take the previous to last one to calculate (point 2.) (receipts only store cumulative gas, so we calculate current.gas_used - previous.gas_used to figure out what current gas usage of transaction was)

Use all previous receipts to calculate total number of logs. (point 3.)

jimpo

👍

jimpo · 2018-09-13T01:40:00Z

+		Some(receipt)
+	}
+
+	fn block_receipts(&self, id: BlockId) -> Option<Vec<LocalizedReceipt>> {


This method could probably use a test.

Added for both block_receipt and transaction_receipt

jimpo · 2018-09-13T01:44:47Z

+		let id = match number.unwrap_or_default() {
+			BlockNumber::Num(n) => BlockId::Number(n),
+			BlockNumber::Earliest => BlockId::Earliest,
+			BlockNumber::Latest | BlockNumber::Pending => BlockId::Latest,


Since there is no Pending state, I'd say either return an empty vector or keep the deprecation log from num_to_id.

jimpo · 2018-09-13T15:42:19Z

+			BlockNumber::Earliest => BlockId::Earliest,
+			BlockNumber::Latest => BlockId::Latest,
+			BlockNumber::Pending => {
+				warn!("`Pending` is deprecated and may be removed in future versions. Falling back to `Latest`");


Hmm, this should only get logged when accessed from the light client impl. So maybe move back to num_to_id on the light client or add a light_client: bool method param?

Moved it to a trait, so it explicitly says it's for light client only.

5chdn · 2018-09-17T17:06:52Z

please rebase on master

…mon-deps * origin/master: ethereum libfuzzer integration small change (#9547) cli: remove reference to --no-ui in --unlock flag help (#9616) remove master from releasable branches (#9655) ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to (#9620) RPC: parity_getBlockReceipts (#9527) Remove unused dependencies (#9589) ignore key_server_cluster randomly failing tests (#9639) ethcore: handle vm exception when estimating gas (#9615) fix bad-block reporting no reason (#9638) Use static call and apparent value transfer for block reward contract code (#9603) HF in POA Sokol (2018-09-19) (#9607) bump smallvec to 0.6 in ethcore-light, ethstore and whisper (#9588) Add constantinople conf to EvmTestClient. (#9570)

roccomuso · 2018-11-22T23:24:40Z

I'm having a weird behavior, calling parity_getBlockReceipts on one of the latest block number I get RPC error (code: -32602): Unknown block number. For that same block number the rpc eth_getBlockByNumber returns it instead.
What am I missing?

* Block receipts RPC. * Use lazy evaluation of block receipts (ecrecover). * Optimize transaction_receipt to prevent performance regression. * Fix RPC grumbles. * Add block & transaction receipt tests. * Fix conversion to block id.

* bump stable to 2.1.10 * RPC: parity_getBlockReceipts (#9527) * Block receipts RPC. * Use lazy evaluation of block receipts (ecrecover). * Optimize transaction_receipt to prevent performance regression. * Fix RPC grumbles. * Add block & transaction receipt tests. * Fix conversion to block id. * Update a few parity-common dependencies (#9663) * Update a few parity-common dependencies * cleanup * cleanup * revert update of ethereum/tests * better reporting of network rlp errors * Use rlp 0.3.0-beta.1 * fix util function get_dummy_blocks * Already a Vec * encode_list returns vec already * Address grumble * No need for betas * Fix double spaces * Fix empty steps (#9939) * Don't send empty step twice or empty step then block. * Perform basic validation of locally sealed blocks. * Don't include empty step twice. * Strict empty steps validation (#10041) * Add two failings tests for strict empty steps. * Implement strict validation of empty steps. * ethcore: enable constantinople on ethereum (#10031) * ethcore: change blockreward to 2e18 for foundation after constantinople * ethcore: delay diff bomb by 2e6 blocks for foundation after constantinople * ethcore: enable eip-{145,1014,1052,1283} for foundation after constantinople * Change test miner max memory to malloc reports. (#10024) * Bump crossbeam. (#10048) * Revert "Bump crossbeam. (#10048)" This reverts commit ed1db0c.

tomusdrw added 5 commits September 11, 2018 13:37

Block receipts RPC.

f8742e4

Merge branch 'master' into td-blockreceipts

4311b4e

Use lazy evaluation of block receipts (ecrecover).

799d56c

Optimize transaction_receipt to prevent performance regression.

1dd3b94

Merge branch 'master' into td-blockreceipts

a066b53

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Sep 11, 2018

5chdn added this to the 2.1 milestone Sep 11, 2018

debris reviewed Sep 12, 2018

View reviewed changes

debris added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 12, 2018

jimpo reviewed Sep 13, 2018

View reviewed changes

tomusdrw added 3 commits September 13, 2018 09:49

Merge branch 'master' into td-blockreceipts

7619f30

Fix RPC grumbles.

fd3e7be

Add block & transaction receipt tests.

f287e58

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Sep 13, 2018

5chdn modified the milestones: 2.1, 2.2 Sep 13, 2018

jimpo reviewed Sep 13, 2018

View reviewed changes

Fix conversion to block id.

8bb8e78

debris approved these changes Sep 17, 2018

View reviewed changes

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 17, 2018

jimpo approved these changes Sep 17, 2018

View reviewed changes

Merge branch 'master' into td-blockreceipts

87de315

5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 19, 2018

debris mentioned this pull request Sep 24, 2018

darwin test randomly failing #9632

Closed

Merge branch 'master' into td-blockreceipts

14c50f6

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 24, 2018

debris mentioned this pull request Sep 25, 2018

ws tests randomly failing #9646

Closed

debris requested a review from andresilva September 25, 2018 11:38

andresilva approved these changes Sep 25, 2018

View reviewed changes

debris merged commit cc963d4 into master Sep 25, 2018

debris deleted the td-blockreceipts branch September 25, 2018 17:06

Tbaut mentioned this pull request Dec 12, 2018

Backports for stable 2.1.10 #10046

Merged

7 tasks

Conversation

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

Uh oh!

Uh oh!

debris 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5chdn commented Sep 17, 2018

Uh oh!

roccomuso commented Nov 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tomusdrw commented Sep 11, 2018 •

edited

Loading