From 8348e116d6f964054ed5bcdbf0860b4812eed020 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrei=20B=C4=83ncioiu?= Date: Mon, 4 Nov 2024 17:02:03 +0200 Subject: [PATCH] Refactor, fix logging. --- txcache/diagnosis.go | 67 ++++++++++++++++++++++++++----- txcache/printing.go | 53 ------------------------ txcache/txCache.go | 4 +- txcache/txCache_test.go | 2 +- txcache/txListBySenderMap.go | 16 ++++++-- txcache/txListBySenderMap_test.go | 22 +++++----- txcache/txListForSender.go | 1 - 7 files changed, 83 insertions(+), 82 deletions(-) delete mode 100644 txcache/printing.go diff --git a/txcache/diagnosis.go b/txcache/diagnosis.go index e0d60ecc..0b790923 100644 --- a/txcache/diagnosis.go +++ b/txcache/diagnosis.go @@ -1,10 +1,26 @@ package txcache import ( + "encoding/hex" + "encoding/json" + "fmt" + "strings" + "github.com/multiversx/mx-chain-core-go/core" logger "github.com/multiversx/mx-chain-logger-go" ) +type printedTransaction struct { + Hash string `json:"hash"` + Nonce uint64 `json:"nonce"` + PPU float64 `json:"ppu"` + GasPrice uint64 `json:"gasPrice"` + GasLimit uint64 `json:"gasLimit"` + Sender string `json:"sender"` + Receiver string `json:"receiver"` + DataLength int `json:"dataLength"` +} + // Diagnose checks the state of the cache for inconsistencies and displays a summary, senders and transactions. func (cache *TxCache) Diagnose(_ bool) { cache.diagnoseCounters() @@ -48,14 +64,47 @@ func (cache *TxCache) diagnoseTransactions() { } transactions := cache.getAllTransactions() - if len(transactions) == 0 { return } numToDisplay := core.MinInt(diagnosisMaxTransactionsToDisplay, len(transactions)) logDiagnoseTransactions.Trace("diagnoseTransactions", "numTransactions", len(transactions), "numToDisplay", numToDisplay) - logDiagnoseTransactions.Trace(marshalTransactionsToNewlineDelimitedJson(transactions[:numToDisplay])) + logDiagnoseTransactions.Trace(marshalTransactionsToNewlineDelimitedJson(transactions[:numToDisplay], "diagnoseTransactions")) +} + +// marshalTransactionsToNewlineDelimitedJson converts a list of transactions to a newline-delimited JSON string. +// Note: each line is indexed, to improve readability. The index is easily removable for if separate analysis is needed. +func marshalTransactionsToNewlineDelimitedJson(transactions []*WrappedTransaction, linePrefix string) string { + builder := strings.Builder{} + builder.WriteString("\n") + + for i, wrappedTx := range transactions { + printedTx := convertWrappedTransactionToPrintedTransaction(wrappedTx) + printedTxJson, _ := json.Marshal(printedTx) + + builder.WriteString(fmt.Sprintf("%s#%d: ", linePrefix, i)) + builder.WriteString(string(printedTxJson)) + builder.WriteString("\n") + } + + builder.WriteString("\n") + return builder.String() +} + +func convertWrappedTransactionToPrintedTransaction(wrappedTx *WrappedTransaction) *printedTransaction { + transaction := wrappedTx.Tx + + return &printedTransaction{ + Hash: hex.EncodeToString(wrappedTx.TxHash), + Nonce: transaction.GetNonce(), + Receiver: hex.EncodeToString(transaction.GetRcvAddr()), + Sender: hex.EncodeToString(transaction.GetSndAddr()), + GasPrice: transaction.GetGasPrice(), + GasLimit: transaction.GetGasLimit(), + DataLength: len(transaction.GetData()), + PPU: wrappedTx.PricePerUnit, + } } func (cache *TxCache) diagnoseSelection() { @@ -63,22 +112,18 @@ func (cache *TxCache) diagnoseSelection() { return } - transactions := cache.doSelectTransactions( - logDiagnoseSelection, - diagnosisSelectionGasRequested, - ) - - displaySelectionOutcome(logDiagnoseSelection, transactions) + transactions := cache.doSelectTransactions(diagnosisSelectionGasRequested) + displaySelectionOutcome(logDiagnoseSelection, "diagnoseSelection", transactions) } -func displaySelectionOutcome(contextualLogger logger.Logger, selection []*WrappedTransaction) { +func displaySelectionOutcome(contextualLogger logger.Logger, linePrefix string, transactions []*WrappedTransaction) { if contextualLogger.GetLevel() > logger.LogTrace { return } - if len(selection) > 0 { + if len(transactions) > 0 { contextualLogger.Trace("displaySelectionOutcome - transactions (as newline-separated JSON):") - contextualLogger.Trace(marshalTransactionsToNewlineDelimitedJson(selection)) + contextualLogger.Trace(marshalTransactionsToNewlineDelimitedJson(transactions, linePrefix)) } else { contextualLogger.Trace("displaySelectionOutcome - transactions: none") } diff --git a/txcache/printing.go b/txcache/printing.go deleted file mode 100644 index faa2b1bb..00000000 --- a/txcache/printing.go +++ /dev/null @@ -1,53 +0,0 @@ -package txcache - -import ( - "encoding/hex" - "encoding/json" - "fmt" - "strings" -) - -type printedTransaction struct { - Hash string `json:"hash"` - Nonce uint64 `json:"nonce"` - PPU float64 `json:"ppu"` - GasPrice uint64 `json:"gasPrice"` - GasLimit uint64 `json:"gasLimit"` - Sender string `json:"sender"` - Receiver string `json:"receiver"` - DataLength int `json:"dataLength"` -} - -// marshalTransactionsToNewlineDelimitedJson converts a list of transactions to a newline-delimited JSON string. -// Note: each line is indexed, to improve readability. The index is easily removable for if separate analysis is needed. -func marshalTransactionsToNewlineDelimitedJson(transactions []*WrappedTransaction) string { - builder := strings.Builder{} - builder.WriteString("\n") - - for i, wrappedTx := range transactions { - printedTx := convertWrappedTransactionToPrintedTransaction(wrappedTx) - printedTxJson, _ := json.Marshal(printedTx) - - builder.WriteString(fmt.Sprintf("#%d: ", i)) - builder.WriteString(string(printedTxJson)) - builder.WriteString("\n") - } - - builder.WriteString("\n") - return builder.String() -} - -func convertWrappedTransactionToPrintedTransaction(wrappedTx *WrappedTransaction) *printedTransaction { - transaction := wrappedTx.Tx - - return &printedTransaction{ - Hash: hex.EncodeToString(wrappedTx.TxHash), - Nonce: transaction.GetNonce(), - Receiver: hex.EncodeToString(transaction.GetRcvAddr()), - Sender: hex.EncodeToString(transaction.GetSndAddr()), - GasPrice: transaction.GetGasPrice(), - GasLimit: transaction.GetGasLimit(), - DataLength: len(transaction.GetData()), - PPU: wrappedTx.PricePerUnit, - } -} diff --git a/txcache/txCache.go b/txcache/txCache.go index 19b96505..72855b86 100644 --- a/txcache/txCache.go +++ b/txcache/txCache.go @@ -66,7 +66,7 @@ func (cache *TxCache) AddTx(tx *WrappedTransaction) (ok bool, added bool) { cache.mutTxOperation.Lock() addedInByHash := cache.txByHash.addTx(tx) - addedInBySender, evicted := cache.txListBySender.addTx(tx) + addedInBySender, evicted := cache.txListBySender.addTxReturnEvicted(tx) cache.mutTxOperation.Unlock() if addedInByHash != addedInBySender { // This can happen when two go-routines concur to add the same transaction: @@ -117,7 +117,7 @@ func (cache *TxCache) SelectTransactions(gasRequested uint64) []*WrappedTransact ) go cache.diagnoseCounters() - go displaySelectionOutcome(logSelect, transactions) + go displaySelectionOutcome(logSelect, "selection", transactions) return transactions } diff --git a/txcache/txCache_test.go b/txcache/txCache_test.go index 132535cd..5b854d1e 100644 --- a/txcache/txCache_test.go +++ b/txcache/txCache_test.go @@ -619,7 +619,7 @@ func TestTxCache_TransactionIsAdded_EvenWhenInternalMapsAreInconsistent(t *testi cache.Clear() // Setup inconsistency: transaction already exists in map by sender, but not in map by hash - cache.txListBySender.addTx(createTx([]byte("alice-x"), "alice", 42)) + cache.txListBySender.addTxReturnEvicted(createTx([]byte("alice-x"), "alice", 42)) require.False(t, cache.Has([]byte("alice-x"))) ok, added = cache.AddTx(createTx([]byte("alice-x"), "alice", 42)) diff --git a/txcache/txListBySenderMap.go b/txcache/txListBySenderMap.go index d4698b7b..55c8bf62 100644 --- a/txcache/txListBySenderMap.go +++ b/txcache/txListBySenderMap.go @@ -31,12 +31,22 @@ func newTxListBySenderMap( } } -// addTx adds a transaction in the map, in the corresponding list (selected by its sender) -func (txMap *txListBySenderMap) addTx(tx *WrappedTransaction) (bool, [][]byte) { +// addTxReturnEvicted adds a transaction in the map, in the corresponding list (selected by its sender). +// This function returns a boolean indicating whether the transaction was added, and a slice of evicted transaction hashes (upon applying sender-level constraints). +func (txMap *txListBySenderMap) addTxReturnEvicted(tx *WrappedTransaction) (bool, [][]byte) { sender := string(tx.Tx.GetSndAddr()) listForSender := txMap.getOrAddListForSender(sender) tx.computePricePerGasUnit(txMap.txGasHandler) - return listForSender.AddTx(tx) + + added, evictedHashes := listForSender.AddTx(tx) + + if listForSender.IsEmpty() { + // Generally speaking, a sender cannot become empty after upon applying sender-level constraints. + // However: + txMap.removeSender(sender) + } + + return added, evictedHashes } // getOrAddListForSender gets or lazily creates a list (using double-checked locking pattern) diff --git a/txcache/txListBySenderMap_test.go b/txcache/txListBySenderMap_test.go index 325fae0f..cb937e61 100644 --- a/txcache/txListBySenderMap_test.go +++ b/txcache/txListBySenderMap_test.go @@ -12,9 +12,9 @@ import ( func TestSendersMap_AddTx_IncrementsCounter(t *testing.T) { myMap := newSendersMapToTest() - myMap.addTx(createTx([]byte("a"), "alice", 1)) - myMap.addTx(createTx([]byte("aa"), "alice", 2)) - myMap.addTx(createTx([]byte("b"), "bob", 1)) + myMap.addTxReturnEvicted(createTx([]byte("a"), "alice", 1)) + myMap.addTxReturnEvicted(createTx([]byte("aa"), "alice", 2)) + myMap.addTxReturnEvicted(createTx([]byte("b"), "bob", 1)) // There are 2 senders require.Equal(t, int64(2), myMap.counter.Get()) @@ -27,9 +27,9 @@ func TestSendersMap_RemoveTx_AlsoRemovesSenderWhenNoTransactionLeft(t *testing.T txAlice2 := createTx([]byte("a2"), "alice", 2) txBob := createTx([]byte("b"), "bob", 1) - myMap.addTx(txAlice1) - myMap.addTx(txAlice2) - myMap.addTx(txBob) + myMap.addTxReturnEvicted(txAlice1) + myMap.addTxReturnEvicted(txAlice2) + myMap.addTxReturnEvicted(txBob) require.Equal(t, int64(2), myMap.counter.Get()) require.Equal(t, uint64(2), myMap.testGetListForSender("alice").countTx()) require.Equal(t, uint64(1), myMap.testGetListForSender("bob").countTx()) @@ -51,7 +51,7 @@ func TestSendersMap_RemoveTx_AlsoRemovesSenderWhenNoTransactionLeft(t *testing.T func TestSendersMap_RemoveSender(t *testing.T) { myMap := newSendersMapToTest() - myMap.addTx(createTx([]byte("a"), "alice", 1)) + myMap.addTxReturnEvicted(createTx([]byte("a"), "alice", 1)) require.Equal(t, int64(1), myMap.counter.Get()) // Bob is unknown @@ -86,9 +86,9 @@ func TestSendersMap_RemoveSendersBulk_ConcurrentWithAddition(t *testing.T) { wg.Add(100) for i := 0; i < 100; i++ { go func(i int) { - myMap.addTx(createTx([]byte("a"), "alice", uint64(i))) - myMap.addTx(createTx([]byte("b"), "bob", uint64(i))) - myMap.addTx(createTx([]byte("c"), "carol", uint64(i))) + myMap.addTxReturnEvicted(createTx([]byte("a"), "alice", uint64(i))) + myMap.addTxReturnEvicted(createTx([]byte("b"), "bob", uint64(i))) + myMap.addTxReturnEvicted(createTx([]byte("c"), "carol", uint64(i))) wg.Done() }(i) @@ -103,7 +103,7 @@ func TestSendersMap_notifyAccountNonce(t *testing.T) { // Discarded notification, since sender not added yet myMap.notifyAccountNonceReturnEvictedTransactions([]byte("alice"), 42) - myMap.addTx(createTx([]byte("tx-42"), "alice", 42)) + myMap.addTxReturnEvicted(createTx([]byte("tx-42"), "alice", 42)) alice, _ := myMap.getListForSender("alice") require.Equal(t, uint64(0), alice.accountNonce.Get()) require.False(t, alice.accountNonceKnown.IsSet()) diff --git a/txcache/txListForSender.go b/txcache/txListForSender.go index 40ccbd6f..bf8ad5fb 100644 --- a/txcache/txListForSender.go +++ b/txcache/txListForSender.go @@ -50,7 +50,6 @@ func (listForSender *txListForSender) AddTx(tx *WrappedTransaction) (bool, [][]b listForSender.onAddedTransaction(tx) - // TODO: Check how does the sender get removed if empty afterwards (maybe the answer is: "it never gets empty after applySizeConstraints()"). evicted := listForSender.applySizeConstraints() return true, evicted }