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
| 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
…ead of `ConnectBlock`
PastaPastaPasta
force-pushed
the
stats-add-basic-islock-clsig
branch
from
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. |
| ::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 a comprehensive enhancement to the blockchain system's monitoring and performance tracking capabilities. The changes primarily focus on integrating a new statistics client across multiple core components of the system, including masternode management, instant send locks, chain locks, and block validation processes. Key modifications span several critical source files:
The modifications consistently introduce a new statistics client ( The changes maintain the existing architectural integrity of the system while providing more granular insights into the blockchain's operational characteristics. The implementation focuses on non-invasive additions that extend monitoring capabilities without fundamentally altering the core logic of the existing components. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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. |
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.