rpc: Avoid getchaintxstats invalid results

maflcko commented at 10:15 am on March 25, 2024: member

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 https://github.com/bitcoin/bitcoin/issues/29328

DrahtBot commented at 10:15 am on March 25, 2024: contributor

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.

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

maflcko force-pushed on Mar 25, 2024

in src/rpc/blockchain.cpp:1642 in fa032a5792 outdated

1636@@ -1637,9 +1637,13 @@ static RPCHelpMan getchaintxstats()
1637                         {RPCResult::Type::STR_HEX, "window_final_block_hash", "The hash of the final block in the window"},
1638                         {RPCResult::Type::NUM, "window_final_block_height", "The height of the final block in the window."},
1639                         {RPCResult::Type::NUM, "window_block_count", "Size of the window in number of blocks"},
1640-                        {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true, "The number of transactions in the window. Only returned if \"window_block_count\" is > 0"},
1641+                        {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true,
1642+                         "The number of transactions in the window. "
1643+                         "Only returned if \"window_block_count\" is > 0 and if txcount is non-zero for the start end end of the window."},

luke-jr commented at 5:24 pm on March 25, 2024:

This is confusing. We know why txcount would be zero, but users might not. Perhaps txcount itself should be omitted when wrong?

maflcko commented at 11:19 am on March 26, 2024:

I think it is fine to return a value of zero, if it is explained why the value would be zero.

However, I’ve made it optional in the last push to address your feedback.

maflcko commented at 3:35 pm on April 26, 2024:

Fixed, so resolving this for now.

maflcko force-pushed on Mar 26, 2024

maflcko requested review from jamesob on Apr 17, 2024

fjahr commented at 2:56 pm on April 23, 2024: contributor

Concept ACK, could we add a test for this?

maflcko commented at 3:00 pm on April 23, 2024: member

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.

in src/rpc/blockchain.cpp:1642 in faeb2056cf outdated

1632@@ -1633,13 +1633,19 @@ static RPCHelpMan getchaintxstats()
1633                     RPCResult::Type::OBJ, "", "",
1634                     {
1635                         {RPCResult::Type::NUM_TIME, "time", "The timestamp for the final block in the window, expressed in " + UNIX_EPOCH_TIME},
1636-                        {RPCResult::Type::NUM, "txcount", "The total number of transactions in the chain up to that point"},
1637+                        {RPCResult::Type::NUM, "txcount", /*optional=*/true,

fjahr commented at 3:34 pm on April 23, 2024:

Not returning a value here might break downstream projects if they are unlucky since this wasn’t optional before. Not sure if deprecation is necessary here but a more safe approach could be to just return 0 if it is unknown. I don’t have a strong opinion on it but usually, we are pretty cautious with a change like this.

maflcko commented at 3:57 pm on April 23, 2024:

I’ve implemented this in fa032a5792c79b76b038827977c8a5eafd8e297b, but changed it after #29720 (review) to faeb2056cf5c63af727994a6775d27a3e160ceed.

When evaluating whether this is a breaking change, I don’t think it can affect any downstream projects, because Assume-Utxo is an incomplete, experimental, regtest-only feature. Also, if changing the txcount return value were to affect downstream projects, then b50554babdddf452acaa51bac757736766c70e81 was already a breaking change.

fjahr commented at 4:06 pm on April 23, 2024:

Yeah, changing the return value wouldn’t be much of a concern, but a missing key has higher potential to crash something.

When evaluating whether this is a breaking change, I don’t think it can affect any downstream projects, because Assume-Utxo is an incomplete, experimental, regtest-only feature.

(*testnet/regtest-only) I’m pretty much with you here but others might not be though the utxo set dump might also be used in a different context. Feel free to leave the approach as is, I am fine with it.

maflcko commented at 4:18 pm on April 23, 2024:

(*testnet/regtest-only)

A good catch. Looks like is in all networks, except for main, so I guess it is still fine.

Sjors commented at 12:05 pm on April 26, 2024:

We’re breaking the format anyway with #29612 so might as well introduce other breaking changes now.

maflcko commented at 12:37 pm on April 26, 2024:

We’re breaking the format anyway with #29612 so might as well introduce other breaking changes now.

Not sure why that pull request would be relevant to the changes here. The two RPCs are completely independent and the behavior or interface of one should not interact with the other.

Sjors commented at 12:57 pm on April 26, 2024:

Both are listed as critical for mainnet release in #29616. If one needs a breaking change, it’s fine for the other to also make one.

maflcko commented at 1:09 pm on April 26, 2024:

Both are listed as critical for mainnet release in #29616. If one needs a breaking change, it’s fine for the other to also make one.

Not sure. That doesn’t mean that #29612 will be merged in its current form. One could come up with a non-breaking version of 29612, for example.

I think the pull requests should be evaluated independently.

In any case, when evaluating whether this pull request is a breaking change, I don’t think it can affect any downstream projects, because Assume-Utxo is an incomplete, experimental, non-mainnet (test-only) feature. Also, if changing the txcount return value were to affect downstream projects, then https://github.com/bitcoin/bitcoin/commit/b50554babdddf452acaa51bac757736766c70e81 was already a breaking change.

maflcko commented at 3:35 pm on April 26, 2024:

Not sure what to do here. I’ll resolve this conversation for now.

maflcko requested review from luke-jr on Apr 23, 2024

in src/rpc/blockchain.cpp:1651 in faeb2056cf outdated

1641                         {RPCResult::Type::NUM, "window_final_block_height", "The height of the final block in the window."},
1642                         {RPCResult::Type::NUM, "window_block_count", "Size of the window in number of blocks"},
1643-                        {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true, "The number of transactions in the window. Only returned if \"window_block_count\" is > 0"},
1644+                        {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true,
1645+                         "The number of transactions in the window. "
1646+                         "Only returned if \"window_block_count\" is > 0 and if txcount exists for the start end end of the window."},

Sjors commented at 12:07 pm on April 26, 2024:

The window can contain the snapshot height? If so then it should really check that all txcount values exist.

maflcko commented at 12:35 pm on April 26, 2024:

The window can contain the snapshot height? If so then it should really check that all txcount values exist.

Why? Can you explain this a bit more? The result is not influenced by values that are not used in the calculation. For example, if the windows is [a, x, y, z, b] and you calculate b - a, then the values of x, y, z are irrelevant.

Sjors commented at 1:02 pm on April 26, 2024:

Let’s say we have blocks [1,2,3,4,5,6,7,8,9] and block 6 is the snapshot. The background sync just processed block 3, foreground sync is past 9. You ask for the window 1 - 7. In that case nChainTx for (1) and (7) are set, but’s not set for (4) and (5).

However as you say, this not a problem when subtracting nChainTx of block (1) from nChainTx of block (7).

maflcko commented at 1:15 pm on April 26, 2024:

However as you say, this not a problem when subtracting nChainTx of block (1) from nChainTx of block (7).

Ok, not sure what to do here then. I’ll probably resolve the thread for now.

Sjors commented at 2:20 pm on April 26, 2024:

Yes, it’s fine.

in src/rpc/blockchain.cpp:1707 in faeb2056cf outdated

1707+        }
1708         ret.pushKV("window_interval", nTimeDiff);
1709-        if (nTimeDiff > 0) {
1710-            ret.pushKV("txrate", ((double)nTxDiff) / nTimeDiff);
1711+        if (nTimeDiff > 0 && window_tx_count) {
1712+            ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff);

Sjors commented at 12:40 pm on April 26, 2024:

0rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
1rpc/blockchain.cpp:1707:34: 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]
2 1707 |             ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff);
3      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~
4rpc/blockchain.cpp:1689: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
5 1689 |     const auto window_tx_count{
6      |                ^~~~~~~~~~~~~~~

Ubuntu 23.10 with gcc 13.2.0

Which seems spurious, but it goes away with e.g.:

0    if (blockcount > 0) {
1        if (window_tx_count) {
2            ret.pushKV("window_tx_count", *window_tx_count);
3            if (nTimeDiff > 0) {
4                ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff);
5            }
6        }
7        ret.pushKV("window_interval", nTimeDiff);
8    }

maflcko commented at 1:14 pm on April 26, 2024:

Which seems spurious, but it goes away with e.g.:

Not sure about changing the code for a broken compiler warning. I am sure this warning already exists in other parts of the code, so if changing the code is a desire, then it should be done for the other places as well (and CI could check against it). However, my recommendation would be to report this to upstream with a minimal reproducer, if it still happens with g++14.0.1-beta

Sjors commented at 1:26 pm on April 26, 2024:

Not sure about changing the code for a broken compiler warning.

It’s also slightly more readable because it checks window_tx_count only once.

I am sure this warning already exists in other parts of the code

This is the only warning thrown.

maflcko commented at 1:28 pm on April 26, 2024:

This is the only warning thrown.

What about all the ones I get, or others? For example: https://api.cirrus-ci.com/v1/task/5983141741985792/logs/make.log

maflcko commented at 1:37 pm on April 26, 2024:

Or https://github.com/bitcoin/bitcoin/issues/29359

Sjors commented at 2:20 pm on April 26, 2024:

Nope, it’s nice and quiet.

maflcko commented at 3:34 pm on April 26, 2024:

Ok, I’ve pushed a commit. Let me know what you think.

maflcko force-pushed on Apr 26, 2024

in src/rpc/blockchain.cpp:1638 in faf9b16769 outdated

1632@@ -1633,13 +1633,19 @@ static RPCHelpMan getchaintxstats()
1633                     RPCResult::Type::OBJ, "", "",
1634                     {
1635                         {RPCResult::Type::NUM_TIME, "time", "The timestamp for the final block in the window, expressed in " + UNIX_EPOCH_TIME},
1636-                        {RPCResult::Type::NUM, "txcount", "The total number of transactions in the chain up to that point"},
1637+                        {RPCResult::Type::NUM, "txcount", /*optional=*/true,
1638+                         "The total number of transactions in the chain up to that point, if known. "
1639+                         "It may be unknown when using Assume-Utxo."},

fjahr commented at 12:37 pm on April 27, 2024:

nit: we usually write assumeutxo without a dash

maflcko commented at 2:52 pm on April 27, 2024:

Thanks, done

in src/rpc/blockchain.cpp:1710 in faf9b16769 outdated

1701     ret.pushKV("window_final_block_height", pindex->nHeight);
1702     ret.pushKV("window_block_count", blockcount);
1703     if (blockcount > 0) {
1704-        ret.pushKV("window_tx_count", nTxDiff);
1705+        if (window_tx_count) {
1706+            ret.pushKV("window_tx_count", *window_tx_count);

fjahr commented at 12:41 pm on April 27, 2024:

nit: I’d prefer window_tx_count.value(), same below

maflcko commented at 2:55 pm on April 27, 2024:

I think both should behave the same in this context.

fjahr commented at 10:24 pm on April 27, 2024:

Yeah, the explicit check makes it safe. I would just always prefer the safer method if it exists and I find it more expressive. But no big deal.

fjahr commented at 1:51 pm on April 27, 2024: contributor

tACK fa28613a2cc75e5668900a11337e441546f86358

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: https://github.com/fjahr/bitcoin/commit/03a0f31dde51c094ad8e3df9f5d72abfe216c981

maflcko commented at 2:49 pm on April 27, 2024: member

03a0f31

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

maflcko force-pushed on Apr 27, 2024

fjahr commented at 10:24 pm on April 27, 2024: contributor

re-ACK fa737100af538f7257e55d88fe1f24246bcce7fc

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

Here is a version with assert_approx: https://github.com/fjahr/bitcoin/commit/24e9a37fb502092048206967dfc792d6af126316

rpc: Avoid getchaintxstats invalid results faa9a1fcd0

rpc: Reorder getchaintxstats output fa060aeb2a

test: Add coverage for getchaintxstats in assumeutxo context ef692e52b4

maflcko force-pushed on May 20, 2024

maflcko commented at 2:41 pm on May 20, 2024: member

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

DrahtBot added the label CI failed on May 20, 2024

DrahtBot commented at 5:04 pm on May 20, 2024: 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

fjahr commented at 9:16 am on May 21, 2024: contributor

re-ACK ef692e52b4121ba3d8de91e45b2d33aff946e8d2

Also reviewed #30144 to confirm the failure is ok.

fanquake referenced this in commit fa8cb0516d on May 22, 2024

DrahtBot removed the label CI failed on May 22, 2024

maflcko requested review from Sjors on May 22, 2024

rpc: Avoid getchaintxstats invalid results #29720

Code Coverage

Reviews

Conflicts