rpc: Avoid getchaintxstats invalid results #29720

maflcko · 2024-03-25T10:15:31Z

The getchaintxstats RPC reply during AU background download may return non-zero, but invalid, values for window_tx_count and txrate.

For example, txcount may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).

Also, txcount may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.

Fix all issues by skipping the returned value if either txcount is unset (equal to zero).
Also, skip txcount in the returned value, if it is unset (equal to zero).

Fixes #29328

DrahtBot · 2024-03-25T10:15:34Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, mzumsande, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29656 (chainparams: Change nChainTx type to uint64_t by fjahr)

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.

src/rpc/blockchain.cpp

fjahr · 2024-04-23T14:56:57Z

Concept ACK, could we add a test for this?

maflcko · 2024-04-23T15:00:51Z

A test is already present in test/functional/feature_assumeutxo.py, see the diff. You can also test it by running this pull against a bitcoind compiled from master and observing the sanitizer crash and the test failure.

src/rpc/blockchain.cpp

fjahr

tACK fa28613

I have written some additional coverage to test the behavior and build my understanding, it could be added here or I can make a follow-up: fjahr@03a0f31

src/rpc/blockchain.cpp

maflcko · 2024-04-27T14:49:39Z

03a0f31

I think assert_equal can generally not be used on floating point values, no?

fjahr · 2024-04-27T22:24:58Z

re-ACK fa737100af538f7257e55d88fe1f24246bcce7fc

I think assert_equal can generally not be used on floating point values, no?

Here is a version with assert_approx: fjahr@24e9a37

maflcko · 2024-05-20T14:41:37Z

@fjahr Thanks! I've pushed your test commit with small fixups.

DrahtBot · 2024-05-20T17:04:42Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/25183174149}

fjahr · 2024-05-21T09:16:46Z

re-ACK ef692e52b4121ba3d8de91e45b2d33aff946e8d2

Also reviewed #30144 to confirm the failure is ok.

…task fa73431 ci: Add mising -Wno-error=maybe-uninitialized to armhf task (MarcoFalke) Pull request description: This happens after bd597c3 in many pull requests as a silent merge conflict. For example: * #29720 (comment) * #29521 (comment) * (Probably many undetected, because the CI task was not yet re-run) * ... ACKs for top commit: fjahr: utACK fa73431 fanquake: ACK fa73431 - many fixed with 13.x Tree-SHA512: 6e6ff8dc6f3c6a2abcd04c4203d3468f6e98c1ad3a4da4ad0037a9ee2cbec6bec079a5f778aba0273e38e173849927abcdfcfba7643d08ed66c1168cb89fab08

src/rpc/blockchain.cpp

fjahr · 2024-07-02T09:32:49Z

re-ACK 2342b46

Confirmed only changes were rebase and typo-fix, per git range-diff master ef692e52b4121ba3d8de91e45b2d33aff946e8d2 2342b46c451658a418f8e28e50b2ad0e5abd284d.

mzumsande

ACK 2342b46

I reviewed the code and tested the getchaintxstats output with a snapshot.

achow101 · 2024-07-02T21:41:53Z

ACK 2342b46

hebasto · 2024-07-11T21:08:56Z

This PR introduced a new -Wmaybe-uninitialized warning when building with GCC < 13:

GCC 11:

  CXX      rpc/libbitcoin_node_a-blockchain.o
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1742:38: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<unsigned int, true, true>::<unnamed>)).std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>::_M_value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 1742 |                 ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff);
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~

GCC 12:

  CXX      rpc/libbitcoin_node_a-blockchain.o
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1742:38: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<unsigned int, true, true>::<unnamed>)).std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>::_M_value’ may be used uninitialized [-Wmaybe-uninitialized]
 1742 |                 ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff);
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~
rpc/blockchain.cpp:1725:16: note: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<unsigned int, true, true>::<unnamed>)).std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>::_M_value’ was declared here
 1725 |     const auto window_tx_count{
      |                ^~~~~~~~~~~~~~~

maflcko · 2024-07-12T08:41:30Z

See #29359

Add `-Wno-error=maybe-uninitialized` for GCC 11.4. See: bitcoin#29720 (comment)

d8c2692 fixup! ci: Test CMake edge cases (Hennadii Stepanov) Pull request description: This PR adds `-Wno-error=maybe-uninitialized` for GCC 11.4. See: bitcoin#29720 (comment). ACKs for top commit: m3dwards: ACK d8c2692 Tree-SHA512: 9c5b5ebf67a0fcc29288943b8f089f4e93bd6cd18e87f0d887cc3f7bc39a59a675ee526158f5c55a6788c9aaddeafa8f3365e436cdee245b8c71fb9f7805f6e0

Summary: The getchaintxstats RPC reply during AU background download may return non-zero, but invalid, values for window_tx_count and txrate. For example, txcount may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations). Also, txcount may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong. Fix all issues by skipping the returned value if either txcount is unset (equal to zero). Also, skip txcount in the returned value, if it is unset (equal to zero). This is a backport of [[bitcoin/bitcoin#29720 | core#29720]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D18053

DrahtBot added the RPC/REST/ZMQ label Mar 25, 2024

maflcko force-pushed the 2403-rpc-int-wrap- branch from 2ece999 to fa032a5 Compare March 25, 2024 10:15

luke-jr reviewed Mar 25, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 25, 2024

chainparams: Change nChainTx type to uint64_t #29656

Merged

maflcko force-pushed the 2403-rpc-int-wrap- branch from fa032a5 to faeb205 Compare March 26, 2024 11:15

maflcko requested a review from jamesob April 17, 2024 18:23

maflcko mentioned this pull request Apr 23, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

fjahr reviewed Apr 23, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

maflcko requested a review from luke-jr April 23, 2024 17:41

Sjors reviewed Apr 26, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Sjors reviewed Apr 26, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2403-rpc-int-wrap- branch 2 times, most recently from fa146d9 to fa28613 Compare April 26, 2024 15:28

fjahr reviewed Apr 27, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Show resolved Hide resolved

maflcko force-pushed the 2403-rpc-int-wrap- branch from fa28613 to fa73710 Compare April 27, 2024 14:52

maflcko force-pushed the 2403-rpc-int-wrap- branch from fa73710 to ef692e5 Compare May 20, 2024 14:41

DrahtBot added the CI failed label May 20, 2024

maflcko mentioned this pull request May 21, 2024

ci: Add missing -Wno-error=maybe-uninitialized to armhf task #30144

Merged

DrahtBot removed the CI failed label May 22, 2024

maflcko requested a review from Sjors May 22, 2024 13:12

achow101 reviewed Jul 1, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

MarcoFalke added 2 commits July 2, 2024 08:46

rpc: Avoid getchaintxstats invalid results

fa2dada

rpc: Reorder getchaintxstats output

faf2a67

maflcko force-pushed the 2403-rpc-int-wrap- branch from ef692e5 to fac11c5 Compare July 2, 2024 06:46

test: Add coverage for getchaintxstats in assumeutxo context

2342b46

maflcko force-pushed the 2403-rpc-int-wrap- branch from fac11c5 to 2342b46 Compare July 2, 2024 06:47

mzumsande reviewed Jul 2, 2024

View reviewed changes

achow101 merged commit 173ab0c into bitcoin:master Jul 2, 2024

maflcko deleted the 2403-rpc-int-wrap- branch July 3, 2024 05:24

hebasto mentioned this pull request Jul 14, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#264

Closed

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 15, 2024

fixup! ci: Test CMake edge cases

323daf2

Add `-Wno-error=maybe-uninitialized` for GCC 11.4. See: bitcoin#29720 (comment)

hebasto mentioned this pull request Jul 15, 2024

cmake, ci: Ignore -Wmaybe-uninitialized for GCC 11.4 hebasto/bitcoin#266

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 15, 2024

fixup! ci: Test CMake edge cases

d8c2692

Add `-Wno-error=maybe-uninitialized` for GCC 11.4. See: bitcoin#29720 (comment)

bitcoin locked and limited conversation to collaborators Jul 12, 2025

rpc: Avoid getchaintxstats invalid results #29720

rpc: Avoid getchaintxstats invalid results #29720

Uh oh!

Conversation

maflcko commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

fjahr commented Apr 23, 2024

Uh oh!

maflcko commented Apr 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 27, 2024

Uh oh!

fjahr commented Apr 27, 2024

Uh oh!

maflcko commented May 20, 2024

Uh oh!

DrahtBot commented May 20, 2024

Uh oh!

fjahr commented May 21, 2024

Uh oh!

Uh oh!

fjahr commented Jul 2, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 2, 2024

Uh oh!

hebasto commented Jul 11, 2024

Uh oh!

maflcko commented Jul 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented Mar 25, 2024 •

edited

Loading

DrahtBot commented Mar 25, 2024 •

edited

Loading