Skip to content

rpc: add cpu_load to getpeerinfo#31672

Open
vasild wants to merge 2 commits into
bitcoin:masterfrom
vasild:peer_cpu_load
Open

rpc: add cpu_load to getpeerinfo#31672
vasild wants to merge 2 commits into
bitcoin:masterfrom
vasild:peer_cpu_load

Conversation

@vasild

@vasild vasild commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

Add a new field cpu_load to the output of getpeerinfo RPC.

It represents the CPU time spent by the message handling thread for the given peer, weighted for the duration of the connection. That is, for example, if two peers are equally demanding and one is connected longer than the other, then they will have the same cpu_load number.


Monitoring CPU usage is useful on its own. Also related to #31033.


This PR uses clock_gettime() (POSIX) and GetThreadTimes() (Windows). An alternative to those, should this be explored are: getrusage() (POSIX) and QueryThreadCycleTime() (Windows, but it counts CPU cycles).

@DrahtBot

DrahtBot commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31672.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK yuvicc
Concept NACK rebroad
Concept ACK theStack, BrandonOdiwuor, laanwj, mzumsande, 1440000bytes, sipa
Stale ACK jonatack

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • per milles -> per mille [“milles” is a misspelling; the standard term is “per mille”]
  • require it be validated -> require it to be validated [missing “to” makes the sentence grammatically broken]

2026-04-29 14:29:49

@DrahtBot

Copy link
Copy Markdown
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/35717681069

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@jonatack

Copy link
Copy Markdown
Member

Concept ACK if this can be a reliable, useful metric and help pave the path to #31033.

@sipa

sipa commented Jan 16, 2025

Copy link
Copy Markdown
Member

Concept ACK

@theStack

Copy link
Copy Markdown
Contributor

Concept ACK

Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows, but on all other systems that we support AFAICT) in both the RPC help and a release note.

@BrandonOdiwuor BrandonOdiwuor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Concept ACK

@fanquake

fanquake commented Jan 17, 2025

Copy link
Copy Markdown
Member

Seems like there was some very brief discussion in #31033, and the conclusion "I guess this should start with planting some metrics", but I'm not sure putting changes into Bitcoin Core is the right first step.

Before changing our API, it'd be good to atleast show some usage that indicates that the changes here are useful. I assume you've already been running this locally, and collecting the data, so it'd be good to know what it turned up?

As noted in #31033, others have also already conducted similar research (https://b10c.me/projects/023-cpu-usage-of-peers/ or https://delvingbitcoin.org/t/cpu-usage-of-peers/196), and it seems like the data you'd expose here, will be less detailed / useful than what can/has already been produced using tracepoints or similar, so I'm wondering if this is the best approach.

@vasild

vasild commented Jan 17, 2025

Copy link
Copy Markdown
Contributor Author

@fanquake IMO, monitoring CPU usage is useful on its own, even if we don't start treating peers differently based on that metric (#31033). In other words, this metric is useful at least as much as a bunch of other metrics in the getpeerinfo output.

Yes, I have been running this locally - there is like 65x difference between the least and most demanding peers. I am curious to correlate this to the messages being sent/received to/from those peers and to have a histogram of the data, not just least / most demanding (e.g. histogram of bitcoin-cli getpeerinfo |jq ".[].cpu_load"). I view those as something nice to build on top of this PR, not as a blocker that's needed to justify the usefulness of the CPU time metric.

@1440000bytes

This comment was marked as abuse.

@vasild

vasild commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows

Right. GetThreadTimes() looks like a promising Windows alternative. I will try to implement that here. Switching to draft because I will push some work-in-progress a bunch of times.

@vasild
vasild marked this pull request as draft January 20, 2025 12:25
@vasild
vasild force-pushed the peer_cpu_load branch 3 times, most recently from 3de9c9c to 8a3ec6f Compare January 22, 2025 14:01
@vasild
vasild marked this pull request as ready for review January 23, 2025 09:58
@vasild

vasild commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

Ready for review. Implemented for Windows as well.

Here is a test program I played with to test this manually. I think it is not worth adding as a unit test or benchmark, but at least it can live here in the comments of this PR:

cpu_time_windows.cpp
#include <assert.h>

#include <windows.h>
#include <winnt.h>

#include <processthreadsapi.h>

#include <iostream>
#include <thread>

void thread(size_t work)
{
    FILETIME before_creation;
    FILETIME before_exit;
    FILETIME before_kernel;
    FILETIME before_user;
    bool ret = GetThreadTimes(GetCurrentThread(),
        &before_creation,
        &before_exit,
        &before_kernel,
        &before_user);
    assert(ret == 1);

    if (work > 10'000) {
        for (size_t i{0}; i < work; ++i) {
            (void)(i * i);
        }
    } else {
        std::this_thread::sleep_for(std::chrono::milliseconds{work});
    }

    FILETIME after_creation;
    FILETIME after_exit;
    FILETIME after_kernel;
    FILETIME after_user;
    ret = GetThreadTimes(GetCurrentThread(),
        &after_creation,
        &after_exit,
        &after_kernel,
        &after_user);
    assert(ret == 1);

    ULARGE_INTEGER before_kernel_;
    before_kernel_.LowPart = before_kernel.dwLowDateTime;
    before_kernel_.HighPart = before_kernel.dwHighDateTime;

    ULARGE_INTEGER before_user_;
    before_user_.LowPart = before_user.dwLowDateTime;
    before_user_.HighPart = before_user.dwHighDateTime;

    ULARGE_INTEGER after_kernel_;
    after_kernel_.LowPart = after_kernel.dwLowDateTime;
    after_kernel_.HighPart = after_kernel.dwHighDateTime;

    ULARGE_INTEGER after_user_;
    after_user_.LowPart = after_user.dwLowDateTime;
    after_user_.HighPart = after_user.dwHighDateTime;

    ULARGE_INTEGER elapsed;
    elapsed.QuadPart = after_kernel_.QuadPart + after_user_.QuadPart - before_kernel_.QuadPart - before_user_.QuadPart;
    ULONGLONG elapsed_ns{elapsed.QuadPart * 100};
    std::cout << "cpu time: " << elapsed_ns / 1'000'000'000.0 << " sec\n";
}

int main ()
{
    std::thread t1{thread, 1000000000};
    std::thread t2{thread, 1000000000};
    std::thread t3{thread, 3000};
    t1.join();
    t2.join();
    t3.join();

    return 0;
}

Comment thread src/rpc/net.cpp Outdated

@maflcko maflcko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(left two nits, feel free to ignore)

Comment thread src/util/time.cpp Outdated
Comment thread src/rpc/net.cpp Outdated
@vasild

vasild commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

28b2a67f13...0f68c47e93: address suggestions by @maflcko

@maflcko

maflcko commented Jan 23, 2025

Copy link
Copy Markdown
Member

The code here assumes that Bitcoin Core is single threaded, which may be true right now, but could change in the future.

@vasild

vasild commented Jun 20, 2025

Copy link
Copy Markdown
Contributor Author

8b8b854346...b25b40ebd5: rebase and pet tidy: time.h -> ctime

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jul 17, 2025
Add a new field `cpu_load` to the output of `getpeerinfo` RPC.

It represents the CPU time spent by the message handling thread for the
given peer, weighted for the duration of the connection. That is, for
example, if two peers are equally demanding and one is connected longer
than the other, then they will have the same `cpu_load` number.

Github-Pull: bitcoin#31672
Rebased-From: f1517d9
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jul 17, 2025
@rebroad

rebroad commented Jul 28, 2025

Copy link
Copy Markdown
Contributor

NACK. cpu load alone is a useless metric. However, CPU load divided by TXs accepted into the chain (or mempool) could be a useful metric.

RPC
---

A new field `cpu_load` has been added to the `getpeerinfo` RPC output. It

@ajtowns ajtowns Oct 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why monitor cpu time rather than real time? Isn't time spent waiting for disk access while looking up the utxo set equally problematic? Waiting on another thread's lock is arguably less problematic (it's the other thread that's delaying things), but if an attacker is deliberately triggering lock contention somehow to delay processing other thread's messages, that would still be a problem.

Adding OS-specific logic rather than just using either real time or a steady clock seems over-complicated here.

[EDIT: I guess in the context of rate-limiting expensive peers, it would be better to have a rolling average of time used for each peer, so that having sent expensive txs five hours ago has ~0 impact on how much cpu time you're getting now]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why monitor cpu time rather than real time?

Because CPU time we know is spent only for this peer. Is the machine further spending other time for this peer? Yes. But that is no reason to not measure CPU time.

Real time, on the other hand depends on other tasks. Like, it might be high because the user launched Photoshop at the same time on his computer. Or because other peers within the Bitcoin Core process are hogging the machine for everybody.

Isn't time spent waiting for disk access while looking up the utxo set equally problematic?

Yes, it is equally or similarly problematic. If possible that should be measured as well - "time spent on IO because of this peer".

@0xB10C

0xB10C commented Jan 26, 2026

Copy link
Copy Markdown
Contributor

fwiw I've been running a node with this patch for a while. Here a some numbers on min/max/mean/median I collected. I'm not sure yet how useful these metrics are from a monitoring standpoint.

image

I also briefly looked into which peers have a higher cpu_load by connection_type and by subver by looking at a getpeerinfo snapshot:

image

Generally, connections that we relay transactions to have a higher cpu_load. Possibly because they also relay transactions to us ( A few outliners are spy-nodes (bitnodes.earn.com, kit.dsn, ..) that we relay transactions to, but don't get any from us. See chart below).

image

vasild and others added 2 commits April 29, 2026 16:11
Add a new field `cpu_load` to the output of `getpeerinfo` RPC.

It represents the CPU time spent by the message handling thread for the
given peer, weighted for the duration of the connection. That is, for
example, if two peers are equally demanding and one is connected longer
than the other, then they will have the same `cpu_load` number.
@vasild

vasild commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

b25b40ebd5ffbb5ae5cb0435aeecefdc86fc372a...52f1efc06afe5787fdc60a190647aadbc4981981: rebase due to conflicts

What is the status here?

I think this is still relevant and would be an interesting metric to have.

@rebroad

CPU load divided by TXs accepted into the chain (or mempool) could be a useful metric.

I agree that would be useful. But for this we need the CPU load. "TXs accepted", if possible, can be added separately.

Looking at @0xB10C's subver/cpu_load chart above - there is one outlier with unusually high cpu_load - the 3rd line /Satoshi:29.2.0/Knots:20251110/, hmm...

@sedited

sedited commented May 2, 2026

Copy link
Copy Markdown
Contributor

Ping for @ajtowns , @sipa, @theStack, and other previous reviewers to take another look here.

@sedited
sedited requested a review from yuvicc May 22, 2026 12:04

@yuvicc yuvicc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code-review ACK 52f1efc

Code looks good to me. Also, tested on macos to see if it reported correctly.

@sipa

sipa commented May 26, 2026

Copy link
Copy Markdown
Member

Concept ACK. I see this as exploratory work in starting monitoring of resource consumption users cause us, not as a useful metric directly.

I'm not sure about making it part of the "production" RPC getpeerinfo however. I think this is work that will possibly need multiple iterations to come up with something that helps us make automated decisions (which, I think, should be the overarching goal here). For example, there may be reasons to use both CPU time (actual computation) and real time (the latency that one node causes us regardless of how) may matter. We may want to incorporate things like work done on other threads (including script checks) if they're blocking. We may want to also keep track of things like how much we benefit (e.g. accepting blocks and transactions from peer) at the same time, and allowing "useful" peers to cause us more work too. In any case, we may want to make things more granular than direct measurement (e.g. in tx orphan handling, work may be done in the context of the peer that gives us the resolving parent, even though it's for an orphan received from another peer). For all these reasons, I think it may be better to create an explicitly experimental "resource usage dump info" RPC than aggregating it in a getpeerinfo number.

Also, I think this should be a decaying average rather than an overall average. Maybe a time factor of 1 hour or so?

@sedited
sedited requested review from danielabrozzoni and removed request for BrandonOdiwuor June 3, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.