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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/22684534883</sub>
==25794==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 152 byte(s) in 1 object(s) allocated from:
[#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)
[#1](/bitcoin-bitcoin/1/) 0x5597c53b3398 in CreateBlockIndexWithNbits(unsigned int) src/test/blockchain_tests.cpp:24:32
[#2](/bitcoin-bitcoin/2/) 0x5597c53b1e58 in blockchain_tests::num_chain_tx_max_invoker() src/test/blockchain_tests.cpp:77:1
[#3](/bitcoin-bitcoin/3/) 0x5597c4fea063 in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:771:14
[#4](/bitcoin-bitcoin/4/) 0x5597c5070afa in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1395:32
[#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
[#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
[#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
[#8](/bitcoin-bitcoin/8/) 0x5597c4f297fe in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1308:16
[#9](/bitcoin-bitcoin/9/) 0x5597c4f214fb in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1404:5
[#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
[#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
[#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
[#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
[#14](/bitcoin-bitcoin/14/) 0x5597c4f1f4a5 in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1722:29
[#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
[#16](/bitcoin-bitcoin/16/) 0x7f0c325b31c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
[#17](/bitcoin-bitcoin/17/) 0x7f0c325b328a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: b58ab3be268281d0cf45f2b22cbd4ba8b4aac9bd)
[#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)
SUMMARY: AddressSanitizer: 152 byte(s) leaked in 1 allocation(s).
Fixed CI
ACK afa67033772d750a15231aa0aa998a0454848bea
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.
Added a scripted diff renaming 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
uint64_t tx_count; //!< total number of transactions between genesis and that timestamp
Should this also be renamed in the scripted diff?
sure, added
@maflcko @ryanofsky curious about your thoughts on https://github.com/fjahr/bitcoin/commit/b9382041e541569a2b8ec7ed1f7fb10aa602277f , i.e. if should take that route with memory optimization or if you think it's not necessary after all.
@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 "<strike>Thing A is not included.</strike> It is now", it would be better to say why it is included and remove the formatting/negation: "Thing A is included, because (...motivation...)"
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).
64d35be20c43f764e5c22657085a790d0adcfe22 should remove this todo comment
Done
concept ACK
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);
Would it make sense to also change this string nchaintx to m_chain_tx_count for consistency?
This would mean that we return a different key in the JSON returned from the RPC. We usually don't do this unless it's needed in order to not break things on the user side. We also only use the m_ prefix for class members.
Concept ACK.
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.
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.
Yes, looks good, thanks! (Can be resolved)
ACK 242116cdf3fac6fa9af1d01387cca2cf4be1ccb 🕕
<details><summary>Show signature</summary>
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: ACK 242116cdf3fac6fa9af1d01387cca2cf4be1ccb 🕕
KrtFcepn0TtAJ0W7GfxawMzY3jMtWFrv0xnB2Ns8BxeEtkJRw9IZoH2BkP3m719p6mBJGmYKIIPBIZZLjpECDw==
</details>
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
Rebased
ACK fab49990eb787aabd908f6ba88f5e00ed9844f91. 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.
Only small test-only change since my last review.
re-ACK fab49990eb787aabd908f6ba88f5e00ed9844f91 😀
<details><summary>Show signature</summary>
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==
</details>
utACK fab49990eb787aabd908f6ba88f5e00ed9844f91, happy to reack after rebase
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-
Thanks for the reviews! Rebased now.
only rebase in scripted-diff, re-ACK bf0efb4fc72d3c49a2c498c944e55466dfa046dc 🔈
<details><summary>Show signature</summary>
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==
</details>
reACK bf0efb4fc72 via range-diff