RPC: parity_getBlockReceipts #9527

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

rpc/src/v1/traits/parity.rs

+		/// 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

rpc/src/v1/impls/light/parity.rs

+	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

rpc/src/v1/impls/eth.rs

+	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

ethcore/src/client/client.rs

+		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

ethcore/src/client/client.rs

+		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

rpc/src/v1/impls/light/parity.rs

+		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

rpc/src/v1/types/block_number.rs

+			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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: parity_getBlockReceipts #9527

RPC: parity_getBlockReceipts #9527

tomusdrw commented Sep 11, 2018 •

edited

Loading

debris left a comment

debris Sep 12, 2018

debris Sep 12, 2018

debris Sep 12, 2018

debris Sep 12, 2018

tomusdrw Sep 13, 2018

jimpo left a comment

jimpo Sep 13, 2018

tomusdrw Sep 13, 2018

jimpo Sep 13, 2018

jimpo Sep 13, 2018

tomusdrw Sep 17, 2018

5chdn commented Sep 17, 2018

roccomuso commented Nov 22, 2018

RPC: parity_getBlockReceipts #9527

RPC: parity_getBlockReceipts #9527

Conversation

tomusdrw commented Sep 11, 2018 • edited Loading

debris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimpo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5chdn commented Sep 17, 2018

roccomuso commented Nov 22, 2018

tomusdrw commented Sep 11, 2018 •

edited

Loading