Skip to content

Disallow copies of CChain#23769

Merged
maflcko merged 2 commits intobitcoin:masterfrom
maflcko:2112-chainNoCopy
Dec 15, 2021
Merged

Disallow copies of CChain#23769
maflcko merged 2 commits intobitcoin:masterfrom
maflcko:2112-chainNoCopy

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 14, 2021 •
edited
Loading

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:

Fix this by disallowing it.

MarcoFalke added 2 commits December 14, 2021 12:42
Disallow copies of CChain
fada66f
style: Use 4 spaces for indendation, not 5
faf2614 
Also, other whitespace fixes in the touched file.

Can be trivially reviewed with "--ignore-all-space --word-diff-regex=. -U0".
@maflcko
Copy link
Member Author

maflcko commented Dec 14, 2021

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.

@maflcko
Copy link
Member Author

maflcko commented Dec 14, 2021

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;
           ^              ~~~~~~~~~~~~~~~~~~~~~~~~~

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 14, 2021

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.

@jamesob
Copy link
Contributor

jamesob commented Dec 14, 2021

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

Great idea. Reviewed code locally and built.

Show signature data 

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

Show platform data 

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

glozow
glozow reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK faf2614, nice.

prusnak
prusnak approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@prusnak prusnak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK faf2614

@DrahtBot DrahtBot mentioned this pull request Dec 14, 2021
Merged
shaavan
shaavan reviewed Dec 15, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🔽 to the second commit.

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

@maflcko
Copy link
Member Author

maflcko commented Dec 15, 2021

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.

@jamesob
Copy link
Contributor

jamesob commented Dec 15, 2021

Ready for merge?

@maflcko maflcko merged commit c09b41d into bitcoin:master Dec 15, 2021
@maflcko maflcko deleted the 2112-chainNoCopy branch December 15, 2021 15:33
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 15, 2021
Merge bitcoin#23769: Disallow copies of CChain
42c6a43
@bitcoin bitcoin locked and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@glozow glozow glozow left review comments

@shaavan shaavan shaavan left review comments

@prusnak prusnak prusnak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @jamesob @prusnak @glozow @shaavan