feat: add transactions.mempool.lockedTransactions and chainlocks.blockHeight stats #6237
Conversation
PastaPastaPasta
force-pushed
the
stats-add-basic-islock-clsig
branch
from
8f47ceb to
ab3598b
Compare
|
Guix Automation has began to build this PR tagged as v21.1.0-devpr6237.ab3598b1. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.1.0-devpr6237.ab3598b1. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v21.1.0-devpr6237.763e4fcc. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.1.0-devpr6237.763e4fcc. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v21.1.0-devpr6237.63241c25. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.1.0-devpr6237.63241c25. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v21.1.0-devpr6237.2f26f788. A new comment will be made when the image is pushed. |
|
Applied |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.1.0-devpr6237.2f26f788. The image should be on dockerhub soon. |
|
pls see 7aa0a48 + clang format is complaining |
4faf35f chore: add `stats` as a pull request header scope (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Would allow tagging #5167, #6267 and #6237 as `feat(stats)`. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code **(note: N/A)** - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ **(note: N/A)** ACKs for top commit: PastaPastaPasta: utACK 4faf35f UdjinM6: utACK 4faf35f thephez: utACK 4faf35f Tree-SHA512: 25cc28792351852b9e920d980b8814d93274bc53d22ce63fb1a5bf32821b10902d22b384a408e1c4a7b97239e53e235c45ac008d0f82e1afe5d6071b392acb47
|
This pull request has conflicts, please rebase. |
| @@ -254,6 +254,9 @@ class CInstantSendManager : public CRecoveredSigsListener | |||
| mutable Mutex cs_pendingRetry; | |||
| std::unordered_set<uint256, StaticSaltedHasher> pendingRetryTxs GUARDED_BY(cs_pendingRetry); | |||
|
|
|||
| mutable Mutex cs_timingsTxSeen; | |||
| std::unordered_map<uint256, int64_t, StaticSaltedHasher> timingsTxSeen GUARDED_BY(cs_timingsTxSeen); | |||
There was a problem hiding this comment.
it seems as this map can indefinitely grow and eat all significant amount of RAM
There was a problem hiding this comment.
resolved in d5dbbdb
src/llmq/instantsend.cpp
Outdated
| if (auto it = timingsTxSeen.find(tx->GetHash()); it == timingsTxSeen.end()) { | ||
| timingsTxSeen[tx->GetHash()] = GetTimeMillis(); | ||
| } |
There was a problem hiding this comment.
let's avoid double lookup on map here:
| if (auto it = timingsTxSeen.find(tx->GetHash()); it == timingsTxSeen.end()) { | |
| timingsTxSeen[tx->GetHash()] = GetTimeMillis(); | |
| } | |
| timingsTxSeen.try_emplace(tx->GetHash(), GetTimeMillis()); |
emplace() and try_emplace() are similar, but difference in performance - one is expecting success by default, other is expecting failure by default
There was a problem hiding this comment.
proof that it works:
$ cat a.cpp
#include <iostream>
#include <unordered_map>
using namespace std;
int main() {
unordered_map<int, int> m;
if (m.try_emplace(2, 3).second) cout << "OK" << endl; else cout << "FAIL" << endl;
if (m.try_emplace(3, 4).second) cout << "OK" << endl; else cout << "FAIL" << endl;
if (m.try_emplace(2, 5).second) cout << "OK" << endl; else cout << "FAIL" << endl;
for (auto i : m) {
cout << i.first << ' ' << i.second << endl;
}
}
$ ./a
OK
OK
FAIL
3 4
2 3
2f26f78 to
3e52422
Compare
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6237.3e524228. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6237.3e524228. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6237.35745503. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6237.35745503. The image should be on dockerhub soon. |
src/validation.cpp
Outdated
| ::g_stats_client->gauge("blocks.tip.SizeBytes", ::GetSerializeSize(block, PROTOCOL_VERSION), 1.0f); | ||
| ::g_stats_client->gauge("blocks.tip.Height", m_chain.Height(), 1.0f); | ||
| ::g_stats_client->gauge("blocks.tip.Height", m_chain.Height() + 1, 1.0f); // without the +1, the "tip.Height" doesn't match rpc calls like `getblockcount` | ||
| ::g_stats_client->gauge("blocks.tip.Version", block.nVersion, 1.0f); | ||
| ::g_stats_client->gauge("blocks.tip.NumTransactions", block.vtx.size(), 1.0f); | ||
| ::g_stats_client->gauge("blocks.tip.SigOps", nSigOps, 1.0f); |
There was a problem hiding this comment.
should drop all updates for blocks.tip.* stats in ConnectBlock()
There was a problem hiding this comment.
Because chain tip is updated in ConnectTip/DisconnectTip, not here.
WalkthroughThe pull request introduces enhancements to the blockchain system's monitoring and performance tracking capabilities by integrating a new statistics client across several core components. Key modifications include:
These changes consistently incorporate the new statistics client ( ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/llmq/instantsend.h (1)
256-257:⚠️ Potential issuePotential unbounded growth of
timingsTxSeenmapThe
timingsTxSeenmap may grow indefinitely because there is no mechanism to limit its size or remove old entries. This could lead to significant memory usage over time.
🧹 Nitpick comments (3)
src/evo/deterministicmns.cpp (1)
649-658: LGTM! Consider wrapping stats in a single blockThe statistics tracking looks good and provides comprehensive metrics about masternodes. Consider wrapping all the stats in a single block to avoid checking
active()multiple times:- if (::g_stats_client->active()) { - ::g_stats_client->gauge("masternodes.count", newList.GetAllMNsCount()); - ::g_stats_client->gauge("masternodes.weighted_count", newList.GetValidWeightedMNsCount()); - ::g_stats_client->gauge("masternodes.enabled", newList.GetValidMNsCount()); - ::g_stats_client->gauge("masternodes.weighted_enabled", newList.GetValidWeightedMNsCount()); - ::g_stats_client->gauge("masternodes.evo.count", newList.GetAllEvoCount()); - ::g_stats_client->gauge("masternodes.evo.enabled", newList.GetValidEvoCount()); - ::g_stats_client->gauge("masternodes.mn.count", newList.GetAllMNsCount() - newList.GetAllEvoCount()); - ::g_stats_client->gauge("masternodes.mn.enabled", newList.GetValidMNsCount() - newList.GetValidEvoCount()); - } + auto* stats = ::g_stats_client; + if (stats->active()) { + const auto all_count = newList.GetAllMNsCount(); + const auto valid_count = newList.GetValidMNsCount(); + const auto evo_count = newList.GetAllEvoCount(); + const auto valid_evo = newList.GetValidEvoCount(); + + stats->gauge("masternodes.count", all_count); + stats->gauge("masternodes.weighted_count", newList.GetValidWeightedMNsCount()); + stats->gauge("masternodes.enabled", valid_count); + stats->gauge("masternodes.weighted_enabled", newList.GetValidWeightedMNsCount()); + stats->gauge("masternodes.evo.count", evo_count); + stats->gauge("masternodes.evo.enabled", valid_evo); + stats->gauge("masternodes.mn.count", all_count - evo_count); + stats->gauge("masternodes.mn.enabled", valid_count - valid_evo); + }This avoids redundant function calls and improves readability.
src/validation.cpp (2)
2689-2698: LGTM! Consider reusing sigops calculationThe sigops calculation looks good but consider reusing the value that was already calculated during block validation to avoid recalculating it here.
2817-2826: LGTM! Consider reusing sigops calculationSimilar to DisconnectTip, consider reusing the sigops value that was calculated during block validation instead of recalculating it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/evo/deterministicmns.cpp(2 hunks)src/init.cpp(2 hunks)src/llmq/chainlocks.cpp(2 hunks)src/llmq/instantsend.cpp(3 hunks)src/llmq/instantsend.h(1 hunks)src/validation.cpp(4 hunks)
🔇 Additional comments (8)
src/init.cpp (2)
836-836: Variable isman initialized correctly
The variable isman is properly initialized using *Assert(node.llmq_ctx->isman).
884-884: Logging the number of locked transactions to stats client
The code correctly logs the number of locked transactions using isman.GetInstantSendLockCount() to the statistics client.
src/llmq/chainlocks.cpp (2)
18-18: Including <stats/client.h> appropriately
The header file <stats/client.h> is correctly included to use ::g_stats_client.
503-503: Logging chain lock block height to stats client
The code correctly logs the chain lock block height using clsig->getHeight() to the statistics client.
src/llmq/instantsend.cpp (3)
26-26: Including <stats/client.h> appropriately
The header file <stats/client.h> is correctly included to use ::g_stats_client.
778-788: Thread-safe access to timingsTxSeen when calculating time_diff
The lambda function correctly locks cs_timingsTxSeen before accessing timingsTxSeen, ensuring thread safety during the calculation of time_diff.
1181-1186: Optimize map insertion to avoid double lookup
To prevent double lookup into timingsTxSeen, consider using try_emplace() which attempts to insert a new element only if the key does not exist:
-if (auto it = timingsTxSeen.find(tx->GetHash()); it == timingsTxSeen.end()) {
- timingsTxSeen[tx->GetHash()] = GetTimeMillis();
-}
+timingsTxSeen.try_emplace(tx->GetHash(), GetTimeMillis());This improves performance by reducing redundant searches in the map.
src/validation.cpp (1)
2304-2305: LGTM! Block height increment is correct
The +1 increment for block height is correct since we want the gauge to match RPC calls like getblockcount. This ensures consistency between metrics and API responses.
|
Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen. |
…ead of `ConnectBlock`
PastaPastaPasta
force-pushed
the
stats-add-basic-islock-clsig
branch
from
7bbbc15 to
ebbc1dd
Compare
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6237.ebbc1dde. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/evo/deterministicmns.cpp (1)
1-1:⚠️ Potential issueFix Clang format issues.
The pipeline reports Clang format differences. Please run clang-format to fix formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
🧹 Nitpick comments (1)
src/evo/deterministicmns.cpp (1)
653-662: Consider optimizing duplicate call to GetValidWeightedMNsCount()There's a redundant call to
newList.GetValidWeightedMNsCount()on lines 655 and 657. If this method is computationally expensive (involves looping through the masternode list), consider storing the result in a local variable to avoid calculating it twice.if (::g_stats_client->active()) { ::g_stats_client->gauge("masternodes.count", newList.GetAllMNsCount()); - ::g_stats_client->gauge("masternodes.weighted_count", newList.GetValidWeightedMNsCount()); + const auto validWeightedCount = newList.GetValidWeightedMNsCount(); + ::g_stats_client->gauge("masternodes.weighted_count", validWeightedCount); ::g_stats_client->gauge("masternodes.enabled", newList.GetValidMNsCount()); - ::g_stats_client->gauge("masternodes.weighted_enabled", newList.GetValidWeightedMNsCount()); + ::g_stats_client->gauge("masternodes.weighted_enabled", validWeightedCount); ::g_stats_client->gauge("masternodes.evo.count", newList.GetAllEvoCount()); ::g_stats_client->gauge("masternodes.evo.enabled", newList.GetValidEvoCount()); ::g_stats_client->gauge("masternodes.mn.count", newList.GetAllMNsCount() - newList.GetAllEvoCount()); ::g_stats_client->gauge("masternodes.mn.enabled", newList.GetValidMNsCount() - newList.GetValidEvoCount()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/evo/deterministicmns.cpp(2 hunks)src/init.cpp(2 hunks)src/llmq/chainlocks.cpp(2 hunks)src/llmq/instantsend.cpp(3 hunks)src/llmq/instantsend.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/llmq/chainlocks.cpp
- src/llmq/instantsend.h
- src/init.cpp
- src/llmq/instantsend.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/evo/deterministicmns.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
🔇 Additional comments (2)
src/evo/deterministicmns.cpp (2)
24-24: LGTM - Added required header for statistics integration.The inclusion of the stats client header is appropriate for implementing the new masternode statistics reporting functionality.
653-662: Include statistics tracking only when client is active - good implementation.The statistics collection is properly wrapped in a check for
::g_stats_client->active(), which aligns with UdjinM6's previous comment about avoiding unnecessary computation during Initial Block Download (IBD).The implementation correctly reports various masternode metrics:
- Total and weighted masternode counts
- Enabled masternode counts (regular and weighted)
- EvoNode counts (total and enabled)
- Regular masternode counts (total and enabled)
This addresses the PR objective of adding masternode statistics.
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6237.ebbc1dde. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6237.38d78924. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6237.38d78924. The image should be on dockerhub soon. |
…tional on either stats or logger being active
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6237.d5dbbdb8. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/instantsend.cpp (1)
1-1654: Fix Clang format issues.
The pipeline failure indicates formatting discrepancies. Please run clang-format (or the project’s equivalent formatting check) on this file to ensure compliance and unblock the CI pipeline.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 623-623: syntax error
(syntaxError)
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/instantsend.cpp(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/instantsend.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (4)
src/llmq/instantsend.cpp (4)
26-26: Include is properly placed.
The addition of<stats/client.h>is consistent with the newly introduced functionality that leverages theg_stats_clientinstance. No issues found.
745-747: Helper function for logging conditions looks good.
This succinct function clearly indicates when IS lock timing stats should be recorded. Implementation is straightforward and aligns with usage patterns in the file.
782-798: Concurrent timing capture logic is sound.
Lockingcs_timingsTxSeenprevents data races when measuring the time difference between the transaction's arrival and the IS lock reception. The code correctly removes the entry fromtimingsTxSeento avoid repeated measurements.
1193-1198: Accurate timestamp assignment for first-seen TX.
UtilizingLOCK(cs_timingsTxSeen)ensures thread safety.try_emplaceproperly avoids overwriting recorded timestamps if the TX is already tracked, which meets the requirement to measure only the first arrival time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/validation.cpp (1)
2963-2966: Consider extracting signature operation counting to a helper functionThe signature operation counting logic is duplicated between DisconnectTip and ConnectTip (lines 3090-3093). Consider extracting this to a helper function to avoid duplication and ensure consistent behavior.
+// Helper function to count signature operations in a block +unsigned int GetBlockSigOpCount(const CBlock& block) { + unsigned int nSigOps = 0; + for (const auto& tx : block.vtx) { + nSigOps += GetLegacySigOpCount(*tx); + } + return nSigOps; +} bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) { // Existing code... - unsigned int nSigOps = 0; - for (const auto& tx : block.vtx) { - nSigOps += GetLegacySigOpCount(*tx); - } + unsigned int nSigOps = GetBlockSigOpCount(block); // Rest of the function... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validation.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check_merge
🔇 Additional comments (3)
src/validation.cpp (3)
2899-2901: LGTM: Added timing measurement for DisconnectTipThis is a good addition that will help with performance monitoring by tracking the execution time of DisconnectTip.
2960-2972: Good addition of block metrics tracking to DisconnectTipThis code adds proper instrumentation to track and report key metrics about disconnected blocks:
- Block execution time
- Block size
- Chain height
- Block version
- Transaction count
- Signature operations count
All metrics are properly reported via the stats client.
3090-3099: Good parallel implementation of block metrics in ConnectTipThis correctly implements the same metrics tracking for ConnectTip as was added to DisconnectTip, ensuring consistent reporting of block statistics in both directions (connecting and disconnecting).
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6237.d5dbbdb8. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6237.c3b1b3e1. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6237.c3b1b3e1. The image should be on dockerhub soon. |
| @@ -2901,6 +2896,8 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr | |||
| AssertLockHeld(cs_main); | |||
| if (m_mempool) AssertLockHeld(m_mempool->cs); | |||
|
|
|||
| int64_t nTime1 = GetTimeMicros(); | |||
There was a problem hiding this comment.
Consider using here SteadyClock::now()? It returns time in time units, not int64_t.
GetTimeMicros is going to be removed by bitcoin#27233
Issue being fixed or feature implemented
Add basic lockedTransactions count and chain locked block height to stats
What was done?
How Has This Been Tested?
Will be tested once this has a docker image
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.