diff --git a/contracts/currency/currency.go b/contracts/currency/currency.go index fb997b6e40..e6104418e5 100644 --- a/contracts/currency/currency.go +++ b/contracts/currency/currency.go @@ -153,6 +153,11 @@ func NewManager(vmRunner vm.EVMRunner) *CurrencyManager { return newManager(GetExchangeRate, vmRunner) } +// NewCacheOnlyManager creates a cache-full currency manager. TESTING PURPOSES ONLY +func NewCacheOnlyManager(currencyCache map[common.Address]*Currency) *CurrencyManager { + return &CurrencyManager{currencyCache: currencyCache} +} + func newManager(_getExchangeRate func(vm.EVMRunner, *common.Address) (*ExchangeRate, error), vmRunner vm.EVMRunner) *CurrencyManager { return &CurrencyManager{ vmRunner: vmRunner, diff --git a/core/tx_list.go b/core/tx_list.go index ffb6734c76..4b3f907a5c 100644 --- a/core/tx_list.go +++ b/core/tx_list.go @@ -696,18 +696,29 @@ func (l *txPricedList) Underpriced(tx *types.Transaction) bool { } func (l *txPricedList) underpricedForMulti(h *multiCurrencyPriceHeap, tx *types.Transaction) bool { - underpriced := l.underpricedFor(h.nativeCurrencyHeap, tx) + // wellpriced returns if, after pruning, tx is above minimum price + // compared to the top of the heap (minimum priced tx in heap) + wellpriced := func(heap *priceHeap) bool { + l.prune(heap) + return heap.Len() > 0 && !h.IsCheaper(tx, heap.list[0]) + } + + // If tx is wellpriced for at least one heap, then it is not underpriced + if wellpriced(h.nativeCurrencyHeap) { + return false + } for _, sh := range h.currencyHeaps { - if l.underpricedFor(sh, tx) { - underpriced = true + if wellpriced(sh) { + return false } } - return underpriced + // While this impl. is returning true when all heaps are empty (h.Len() == 0), that border + // case is managed on invokation of this fn. + return true } -// underpricedFor checks whether a transaction is cheaper than (or as cheap as) the -// lowest priced (remote) transaction in the given heap. -func (l *txPricedList) underpricedFor(h *priceHeap, tx *types.Transaction) bool { +// prune discards from the top of the heap txs that are no longer in the pool +func (l *txPricedList) prune(h *priceHeap) { // Discard stale price points if found at the heap start for len(h.list) > 0 { head := h.list[0] @@ -718,13 +729,6 @@ func (l *txPricedList) underpricedFor(h *priceHeap, tx *types.Transaction) bool } break } - // Check if the transaction is underpriced or not - if len(h.list) == 0 { - return false // There is no remote transaction at all. - } - // If the remote transaction is even cheaper than the - // cheapest one tracked locally, reject it. - return h.cmp(h.list[0], tx) >= 0 } // Discard finds a number of most underpriced transactions, removes them from the diff --git a/core/tx_multicurrency_priceheap.go b/core/tx_multicurrency_priceheap.go index 0eac32bafb..5a809179c7 100644 --- a/core/tx_multicurrency_priceheap.go +++ b/core/tx_multicurrency_priceheap.go @@ -10,9 +10,37 @@ import ( type CurrencyCmpFn func(*big.Int, *common.Address, *big.Int, *common.Address) int -// IsCheaper returns true if tx1 is cheaper than tx2 (GasPrice with currency comparison) -func (cc CurrencyCmpFn) IsCheaper(tx1, tx2 *types.Transaction) bool { - return cc(tx1.GasPrice(), tx1.FeeCurrency(), tx2.GasPrice(), tx2.FeeCurrency()) < 0 +func (cc CurrencyCmpFn) GasTipCapCmp(tx, other *types.Transaction) int { + return cc(tx.GasTipCap(), tx.FeeCurrency(), other.GasTipCap(), other.FeeCurrency()) +} + +// EffectiveGasTipCmp returns the same comparison result as Transaction.EffectiveGasTipCmp +// but taking into account the exchange rate comparison of the CurrencyCmpFn. +// Each baseFee is expressed in each tx's currency. +func (cc CurrencyCmpFn) EffectiveGasTipCmp(tx, other *types.Transaction, baseFeeA, baseFeeB *big.Int) int { + return cc(tx.EffectiveGasTipValue(baseFeeA), tx.FeeCurrency(), other.EffectiveGasTipValue(baseFeeB), other.FeeCurrency()) +} + +func (cc CurrencyCmpFn) GasFeeCapCmp(a, b *types.Transaction) int { + return cc(a.GasFeeCap(), a.FeeCurrency(), b.GasFeeCap(), b.FeeCurrency()) +} + +// Cmp returns the same comparison as the priceHeap comparison but taking into account +// the exchange rate comparison of the CurrencyCmpFn. +// Each baseFee is expressed in each tx's currency. +func (cc CurrencyCmpFn) Cmp(a, b *types.Transaction, baseFeeA, baseFeeB *big.Int) int { + if baseFeeA != nil && baseFeeB != nil { + // Compare effective tips if baseFee is specified + if c := cc.EffectiveGasTipCmp(a, b, baseFeeA, baseFeeB); c != 0 { + return c + } + } + // Compare fee caps if baseFee is not specified or effective tips are equal + if c := cc.GasFeeCapCmp(a, b); c != 0 { + return c + } + // Compare tips if effective tips and fee caps are equal + return cc.GasTipCapCmp(a, b) } // multiCurrencyPriceHeap is a heap.Interface implementation over transactions @@ -34,8 +62,10 @@ func newMultiCurrencyPriceHeap(currencyCmp CurrencyCmpFn, gpm GasPriceMinimums) // inner state - nativeCurrencyHeap: &priceHeap{}, - currencyHeaps: make(map[common.Address]*priceHeap), + nativeCurrencyHeap: &priceHeap{}, // Not initializing the basefee + // since it gets updated as soon as the node starts, and + // tx pool tests (upstream) assume baseFee == nil + currencyHeaps: make(map[common.Address]*priceHeap), } } @@ -79,11 +109,18 @@ func (h *multiCurrencyPriceHeap) cheapestTxs() []*types.Transaction { return txs } +// IsCheaper returs true iff tx1 effective gas price <= tx2's +func (h *multiCurrencyPriceHeap) IsCheaper(tx1, tx2 *types.Transaction) bool { + baseFee1 := h.getHeapFor(tx1).baseFee + baseFee2 := h.getHeapFor(tx2).baseFee + return h.currencyCmp.Cmp(tx1, tx2, baseFee1, baseFee2) <= 0 +} + func (h *multiCurrencyPriceHeap) cheapestTx() *types.Transaction { txs := h.cheapestTxs() var cheapestTx *types.Transaction for _, tx := range txs { - if cheapestTx == nil || h.currencyCmp.IsCheaper(tx, cheapestTx) { + if cheapestTx == nil || h.IsCheaper(tx, cheapestTx) { cheapestTx = tx } } diff --git a/core/tx_multicurrency_priceheap_test.go b/core/tx_multicurrency_priceheap_test.go index d2cca07ce3..4bdbab7dee 100644 --- a/core/tx_multicurrency_priceheap_test.go +++ b/core/tx_multicurrency_priceheap_test.go @@ -2,9 +2,11 @@ package core import ( "math/big" + "sync/atomic" "testing" "github.com/celo-org/celo-blockchain/common" + "github.com/celo-org/celo-blockchain/contracts/currency" "github.com/celo-org/celo-blockchain/core/types" "github.com/stretchr/testify/assert" ) @@ -99,11 +101,14 @@ func TestMultiPushPop(t *testing.T) { c1 := curr(1) c2 := curr(2) + // BaseFee per currency gpm := map[common.Address]*big.Int{ - *c1: big.NewInt(10), - *c2: big.NewInt(20), + *c1: big.NewInt(10), + *c2: big.NewInt(1), + common.ZeroAddress: big.NewInt(150), // celo currency base fee } var cmp CurrencyCmpFn = func(p1 *big.Int, cc1 *common.Address, p2 *big.Int, cc2 *common.Address) int { + // currency1 = x10, currency2 = x100, currency nil (or other) = x1 var val1 int = int(p1.Int64()) var val2 int = int(p2.Int64()) if common.AreEqualAddresses(cc1, c1) { @@ -121,57 +126,58 @@ func TestMultiPushPop(t *testing.T) { return val1 - val2 } m := newMultiCurrencyPriceHeap(cmp, gpm) - m.Push(txC(100, c1)) // 1000 - m.Push(txC(250, c1)) // 2500 - m.Push(txC(50, c1)) // 500 - m.Push(txC(200, c1)) // 2000 - m.Push(txC(75, c1)) // 750 - - m.Push(txC(9, c2)) // 900 - m.Push(txC(26, c2)) // 2600 - m.Push(txC(4, c2)) // 400 - m.Push(txC(21, c2)) // 2100 - m.Push(txC(7, c2)) // 700 - - m.Push(tx(1100)) // 1100 - m.Push(tx(2700)) // 2700 - m.Push(tx(560)) // 560 - m.Push(tx(2150)) // 2150 - m.Push(tx(750)) // 750 + m.UpdateFeesAndCurrencies(cmp, gpm) + m.Push(txC(100, c1)) // 100 * 10 - 10 * 10 = 900 (subtracting basefee x currencyValue) + m.Push(txC(250, c1)) // 2500 - 10 * 10 = 2400 + m.Push(txC(50, c1)) // 500 - 100 = 400 + m.Push(txC(200, c1)) // 2000 - 100 = 1900 + m.Push(txC(75, c1)) // 750 - 100 = 650 + + m.Push(txC(9, c2)) // 900 - 1 * 100 = 800 + m.Push(txC(26, c2)) // 2600 - 100 = 2500 + m.Push(txC(4, c2)) // 400 - 100 = 300 + m.Push(txC(21, c2)) // 2100 - 100 = 2000 + m.Push(txC(7, c2)) // 700 - 100 = 600 + + m.Push(tx(1100)) // 1100 - 150 = 950 + m.Push(tx(2700)) // 2700 - 150 = 2550 + m.Push(tx(560)) // 560 - 150 = 410 + m.Push(tx(2150)) // 2150 - 150 = 2000 + m.Push(tx(750)) // 750 - 150 = 600 assert.Equal(t, 15, m.Len()) tm := m.Pop() assert.Equal(t, 14, m.Len()) - // 400 + // 300 assert.Equal(t, big.NewInt(4), tm.GasPrice()) assert.Equal(t, c2, tm.FeeCurrency()) tm2 := m.Pop() assert.Equal(t, 13, m.Len()) - // 500 + // 400 assert.Equal(t, big.NewInt(50), tm2.GasPrice()) assert.Equal(t, c1, tm2.FeeCurrency()) tm3 := m.Pop() assert.Equal(t, 12, m.Len()) - // 560 + // 410 assert.Equal(t, big.NewInt(560), tm3.GasPrice()) assert.Nil(t, tm3.FeeCurrency()) // A few more re-pushes - m.Push(tx(585)) // 585 - m.Push(txC(3, c2)) // 300 + m.Push(tx(585)) // 435 + m.Push(txC(3, c2)) // 200 assert.Equal(t, 14, m.Len()) tm4 := m.Pop() assert.Equal(t, 13, m.Len()) - // 300 + // 200 assert.Equal(t, big.NewInt(3), tm4.GasPrice()) assert.Equal(t, c2, tm4.FeeCurrency()) tm5 := m.Pop() assert.Equal(t, 12, m.Len()) - // 585 + // 435 assert.Equal(t, big.NewInt(585), tm5.GasPrice()) assert.Nil(t, tm5.FeeCurrency()) } @@ -181,8 +187,9 @@ func TestMultiAddInit(t *testing.T) { c2 := curr(2) gpm := map[common.Address]*big.Int{ - *c1: big.NewInt(10), - *c2: big.NewInt(20), + *c1: big.NewInt(10), + *c2: big.NewInt(1), + common.ZeroAddress: big.NewInt(150), // celo currency base fee } var cmp CurrencyCmpFn = func(p1 *big.Int, cc1 *common.Address, p2 *big.Int, cc2 *common.Address) int { var val1 int = int(p1.Int64()) @@ -202,17 +209,18 @@ func TestMultiAddInit(t *testing.T) { return val1 - val2 } m := newMultiCurrencyPriceHeap(cmp, gpm) - m.Add(txC(100, c1)) // 1000 - m.Add(txC(250, c1)) // 2500 - m.Add(txC(50, c1)) // 500 - m.Add(txC(200, c1)) // 2000 - m.Add(txC(75, c1)) // 750 - - m.Add(txC(9, c2)) // 900 - m.Add(txC(26, c2)) // 2600 - m.Add(txC(4, c2)) // 400 - m.Add(txC(21, c2)) // 2100 - m.Add(txC(7, c2)) // 700 + m.UpdateFeesAndCurrencies(cmp, gpm) + m.Add(txC(100, c1)) // 100 * 10 - 10 * 10 = 900 (subtracting basefee x currencyValue) + m.Add(txC(250, c1)) // 2500 - 10 * 10 = 2400 + m.Add(txC(50, c1)) // 500 - 100 = 400 + m.Add(txC(200, c1)) // 2000 - 100 = 1900 + m.Add(txC(75, c1)) // 750 - 100 = 650 + + m.Add(txC(9, c2)) // 900 - 1 * 100 = 800 + m.Add(txC(26, c2)) // 2600 - 100 = 2500 + m.Add(txC(4, c2)) // 400 - 100 = 300 + m.Add(txC(21, c2)) // 2100 - 100 = 2000 + m.Add(txC(7, c2)) // 700 - 100 = 600 m.Add(tx(1100)) // 1100 m.Add(tx(2700)) // 2700 @@ -266,6 +274,48 @@ func TestMultiAddInit(t *testing.T) { assert.Equal(t, c2, tm5.FeeCurrency()) } +func TestCmp(t *testing.T) { + curr1 := common.HexToAddress("abc1") + ex1, _ := currency.NewExchangeRate(common.Big1, common.Big2) // 1 of curr1 is 2 celos + curr2 := common.HexToAddress("abc2") + ex2, _ := currency.NewExchangeRate(common.Big1, common.Big3) // 1 of curr2 is 3 celos + rates := make(map[common.Address]*currency.Currency) + rates[curr1] = currency.NewCurrency(curr1, *ex1) + rates[curr2] = currency.NewCurrency(curr2, *ex2) + var fn CurrencyCmpFn = func(p1 *big.Int, c1 *common.Address, p2 *big.Int, c2 *common.Address) int { + if c1 == nil || c2 == nil { + t.Fatal() + } + var c1Obj, c2Obj *currency.Currency + c1Obj = rates[*c1] + c2Obj = rates[*c2] + if c1Obj == nil || c2Obj == nil { + t.Fatal() + } + return c1Obj.CmpToCurrency(p1, p2, c2Obj) + } + + // Same currency, No basefees + assert.Equal(t, 0, fn.Cmp(txC(1, &curr1), txC(1, &curr1), nil, nil)) + assert.Equal(t, 1, fn.Cmp(txC(3, &curr2), txC(2, &curr2), nil, nil)) + assert.Equal(t, -1, fn.Cmp(txC(3, &curr2), txC(4, &curr2), nil, nil)) + + // Diff currencies, No basefees + assert.Equal(t, -1, fn.Cmp(txC(1, &curr1), txC(1, &curr2), nil, nil)) + assert.Equal(t, 1, fn.Cmp(txC(1, &curr2), txC(1, &curr1), nil, nil)) + assert.Equal(t, 0, fn.Cmp(txC(3, &curr1), txC(2, &curr2), nil, nil)) + + // Same currency, with basefee + assert.Equal(t, 0, fn.Cmp(txC(10, &curr1), txC(10, &curr1), common.Big2, common.Big2)) + assert.Equal(t, 1, fn.Cmp(txC(9, &curr1), txC(8, &curr1), common.Big2, common.Big2)) + assert.Equal(t, -1, fn.Cmp(txC(5, &curr1), txC(6, &curr1), common.Big3, common.Big3)) + + // Diff currencies, with basefee + assert.Equal(t, 0, fn.Cmp(txC(6, &curr1), txC(4, &curr2), common.Big3, common.Big2)) + assert.Equal(t, 1, fn.Cmp(txC(6, &curr1), txC(4, &curr2), common.Big2, common.Big2)) + assert.Equal(t, -1, fn.Cmp(txC(6, &curr1), txC(4, &curr2), common.Big3, common.Big1)) +} + func TestClear(t *testing.T) { c := curr(1) gpm := map[common.Address]*big.Int{ @@ -286,6 +336,8 @@ func TestClear(t *testing.T) { } func TestIsCheaper_FwdFields(t *testing.T) { + // Tests that the CurrencyCmpFn receives the + // proper fields for comparison in a gas fee cap cmp curr1 := common.BigToAddress(big.NewInt(123)) price1 := big.NewInt(100) curr2 := common.BigToAddress(big.NewInt(123)) @@ -305,10 +357,12 @@ func TestIsCheaper_FwdFields(t *testing.T) { assert.Equal(t, curr2, *c2) return -1 } - assert.True(t, cmp.IsCheaper(tx1, tx2)) + assert.True(t, cmp.Cmp(tx1, tx2, nil, nil) == -1) } func TestIsCheaper(t *testing.T) { + // Tests that the result of the currency comparison function is + // properly being returned in the tx comparison function tx1 := types.NewTx(&types.LegacyTx{}) tx2 := types.NewTx(&types.LegacyTx{}) var cheaper CurrencyCmpFn = func(p1 *big.Int, c1 *common.Address, p2 *big.Int, c2 *common.Address) int { @@ -320,7 +374,228 @@ func TestIsCheaper(t *testing.T) { var notCheaper CurrencyCmpFn = func(p1 *big.Int, c1 *common.Address, p2 *big.Int, c2 *common.Address) int { return 1 } - assert.True(t, cheaper.IsCheaper(tx1, tx2)) - assert.False(t, equal.IsCheaper(tx1, tx2)) - assert.False(t, notCheaper.IsCheaper(tx1, tx2)) + assert.True(t, cheaper.Cmp(tx1, tx2, nil, nil) <= 0) + assert.True(t, equal.Cmp(tx1, tx2, nil, nil) <= 0) + assert.False(t, notCheaper.Cmp(tx1, tx2, nil, nil) <= 0) +} + +// TestMulticurrencyUnderpriced tests that the underpriced method from pricedList functions +// properly when handling many different currencies. +func TestMulticurrencyUnderpriced(t *testing.T) { + curr1 := common.HexToAddress("aaaa1") + rate1, _ := currency.NewExchangeRate(common.Big1, common.Big1) + curr2 := common.HexToAddress("aaaa2") + rate2, _ := currency.NewExchangeRate(common.Big1, common.Big2) + curr3 := common.HexToAddress("aaaa3") + rate3, _ := currency.NewExchangeRate(common.Big1, common.Big3) + currCache := map[common.Address]*currency.Currency{ + curr1: currency.NewCurrency(curr1, *rate1), + curr2: currency.NewCurrency(curr2, *rate2), + curr3: currency.NewCurrency(curr3, *rate3), + } + currencyManager := currency.NewCacheOnlyManager(currCache) + txPoolCtx := txPoolContext{ + &SysContractCallCtx{ + whitelistedCurrencies: map[common.Address]struct{}{curr1: {}, curr2: {}, curr3: {}}, + gasPriceMinimums: map[common.Address]*big.Int{curr1: nil, curr2: nil, curr3: nil}, + }, + currencyManager, + nil, + } + ctxVal := atomic.Value{} + ctxVal.Store(txPoolCtx) + + type addTx struct { + tx *types.Transaction + local bool + } + type testCase struct { + desc string + newTx *types.Transaction + expected bool + } + tests := []struct { + name string + existingTxs []addTx + cases []testCase + }{ + { + name: "Empty tx list (Never underpriced)", + existingTxs: []addTx{}, + cases: []testCase{ + { + desc: "Normal tx", + newTx: tx(5), + expected: false, + }, + { + desc: "Celo tx", + newTx: txC(5, &curr2), + expected: false, + }, + }, + }, + { + name: "Single tx in list", + existingTxs: []addTx{ + {txC(5, &curr2), false}, + }, + cases: []testCase{ + { + desc: "Underpriced normal transaction", + newTx: tx(10), + expected: true, + }, + { + desc: "Accepted normal transaction", + newTx: tx(11), + expected: false, + }, + { + desc: "Underpriced celo transaction", + newTx: txC(5, &curr2), + expected: true, + }, + { + desc: "Accepted celo transaction", + newTx: txC(6, &curr2), + expected: false, + }, + { + desc: "Underpriced celo transaction with different currency", + newTx: txC(3, &curr3), + expected: true, + }, + { + desc: "Accepted celo transaction with different currency", + newTx: txC(4, &curr3), + expected: false, + }, + }, + }, + { + name: "Mixed existing transactions", + existingTxs: []addTx{ + {tx(3), false}, + {txC(5, &curr1), false}, + }, + cases: []testCase{ + { + desc: "Accepted native", + newTx: tx(4), + expected: false, + }, + { + desc: "Underpriced native", + newTx: tx(3), + expected: true, + }, + { + desc: "Accepted cheapest curr1", + newTx: txC(5, &curr1), + expected: false, + }, + { + desc: "Underpriced curr1", + newTx: txC(3, &curr1), + expected: true, + }, + { + desc: "Accepted cheapest curr3", + newTx: txC(2, &curr3), + expected: false, + }, + { + desc: "Underpriced curr3", + newTx: txC(1, &curr3), + expected: true, + }, + }, + }, + { + name: "Multiple existing transactions", + existingTxs: []addTx{ + {tx(3), false}, + {tx(5), false}, + }, + cases: []testCase{ + { + desc: "Accepted native", + newTx: tx(4), + expected: false, + }, + { + desc: "Underpriced native", + newTx: tx(3), + expected: true, + }, + { + desc: "Accepted cheapest curr1", + newTx: txC(5, &curr1), + expected: false, + }, + { + desc: "Underpriced curr1", + newTx: txC(3, &curr1), + expected: true, + }, + { + desc: "Accepted cheapest curr3", + newTx: txC(2, &curr3), + expected: false, + }, + { + desc: "Underpriced curr3", + newTx: txC(1, &curr3), + expected: true, + }, + }, + }, + { + name: "Many txs from many currencies", + existingTxs: []addTx{ + {txC(3, nil), false}, // Cheapest native currency: 3 + {txC(6, nil), false}, + {txC(5, &curr1), false}, // Cheapest curr1: 5 + {txC(2, &curr2), false}, // Cheapest curr2: 4 + {txC(2, &curr2), false}, + {txC(1, &curr3), false}, // Cheapest curr3: 3 + }, + cases: []testCase{ + { + desc: "Accepted cheapest curr1", + newTx: txC(4, &curr1), + expected: false, + }, + { + desc: "Underpriced native", + newTx: tx(3), + expected: true, + }, + { + desc: "Underpriced curr2", + newTx: txC(1, &curr2), + expected: true, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + all := newTxLookup() + + for _, tx := range tt.existingTxs { + all.Add(tx.tx, tx.local) + } + + pricedList := newTxPricedList(all, &ctxVal, 1024) + pricedList.Reheap() + + for _, tc := range tt.cases { + if got := pricedList.Underpriced(tc.newTx); got != tc.expected { + t.Errorf("%s: txPricedList.Underpriced() = %v, want %v", tc.desc, got, tc.expected) + } + } + }) + } }