Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Avoid getchaintxstats invalid results #29720

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Mar 25, 2024

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
Copy link
Contributor

DrahtBot commented Mar 25, 2024

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

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 Outdated Show resolved Hide resolved
@fjahr
Copy link
Contributor

fjahr commented Apr 23, 2024

Concept ACK, could we add a test for this?

@maflcko
Copy link
Member Author

maflcko commented Apr 23, 2024

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.

@maflcko maflcko requested a review from luke-jr April 23, 2024 17:41
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@maflcko maflcko force-pushed the 2403-rpc-int-wrap- branch 2 times, most recently from fa146d9 to fa28613 Compare April 26, 2024 15:28
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
@maflcko
Copy link
Member Author

maflcko commented Apr 27, 2024

03a0f31

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

@fjahr
Copy link
Contributor

fjahr commented Apr 27, 2024

re-ACK fa73710

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

Here is a version with assert_approx: fjahr@24e9a37

@maflcko
Copy link
Member Author

maflcko commented May 20, 2024

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

@DrahtBot
Copy link
Contributor

🚧 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

@maflcko
Copy link
Member Author

maflcko commented May 21, 2024

The CI failure can be ignored during review. It is fixed in #30144

@fjahr
Copy link
Contributor

fjahr commented May 21, 2024

re-ACK ef692e5

Also reviewed #30144 to confirm the failure is ok.

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.

assumeutxo: nTx and nChainTx in RPC results are off
5 participants