cleanup: Remove ProcessBlockStats #4819
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4819 +/- ##
==========================================
+ Coverage 53.40% 53.44% +0.03%
==========================================
Files 422 422
Lines 53859 53832 -27
==========================================
+ Hits 28762 28768 +6
+ Misses 22800 22767 -33
Partials 2297 2297
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
data/pools/transactionPool.go
Outdated
| @@ -548,20 +534,9 @@ func (pool *TransactionPool) OnNewBlock(block bookkeeping.Block, delta ledgercor | |||
| stats = pool.recomputeBlockEvaluator(committedTxids, knownCommitted) | |||
There was a problem hiding this comment.
the knownCommitted var is passed uninitialized
There was a problem hiding this comment.
I've actually removed it entirely in the latest change since it was only used for hint computation and only nodes with enableProcessBlockStats ever went into the loop that would initialize it.
| pool.pendingMu.RLock() | ||
| for txid := range committedTxids { | ||
| if _, ok := pool.pendingTxids[txid]; ok { | ||
| knownCommitted++ |
There was a problem hiding this comment.
maybe leave these two
There was a problem hiding this comment.
I agree that we should probably leave this and based on other feedback I'm putting EnableProcessBlockStats config back. The open question here is should we leave this block for both EnableAssembleMetrics and this or just for the former.
The concern with enabling for both is that it increases pool locking for all relays emitting AssembleBlock which is many more than currently have ProcessBlockStats enabled
There was a problem hiding this comment.
We actually already have earlyCommittedCount on the stat which should be a proxy for knownCommitted here?
This reverts commit 39b9dc8.
…ncomitted counting
|
Closing stale PR for now |
Summary
ProcessBlockStatsis largely replaced byAssembleBlockMetricsand is currently only being emitted by a single network participant. This PR removes it since #4795 split out the missing granularity for transactions not included. The only currently missing piece of functionality is tracking of unknownCommitted transactions. That requires an additional loop through all transactions to create the counter so didn't include it by default unless we think it's necessary.The last commit of this PR
39b9dc8d...is a sed replace to remove references toEnableProcessBlockStatsfrom configs and templates. Relevant code changes are in prior commits. Here is a convenient link that excludes the last sed commit https://github.com/algorand/go-algorand/pull/4819/files/7aae739ad857144c41c2637fac4b42642832a1b1Test Plan
Should pass existing tests