This picks up the work from #29331 and closes #29258.
This simply changes the type and addresses the comments from #29331 by changing the type in all relevant places and removing unnecessary casts. This also adds an extremely simple unit test.
Additionally this modernizes the name of nChainTx which helps reviewers check all use of the symbol and can make silent merge conflicts.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, glozow |
| Concept ACK | naiyoma |
| Stale ACK | Randy808, marcofleon, BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
🚧 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.
0==25794==ERROR: LeakSanitizer: detected memory leaks
1
2Direct leak of 152 byte(s) in 1 object(s) allocated from:
3 [#0](/bitcoin-bitcoin/0/) 0x5597c4ef3931 in operator new(unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x1276931) (BuildId: dc550d0a0634dbe1f98f193b152915bbb7968f28)
4 [#1](/bitcoin-bitcoin/1/) 0x5597c53b3398 in CreateBlockIndexWithNbits(unsigned int) src/test/blockchain_tests.cpp:24:32
5 [#2](/bitcoin-bitcoin/2/) 0x5597c53b1e58 in blockchain_tests::num_chain_tx_max_invoker() src/test/blockchain_tests.cpp:77:1
6 [#3](/bitcoin-bitcoin/3/) 0x5597c4fea063 in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:771:14
7 [#4](/bitcoin-bitcoin/4/) 0x5597c5070afa in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1395:32
8 [#5](/bitcoin-bitcoin/5/) 0x5597c5070afa in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:137:18
9 [#6](/bitcoin-bitcoin/6/) 0x5597c4f2922f in int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()>>(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:308:30
10 [#7](/bitcoin-bitcoin/7/) 0x5597c4f2922f in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:910:16
11 [#8](/bitcoin-bitcoin/8/) 0x5597c4f297fe in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1308:16
12 [#9](/bitcoin-bitcoin/9/) 0x5597c4f214fb in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1404:5
13 [#10](/bitcoin-bitcoin/10/) 0x5597c4f214fb in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /usr/include/boost/test/impl/unit_test_monitor.ipp:49:9
14 [#11](/bitcoin-bitcoin/11/) 0x5597c4fa3c92 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:815:44
15 [#12](/bitcoin-bitcoin/12/) 0x5597c4fa2c0f in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
16 [#13](/bitcoin-bitcoin/13/) 0x5597c4fa2c0f in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
17 [#14](/bitcoin-bitcoin/14/) 0x5597c4f1f4a5 in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1722:29
18 [#15](/bitcoin-bitcoin/15/) 0x5597c4f54a47 in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /usr/include/boost/test/impl/unit_test_main.ipp:250:9
19 [#16](/bitcoin-bitcoin/16/) 0x7f0c325b31c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
20 [#17](/bitcoin-bitcoin/17/) 0x7f0c325b328a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
21 [#18](/bitcoin-bitcoin/18/) 0x5597c4e17804 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x119a804) (BuildId: dc550d0a0634dbe1f98f193b152915bbb7968f28)
22
23SUMMARY: AddressSanitizer: 152 byte(s) leaked in 1 allocation(s).
In some draft by sjors from 5 years ago I saw that he took the chance to rename the variable to
m_chain_num_tx. I didn’t do this now becausenChainTxisn’t terrible IMO, just outdated, but happy to do this if people think it’s valuable while I touch these lines.
I’d say it should. Reviewers will have to look up every use of the symbol anyway. Also, a rename turns silent merge conflicts into compile failures, so devs are notified if an old use of the symbol remains, which may assume a 32-bit width.
nChainTx to m_chain_tx_count.
68@@ -69,7 +69,7 @@ struct AssumeutxoData {
69 */
70 struct ChainTxData {
71 int64_t nTime; //!< UNIX timestamp of last known number of transactions
72- int64_t nTxCount; //!< total number of transactions between genesis and that timestamp
73+ uint64_t nTxCount; //!< total number of transactions between genesis and that timestamp
0 uint64_t tx_count; //!< total number of transactions between genesis and that timestamp
Should this also be renamed in the scripted diff?
@maflcko @ryanofsky curious about your thoughts on fjahr@b938204 , i.e. if should take that route with memory optimization or if you think it’s not necessary after all.
I don’t think it is needed. If it was done, it should be a bitfield instead, no? See also #29258 (comment) ?
I don’t think it is needed. If it was done, it should be a bitfield instead, no? See also #29258 (comment) ?
I could change the approach to bitfields, sure. I don’t think we are using them anywhere yet so I wasn’t sure if it would really be the preferred approach. But if it’s not needed, I will just leave the PR as is for now.
I don’t think it is needed. If it was done, it should be a bitfield instead, no? See also #29258 (comment) ?
I could change the approach to bitfields, sure. I don’t think we are using them anywhere yet so I wasn’t sure if it would really be the preferred approach. But if it’s not needed, I will just leave the PR as is for now.
Yeah, I think the current approach is fine. bitfields can be done in a follow-up, if needed. One thing to look out for would be to check that the integer sanitizer works properly on bitfields as well.
Concept ACK. I think the pull description can be rewritten, since it ends up in plain text in the merge commit. Instead of writing “Thing A is not included. It is now”, it would be better to say why it is included and remove the formatting/negation: “Thing A is included, because (…motivation…)”
I have updated the description.
173@@ -174,7 +174,7 @@ class CBlockIndex
174 //! to the genesis block or an assumeutxo snapshot block have reached the
175 //! VALID_TRANSACTIONS level.
176 //! Change to 64-bit type before 2024 (assuming worst case of 60 byte transactions).
2715@@ -2716,7 +2716,7 @@ UniValue CreateUTXOSnapshot(
2716 result.pushKV("base_height", tip->nHeight);
2717 result.pushKV("path", path.utf8string());
2718 result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString());
2719- result.pushKV("nchaintx", tip->nChainTx);
nchaintx to m_chain_tx_count for consistency?
m_ prefix for class members.
nTxCount and nTxDiff, since it’s not just nChainTx that’s changing to uint64_t.
Rebased and addressed comments
Nit: IMO, https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22 message should be amended to also include nTxCount and nTxDiff, since it’s not just nChainTx that’s changing to uint64_t.
Naming all the local variables that are touched isn’t necessary I think but I added a comment in the commit description.
75@@ -76,4 +76,11 @@ BOOST_AUTO_TEST_CASE(get_difficulty_for_very_high_target)
76 TestDifficulty(0x12345678, 5913134931067755359633408.0);
77 }
78
79+BOOST_AUTO_TEST_CASE(num_chain_tx_max)
80+{
81+ std::unique_ptr<CBlockIndex> block_index{CreateBlockIndexWithNbits(0x1f111111)};
82+ block_index->nChainTx = std::numeric_limits<uint64_t>::max();
83+ BOOST_CHECK_EQUAL(block_index->nChainTx, std::numeric_limits<uint64_t>::max());
nit in e78cf83beec2f508823803947bdc3d9bcb7c7217: Not sure why nbits matter for this test.
I think you can just write CBlockIndex block{.nChainTx=..max()};, replacing the first two lines.
nit in e78cf83: Not sure why nbits matter for this test.
Yeah, I had just reused the helper method from this test but it’s not needed indeed. I am using an empty constructor now.
I think you can just write
CBlockIndex block{.nChainTx=..max()};, replacing the first two lines.
I am unfamiliar with the ..max() syntax and I am also getting a compiler error when I try to use a designated initializer here, I didn’t investigate deeper why that is though.
ACK 242116cdf3fac6fa9af1d01387cca2cf4be1ccb 🕕
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 242116cdf3fac6fa9af1d01387cca2cf4be1ccb 🕕
3KrtFcepn0TtAJ0W7GfxawMzY3jMtWFrv0xnB2Ns8BxeEtkJRw9IZoH2BkP3m719p6mBJGmYKIIPBIZZLjpECDw==
Additionally this modernizes the name of
nChainTxtom_chain_num_txwhich helps reviewers check all use of the symbol and can make silent merge conflicts.
I think you can drop the new name from the pull description. It is wrong right now (doesn’t match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.
I think you can drop the new name from the pull description. It is wrong right now (doesn’t match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.
done
make check and the individual test. I read a bit of the background (links provided in the PR description) and there doesn’t seem to be any issue with this change in validation.
Only small test-only change since my last review.
re-ACK fab49990eb787aabd908f6ba88f5e00ed9844f91 😀
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK fab49990eb787aabd908f6ba88f5e00ed9844f91 😀
3Fsjrn1dY2sNZN+BXoOuWHpOVhTzOtT0SOzaN6cIZIETwTJLL2IVNRIzT1/frUp+yAK9F3ug64qRRRpIHBVuADQ==
Code Review ACK fab49990eb787aabd908f6ba88f5e00ed9844f91
Looks Good To Me
Also update types of assumeutxo chainparams and some related local variables for
consistency.
Co-authored-by: russeree <reese.russell@ymail.com>
-BEGIN VERIFY SCRIPT-
sed -i 's/nChainTx/m_chain_tx_count/g' $(git grep -l 'nChainTx' ./src)
sed -i 's/nTxCount/tx_count/g' $(git grep -l 'nTxCount' ./src)
-END VERIFY SCRIPT-
only rebase in scripted-diff, re-ACK bf0efb4fc72d3c49a2c498c944e55466dfa046dc 🔈
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: only rebase in scripted-diff, re-ACK bf0efb4fc72d3c49a2c498c944e55466dfa046dc 🔈
3ruOHRLnefy5sonIHAM71FuUUYpKz/jrDSkuVcFSapzqkvh3kq31AlfTk8GrfX+nBCW3RmnogkSesMTaFxEtkAg==
Milestone
28.0