Skip to content

rpc, net: remove startingheight field of getpeerinfo RPC and from node state#34796

Merged
sedited merged 2 commits intobitcoin:masterfrom
theStack:2026-remove-starting_height_field
Mar 12, 2026
Merged

rpc, net: remove startingheight field of getpeerinfo RPC and from node state#34796
sedited merged 2 commits intobitcoin:masterfrom
theStack:2026-remove-starting_height_field

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Mar 11, 2026

This PR completely removes the startingheight field in the getpeerinfo RPC, previously deprecated in v31.0 (see #34197). The second commit goes one step further: the only remaining usage for a Peers m_starting_height field is for printing P2P debug messages. Considering that the information is untrusted and not deemed useful (see discussion #33990), remove the field and only print the starting height information once at reception of the VERSION message (suggested by ajtowns in #33990 (comment)), without saving it.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 11, 2026 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK w0xlt, sedited, stickies-v
Stale ACK paseco-qu, maflcko, rkrux

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:

  • #34588 (refactor: decompose Peer struct into focused sub-components by w0xlt)
  • #34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)

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.

@theStack theStack force-pushed the 2026-remove-starting_height_field branch from 9646082 to e042592 Compare March 11, 2026 02:45
@paseco-qu
Copy link

paseco-qu commented Mar 11, 2026

tACK e042592 .

python3 build/test/functional/test_runner.py rpc_net.py

TEST STATUS DURATION
rpc_net.py --v1transport ✓ Passed 7 s
rpc_net.py --v2transport ✓ Passed 7 s
ALL ✓ Passed 14 s (accumulated)

Also did a quick manual check on regtest with two connected nodes.

build/bin/bitcoind -regtest -datadir=/tmp/btc1 -daemon
> Bitcoin Core starting
build/bin/bitcoind -regtest -datadir=/tmp/btc2 -port=18445 -rpcport=18443 -daemon
> Bitcoin Core starting
build/bin/bitcoin-cli -regtest -datadir=/tmp/btc1 addnode "127.0.0.1:18445" onetry
build/bin/bitcoin-cli -regtest -datadir=/tmp/btc1 getpeerinfo | jq 'all(.[]; has("startingheight") | not)'
> true

stickies-v
stickies-v approved these changes Mar 11, 2026
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK e042592

I agree with the deprecation rationale in the predecessor PR, and thus also with this removal.

The start_height field is now only still used to produce a Debug log message. We also still advertise our own m_best_height to peers, which is fine because it's free and we continue to adhere to the network protocol, even if the field seems to offer little benefit.

@maflcko maflcko added this to the 32.0 milestone Mar 11, 2026
@maflcko
Copy link
Member

maflcko commented Mar 11, 2026

review ACK e042592 🚍

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e0425924291020a4cc9498d686410ea09d704889 🚍
hifMtQ8VwzmwmuWnmHrQELhcYUzQVnMGF1FEww2Qb3x+gNInpdLlyG9TI8i24AoJxhn3f7misRTrV+bo4WQ2DA==

rkrux
rkrux reviewed Mar 11, 2026
View reviewed changes
Copy link
Contributor

@rkrux rkrux left a comment •
edited
Loading

Choose a reason for hiding this comment

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

code review ACK e042592

Follow up to the previous deprecation PR gone in v31.

sedited
sedited approved these changes Mar 11, 2026
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK e042592

Seems fine to remove the starting height from the debug log too.

@theStack theStack force-pushed the 2026-remove-starting_height_field branch from e042592 to 745ad94 Compare March 11, 2026 15:14
@theStack
Copy link
Contributor Author

theStack commented Mar 11, 2026

Rebased on master (due to conflict after #34389). Thanks for the quick reviews!

w0xlt
w0xlt reviewed Mar 11, 2026
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK 745ad94

sedited
sedited approved these changes Mar 11, 2026
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

Re-ACK 745ad94

@DrahtBot DrahtBot mentioned this pull request Mar 12, 2026
Open
@stickies-v
Copy link
Contributor

stickies-v commented Mar 12, 2026

re-ACK 745ad94, no changes except for addressing minor merge conflict

@sedited sedited merged commit 51a4dc5 into bitcoin:master Mar 12, 2026
26 checks passed
@DrahtBot DrahtBot mentioned this pull request Mar 12, 2026
Merged
@kevkevinpal kevkevinpal mentioned this pull request Mar 12, 2026
Closed
@theStack theStack deleted the 2026-remove-starting_height_field branch March 12, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sedited sedited sedited approved these changes

@maflcko maflcko Awaiting requested review from maflcko

@stickies-v stickies-v Awaiting requested review from stickies-v

@rkrux rkrux Awaiting requested review from rkrux

+1 more reviewer

@w0xlt w0xlt w0xlt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

32.0

Development

Successfully merging this pull request may close these issues.

8 participants

@theStack @DrahtBot @paseco-qu @maflcko @stickies-v @rkrux @sedited @w0xlt