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

tx-pool: accept local tx with higher gas price when pool full #10901

Merged
merged 6 commits into from
Aug 13, 2019
Merged

tx-pool: accept local tx with higher gas price when pool full #10901

merged 6 commits into from
Aug 13, 2019

Conversation

@ascjones
Copy link
Contributor

@ascjones ascjones commented Jul 18, 2019 •
edited
Loading

Attempt at resolving #10671.

I believe what was happening:

  1. Pool becomes full (memory or transaction limit exceeded)
  2. Local tx(1) submitted with gas price x
  3. tx(1) accepted, replaces worst transaction (because it is better, local priority and/or higher gas price)
  4. Local tx(2) submitted with same sender and nonce as tx(1), but a higher gas price.
  5. tx(2) rejected (as described in Local transactions with same nonce but significantly higher gas price are rejected #10671) because it falls foul of the logic introduced in tx-pool: check transaction readiness before replacing #10526: a ready transaction cannot be replaced by a non-ready transaction. The existence of tx(1) meant that the new tx was being calculated as not ready, and thus rejected as it is in this case.

This fix will accept any transaction which has the same sender/nonce and a higher gas price.

@ascjones ascjones requested a review from tomusdrw July 18, 2019 14:22
@ascjones ascjones marked this pull request as ready for review July 19, 2019 14:53
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 24, 2019
@ordian ordian added this to the 2.7 milestone Jul 24, 2019
@ordian ordian added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Jul 24, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Aug 10, 2019

@tomusdrw can you take a look at this please? :)

tomusdrw
tomusdrw reviewed Aug 13, 2019
View reviewed changes
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, but we could avoid dropping old transaction from the pool actually.

miner/src/pool/replace.rs Outdated
// check for an existing transaction with the same nonce
if let Some(txs) = new.pooled_by_sender {
if let Ok(index) = txs.binary_search_by(|old| self.scoring.compare(old, new)) {
return self.scoring.choose(&txs[index], new)
Copy link
Collaborator

@tomusdrw tomusdrw Aug 13, 2019

Choose a reason for hiding this comment

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

We should convert ReplaceOld to InsertNew, to prevent removing the worst transaction from the pool - since we detected that this is a replacement transaction, it will, well, replace some other transaction anyway.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 13, 2019
@ascjones ascjones merged commit bd2e4f9 into master Aug 13, 2019
@ascjones ascjones deleted the aj-local-tx branch August 13, 2019 13:21
s3krit pushed a commit that referenced this pull request Aug 14, 2019
@ascjones @s3krit
tx-pool: accept local tx with higher gas price when pool full (#10901)
a6c3faa 
* tx-pool: accept local tx with higher gas price when pool full

* Revert "tx-pool: accept local tx with higher gas price when pool full"

This reverts commit 9d4adc5

* tx-pool: new tx with same nonce as existing is ready

* tx-pool: explicit check for replacement tx (same sender & nonce)

* Update comment

Co-Authored-By: Tomasz Drwięga <[email protected]>

* Replace `ReplaceOld` with `InsertNew` for replacement txs
ordian pushed a commit that referenced this pull request Aug 15, 2019
Merge branch 'master' into rpc-txpool-error
50cb305 
* master:
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
  Extract Machine from ethcore (#10949)
  removed redundant state_root function from spec, improve spec error types (#10955)
  Add support for Energy Web Foundation's new chains (#10957)
  [evmbin] add more tests to main.rs (#10956)
  Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)
  unify loading spec && further spec cleanups (#10948)
  refactor: Refactor evmbin CLI (#10742)
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated [email protected] key (#10939)
  Change the return type of step_inner function. (#10940)
ordian pushed a commit that referenced this pull request Aug 15, 2019
Merge branch 'master' into dp/chore/extract-engine-trait
dcaf256 
* master:
  Verify transaction against its block during import (#10954)
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
dvdplm added a commit that referenced this pull request Aug 15, 2019
@dvdplm
Merge branch 'master' into dp/chore/extract-clique
42e1d57 
* master:
  Extract the Engine trait (#10958)
  Better error message for rpc gas price errors (#10931)
  [.gitlab.yml] cargo check ethcore benches (#10965)
  Verify transaction against its block during import (#10954)
  [evmbin] fix compilation (#10976)
  Update to latest trie version. (#10972)
  [blooms-db] Fix benchmarks (#10974)
  Fix ethcore/benches build. (#10964)
  tx-pool: accept local tx with higher gas price when pool full (#10901)
  Disable unsyncable expanse chain (#10926)
@hackaugusto hackaugusto mentioned this pull request Sep 9, 2019
Closed
s3krit pushed a commit that referenced this pull request Sep 11, 2019
@ascjones @s3krit
tx-pool: accept local tx with higher gas price when pool full (#10901)
c80f98d 
* tx-pool: accept local tx with higher gas price when pool full

* Revert "tx-pool: accept local tx with higher gas price when pool full"

This reverts commit 9d4adc5

* tx-pool: new tx with same nonce as existing is ready

* tx-pool: explicit check for replacement tx (same sender & nonce)

* Update comment

Co-Authored-By: Tomasz Drwięga <[email protected]>

* Replace `ReplaceOld` with `InsertNew` for replacement txs
@s3krit s3krit mentioned this pull request Sep 12, 2019
Merged
s3krit added a commit that referenced this pull request Sep 12, 2019
@s3krit
v2.5.8-stable (#11041)
45f27ce 
* add more tx tests (#11038)
* Fix parallel transactions race-condition (#10995)
* Add blake2_f precompile (#11017)
* [trace] introduce trace failed to Ext (#11019)
* Edit publish-onchain.sh to use https (#11016)
* Fix deadlock in network-devp2p (#11013)
* EIP 1108: Reduce alt_bn128 precompile gas costs (#11008)
* xDai chain support and nodes list update (#10989)
* EIP 2028: transaction gas lowered from 68 to 16 (#10987)
* EIP-1344 Add CHAINID op-code (#10983)
* manual publish jobs for releases, no changes for nightlies (#10977)
* [blooms-db] Fix benchmarks (#10974)
* Verify transaction against its block during import (#10954)
* Better error message for rpc gas price errors (#10931)
* tx-pool: accept local tx with higher gas price when pool full (#10901)
* Fix fork choice (#10837)
* Cleanup unused vm dependencies (#10787)
* Fix compilation on recent nightlies (#10991)
@ascjones ascjones mentioned this pull request Oct 1, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dvdplm dvdplm dvdplm approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

5 participants

@ascjones @dvdplm @tomusdrw @ordian