Disallow copies of CChain #23769

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.

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.

Can be tested with:

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 050d9dd980..ea51684f8a 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1085,8 +1085,8 @@ static RPCHelpMan pruneblockchain()
 
     ChainstateManager& chainman = EnsureAnyChainman(request.context);
     LOCK(cs_main);
-    CChainState& active_chainstate = chainman.ActiveChainstate();
-    CChain& active_chain = active_chainstate.m_chain;
+    CChainState active_chainstate = chainman.ActiveChainstate();
+    CChain active_chain = active_chainstate.m_chain;
 
     int heightParam = request.params[0].get_int();
     if (heightParam < 0)

Result:

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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

Great idea. Reviewed code locally and built.

<details><summary>Show signature data</summary> <p>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK faf2614f60efe972b47b6fa00cfbc22d04ea7239 ([`jamesob/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai`](https://github.com/jamesob/bitcoin/tree/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai))

Great idea. Reviewed code locally and built.

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmG4shwACgkQepNdrbLE
TwVX2w/+LhNLO5b9SbDP5Jw5YuHaTXipNhdVx7bUVgeXXkHZSMU7GI2hNIdR18NU
uiGud0KJoJUSNot6+VLHSX2RCnuu4ZaPZru25d4yRxnozZGMlk7V1Akhjuj2ORKo
/LbZfvDhHGAejCwUb48QhHVeyCoMjVNhLdqvqFDLtqTcudVjbc1EV69e1B0DTcU/
YfKASrRVZ67cJp82Jps6JRboTA+NHGSmDLXq7DcM5RkSzLsYVdKZ6Cc2aKG3I4fF
1t3ULCNJoUv+Az6fyAUHxFZs0R/ao697hefgpXBeNNH7XRkYUzosRZnrzSU6x13N
GiivL3FTXI6Wq5p5b1adXLXYm+x9odhkd0eQHWx4aWwHUB2g/vN+fzpMztqY5pkM
QMWLP//xE6mB9olfFBSpKVIpEeZ7oO+MAtbS051JGX9WcCrdACYClPGJ5ERY4rPO
0CWiey+ja7/c+b9XxXUDUBGT2RgHzIl2ae+00F0elOycjR5ifD4FnBQwXkjX9l9W
eNeLjO6hBFXYxX1nliLeyul1sD0L2qhi+RNFAVqa+/xBWaGHnPhg6TexBYtPM6QJ
s5GEMztEy5WcH7R2feJF694aphpy8T+YgmbthCFi4YfV3rB8sSRWrWH/b02RgESB
ma/nrKo8zb5/H+NtS0oZg84CYY00PaNPAqtLQ9eovXfvIJUu2XA=
=nrdV
-----END PGP SIGNATURE-----

</p></details>

<details><summary>Show platform data</summary> <p>

Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28

Configured 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

Compiled 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

Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25

</p></details>

utACK faf2614f60efe972b47b6fa00cfbc22d04ea7239, nice.

utACK faf2614

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.

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

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.

Ready for merge?