chainparams: Change nChainTx type to uint64_t

[fjahr]

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 to m_chain_num_tx which helps reviewers check all use of the symbol and can make silent merge conflicts.

[DrahtBot]

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
Concept ACK	maflcko
Stale ACK	Randy808

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:

• #29720 (rpc: Avoid getchaintxstats invalid results by maflcko)

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]

🚧 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/22684534883}

[fanquake]

==25794==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 152 byte(s) in 1 object(s) allocated from:
    #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)
    #1 0x5597c53b3398 in CreateBlockIndexWithNbits(unsigned int) src/test/blockchain_tests.cpp:24:32
    #2 0x5597c53b1e58 in blockchain_tests::num_chain_tx_max_invoker() src/test/blockchain_tests.cpp:77:1
    #3 0x5597c4fea063 in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:771:14
    #4 0x5597c5070afa in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1395:32
    #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
    #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
    #7 0x5597c4f2922f in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:910:16
    #8 0x5597c4f297fe in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1308:16
    #9 0x5597c4f214fb in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1404:5
    #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
    #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
    #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
    #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
    #14 0x5597c4f1f4a5 in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1722:29
    #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
    #16 0x7f0c325b31c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
    #17 0x7f0c325b328a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
    #18 0x5597c4e17804 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x119a804) (BuildId: dc550d0a0634dbe1f98f193b152915bbb7968f28)

SUMMARY: AddressSanitizer: 152 byte(s) leaked in 1 allocation(s).

[fjahr]

Fixed CI

[Randy808]

ACK afa6703

[maflcko]

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 because nChainTx isn’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.

[fjahr]

Added a scripted diff renaming nChainTx to m_chain_tx_count.

[fjahr]

@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.

[maflcko]

@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) ?

[fjahr]

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.

[maflcko]

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.

[maflcko]

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...)"

[fjahr]

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.