Disallow copies of CChain #23769

1. 
   MarcoFalke commented at 11:49 am on December 14, 2021: member

   Creating a copy of the chain is not a valid use case in normal operation. Also, it massively degrades performance.

   However, it seems to be a mistake that no one looks out for during review:

   • #22677 (review)

   Fix this by disallowing it.

2. Disallow copies of CChain fada66fc2c
3. style: Use 4 spaces for indendation, not 5

   Also, other whitespace fixes in the touched file.
   
   Can be trivially reviewed with "--ignore-all-space --word-diff-regex=. -U0".

   faf2614f60
4. MarcoFalke added the label Refactoring on Dec 14, 2021
5. 
   MarcoFalke commented at 11:53 am on December 14, 2021: member

   More than happy to drop the second commit if it is “too controversial”, but I think it makes future editing of the file with modern editors/workflows a lot easier. And I doubt there will be any conflicts.
6. 
   MarcoFalke commented at 11:58 am on December 14, 2021: member

   Can be tested with:

    0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
    1index 050d9dd980..ea51684f8a 100644
    2--- a/src/rpc/blockchain.cpp
    3+++ b/src/rpc/blockchain.cpp
    4@@ -1085,8 +1085,8 @@ static RPCHelpMan pruneblockchain()
    5 
    6     ChainstateManager& chainman = EnsureAnyChainman(request.context);
    7     LOCK(cs_main);
    8-    CChainState& active_chainstate = chainman.ActiveChainstate();
    9-    CChain& active_chain = active_chainstate.m_chain;
   10+    CChainState active_chainstate = chainman.ActiveChainstate();
   11+    CChain active_chain = active_chainstate.m_chain;
   12 
   13     int heightParam = request.params[0].get_int();
   14     if (heightParam < 0)
   

   Result:

   0rpc/blockchain.cpp:1089:12: error: call to deleted constructor of 'CChain'
   1    CChain active_chain = active_chainstate.m_chain;
   2           ^              ~~~~~~~~~~~~~~~~~~~~~~~~~
   

7. 
   DrahtBot commented at 1:23 pm on December 14, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22932 (Guard CBlockIndex::nStatus by cs_main, require GetBlockPos/GetUndoPos to hold cs_main by jonatack)

   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.

8. 
   jamesob commented at 3:03 pm on December 14, 2021: member

   ACK faf2614f60efe972b47b6fa00cfbc22d04ea7239 (jamesob/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai)

   Great idea. Reviewed code locally and built.

    0-----BEGIN PGP SIGNED MESSAGE-----
    1Hash: SHA512
    2
    3ACK faf2614f60efe972b47b6fa00cfbc22d04ea7239 ([`jamesob/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai`](https://github.com/jamesob/bitcoin/tree/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai))
    4
    5Great idea. Reviewed code locally and built.
    6
    7-----BEGIN PGP SIGNATURE-----
    8
    9iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmG4shwACgkQepNdrbLE
   10TwVX2w/+LhNLO5b9SbDP5Jw5YuHaTXipNhdVx7bUVgeXXkHZSMU7GI2hNIdR18NU
   11uiGud0KJoJUSNot6+VLHSX2RCnuu4ZaPZru25d4yRxnozZGMlk7V1Akhjuj2ORKo
   12/LbZfvDhHGAejCwUb48QhHVeyCoMjVNhLdqvqFDLtqTcudVjbc1EV69e1B0DTcU/
   13YfKASrRVZ67cJp82Jps6JRboTA+NHGSmDLXq7DcM5RkSzLsYVdKZ6Cc2aKG3I4fF
   141t3ULCNJoUv+Az6fyAUHxFZs0R/ao697hefgpXBeNNH7XRkYUzosRZnrzSU6x13N
   15GiivL3FTXI6Wq5p5b1adXLXYm+x9odhkd0eQHWx4aWwHUB2g/vN+fzpMztqY5pkM
   16QMWLP//xE6mB9olfFBSpKVIpEeZ7oO+MAtbS051JGX9WcCrdACYClPGJ5ERY4rPO
   170CWiey+ja7/c+b9XxXUDUBGT2RgHzIl2ae+00F0elOycjR5ifD4FnBQwXkjX9l9W
   18eNeLjO6hBFXYxX1nliLeyul1sD0L2qhi+RNFAVqa+/xBWaGHnPhg6TexBYtPM6QJ
   19s5GEMztEy5WcH7R2feJF694aphpy8T+YgmbthCFi4YfV3rB8sSRWrWH/b02RgESB
   20ma/nrKo8zb5/H+NtS0oZg84CYY00PaNPAqtLQ9eovXfvIJUu2XA=
   21=nrdV
   22-----END PGP SIGNATURE-----
   

   0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
   1
   2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --no-create --no-recursion
   3
   4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
   5
   6Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25
   

9. 
   glozow commented at 4:26 pm on December 14, 2021: member

   utACK faf2614f60efe972b47b6fa00cfbc22d04ea7239, nice.
10. prusnak approved
11. 
    prusnak commented at 6:20 pm on December 14, 2021: contributor

    utACK faf2614
12. 
    shaavan commented at 2:40 pm on December 15, 2021: contributor

    Concept ACK

    The PR disallows copying of CChain by explicitly deleting the copy constructor and copy assignment operator. I think this is a very efficient way of preventing copying of the CChain object.

    I also agree with the second commit. The formatting needs to be fixed. And it’s better sooner than later. I would suggest also incorporating formatting correction done by :arrow_down_small: to the second commit.

    0git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
    

13. 
    MarcoFalke commented at 2:52 pm on December 15, 2021: member

    I would suggest also incorporating formatting correction done by arrow_down_small to the second commit.

    I am not sure if those actually improve readability. I think this can be done when the lines are touched the next time. Not worth to invalidate all the acks here.

14. 
    jamesob commented at 2:53 pm on December 15, 2021: member

    Ready for merge?
15. MarcoFalke merged this on Dec 15, 2021
16. MarcoFalke closed this on Dec 15, 2021

    ---

17. MarcoFalke deleted the branch on Dec 15, 2021
18. sidhujag referenced this in commit 42c6a43453 on Dec 15, 2021
19. DrahtBot locked this on Dec 15, 2022

Contributors
MarcoFalke DrahtBot jamesob glozow prusnak shaavan

Labels
Refactoring