Skip to content
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.

Sign up for GitHub

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

eth/gasprice: set default percentile to 60%, count blocks instead of transactions #15828

Merged
merged 9 commits into from
Jan 10, 2018
+34 −19
Merged

eth/gasprice: set default percentile to 60%, count blocks instead of transactions #15828

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
83fe302
eth: Set default GPO percentile to 35%, and count blocks instead of t…
Arachnid Jan 7, 2018
3d1d61e
eth/gasprice: Don't include transactions made by the miner themselves
Arachnid Jan 7, 2018
8c2e33f
eth: Use 60% for GPO, not 35%
Arachnid Jan 7, 2018
85cc916
eth: Reduce the GPO lookback to 20 blocks from 100, for speed
Arachnid Jan 7, 2018
ef4d9fb
eth/gasprice: Add mistakenly deleted 'continue'
Arachnid Jan 7, 2018
67d1681
eth/gasprice: gofmt
Arachnid Jan 8, 2018
38e8ac0
eth/gasprice: Sort transactions by price first, to minimise use of ec…
Arachnid Jan 9, 2018
cd85458
eth/gasprice: gofmt
Arachnid Jan 9, 2018
26f303f
eth/gasprice: Make a copy of transaction pointers before sorting
Arachnid Jan 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions eth/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ var DefaultConfig = Config{

TxPool: core.DefaultTxPoolConfig,
GPO: gasprice.Config{
Blocks: 10,
Percentile: 50,
Blocks: 20,
Percentile: 60,
},
}

Expand Down
37 changes: 22 additions & 15 deletions eth/gasprice/gasprice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"sync"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/internal/ethapi"
"github.com/ethereum/go-ethereum/params"
"github.com/ethereum/go-ethereum/rpc"
Expand Down Expand Up @@ -101,9 +102,9 @@ func (gpo *Oracle) SuggestPrice(ctx context.Context) (*big.Int, error) {
ch := make(chan getBlockPricesResult, gpo.checkBlocks)
sent := 0
exp := 0
var txPrices []*big.Int
var blockPrices []*big.Int
for sent < gpo.checkBlocks && blockNum > 0 {
go gpo.getBlockPrices(ctx, blockNum, ch)
go gpo.getBlockPrices(ctx, types.MakeSigner(gpo.backend.ChainConfig(), big.NewInt(int64(blockNum))), blockNum, ch)
sent++
exp++
blockNum--
Expand All @@ -115,25 +116,25 @@ func (gpo *Oracle) SuggestPrice(ctx context.Context) (*big.Int, error) {
return lastPrice, res.err
}
exp--
if len(res.prices) > 0 {
txPrices = append(txPrices, res.prices...)
if res.price != nil {
blockPrices = append(blockPrices, res.price)
continue
}
if maxEmpty > 0 {
maxEmpty--
continue
}
if blockNum > 0 && sent < gpo.maxBlocks {
go gpo.getBlockPrices(ctx, blockNum, ch)
go gpo.getBlockPrices(ctx, types.MakeSigner(gpo.backend.ChainConfig(), big.NewInt(int64(blockNum))), blockNum, ch)
sent++
exp++
blockNum--
}
}
price := lastPrice
if len(txPrices) > 0 {
sort.Sort(bigIntArray(txPrices))
price = txPrices[(len(txPrices)-1)*gpo.percentile/100]
if len(blockPrices) > 0 {
sort.Sort(bigIntArray(blockPrices))
price = blockPrices[(len(blockPrices)-1)*gpo.percentile/100]
}
if price.Cmp(maxPrice) > 0 {
price = new(big.Int).Set(maxPrice)
Expand All @@ -147,24 +148,30 @@ func (gpo *Oracle) SuggestPrice(ctx context.Context) (*big.Int, error) {
}

type getBlockPricesResult struct {
prices []*big.Int
err error
price *big.Int
err error
}

// getLowestPrice calculates the lowest transaction gas price in a given block
// and sends it to the result channel. If the block is empty, price is nil.
func (gpo *Oracle) getBlockPrices(ctx context.Context, blockNum uint64, ch chan getBlockPricesResult) {
func (gpo *Oracle) getBlockPrices(ctx context.Context, signer types.Signer, blockNum uint64, ch chan getBlockPricesResult) {
block, err := gpo.backend.BlockByNumber(ctx, rpc.BlockNumber(blockNum))
if block == nil {
ch <- getBlockPricesResult{nil, err}
return
}
txs := block.Transactions()
prices := make([]*big.Int, len(txs))
for i, tx := range txs {
prices[i] = tx.GasPrice()
var minPrice *big.Int
for _, tx := range txs {
sender, err := types.Sender(signer, tx)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit reluctant about this change here as is. This essentially does an ecrecover. So for a single gas estimation, on mainnet we would iterate over 20 blocks x 200-300 tx, that's 4-6K ecrecovers. That would imho be a very expensive operation to do, borderline unusable on a mobile phone (maybe fully).

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could cache the senders of transactions so we don't need to ecrecover all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In here, or in general? We could modify this to not reprocess existing blocks, if they've been seen already by a previous call to the oracle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm very mistaken, tx sender caching helps here. The GPO loads recent blocks, so they'll be in the BlockChain LRU. The txs in those blocks have cached sender information.

if err != nil || sender == block.Coinbase() {
continue
}
if minPrice == nil || tx.GasPrice().Cmp(minPrice) < 0 {
minPrice = tx.GasPrice()
Copy link
Member

Choose a reason for hiding this comment

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

tx.GasPrice creates a copy internally. Calling it twice here will alloc twice. It would be cleaner to do:

if curPrice := tx.GasPrice(); minPrice == nil || curPrice.Cmp(minPrice) < 0 {
 	minPrice = curPrice
}

}
}
ch <- getBlockPricesResult{prices, nil}
ch <- getBlockPricesResult{minPrice, nil}
}

type bigIntArray []*big.Int
Expand Down