chainparams: Change nChainTx type to uint64_t #29656

fjahr · 2024-03-14T23:34:35Z

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.

DrahtBot · 2024-03-14T23:34:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30560 (refactor: Add consteval uint256 constructor by hodlinator)
#30377 (refactor: Add consteval uint256{"str"} by hodlinator)
#30364 (logging: Replace LogError and LogWarning with LogAlert by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint 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 · 2024-03-15T00:33:35Z

🚧 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 · 2024-03-15T14:34:51Z

==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 · 2024-03-15T17:34:13Z

Fixed CI

Randy808 · 2024-03-18T12:52:57Z

ACK afa6703

maflcko · 2024-03-18T13:03:52Z

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 · 2024-03-18T22:30:17Z

Added a scripted diff renaming nChainTx to m_chain_tx_count.

src/kernel/chainparams.h

fjahr · 2024-03-20T14:42:57Z

@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 · 2024-03-24T14:32:14Z

@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 · 2024-03-25T20:42:29Z

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 · 2024-03-28T08:08:48Z

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 · 2024-03-28T08:15:36Z

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 · 2024-04-27T21:45:24Z

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.

fjahr · 2024-07-10T22:20:05Z

Rebased

marcofleon

ACK fab4999. Reviewed the changes, grepped for the old variable names, and ran 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.

maflcko · 2024-08-01T18:32:43Z

Only small test-only change since my last review.

re-ACK fab4999 😀

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK fab49990eb787aabd908f6ba88f5e00ed9844f91 😀
Fsjrn1dY2sNZN+BXoOuWHpOVhTzOtT0SOzaN6cIZIETwTJLL2IVNRIzT1/frUp+yAK9F3ug64qRRRpIHBVuADQ==

glozow

utACK fab4999, happy to reack after rebase

BrandonOdiwuor

Code Review ACK fab4999

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-

fjahr · 2024-08-04T13:49:25Z

Thanks for the reviews! Rebased now.

maflcko · 2024-08-05T06:58:05Z

only rebase in scripted-diff, re-ACK bf0efb4 🔈

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: only rebase in scripted-diff, re-ACK bf0efb4fc72d3c49a2c498c944e55466dfa046dc 🔈
ruOHRLnefy5sonIHAM71FuUUYpKz/jrDSkuVcFSapzqkvh3kq31AlfTk8GrfX+nBCW3RmnogkSesMTaFxEtkAg==

glozow · 2024-08-05T08:23:37Z

reACK bf0efb4 via range-diff

DrahtBot added the CI failed label Mar 15, 2024

fjahr force-pushed the 2024-03-nchaintx-64 branch 2 times, most recently from ff1828c to 2677a24 Compare March 15, 2024 16:19

DrahtBot removed the CI failed label Mar 15, 2024

fjahr force-pushed the 2024-03-nchaintx-64 branch from 2011c2d to cef0434 Compare March 18, 2024 23:09

DrahtBot added the CI failed label Mar 18, 2024

This was referenced Mar 19, 2024

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

assumeutxo: Get rid of faked nTx and nChainTx values #29370

Merged

DrahtBot removed the CI failed label Mar 19, 2024

DrahtBot mentioned this pull request Mar 19, 2024

reduce cs_main scope, guard block index 'nFile' under a local mutex #27006

Closed

maflcko reviewed Mar 20, 2024

View reviewed changes

src/kernel/chainparams.h Outdated Show resolved Hide resolved

fjahr force-pushed the 2024-03-nchaintx-64 branch from cef0434 to 2d346ef Compare March 20, 2024 14:29

DrahtBot added the Needs rebase label Mar 20, 2024

fjahr force-pushed the 2024-03-nchaintx-64 branch from 2d346ef to 557a9cf Compare March 20, 2024 20:15

DrahtBot removed the Needs rebase label Mar 20, 2024

DrahtBot mentioned this pull request Mar 25, 2024

rpc: Avoid getchaintxstats invalid results #29720

Merged

DrahtBot mentioned this pull request Apr 1, 2024

Testnet4 including PoW difficulty adjustment fix #29775

Merged

DrahtBot added the Needs rebase label Jul 10, 2024

fjahr force-pushed the 2024-03-nchaintx-64 branch from a2d8a4d to fab4999 Compare July 10, 2024 22:19

DrahtBot removed the Needs rebase label Jul 10, 2024

DrahtBot mentioned this pull request Jul 19, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

This was referenced Jul 30, 2024

rpc: fix maybe-uninitialized compile warning in getchaintxstats #30544

Merged

refactor: Add consteval uint256 constructor #30560

Merged

marcofleon reviewed Aug 1, 2024

View reviewed changes

DrahtBot requested a review from maflcko August 1, 2024 17:57

maflcko added this to the 28.0 milestone Aug 1, 2024

DrahtBot added the Needs rebase label Aug 2, 2024

glozow reviewed Aug 2, 2024

View reviewed changes

BrandonOdiwuor reviewed Aug 2, 2024

View reviewed changes

fjahr and others added 3 commits August 4, 2024 12:12

chainparams: Change nChainTx to uint64_t

dc2938e

Also update types of assumeutxo chainparams and some related local variables for consistency. Co-authored-by: russeree <reese.russell@ymail.com>

test: Add basic check for nChainTx type

72e5d1b

scripted-diff: Modernize naming of nChainTx and nTxCount

bf0efb4

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

fjahr force-pushed the 2024-03-nchaintx-64 branch from fab4999 to bf0efb4 Compare August 4, 2024 12:25

DrahtBot removed the Needs rebase label Aug 4, 2024

DrahtBot requested review from glozow, marcofleon and BrandonOdiwuor August 5, 2024 06:58

glozow removed request for BrandonOdiwuor, glozow and naiyoma August 5, 2024 08:34

glozow merged commit 1a19a4d into bitcoin:master Aug 5, 2024
16 checks passed

bitcoin locked and limited conversation to collaborators Aug 5, 2025

chainparams: Change nChainTx type to uint64_t #29656

chainparams: Change nChainTx type to uint64_t #29656

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Mar 15, 2024

Uh oh!

fanquake commented Mar 15, 2024

Uh oh!

fjahr commented Mar 15, 2024

Uh oh!

Randy808 commented Mar 18, 2024

Uh oh!

maflcko commented Mar 18, 2024

Uh oh!

fjahr commented Mar 18, 2024

Uh oh!

Uh oh!

fjahr commented Mar 20, 2024

Uh oh!

maflcko commented Mar 24, 2024

Uh oh!

fjahr commented Mar 25, 2024

Uh oh!

maflcko commented Mar 28, 2024

Uh oh!

maflcko commented Mar 28, 2024

Uh oh!

fjahr commented Apr 27, 2024

Uh oh!

fjahr commented Jul 10, 2024

Uh oh!

marcofleon left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 1, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Aug 4, 2024

Uh oh!

maflcko commented Aug 5, 2024

Uh oh!

glozow commented Aug 5, 2024

Uh oh!

Uh oh!

Uh oh!

fjahr commented Mar 14, 2024 •

edited

Loading

DrahtBot commented Mar 14, 2024 •

edited

Loading