Skip to content

Commit

Permalink
Fix after review, drop logic around fnv32.
Browse files Browse the repository at this point in the history
  • Loading branch information
andreibancioiu committed Nov 14, 2024
1 parent fa41478 commit 68835d1
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 45 deletions.
4 changes: 2 additions & 2 deletions txcache/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ That is, for contract calls, the PPU is not equal to the gas price, but much low

Transaction **A** is considered **more valuable (for the Network)** than transaction **B** if **it has a higher PPU**.

If two transactions have the same PPU, they are ordered using an arbitrary, but deterministic rule: the transaction with the higher [fnv32(transactionHash)](https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function) "wins" the comparison.
If two transactions have the same PPU, they are ordered using an arbitrary, but deterministic rule: the transaction with the "lower" transaction hash "wins" the comparison.

Pseudo-code:

Expand All @@ -103,7 +103,7 @@ func isTransactionMoreValuableForNetwork(A, B):
return true
if A.ppu < B.ppu:
return false
return fnv32(A.hash) > fnv32(B.hash)
return A.hash < B.hash
```

### Paragraph 4
Expand Down
29 changes: 7 additions & 22 deletions txcache/selection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,13 @@ func TestTxCache_SelectTransactions_Dummy(t *testing.T) {
cache := newUnconstrainedCacheToTest()

cache.AddTx(createTx([]byte("hash-alice-4"), "alice", 4))
require.Equal(t, 3193030061, int(fnv32("hash-alice-4")))

cache.AddTx(createTx([]byte("hash-alice-3"), "alice", 3))
require.Equal(t, 3193030058, int(fnv32("hash-alice-3")))

cache.AddTx(createTx([]byte("hash-alice-2"), "alice", 2))
require.Equal(t, 3193030059, int(fnv32("hash-alice-2")))

cache.AddTx(createTx([]byte("hash-alice-1"), "alice", 1))
require.Equal(t, 3193030056, int(fnv32("hash-alice-1")))

cache.AddTx(createTx([]byte("hash-bob-7"), "bob", 7))
require.Equal(t, 187766579, int(fnv32("hash-bob-7")))

cache.AddTx(createTx([]byte("hash-bob-6"), "bob", 6))
require.Equal(t, 187766578, int(fnv32("hash-bob-6")))

cache.AddTx(createTx([]byte("hash-bob-5"), "bob", 5))
require.Equal(t, 187766577, int(fnv32("hash-bob-5")))

cache.AddTx(createTx([]byte("hash-carol-1"), "carol", 1))
require.Equal(t, 3082288595, int(fnv32("hash-carol-1")))

selected, accumulatedGas := cache.SelectTransactions(math.MaxUint64, math.MaxInt)
require.Len(t, selected, 8)
Expand All @@ -47,10 +32,10 @@ func TestTxCache_SelectTransactions_Dummy(t *testing.T) {
require.Equal(t, "hash-alice-2", string(selected[1].TxHash))
require.Equal(t, "hash-alice-3", string(selected[2].TxHash))
require.Equal(t, "hash-alice-4", string(selected[3].TxHash))
require.Equal(t, "hash-carol-1", string(selected[4].TxHash))
require.Equal(t, "hash-bob-5", string(selected[5].TxHash))
require.Equal(t, "hash-bob-6", string(selected[6].TxHash))
require.Equal(t, "hash-bob-7", string(selected[7].TxHash))
require.Equal(t, "hash-bob-5", string(selected[4].TxHash))
require.Equal(t, "hash-bob-6", string(selected[5].TxHash))
require.Equal(t, "hash-bob-7", string(selected[6].TxHash))
require.Equal(t, "hash-carol-1", string(selected[7].TxHash))
})

t.Run("alice > carol > bob", func(t *testing.T) {
Expand Down Expand Up @@ -89,9 +74,9 @@ func TestTxCache_SelectTransactionsWithBandwidth_Dummy(t *testing.T) {
require.Equal(t, 750000, int(accumulatedGas))

// Check order
require.Equal(t, "hash-carol-1", string(selected[0].TxHash))
require.Equal(t, "hash-bob-5", string(selected[1].TxHash))
require.Equal(t, "hash-bob-6", string(selected[2].TxHash))
require.Equal(t, "hash-bob-5", string(selected[0].TxHash))
require.Equal(t, "hash-bob-6", string(selected[1].TxHash))
require.Equal(t, "hash-carol-1", string(selected[2].TxHash))
require.Equal(t, "hash-alice-1", string(selected[3].TxHash))
require.Equal(t, "hash-bob-7", string(selected[4].TxHash))
})
Expand Down
18 changes: 3 additions & 15 deletions txcache/wrappedTransaction.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package txcache

import (
"bytes"
"sync/atomic"

"github.com/multiversx/mx-chain-core-go/data"
Expand All @@ -18,7 +19,6 @@ type WrappedTransaction struct {
Size int64

PricePerUnit atomic.Uint64
HashFnv32 atomic.Uint32
}

// precomputeFields computes (and caches) the (average) price per gas unit.
Expand All @@ -31,18 +31,6 @@ func (wrappedTx *WrappedTransaction) precomputeFields(txGasHandler TxGasHandler)
}

wrappedTx.PricePerUnit.Store(fee / gasLimit)
wrappedTx.HashFnv32.Store(fnv32(string(wrappedTx.TxHash)))
}

// fnv32 implements https://en.wikipedia.org/wiki/Fowler–Noll–Vo_hash_function for 32 bits
func fnv32(key string) uint32 {
hash := uint32(2166136261)
const prime32 = uint32(16777619)
for i := 0; i < len(key); i++ {
hash *= prime32
hash ^= uint32(key[i])
}
return hash
}

// Equality is out of scope (not possible in our case).
Expand All @@ -54,6 +42,6 @@ func (wrappedTx *WrappedTransaction) isTransactionMoreValuableForNetwork(otherTr
return ppu > ppuOther
}

// In the end, compare by hash number of transaction hash
return wrappedTx.HashFnv32.Load() > otherTransaction.HashFnv32.Load()
// In the end, compare by transaction hash
return bytes.Compare(wrappedTx.TxHash, otherTransaction.TxHash) < 0
}
6 changes: 0 additions & 6 deletions txcache/wrappedTransaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,13 @@ func TestWrappedTransaction_precomputeFields(t *testing.T) {
tx.precomputeFields(txGasHandler)

require.Equal(t, oneBillion, int(tx.PricePerUnit.Load()))
require.Equal(t, 84696446, int(tx.HashFnv32.Load()))
})

t.Run("move balance gas limit and execution gas limit (1)", func(t *testing.T) {
tx := createTx([]byte("b"), "b", 1).withDataLength(1).withGasLimit(51501).withGasPrice(oneBillion)
tx.precomputeFields(txGasHandler)

require.Equal(t, 999_980_777, int(tx.PricePerUnit.Load()))
require.Equal(t, 84696445, int(tx.HashFnv32.Load()))
})

t.Run("move balance gas limit and execution gas limit (2)", func(t *testing.T) {
Expand All @@ -32,9 +30,7 @@ func TestWrappedTransaction_precomputeFields(t *testing.T) {

actualFee := 51500*oneBillion + (oneMilion-51500)*oneBillion/100
require.Equal(t, 60_985_000_000_000, actualFee)

require.Equal(t, actualFee/oneMilion, int(tx.PricePerUnit.Load()))
require.Equal(t, 84696444, int(tx.HashFnv32.Load()))
})
}

Expand All @@ -54,11 +50,9 @@ func TestWrappedTransaction_isTransactionMoreValuableForNetwork(t *testing.T) {
t.Run("decide by transaction hash (set them up to have the same PPU)", func(t *testing.T) {
a := createTx([]byte("a-7"), "a", 7)
a.precomputeFields(txGasHandler)
require.Equal(t, 2191299170, int(a.HashFnv32.Load()))

b := createTx([]byte("b-7"), "b", 7)
b.precomputeFields(txGasHandler)
require.Equal(t, 1654268265, int(b.HashFnv32.Load()))

require.True(t, a.isTransactionMoreValuableForNetwork(b))
})
Expand Down

0 comments on commit 68835d1

Please sign in to comment.