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

Minimal effective gas price in the queue #8934

Merged
merged 8 commits into from
Jun 30, 2018
Merged

Minimal effective gas price in the queue #8934

merged 8 commits into from
Jun 30, 2018

Conversation

tomusdrw
Copy link
Collaborator

For small (and saturated) queues this optimizes the verification pipeline to avoid recovering a sender in case we are quite sure that the transaction won't get into the pool anyway.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 20, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 20, 2018
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.

Looks good. Noticed an unrelated potential issue in pool.remove_worst()

return None
}

self.worst_transactions.iter().next().map(|x| x.score.clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Using next() here which looks correct (worst_transactions is ordered by ascending score). However I noticed: https://github.com/paritytech/parity/blob/0335e927e5d2133ceba4b54579094f1b4d810eac/transaction-pool/src/pool.rs#L277
Is the next_back() wrong then? Seems like it would check for the least worst transaction to replace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch, I'll prepare a test and fix that issue with remove_worst

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it's correct in remove_worst. ScoreWithRef has a custom Ord implementation that is ascending if the internal score is descending. So the error is actually here, will make sure to add a correct test to cover this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, preparing a test case to cover that and a fix.

@5chdn
Copy link
Contributor

5chdn commented Jun 22, 2018

Needs a 2nd review.

@tomusdrw tomusdrw added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 22, 2018
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jun 22, 2018
@tomusdrw
Copy link
Collaborator Author

@ascjones thanks for the review, the issue with worst_transaction is addressed now.

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.

Approval 2.0 after worst_transaction fixes. (Still needs a second reviewer though)

@jimpo
Copy link
Contributor

jimpo commented Jun 25, 2018

Code looks fine, but I think this can be better encapsulated in scoring. Instead of passing the gas price into the verifier, could the queue just check should_replace against the pool's worst_transaction directly? This is more in line with the check that happens in import anyway, and this way the score <-> gas price equivalence doesn't leak as much.

The current implementation also has the effect of potentially rejecting txs even if the queue is not full (but at 90% capacity), which I'm not sure is intended/desirable.

EDIT: I see using should_replace is not so easy because it takes VerifiedTransaction, not verifier::Transaction. Note that this change modifies behavior in the case that the worst transaction is a higher-gas-price, higher-nonce transaction by the same sender as the incoming one. The current behavior is to accept the new transaction because the nonce is lower, whereas after the change it would be rejected.

@tomusdrw
Copy link
Collaborator Author

@jimpo thanks for the review, I've updated the PR a bit:

  • Removed the threshold (it didn't have much sense)
  • Moved the decision making to the Scoring implementation.

Unfortunately we are not able to re-use should_replace, since the whole point is that it's just a heuristic to quickly reject some transactions, we want to do it without recovering the sender in verification, so there is no way to check if the nonce is lower/higher.

That said it's not really that much of an issue, the pool decisions are always local and are not part of the consensus itself. To have the most accurate image of the pending transactions one should run with big enough pool to store them all.

Copy link
Collaborator

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

lgtm, cargo test --all also completes without any errors locally

@5chdn 5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 29, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 29, 2018

Please rebase on latest master to fix builds.

@5chdn 5chdn mentioned this pull request Jun 30, 2018
12 tasks
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Jun 30, 2018
@5chdn 5chdn merged commit 1792725 into master Jun 30, 2018
@5chdn 5chdn deleted the td-min-eff-gas-price branch June 30, 2018 09:11
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)
andresilva pushed a commit that referenced this pull request Jul 7, 2018
* 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.
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:
  Attempt to graceful shutdown in case of panics (openethereum#8999)
  simplify kvdb error types (openethereum#8924)
  Add option for user to set max size limit for RPC requests (openethereum#9010)
  bump ntp to 0.5.0 (openethereum#9009)
  Removed duplicate dependency (openethereum#9021)
  Minimal effective gas price in the queue (openethereum#8934)
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