Backporting Statoshi and bitcoin#16728 #2515

UdjinM6 · 2018-11-29T18:36:59Z

This backports some of https://github.com/jlopp/statoshi.

Missing stuff: README.md and client name changes, segwit and fee estimation stats.

New stuff: fixed cross-platform compilation, added new ~~configure and~~ cmd-line options.

~~Not tested yet, feel free to play with it and report if it works or not :)~~

NOTE: this version is (re)based on develop and is NOT compatible with mainnet atm. To run it on mainnet pealse use this fork https://github.com/UdjinM6/dash/tree/dashtoshi_v13 which has the same commits backported to master.

Should be fully compatible with mainnet now that spork15 is activated.

thephez · 2018-11-29T18:38:45Z

@UdjinM6 Umm, "Dashtoshi" maybe? 😉

schinzelh · 2018-11-29T18:43:15Z

I think the point is to have „stat“ in the name :-) Statdashi? Statduff? :-D

src/statsd_client.cpp

codablock · 2018-11-30T06:28:21Z

This is on my wishlist since some time already 👍 However, there are some issues with this implementation:

Sending seems to happen synchronously. Even if it's UDP (and probably non-blocking), that might still turn out to be a problem and have an impact on performance.
It doesn't use any authentication, which means if someone knows the statsd host, he can spam it with whatever faked data he wants just to make some trouble. This could be ignored in the beginning by instructing users of this to protect their hosts through IP filtering, but it makes public statsd hosts quite limited.
It clutters the code quite a bit with all the stats code...but well, I assume we have to accept this :)

codablock · 2018-11-30T06:37:50Z

Oh and regarding the name, I'm for keeping statoshi. If it turns out to be used by more projects, fancy UIs and specialized daemons will pop up and then it's good to let people know that we're compatible.

thephez · 2018-12-10T21:47:56Z

@UdjinM6 Will it be difficult to add some Dash-specific stats to this? For example, it would be interesting to see things like the average "lock time" for InstantSend txs (time between seeing the ix and receiving the lock), number of successful/failed tx locks, etc.

UdjinM6 · 2018-12-11T07:49:30Z

@thephez shouldn't be very hard I guess but maybe in another PR. And first we have to make it compilable if we ever want to merge this... (windows build fails atm)

src/init.cpp

src/net.cpp

src/rpc/blockchain.cpp

PastaPastaPasta · 2020-11-09T16:45:23Z

src/statsd_client.cpp

Would it be proper to add an MIT header?

This backports some of https://github.com/jlopp/statoshi. Missing stuff: README.md and client name changes, segwit and fee estimation stats. Fix RejectCodeToString Fix copy-paste mistake s/InvalidBlockFound/InvalidChainFound/

8a3b2eb move-only: move coins statistics utils out of RPC (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin#15606 Issue: bitcoin#15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- In the short-term, this move-only commit will help with fuzzing (bitcoin#15606 (comment)). Later, these procedures will be used to compute statistics (particularly a content hash) for UTXO sets coming in from snapshots. Most easily reviewed with `git ... --color-moved=dimmed_zebra`. A nice follow-up would be adding unittests, which I'll do if nobody else gets around to it. ACKs for top commit: MarcoFalke: ACK 8a3b2eb, checked --color-moved=dimmed-zebra Tree-SHA512: a187d2f7590ad2450b8e8fa3d038c80a04fc3d903618c24222d7e3172250ce51badea35860c86101f2ba266eb4354e6efb8d7d508b353f29276e4665a1efdf74

- Reuse some functionality from netbase - Switch from GetRand to FastRandomContext - Drop `using namespace std` and add `// namespace statsd`

…<smth> (enabled/host/port/ns/period)

…-locale-dependence.sh

And bail out early if stats are disabled

src/init.cpp

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta

utACK

xdustinface

One question see comments. And i guess it would make sense to squash fae521f into 36d2753 and the rest into 7f578c8 and merge without squash or mention the bitcoin backport in the title and squash merge. Thoughts?

src/node/coinstats.cpp

UdjinM6 · 2020-12-15T10:17:43Z

I'm leaning towards mentioning bitcoin 16728 in the title.

xdustinface

utACK

* Backport Statoshi This backports some of https://github.com/jlopp/statoshi. Missing stuff: README.md and client name changes, segwit and fee estimation stats. Fix RejectCodeToString Fix copy-paste mistake s/InvalidBlockFound/InvalidChainFound/ * Merge bitcoin#16728: move-only: move coins statistics utils out of RPC 8a3b2eb move-only: move coins statistics utils out of RPC (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin#15606 Issue: bitcoin#15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- In the short-term, this move-only commit will help with fuzzing (bitcoin#15606 (comment)). Later, these procedures will be used to compute statistics (particularly a content hash) for UTXO sets coming in from snapshots. Most easily reviewed with `git ... --color-moved=dimmed_zebra`. A nice follow-up would be adding unittests, which I'll do if nobody else gets around to it. ACKs for top commit: MarcoFalke: ACK 8a3b2eb, checked --color-moved=dimmed-zebra Tree-SHA512: a187d2f7590ad2450b8e8fa3d038c80a04fc3d903618c24222d7e3172250ce51badea35860c86101f2ba266eb4354e6efb8d7d508b353f29276e4665a1efdf74 * Fix 16728 * Modernize StatsdClient - Reuse some functionality from netbase - Switch from GetRand to FastRandomContext - Drop `using namespace std` and add `// namespace statsd` * Introduce PeriodicStats and make StatsdClient configurable via -stats<smth> (enabled/host/port/ns/period) * Move/rename tip stats from CheckBlock to ConnectBlock * Add new false positives to lint-format-strings.py * Add snprintf in statsd_client to the list of known violations in lint-locale-dependence.sh * Fix incorrect include guard * Use bracket syntax includes * Replace magic numbers with defaults * Move connection stats calculation into its own function And bail out early if stats are disabled * assert in PeriodicStats Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: MarcoFalke <falke.marco@gmail.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

…global `unique_ptr` cc998ab fmt: apply `clang-format-diff.py` suggestions (Kittywhiskers Van Gogh) 0401c58 stats: `const`-ify variables and arguments (Kittywhiskers Van Gogh) 9f96723 stats: stop using error codes, switch over to `bool` (Kittywhiskers Van Gogh) 1a81979 stats: initialize socket after we have a valid socket address (Kittywhiskers Van Gogh) dbbfc8d stats: use `Socks` wrapper, use `CService` to generate our `sockaddr` (Kittywhiskers Van Gogh) 2def905 stats: move init logic into constructor (Kittywhiskers Van Gogh) 4bc727c stats: clean up randomization code, move `FastRandomContext` inward (Kittywhiskers Van Gogh) 840241e stats: cleanup error logging, improve code sanity (Kittywhiskers Van Gogh) 85890dd docs: add copyright notice to source file, update notice in header (Kittywhiskers Van Gogh) a9d1b14 stats: move `_StatsdClientData` variables into `StatsdClient` (Kittywhiskers Van Gogh) 30c30c1 stats: fetch all arguments needed when constructing `g_stats_client` (Kittywhiskers Van Gogh) 5133d88 stats: s/statsClient/g_stats_client/g (Kittywhiskers Van Gogh) f81951d stats: make `statsClient` a `std::unique_ptr`, denote as global variable (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Support for transmitting stats to a Statsd server has been courtesy of Statoshi ([repo](https://github.com/jlopp/statoshi)), implemented Dec, 2020 by [dash#2515](#2515) but since then, it hasn't gotten much attention aside from benefiting from codebase-wide changes and the occasional compiler appeasement. This pull request aims to give our statistics code some TLC. Changes include: * Limiting initialization to solely during construction and moving the responsibility of fetching arguments outside of `statsd::StatsdClient`. * Using the RAII `Socks` wrapper as early as possible (we still need to construct a raw socket ourselves but this is done in the initializer and control is moved to the wrapper and everywhere else, the wrapper is used) * Utilizing existing networking code to generate the socket address * This lets us trivially allow IPv6 connections as the responsibility to construct it safely is moved to `CService`. * Using `std::string` and our string manipulation capabilities (replacing `snprintf` with `strprintf`), replacing platform-specific types (replacing `short` with `uint16_t`). ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [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 ACKs for top commit: PastaPastaPasta: utACK [cc998ab](cc998ab) UdjinM6: utACK cc998ab Tree-SHA512: 433c92160d6ac7ebb8582ada3cbb65ead7913618266b773619a528c90dfe0e286aafa46dc3b0bca62f246938e5948a732080e2cddba942d3627f007ca6efcc1f

UdjinM6 changed the title ~~[WIP] Dahstoshi~~ [WIP] Dashtoshi Nov 29, 2018

UdjinM6 changed the title ~~[WIP] Dashtoshi~~ [WIP] Dashtoshi/Statdashy/DashStats - backporting Statoshi Nov 29, 2018

UdjinM6 force-pushed the dashtoshi branch 2 times, most recently from 41c259f to a6106a6 Compare November 29, 2018 19:29

nmarley reviewed Nov 29, 2018

View reviewed changes

src/statsd_client.cpp Outdated Show resolved Hide resolved

UdjinM6 force-pushed the dashtoshi branch from 47f728f to 205ee04 Compare December 7, 2018 14:10

UdjinM6 force-pushed the dashtoshi branch 4 times, most recently from 81130cc to e935c10 Compare December 14, 2018 16:42

UdjinM6 force-pushed the dashtoshi branch from b3bceac to 5d88b76 Compare January 14, 2019 16:13

UdjinM6 force-pushed the dashtoshi branch 2 times, most recently from 244dcd5 to 77f8bed Compare February 2, 2019 17:18

UdjinM6 force-pushed the dashtoshi branch from 77f8bed to 9a67be7 Compare March 12, 2019 13:24

UdjinM6 force-pushed the dashtoshi branch from 9a67be7 to dfcc531 Compare March 25, 2019 16:35

UdjinM6 force-pushed the dashtoshi branch 2 times, most recently from 900f27d to 9495265 Compare April 8, 2019 08:27

UdjinM6 changed the title ~~[WIP] Dashtoshi/Statdashy/DashStats - backporting Statoshi~~ [WIP] Backporting Statoshi Apr 8, 2019

UdjinM6 force-pushed the dashtoshi branch from 387e3aa to 3ee4727 Compare April 13, 2019 17:28

UdjinM6 changed the title ~~[WIP] Backporting Statoshi~~ Backporting Statoshi Apr 20, 2019

UdjinM6 force-pushed the dashtoshi branch 2 times, most recently from 440b4ea to 2fcf82f Compare April 30, 2019 13:33

UdjinM6 force-pushed the dashtoshi branch 2 times, most recently from c5f6eb6 to a7191ef Compare May 13, 2019 11:31

PastaPastaPasta reviewed Nov 9, 2020

View reviewed changes

UdjinM6 force-pushed the dashtoshi branch from dfd3dfb to fa5470f Compare November 17, 2020 22:09

UdjinM6 and others added 12 commits December 1, 2020 04:36

Backport Statoshi

7f578c8

This backports some of https://github.com/jlopp/statoshi. Missing stuff: README.md and client name changes, segwit and fee estimation stats. Fix RejectCodeToString Fix copy-paste mistake s/InvalidBlockFound/InvalidChainFound/

Fix 16728

fae521f

Modernize StatsdClient

7000a41

- Reuse some functionality from netbase - Switch from GetRand to FastRandomContext - Drop `using namespace std` and add `// namespace statsd`

Introduce PeriodicStats and make StatsdClient configurable via -stats…

8db7c2a

…<smth> (enabled/host/port/ns/period)

Move/rename tip stats from CheckBlock to ConnectBlock

6947532

Add new false positives to lint-format-strings.py

623ae5c

Add snprintf in statsd_client to the list of known violations in lint…

c954800

…-locale-dependence.sh

Fix incorrect include guard

cfcb621

Use bracket syntax includes

efbb9d7

Replace magic numbers with defaults

bf14cfa

Move connection stats calculation into its own function

9badbc9

And bail out early if stats are disabled

UdjinM6 force-pushed the dashtoshi branch from 03c86ff to 9badbc9 Compare December 1, 2020 01:37

PastaPastaPasta requested changes Dec 9, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

assert in PeriodicStats

0a90cc4

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta approved these changes Dec 9, 2020

View reviewed changes

PastaPastaPasta requested a review from xdustinface December 9, 2020 21:04

xdustinface requested changes Dec 15, 2020

View reviewed changes

src/node/coinstats.cpp Show resolved Hide resolved

xdustinface approved these changes Dec 15, 2020

View reviewed changes

UdjinM6 changed the title ~~Backporting Statoshi~~ Backporting Statoshi and bitcoin 16728 Dec 15, 2020

PastaPastaPasta changed the title ~~Backporting Statoshi and bitcoin 16728~~ Backporting Statoshi and bitcoin#16728 Dec 15, 2020

PastaPastaPasta merged commit b559a8f into dashpay:develop Dec 15, 2020

UdjinM6 added this to the 17 milestone Mar 1, 2021

UdjinM6 deleted the dashtoshi branch July 1, 2021 21:47

kwvg mentioned this pull request Sep 3, 2024

refactor(stats): modernize statsd::StatsdClient, make global unique_ptr #5167

Merged

5 tasks

Backporting Statoshi and bitcoin#16728 #2515

Backporting Statoshi and bitcoin#16728 #2515

Uh oh!

Conversation

UdjinM6 commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thephez commented Nov 29, 2018

Uh oh!

schinzelh commented Nov 29, 2018 via email

Uh oh!

Uh oh!

codablock commented Nov 30, 2018

Uh oh!

codablock commented Nov 30, 2018

Uh oh!

thephez commented Dec 10, 2018

Uh oh!

UdjinM6 commented Dec 11, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 commented Dec 15, 2020

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

UdjinM6 commented Nov 29, 2018 •

edited

Loading

xdustinface left a comment •

edited

Loading