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

Improve should_replace on NonceAndGasPrice #8980

Merged
merged 3 commits into from
Jun 27, 2018

Conversation

jimpo
Copy link
Contributor

@jimpo jimpo commented Jun 25, 2018

The current implementation of should_replace calls choose with transactions between potentially different senders, which violates the expectation. As a result, transactions may get replaced incorrectly with ones with a lower gas price if the nonces differ.

Noticed during review of #8934.

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 26, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 26, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 26, 2018

Please rebase on latest master to fix CI.

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!

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 27, 2018
@debris debris merged commit 9b5483a into openethereum:master Jun 27, 2018
@jimpo jimpo deleted the should-replace branch June 27, 2018 20:44
dvdplm added a commit that referenced this pull request Jul 2, 2018
* master:
  Minimal effective gas price in the queue (#8934)
  parity: fix db path when migrating to blooms db (#8975)
  Preserve the current abort behavior (#8995)
  Improve should_replace on NonceAndGasPrice (#8980)
  Tentative fix for missing dependency error (#8973)
@5chdn 5chdn mentioned this pull request Jul 7, 2018
12 tasks
andresilva pushed a commit that referenced this pull request Jul 7, 2018
* Additional tests for NonceAndGasPrice::should_replace.

* Fix should_replace in the distinct sender case.

* Use natural priority ordering to simplify should_replace.
5chdn added a commit that referenced this pull request Jul 9, 2018
* parity-version: bump beta to 1.11.6

* scripts: remove md5 checksums (#8884)

* Add support for --chain tobalaba

* Convert indents to tabs :)

* Fixes for misbehavior reporting in AuthorityRound (#8998)

* aura: only report after checking for repeated skipped primaries

* aura: refactor duplicate code for getting epoch validator set

* aura: verify_external: report on validator set contract instance

* aura: use correct validator set epoch number when reporting

* aura: use epoch set when verifying blocks

* aura: report skipped primaries when generating seal

* aura: handle immediate transitions

* aura: don't report skipped steps from genesis to first block

* aura: fix reporting test

* aura: refactor duplicate code to handle immediate_transitions

* aura: let reporting fail on verify_block_basic

* aura: add comment about possible failure of reporting

* Only return error log for rustls (#9025)

* Transaction Pool improvements (#8470)

* Don't use ethereum_types in transaction pool.

* Hide internal insertion_id.

* Fix tests.

* Review grumbles.

* Improve should_replace on NonceAndGasPrice (#8980)

* Additional tests for NonceAndGasPrice::should_replace.

* Fix should_replace in the distinct sender case.

* Use natural priority ordering to simplify should_replace.

* Minimal effective gas price in the queue (#8934)

* Minimal effective gas price.

* Fix naming, add test

* Fix minimal entry score and add test.

* Fix worst_transaction.

* Remove effective gas price threshold.

* Don't leak gas_price decisions out of Scoring.

* Never drop local transactions from different senders. (#9002)

* Recently rejected cache for transaction queue (#9005)

* Store recently rejected transactions.

* Don't cache AlreadyImported rejections.

* Make the size of transaction verification queue dependent on pool size.

* Add a test for recently rejected.

* Fix logging for recently rejected.

* Make rejection cache smaller.

* obsolete test removed

* obsolete test removed

* Construct cache with_capacity.

* Optimize pending transactions filter (#9026)

* rpc: return unordered transactions in pending transactions filter

* ethcore: use LruCache for nonce cache

Only clear the nonce cache when a block is retracted

* Revert "ethcore: use LruCache for nonce cache"

This reverts commit b382c19.

* Use only cached nonces when computing pending hashes.

* Give filters their own locks, so that they don't block one another.

* Fix pending transaction count if not sealing.

* Clear cache only when block is enacted.

* Fix RPC tests.

* Address review comments.

* A last bunch of txqueue performance optimizations (#9024)

* Clear cache only when block is enacted.

* Add tracing for cull.

* Cull split.

* Cull after creating pending block.

* Add constant, remove sync::read tracing.

* Reset debug.

* Remove excessive tracing.

* Use struct for NonceCache.

* Fix build

* Remove warnings.

* Fix build again.

* miner: add missing macro use for trace_time

* ci: remove md5 merge leftovers
ordian added a commit to ordian/parity that referenced this pull request Jul 9, 2018
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  parity: fix db path when migrating to blooms db (openethereum#8975)
  Preserve the current abort behavior (openethereum#8995)
  Improve should_replace on NonceAndGasPrice (openethereum#8980)
  Tentative fix for missing dependency error (openethereum#8973)
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.

5 participants