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

Add filtering capability to parity_pendingTransactions (issue 8269) #10506

Merged
merged 52 commits into from
Jun 28, 2019

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Mar 21, 2019

Hi

This pull request adds filtering capabilities for parity_pendingTransactions, as requested in issue #8269.

Changes

  • MinerService now has a new ready_transactions_filtered method, which can filter transactions by hash, sender or receiver.
  • The regular ready_transactions method wraps over the filter, therefore ready_transactions_filtered can be called directly from the RPC interface without having to adjust code through the whole codebase.

Considerations
I'm having slight trouble testing the code; ready_transactions (the original implementation) always returns two pending transactions on me, never more. This makes testing inconvenient. It might be related to a detail in the test miner that I'm missing, basing my assumption on this method description:

/// Get a list of all ready transactions either ordered by priority or unordered (cheaper).
///
/// Depending on the settings may look in transaction pool or only in pending block.
/// If you don't need a full set of transactions, you can add max_len and create only a limited set of
/// transactions.

If someone can point me in the right direction I can write some complete test cases (so far, NO testing is being done).

See fn filter_pending_transactions_by_tx_hash() in ethcore/src/miner/miner.rs for more.

Any feedback is welcome!

@parity-cla-bot

This comment has been minimized.

@lamafab

This comment has been minimized.

@parity-cla-bot

This comment has been minimized.

@lamafab

This comment has been minimized.

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@soc1c soc1c added this to the 2.5 milestone Mar 22, 2019
@soc1c soc1c added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Mar 22, 2019
@soc1c soc1c modified the milestones: 2.5, 2.6 Apr 2, 2019
@folsen folsen requested review from tomusdrw and ascjones May 31, 2019 14:26
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good as a starting point, I think I'd go one step further and make the implementation more complete in terms of possible options to filtering but also to avoid breaking backward compatibility (the preoposed filtering object could be made optional with a some default value).

I'm having slight trouble testing the code; ready_transactions (the original implementation) always returns two pending transactions on me, never more.

That's because RPC tests are not using the real MiningService, but rather one for tests.
It would be best to test the serialisation/deserialisation in RPC tests, but the filtering logic deserves a separate test in miner module.

ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved
ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved
ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved
ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved
ethcore/src/miner/mod.rs Outdated Show resolved Hide resolved
@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 3, 2019
@ascjones
Copy link
Contributor

ascjones commented Jun 4, 2019

@lamafab needs merging with master

@lamafab
Copy link
Contributor Author

lamafab commented Jun 4, 2019

@tomusdrw @ascjones How quickly do those changes need to be implemented? I'm currently going through exam week, but I can complete this PR, soon. However, I can not guarantee on exactly when...

Regarding testing: I will skip the direct MiningService tests and use the RPC API, as I did in an other PR.

@lamafab
Copy link
Contributor Author

lamafab commented Jun 24, 2019

@tomusdrw @ascjones Alright, I did it! The unified FilterOptions are now implemented in this PR.

pub struct FilterOptions {
    pub sender: FilterOperator<Address>,
    pub receiver: FilterOperator<Address>,
    pub gas: FilterOperator<U256>,
    pub gas_price: FilterOperator<U256>,
    pub value: FilterOperator<U256>,
    pub nonce: FilterOperator<U256>,
}

pub enum FilterOperator<T> {
    Any,
    Eq(T),
    GreaterThan(T),
    LessThan(T),
    ContractCreation, // only used for `receiver`
}

What this PR contains:

  • The parity_pendingTransactions RPC API can take optional filter operators like the following:
    {
        "sender": {
            "eq": "0x5f3dffcf347944d3739b0805c934d86c8621997f"
        },
        "gas": {
            "gt": "0x493e0"
        }
    }
  • Since not all operators can be used equally on each filter, the allowed/disallowed operators are caught during the deserialization process with a custom implementation of Deserialize (those tests are included in the same location where FilterOptions is defined).

What's missing

  • One could argue that there should be GreaterEqualThan and LessEqualThan operators.
  • While there are deserialization tests, there are no logic tests for the filtering. @tomusdrw I did a code base search and could not find a lot of useful information regarding testing that API (there are some very simple/oversimplified tests). Can you tell me where those logic tests should be placed and what testing data I should use (or to be more specific: how should the RPC API test server be created?). I can hardly use Gitter during the workday, so I'd appreciate it if you could give me some advice here.

Thank you very much (and for the patience!)

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome work! Only few small nits.

ethcore/Cargo.toml Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Show resolved Hide resolved
ethcore/src/miner/filter_options.rs Outdated Show resolved Hide resolved
}

#[cfg(test)]
mod tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

ethcore/types/src/transaction/transaction.rs Outdated Show resolved Hide resolved
ethcore/types/src/transaction/transaction.rs Outdated Show resolved Hide resolved
ethcore/types/src/transaction/transaction.rs Outdated Show resolved Hide resolved
rpc/src/v1/impls/light/parity.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Member

Amazing work 🤝

Copy link
Member

@seunlanlege seunlanlege left a comment

Choose a reason for hiding this comment

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

Beautiful work here.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

👀

@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 27, 2019
@ordian ordian added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jun 27, 2019
@lamafab
Copy link
Contributor Author

lamafab commented Jun 27, 2019

Thanks for the nice feedback, everyone. I've added some very small changes (like adding a newline at the end of the file as @seunlanlege suggested).

EDIT: Looks like the failed CI tests are not related to my changes... (?) Last time this happened too, I remember restarting the CI tests solved the issues.

@seunlanlege seunlanlege merged commit 3f61f2d into openethereum:master Jun 28, 2019
dvdplm added a commit that referenced this pull request Jun 28, 2019
…ckChain

* master:
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  removed EthEngine alias (#10805)
  wait a bit longer in should_check_status_of_request_when_its_resolved (#10808)
  Do not drop the peer with None difficulty (#10772)
  ethcore-bloom-journal updated to 2018 (#10804)
  ethcore-light uses bincode 1.1 (#10798)
  Fix a few typos and unused warnings. (#10803)
  updated project to ansi_term 0.11 (#10799)
  added new ropsten-bootnode and removed old one (#10794)
  updated price-info to edition 2018 (#10801)
  ethcore-network-devp2p uses igd 0.9 (#10797)
  updated parity-local-store to edition 2018 and removed redundant Error type (#10800)
dvdplm added a commit that referenced this pull request Jul 1, 2019
…me-parent

* master: (24 commits)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  removed EthEngine alias (#10805)
  wait a bit longer in should_check_status_of_request_when_its_resolved (#10808)
  Do not drop the peer with None difficulty (#10772)
  ethcore-bloom-journal updated to 2018 (#10804)
  ethcore-light uses bincode 1.1 (#10798)
  Fix a few typos and unused warnings. (#10803)
  updated project to ansi_term 0.11 (#10799)
  added new ropsten-bootnode and removed old one (#10794)
  updated price-info to edition 2018 (#10801)
  ethcore-network-devp2p uses igd 0.9 (#10797)
  updated parity-local-store to edition 2018 and removed redundant Error type (#10800)
  Cleanup unused vm dependencies (#10787)
  ...
dvdplm added a commit that referenced this pull request Jul 3, 2019
* master: (22 commits)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  removed EthEngine alias (#10805)
  wait a bit longer in should_check_status_of_request_when_its_resolved (#10808)
  Do not drop the peer with None difficulty (#10772)
  ethcore-bloom-journal updated to 2018 (#10804)
  ethcore-light uses bincode 1.1 (#10798)
  Fix a few typos and unused warnings. (#10803)
  updated project to ansi_term 0.11 (#10799)
  added new ropsten-bootnode and removed old one (#10794)
  updated price-info to edition 2018 (#10801)
  ...
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master: (21 commits)
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants