Disallow copies of CChain #23769
Disallow copies of CChain #23769
Conversation
Also, other whitespace fixes in the touched file. Can be trivially reviewed with "--ignore-all-space --word-diff-regex=. -U0".
|
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: |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
ACK faf2614 ( Great idea. Reviewed code locally and built. Show signature data
Show platform data
|
There was a problem hiding this comment.
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
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? |
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.