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

fixed ethcore tx_filter #8200

Merged
merged 1 commit into from
Mar 28, 2018
Merged

fixed ethcore tx_filter #8200

merged 1 commit into from
Mar 28, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Mar 23, 2018

  • TransactionFilter was never added as ChainNotify
  • therefore cache was never cleared
  • this could lead to incorrect permissions being granted

a simplest solution is to remove a cache at all, cause it would be cleared on every block anyway use lru_cache instead of manually cleaning the cache

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 23, 2018
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.

This is pretty hot code during transaction import (note we are checking EVERY transaction), imho cache would make sense here. Why not add it to ChainNotify?

@5chdn 5chdn added this to the 1.11 milestone Mar 23, 2018
@debris
Copy link
Collaborator Author

debris commented Mar 24, 2018

This is pretty hot code during transaction import (note we are checking EVERY transaction), imho cache would make sense here

You are right. I looked at the code again and actually not cleaning the cache does not lead to incorrect behaviour. Cache just becomes useless.

Why not add it to ChainNotify?

I want to get rid of ChainNotify

I updated the pr. Replaced HashMap with LruCache

@debris
Copy link
Collaborator Author

debris commented Mar 28, 2018

cc @andresilva @tomusdrw

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM!

@andresilva andresilva merged commit 23ea479 into master Mar 28, 2018
@5chdn 5chdn deleted the fixed_tx_filter branch March 28, 2018 10:21
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 16, 2018
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants