New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd, core, eth, miner: remove txpool gas price limits #14442

Merged
merged 3 commits into from May 16, 2017

Conversation

Projects
None yet
4 participants
@karalabe
Member

karalabe commented May 8, 2017

Historically our transaction pool enforced all transactions to meet a certain pricing threshold to accept and forward. This however is limiting the network from propagating low price transactions, even if there is some miner out there willing to process them. This PR attempts to fix this.

The motto of the PR (i.e. base design rationale): We don't care if the pool contains a ton of non-executable-because-insanely-cheap transactions, since we'll have a hard upper cap and the cheap transactions get kicked out the moment there's something more expensive arriving.

Side note: the PR reduces the default gas price from 20 Shannon to 18 Shannon. This is actually a noop since our miner code had a weird snippet that set its accepted gas limit to 90% of what thee user specified. :| I removed that.

cmd, core, eth, miner: remove txpool gas price limits

First up, our previous code was messy when it came to gas price handling and tracking local transactions as both were done both in the miner as well as the tx pool. However it makes absolutely no sense to track these in two places. If the pool already enforces the correct prices, the miner shouldn't need to care at all, just mint whatever it's given. As such, this commit drops all notions of gas price and local transactions from the miner, and whenever miner.setGasPrice is called, it actually sets it on the tx pool directly. This has the nice side effect that it also gets rid of a core.GasPriceChanged event which was propagated from the miner to the pool via the event bus.

The crux of the commit is the implementation of txPricedMap. It is a data structure that maintains the set of all transactions, grouped into price-point buckets (hash map), also featuring a heap based index to quickly find the cheapest price points. This data structure supports a few handy operations such as adding a new transaction, removing an existing one, discarding all transaction below a price threshold, discarding a number of cheapest transactions and checking if a tx is under priced or not. All these operations have special handling for local transactions, which are not subject to the pricing rules.

With this data structure given, we can add it as a "virtual" layer on top of the existing transaction pool. Every time a transaction is added or removed from the pool via any existing method, it's added to this price pool too without caring about its price at all. However if the transaction pool saturates (count >= maxPending + maxQueued), any new transaction being added will first ensure it's not under priced (more expensive than the cheapest). If so, the price pool can discard the cheapest N transactions to make room for the new one.

If the user changes the minimum gas price via miner.setGasPrice (implicitly done when running miner.Start), the pool gains a new limit and enforces it immediately via capping the price pool to the new threshold, discarding any transactions priced below it.

core: enforce min 10% price bump for replacement txs

Since the first commit enables users to potentially add transactions that are way below any price point accepted by miners, those transactions will get stuck in the network until they are flushed out by more expensive ones.

The catch here is that a malicious user might add a transaction at 1 Wei price, replace it with 2 Wei, then 3 Wei and so on. Each subsequent transaction is more expensive, so is accepted, but would wreck havoc in the networking layer with transactions rotating like crazy. This commit prevents that by enforcing a minimum price bump of 10% for any transaction that wishes to replace an already existing one in the pool.

It also features an extra optimization compared to the old code, whereby replacement transactions are injected directly into the executable list (if it has a good nonce), instead of adding it into the non-executable queue and then promoting it later. This should help prevent replacement transactions from wasting computing resources by placing them into the queue and afterwards moving the to pending.

core: replace txpool priced map with a simple heap

The worst case scenario for the priced map data structure is if all transactions in the dataset have different price points. However with an active fee market, this is somewhat to be expected. As such, per the suggestion of @Arachnid, this commit dumps the map out of the data structure, and instead of a price-point heap, it uses a full transaction heap sorted by gas price. This should make the code a tad simpler to reason about while maintaining the same (expected case) runtime/memory complexity.

Show outdated Hide outdated core/tx_list.go
Show outdated Hide outdated core/tx_list.go

@karalabe karalabe added this to the 1.6.2 milestone May 9, 2017

@karalabe karalabe requested review from fjl and Arachnid May 9, 2017

if old != nil && old.GasPrice().Cmp(tx.GasPrice()) >= 0 {
return false, nil
if old != nil {
threshold := new(big.Int).Div(new(big.Int).Mul(old.GasPrice(), big.NewInt(100+minPriceBumpPercent)), big.NewInt(100))

This comment has been minimized.

@holiman

holiman May 9, 2017

Contributor

old gasPrice==0 => threshold = 0.
old gasPrice==3 => threshold= 3

(with minbumppercent at 25).

Should add some handling of the lower extremes.

@holiman

holiman May 9, 2017

Contributor

old gasPrice==0 => threshold = 0.
old gasPrice==3 => threshold= 3

(with minbumppercent at 25).

Should add some handling of the lower extremes.

This comment has been minimized.

@karalabe

karalabe May 9, 2017

Member

Actually, if the price == threshold, the new transaction is discarded.

if threshold.Cmp(tx.GasPrice()) >= 0 {
	return false, nil
}
@karalabe

karalabe May 9, 2017

Member

Actually, if the price == threshold, the new transaction is discarded.

if threshold.Cmp(tx.GasPrice()) >= 0 {
	return false, nil
}

This comment has been minimized.

This comment has been minimized.

@holiman

holiman May 9, 2017

Contributor

Ah, sorry, well done!

@holiman

holiman May 9, 2017

Contributor

Ah, sorry, well done!

return false, nil
if old != nil {
threshold := new(big.Int).Div(new(big.Int).Mul(old.GasPrice(), big.NewInt(100+minPriceBumpPercent)), big.NewInt(100))
if threshold.Cmp(tx.GasPrice()) >= 0 {

This comment has been minimized.

@Arachnid

Arachnid May 9, 2017

Contributor

Doesn't this return if the value is greater than the threshold?

@Arachnid

Arachnid May 9, 2017

Contributor

Doesn't this return if the value is greater than the threshold?

This comment has been minimized.

@karalabe

karalabe May 9, 2017

Member

The comparison sign is always in the same "direction" that the operations are being compared.

threshold.Cmp(tx.GasPrice()) >= 0 === threshold >= tx.GasPrice()

@karalabe

karalabe May 9, 2017

Member

The comparison sign is always in the same "direction" that the operations are being compared.

threshold.Cmp(tx.GasPrice()) >= 0 === threshold >= tx.GasPrice()

This comment has been minimized.

@Arachnid

Arachnid May 9, 2017

Contributor

That's only useful if you know the operator in question! I'm used to cmp(x,y) being equivalent to (x-y). :/

@Arachnid

Arachnid May 9, 2017

Contributor

That's only useful if you know the operator in question! I'm used to cmp(x,y) being equivalent to (x-y). :/

This comment has been minimized.

@karalabe

karalabe May 9, 2017

Member

And that's exactly the same thing

@karalabe

karalabe May 9, 2017

Member

And that's exactly the same thing

Show outdated Hide outdated core/tx_list.go

karalabe added some commits May 8, 2017

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe
Member

karalabe commented May 9, 2017

@Arachnid PTAL

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 9, 2017

Contributor

@karalabe What changed? I don't see any new commits.

Contributor

Arachnid commented May 9, 2017

@karalabe What changed? I don't see any new commits.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 9, 2017

Member
Member

karalabe commented May 9, 2017

@fjl

fjl approved these changes May 16, 2017

@fjl fjl merged commit a2f23ca into ethereum:master May 16, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment