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

Optimize pending transactions filter #9014

Closed
wants to merge 9 commits into from

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Jun 29, 2018

And make pending transaction count display on ethstats again!

@andresilva
Copy link
Contributor Author

Fat fingers.

@andresilva andresilva closed this Jun 29, 2018
@tomusdrw tomusdrw added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. B0-patchthis labels Jun 29, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 29, 2018
@tomusdrw tomusdrw changed the title Andre/optimize pending set filter Optimize pending transactions filter Jun 29, 2018
@tomusdrw tomusdrw reopened this Jun 29, 2018
PollFilter::Logs(.., ref filter) => Some(filter.clone()),
_ => None,
})) {
Some(filter) => filter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Intentionally changed the behaviour here, returning empty vector when querying e.g. pendingTransactionFilter for logs was weird. I think "Filter not found" is more appropriate response.

@5chdn 5chdn mentioned this pull request Jun 30, 2018
12 tasks
@5chdn
Copy link
Contributor

5chdn commented Jul 2, 2018

Why is this on ice?

@andresilva
Copy link
Contributor Author

I opened this PR eagerly by mistake (was just checking this branch with @tomusdrw). I think @tomusdrw is still benchmarking this so it's possible there might be some more changes needed.

(Super dumb, but up until now I though that label meant "oh nice" 😅)

@tomusdrw tomusdrw removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 2, 2018
@5chdn
Copy link
Contributor

5chdn commented Jul 2, 2018

Oh, nice! 😆

@5chdn 5chdn added the A0-pleasereview 🤓 Pull request needs code review. label Jul 2, 2018
@tomusdrw
Copy link
Collaborator

tomusdrw commented Jul 2, 2018

Yeah, sorry about that. I labelled the before after I realised that Andre closed it. I'm done with it now, so it can be left open.

As for the context of the PR:
Currently pending transactions filter is using the very inefficient ready_transactions method from miner. For large pools it takes significant amount of time to construct the pending set, because:

  1. We return transactions ordered by priority (gas price)
  2. We check for readiness of transactions - i.e. we need to check the state nonce for every sender, and it's really expensive.
    Because of that it can take seconds to construct an ordered pending set of 128k transactions (~110k senders).

The pending transactions filter doesn't require such pending set though, we are ok with unordered hashes and we don't need to be super sure about the nonce, so this PR:

  1. Uses unordered transactions from the queue
  2. Doesn't use the default Ready (which goes to the state), but rather assumes the transactions in the pool are ready - this may give us a couple of false-positives, but it's not critical for pending transactions hashes.

Thanks to this it works really fast (just constructs the BTreeSet and then does the difference).

@5chdn
Copy link
Contributor

5chdn commented Jul 2, 2018

Please assign reviewers yourself. I'm confused about the authorship of this PR :)

@tomusdrw tomusdrw closed this Jul 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants