Add pagination for trace_filter rpc method #6312

grbIzl · 2017-08-16T13:05:26Z

This work is done in terms of Trace & Fat DB module improvement suggestions #4858 issue.
trace_filter method was extended with two optional parameters offset and count in order to paginate through output traces.
For offset the new type was created TraceIndex in order to identify the trace. Inside the client it's converted into TraceId
New tests were not created as we have only trivial implementation for these methods in test_client.

tomusdrw · 2017-08-16T15:00:05Z

ethcore/src/client/client.rs

@@ -1641,7 +1641,7 @@ impl BlockChainClient for Client {
 		self.chain.read().logs(blocks, |entry| filter.matches(entry), filter.limit)
 	}

-	fn filter_traces(&self, filter: TraceFilter) -> Option<Vec<LocalizedTrace>> {
+	fn filter_traces(&self, filter: TraceFilter, after: Option<&TraceId>, count: Option<u64>) -> Option<Vec<LocalizedTrace>> {


Why not making it part of TraceFilter?

Actually there were 3 options how to add this here:

Modify TraceFIlter

Add optional parameters to filter_traces

Instead add new method like filter_traces_with_pagination
I've discussed it with @arkpar and we chosen the second one. But frankly all of them pretty similar. There is no decisive advantage for any of them.

Using separate parameters breaks backward compatibility, we already support limit parameter for eth_getLogs filter object and that would be inline with that.

Would be good if those parameters could be omitted (using Trailing), but we don't suport two optional parameters.

IMHO would be better to make it part of TraceFilter object.

tomusdrw · 2017-08-16T15:01:59Z

ethcore/src/client/traits.rs

@@ -195,7 +195,7 @@ pub trait BlockChainClient : Sync + Send {
 	fn replay(&self, t: TransactionId, analytics: CallAnalytics) -> Result<Executed, CallError>;

 	/// Returns traces matching given filter.
-	fn filter_traces(&self, filter: TraceFilter) -> Option<Vec<LocalizedTrace>>;
+	fn filter_traces(&self, filter: TraceFilter, after: Option<&TraceId>, count: Option<u64>) -> Option<Vec<LocalizedTrace>>;


usize will be enough for count

svyatonik · 2017-08-29T15:50:49Z

ethcore/src/client/client.rs

+
+				let mut traces_iter = traces.into_iter();
+				if let Some(after) = after {
+					if let TransactionId::Location(BlockId::Hash(block_hash), transaction_number) = after.transaction {


Why does it quietly ignores after argument if it is Some(TransactionId::Hash(_))? I can see that in RPC only TransactionId::Location is used, but it is not good to have public method in separate (not RPC crate) which accepts two options and quietly works for single option only.

svyatonik · 2017-08-29T15:51:11Z

ethcore/src/client/client.rs

+				let mut traces_iter = traces.into_iter();
+				if let Some(after) = after {
+					if let TransactionId::Location(BlockId::Hash(block_hash), transaction_number) = after.transaction {
+						match traces_iter.find(|ref trace| trace.block_hash == block_hash


ref is not needed in ref trace here

svyatonik · 2017-08-29T15:51:42Z

ethcore/src/client/client.rs

+					}
+				}
+				if let Some(count) = count {
+					traces_iter = traces_iter.take(count as usize).collect::<Vec<_>>().into_iter();


count is already usize, so count as usize is not needed here

svyatonik · 2017-08-29T15:52:18Z

ethcore/src/client/client.rs

+						}
+					}
+				}
+				if let Some(count) = count {


You could avoid extra allocation here like that:

Some(match count { Some(count) => traces_iter.take(count as usize).collect(), None => traces_iter.collect(), })

debris · 2017-08-31T10:46:08Z

ethcore/src/client/client.rs

+						TransactionId::Hash(transaction_hash) => {
+							match traces_iter.find(|trace| trace.transaction_hash == transaction_hash) {
+								Some(_) => {},
+								None => return None,


it's more idiomatic to write:

if traces_iter.find(|trace| trace.transaction_hash == transaction_hash).is_none() { return None; }

debris · 2017-08-31T10:53:38Z

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

@@ -31,7 +31,7 @@ pub struct TracesClient;
 impl Traces for TracesClient {
 	type Metadata = Metadata;

-	fn filter(&self, _filter: TraceFilter) -> Result<Option<Vec<LocalizedTrace>>, Error> {
+	fn filter(&self, _filter: TraceFilter, _offset: Option<TraceIndex>, _count: Option<usize>) -> Result<Option<Vec<LocalizedTrace>>, Error> {


wouldn't offset being usize be faster and easier to use (and to implement)?

With offset being TraceIndex you can't fetch traces in parallel and/or from multiple clients. I imagine this might be a common practice for exchanges.

// thread 0,
ethereum_client_0.filter(filter_options, 0, 1000);

// thread 1,
ethereum_client_1.filter(filter_options, 1000, 2000);

// thread 2,
ethereum_client_2.filter(filter_options, 2000, 3000);

tomusdrw · 2017-09-18T08:12:25Z

ethcore/src/client/client.rs

-				Some(traces)
+				let traces = self.tracedb.read().filter(&db_filter);
+				if traces.is_empty() {
+					return None;


Why not Some(vec![]) - I think None was used to indicate invalid block range.

gavofyork · 2017-09-26T11:04:17Z

@grbIzl still waiting for this (fairly minor) change...

SinErgy84 · 2018-02-23T12:15:53Z

Please keep your wiki up2date ...
mentioned changes are missing on https://github.com/paritytech/parity/wiki/JSONRPC-trace-module#trace_filter

5chdn · 2018-02-23T14:26:58Z

FYI https://github.com/paritytech/wiki/blob/master/JSONRPC-trace-module.md#trace_filter

grbIzl added A0-pleasereview 🤓 Pull request needs code review. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Aug 16, 2017

tomusdrw reviewed Aug 16, 2017

View reviewed changes

grbIzl added the M6-rpcapi 📣 RPC API. label Aug 17, 2017

grbIzl added 5 commits August 28, 2017 15:52

Optional offset and count added on client level

bfecd7a

Rpc interface extended

50be1ff

Tests corrected

f92a66b

Filtering code refactored a little bit

44a6332

Offset type was changed to usize

d3ae485

grbIzl force-pushed the Fix-4858-p2 branch from 89710e5 to d3ae485 Compare August 28, 2017 13:55

svyatonik reviewed Aug 29, 2017

View reviewed changes

NikVolf added the M7-ui label Aug 30, 2017

Handling of block hash in offset added

b746519

debris reviewed Aug 31, 2017

View reviewed changes

grbIzl added 3 commits September 1, 2017 15:09

Offset reworked to number

b938eee

Trailing comma removed

0011823

Import for Quantity type added

44bd019

grbIzl added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 5, 2017

grbIzl added 2 commits September 11, 2017 17:12

Offset and count moved into filter object

b316852

Lost comma removed

b18d555

grbIzl mentioned this pull request Sep 12, 2017

Cannot run Rust tests on Parity Gitlab in the specific scenario #6507

Closed

grbIzl added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 12, 2017

tomusdrw approved these changes Sep 18, 2017

View reviewed changes

tomusdrw 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 18, 2017

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

Return empty vec instead of None in case of incorrect range

e31c987

grbIzl added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 28, 2017

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 2, 2017

gavofyork merged commit 0a5c9b0 into master Oct 3, 2017

gavofyork deleted the Fix-4858-p2 branch October 3, 2017 08:03

debris mentioned this pull request Feb 23, 2018

Add documentation for trace_filter rpc method #7989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination for trace_filter rpc method #6312

Add pagination for trace_filter rpc method #6312

grbIzl commented Aug 16, 2017

tomusdrw Aug 16, 2017

grbIzl Aug 16, 2017

tomusdrw Sep 4, 2017

tomusdrw Aug 16, 2017

grbIzl Aug 16, 2017

svyatonik Aug 29, 2017

svyatonik Aug 29, 2017

svyatonik Aug 29, 2017

svyatonik Aug 29, 2017

debris Aug 31, 2017

debris Aug 31, 2017 •

edited

Loading

tomusdrw Sep 18, 2017

gavofyork commented Sep 26, 2017

SinErgy84 commented Feb 23, 2018

5chdn commented Feb 23, 2018

Add pagination for trace_filter rpc method #6312

Add pagination for trace_filter rpc method #6312

Conversation

grbIzl commented Aug 16, 2017

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

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

debris Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Sep 26, 2017

SinErgy84 commented Feb 23, 2018

5chdn commented Feb 23, 2018

debris Aug 31, 2017 •

edited

Loading