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

[ethcore]: apply filter when PendingSet::AlwaysQueue in ready_transactions_filtered #11227

Merged
merged 14 commits into from
Nov 25, 2019
Merged

[ethcore]: apply filter when PendingSet::AlwaysQueue in ready_transactions_filtered #11227

merged 14 commits into from
Nov 25, 2019

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Oct 31, 2019 •
edited
Loading

Fixes that the filter is applied for when parity is executed with --relay-set= cheap i.e, fetch transactions from the transaction queue

The important changes are:

  1. Apply filter after calling self.transaction_queue.pending(... same for from_pending
  2. Moved the methods related to filtering in filter_options to avoid doing several times
  3. Added some basic tests in ethcore/miner/miner
  4. First, I attempted to mock this with RPC tests but it wouldn't test the real filtering logic so I didn't bother.
  5. Fixed a bunch of whitespaces, replaced spaces with tabs

The semantics will be different between from_queue and from_pending,

  • from_queue: will receive a Vec with at most max_len and then the filter is applied, to avoid allocating a huge vector if there are many transactions in the queue
  • from_pending: will iterate through the pending transactions and apply filter until max_len transactions are collected or all have been visited.

@niklasad1
[ethcore]: apply filter when from_queue
09be238 
In `ready_transactions_filtered` the filter were never applied when
the options PendingSet::AlwaysQueue` was configured which this fixes

It also adds a two tests for it
@niklasad1
Merge remote-tracking branch 'origin/master' into na-rpc-pending-tran…
cf15877 
…sactions-filter
@niklasad1
[ethcore test-helpers]: stray printlns
56a116d
niklasad1
niklasad1 commented Oct 31, 2019
View reviewed changes
ethcore/src/miner/filter_options.rs
use std::fmt;
use std::marker::PhantomData;

use ethereum_types::{Address, U256};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

most changes are whitespaces

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 2, 2019
@niklasad1 niklasad1 added this to the 2.7 milestone Nov 2, 2019
@niklasad1 niklasad1 marked this pull request as ready for review November 2, 2019 10:02
@soc1c soc1c mentioned this pull request Nov 5, 2019
Merged
35 tasks
dvdplm
dvdplm approved these changes Nov 6, 2019
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

minor nits, looks good

ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/miner.rs Show resolved Hide resolved
ethcore/src/miner/miner.rs Show resolved Hide resolved
ethcore/src/miner/miner.rs Show resolved Hide resolved
niklasad1 and others added 7 commits November 6, 2019 23:12
@niklasad1 @dvdplm
doc: nit
1abcc89 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
doc: nit
6ada111 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
doc: nit
a8a4889 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
doc: nit
6069b03 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1 @dvdplm
doc: nit
266db00 
Co-Authored-By: David <dvdplm@gmail.com>
@niklasad1
doc(miner filter): simplify documentation
5e97192
@niklasad1
Merge remote-tracking branch 'origin/master' into na-rpc-pending-tran…
75ae010 
…sactions-filter
@niklasad1 niklasad1 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 Nov 7, 2019
ordian
ordian approved these changes Nov 25, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

LGTM

@ordian 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 Nov 25, 2019
@ordian ordian merged commit dcb69ba into master Nov 25, 2019
@ordian ordian deleted the na-rpc-pending-transactions-filter branch November 25, 2019 11:03
ordian added a commit that referenced this pull request Nov 25, 2019
@ordian
Merge branch 'master' into dp/chore/use-upstream-rocksdb
c694323 
* master:
  [ethcore]: apply filter when `PendingSet::AlwaysQueue` in `ready_transactions_filtered` (#11227)
  Update lib.rs (#11286)
  Don't prune ancient state when instantiating a Client (#11270)
  fixed verify_uncles error type (#11276)
  ethcore: fix rlp deprecation warnings (#11280)
@niklasad1 niklasad1 mentioned this pull request Dec 17, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@niklasad1 @dvdplm @ordian