Skip to content

Conversation

arnabsen1729
Copy link
Contributor

This PR is a follow-up to the #22902.

Previously, the tracepoint utxocache:flush was called, even when it was not flushing. So, the tracepoint is now scoped to write only when coins cache to disk.

Previously, the `utxocache:flush` tracepoint was in the wrong scope and
reached every time `CChainState::FlushStateToDisk` was called, even when
there was no flushing of the cache. The tracepoint is now properly scoped
and will be reached during a full flush.

Inside the scope, the `fDoFullFlush` value will always be `true`, so it
doesn't need to be logged separately. Hence, it's dropped from the
tracepoint arguments.
@fanquake
Copy link
Member

cc @0xB10C @jb55 @laanwj

@0xB10C
Copy link
Contributor

0xB10C commented Jan 4, 2022

Concept ACK. These are important fixups and followups to #22902. Will review and test this.

The utxocache tracepoints still require manual testing before this PR is merged as #22902 didn't receive throughout testing.

@jb55
Copy link
Contributor

jb55 commented Jan 4, 2022

Concept ACK

fanquake added a commit that referenced this pull request Jan 10, 2022
…ilds with USDT tracepoints

6200fbf build: rename --enable-ebpf to --enable-usdt (0xb10c)
e158a2a build: add systemtap's sys/sdt.h as depends (0xb10c)

Pull request description:

  There has been light conceptual agreement on including the Userspace, Statically Defined Tracing tracepoints in Bitcoin Core release builds. This, for example, enables user to hook into production deployments, if they need to. Binaries don't have to be switched out. This is possible because we don't do [expensive computations](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#no-expensive-computations-for-tracepoints) only needed for the tracepoints. The tracepoints are NOPs when not used.

  Systemtap's `sys/sdt.h` header is required to build Bitcoin Core with USDT support. The header file defines the `DTRACE_PROBE` macros used in [`src/util/trace.h`](https://github.com/bitcoin/bitcoin/blob/master/src/util/trace.h). This PR adds Systemtap 4.5 (May 2021) as dependency. GUIX builds for Linux hosts now include the tracepoints.

  Closes #23297.

ACKs for top commit:
  fanquake:
    ACK 6200fbf - tested enabling / disabling and with/without SDT from depends. We can follow up with #23819, #23907 and #23296, and if any serious issues arise before feature freeze, it is easy for us to flip depends such that USDT becomes opt-in, rather than opt-out, and thus, releases would be tracepoint free.

Tree-SHA512: 0263f44892bf8450e8a593e4de7a498243687f8d81269e1c3283fa8354922c7cf93fddef4b92cf5192d33798424aa5812e03e68ef8de31af078a32dd34021382
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 10, 2022
…GUIX builds with USDT tracepoints

6200fbf build: rename --enable-ebpf to --enable-usdt (0xb10c)
e158a2a build: add systemtap's sys/sdt.h as depends (0xb10c)

Pull request description:

  There has been light conceptual agreement on including the Userspace, Statically Defined Tracing tracepoints in Bitcoin Core release builds. This, for example, enables user to hook into production deployments, if they need to. Binaries don't have to be switched out. This is possible because we don't do [expensive computations](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#no-expensive-computations-for-tracepoints) only needed for the tracepoints. The tracepoints are NOPs when not used.

  Systemtap's `sys/sdt.h` header is required to build Bitcoin Core with USDT support. The header file defines the `DTRACE_PROBE` macros used in [`src/util/trace.h`](https://github.com/bitcoin/bitcoin/blob/master/src/util/trace.h). This PR adds Systemtap 4.5 (May 2021) as dependency. GUIX builds for Linux hosts now include the tracepoints.

  Closes bitcoin#23297.

ACKs for top commit:
  fanquake:
    ACK 6200fbf - tested enabling / disabling and with/without SDT from depends. We can follow up with bitcoin#23819, bitcoin#23907 and bitcoin#23296, and if any serious issues arise before feature freeze, it is easy for us to flip depends such that USDT becomes opt-in, rather than opt-out, and thus, releases would be tracepoint free.

Tree-SHA512: 0263f44892bf8450e8a593e4de7a498243687f8d81269e1c3283fa8354922c7cf93fddef4b92cf5192d33798424aa5812e03e68ef8de31af078a32dd34021382
@0xB10C
Copy link
Contributor

0xB10C commented Jan 17, 2022

Added a bitcoind.observer dashboard based on the flush and add/spent/uncache tracepoints to observe their behavior a bit: https://bitcoind.observer/d/beTN4K27z/utxo-cache (this is currently running a pruned IBD and the UTXO cache is flushed for pruning before it reaches it's maximum capacity -- see #20827)

@fanquake fanquake added this to the 23.0 milestone Jan 18, 2022
@0xB10C
Copy link
Contributor

0xB10C commented Jan 25, 2022

My site currently is currently only picking up flushes related to a prune event. I'll investigate. Was an error on my side. The tracepoints are working as expected.

@0xB10C
Copy link
Contributor

0xB10C commented Feb 9, 2022

Could add a note in the utxoscache:uncache, utxoscache:add, and utxocache:spent tracepoint documentation that we don't have only one 'the utxo cache'. We sometimes work with temporary UTXO caches. For example, in TestBlockValidity() after before mining or during mempool consistency checks (frequent on regtest). Tracepoints are triggered for temporary caches too. Noticed this while writing functional tests for the tracepoints.

@arnabsen1729
Copy link
Contributor Author

Could add a note in the utxoscache:uncache, utxoscache:add, and utxocache:spent tracepoint documentation that we don't have only one 'the utxo cache'. We sometimes work with temporary UTXO caches. For example, in TestBlockValidity() after mining or during mempool consistency checks (frequent on regtest). Tracepoints are triggered for temporary caches too. Noticed this while writing functional tests for the tracepoints.

Okay, will update the docs 👍

@0xB10C
Copy link
Contributor

0xB10C commented Feb 17, 2022

Feel free to squash this commit 0xB10C@287351a documenting this #23907 (comment) into the tracing: misc follow-ups to 22902 too (while copying the commit subject and description into the squash commit description).

I've cherry picked the commits from #24358 (tracepoint tests) onto this PR (currently git cherry-pick 1e8aa02ec5cc2819c67ef40a7573c4b23a4c11cc..db3a05f50ee5d3cafa226882eade531a7ebff6b1; 0xB10C/bitcoin:2022-02-23907+tracepoint-tests) to run the tracepoint tests against this PR. Tests pass!

- mention 'Lost X events' workaround
- clarify flush tracepoint docs
- fix typo in tracepoint context
- clarify flush for prune
    The documentation and examples for the `fFlushForPrune` argument
    of the utxocache flush tracepoint weren't clear without looking
    at the code.

    See these comments: bitcoin#22902 (comment)

- doc: note that there can be temporary UTXO caches
    Bitcoin Core uses temporary clones of it's _main_ UTXO cache in some
    places. The utxocache:add and :spent tracepoints are triggered when
    temporary caches are changed too. This is documented.
@arnabsen1729 arnabsen1729 force-pushed the 2021-12-utxocache-usdt-fix branch from 261db41 to 799968e Compare February 18, 2022 15:22
Copy link
Contributor

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 799968e

Code and doc changes reviewed. Tested with #24358.

@fanquake fanquake merged commit 2b0735d into bitcoin:master Feb 20, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 20, 2022
…coin#22902

799968e tracing: misc follow-ups to 22902 (0xb10c)
36a6584 tracing: correctly scope utxocache:flush tracepoint (Arnab Sen)

Pull request description:

  This PR is a follow-up to the [bitcoin#22902](bitcoin#22902).

  Previously, the tracepoint `utxocache:flush` was called, even when it was not flushing. So, the tracepoint is now scoped to write only when coins cache to disk.

ACKs for top commit:
  0xB10C:
    ACK 799968e

Tree-SHA512: ebb096cbf991c551c81e4339821f10d9768c14cf3d8cb14d0ad851acff5980962228a1c746914c6aba3bdb27e8be53b33349c41efe8bab5542f639916e437b5f
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 1, 2022
Summary:
```
The tracepoints are further documented in docs/tracing.md and the usage is shown via the two newly added example scripts in contrib/tracing/.
```

Backport of [[bitcoin/bitcoin#22902 | core#22902]].
Backport of [[bitcoin/bitcoin#23907 | core#23907]].

Notes:
 - The amount is passed as string because our type can't be converted to int
 - CTxOut and CCoin accessors are used where applicable
 - I removed the casts that are both useless and even mixing stding and posix types

Test Plan:
Build with tracing, run bitcoind, then:
  sudo ../contrib/tracing/log_utxocache_flush.py ./src/bitcoind
  sudo bpftrace ../contrib/tracing/log_utxos.bt

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Subscribers: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D12715
@bitcoin bitcoin locked and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants