Mrs Peacock in The Library with The Candlestick (killed main.{h,cpp})

TheBlueMatt commented at 0:09 am on December 2, 2016: member

🎵I can show you the world Shining, shimmering splendid Tell me, princess, now when did You last let your heart decide!

I can open your eyes Take you wonder by wonder Over sideways and under On a magic carpet ride

A whole new world A new fantastic point of view No one to tell us no Or where to go Or say we’re only dreaming

A whole new world A dazzling place I never knew But when I’m way up here It’s crystal clear That now I’m in a whole new world with you

Now I’m in a whole new world with you

Unbelievable sights Indescribable feeling Soaring, tumbling, freewheeling Through an endless diamond sky A whole new world (Don’t you dare close your eyes) A hundred thousand things to see (Hold your breath it gets better) I’m like a shooting star I’ve come so far I can’t go back to where I used to be

A whole new world (Every turn a surprise) With new horizons to pursue (Every moment, red-letter)

I’ll chase them anywhere There’s time to spare Let me share this whole new world with you

A whole new world (A whole new world) That’s where we’ll be (That’s where we’ll be)

A thrilling chase

A wondrous place

For you and me🎵

Remove orphan state wipe from UnloadBlockIndex.

As orphan state is now "network state", like in
d6ea737be19a0001e69e4e854eb1cef21523ea7a,

UnloadBlockIndex is only used during init if we end up reindexing
to clear our block state so that we can start over. However, at
that time no connections have been brought up as CConnman hasn't
been started yet, so all of the network processing state logic is
empty when its called.

87c35f5843

TheBlueMatt force-pushed on Dec 2, 2016

TheBlueMatt commented at 0:23 am on December 2, 2016: member

Note that the main.h move is in its own commit separate from the main.h wrapper-file creation so that git tracks the rename and (at least some) rebases will be possible.

TheBlueMatt force-pushed on Dec 2, 2016

TheBlueMatt commented at 1:14 am on December 2, 2016: member

Nevermind, git apparently doesnt track across this kind of move if they are both in a merge commit, so instead ran sed s/#include “main.h”/#include “validation.h”/

TheBlueMatt force-pushed on Dec 2, 2016

TheBlueMatt commented at 1:35 am on December 2, 2016: member

Fixed includes, diff from 3b92e2d52fab7f5fccb5fd5e09cf31ee8327db32 to 84922e4bf4c38227fbbbede50e09c87fe2a5c7f0 is

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 08902bf..9cfb522 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -6,7 +6,6 @@
 5 #include "validation.h"
 6 
 7 #include "arith_uint256.h"
 8-#include "blockencodings.h"
 9 #include "chainparams.h"
10 #include "checkpoints.h"
11 #include "checkqueue.h"
12@@ -15,7 +14,6 @@
13 #include "consensus/validation.h"
14 #include "hash.h"
15 #include "init.h"
16-#include "merkleblock.h"
17 #include "policy/fees.h"
18 #include "policy/policy.h"
19 #include "pow.h"
20diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp
21index 6a778da..99d42ab 100644
22--- a/src/zmq/zmqpublishnotifier.cpp
23+++ b/src/zmq/zmqpublishnotifier.cpp
24@@ -3,6 +3,7 @@
25 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
26 
27 #include "chainparams.h"
28+#include "streams.h"
29 #include "zmqpublishnotifier.h"
30 #include "validation.h"
31 #include "util.h"

sipa commented at 1:52 am on December 2, 2016: member

utACK d90cfde1c5edada8b524ee563c3daf0b71230fcf

EDIT: obsoleted by squash. See later comment.

fanquake added the label Refactoring on Dec 2, 2016

jtimon commented at 3:35 am on December 2, 2016: contributor

Fast review ACK 585a190 , but I didn’t verified moveonly-ness, I didn’t even tried to compile and I didn’t read the full description of the PR.

jonasschnelli commented at 7:27 am on December 2, 2016: contributor

utACK 585a190c97e7e20b35bc86f3f677c9f0b39e6dc9. Testing this now on current master (= 6a2379cfabe27574d8d63010f5af38b98ee7fc4d).

in src/net_processing.h: in 585a190c97 outdated

46+ * @param[in]   pto             The node which we are sending messages to.
47+ * @param[in]   connman         The connection manager for that node.
48+ */
49+bool SendMessages(CNode* pto, CConnman& connman);
50+
51+#endif // BITCOIN_MAIN_H

paveljanik commented at 9:11 am on December 2, 2016:

wrong comment.

paveljanik commented at 9:12 am on December 2, 2016:

wrong comment

in src/net_processing.h: in 585a190c97 outdated

0@@ -0,0 +1,51 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2015 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#ifndef BITCOIN_NET_PROCESSING_H

paveljanik commented at 9:12 am on December 2, 2016:

this header guard in .cpp file?

TheBlueMatt commented at 5:21 pm on December 2, 2016:

Huh?

paveljanik commented at 5:26 pm on December 2, 2016:

Hmm, looks like I reviewed it without clicking on “Load diff” and this puzzled me. Can be ignored.

instagibbs commented at 3:07 pm on December 2, 2016: member

Might be meaningless but should cs_main be renamed as well?

Move network-msg-processing code out of main to its own file e736772c56

Rename the remaining main.{h,cpp} to validation.{h,cpp} 76faa3cdfe

TheBlueMatt force-pushed on Dec 2, 2016

TheBlueMatt commented at 5:44 pm on December 2, 2016: member

Fixed wrong comment in net_processing.h, squashed. @instagibbs I’m ok with sed’ing the headers of most files, but cs_main maybe should come later

morcos commented at 7:07 pm on December 2, 2016: member

lightly tested ACK 76faa3cdfedbd3fc

in src/main.cpp: in 87c35f5843 outdated

4310@@ -4311,8 +4311,6 @@ void UnloadBlockIndex()
4311     pindexBestInvalid = NULL;
4312     pindexBestHeader = NULL;
4313     mempool.clear();
4314-    mapOrphanTransactions.clear();
4315-    mapOrphanTransactionsByPrev.clear();

sdaftuar commented at 7:47 pm on December 2, 2016:

We also call UnloadBlockIndex in the unit tests, to clear state in between tests. There is a test that exercises the orphan handling, and fortunately it leaves the orphan map empty at the end, but that’s sort of a coincidence. How about we add this functionality to clear the orphan map in some new function, and call it from TestingSetup::~TestingSetup() in test_bitcoin.cpp?

TheBlueMatt commented at 10:17 pm on December 2, 2016:

I’d kinda prefer to leave it, actually…adding more extern … mapOrphanTransactions to more files seems super gross, and we already have a check that mapOrphanTransactions{ByPrev} is empty at the end of all tests that use it, so I think its fine.

in src/net_processing.cpp: in e736772c56 outdated

32+
33+#include <boost/thread.hpp>
34+
35+using namespace std;
36+
37+#if defined(NDEBUG)

sdaftuar commented at 8:54 pm on December 2, 2016:

Looks like this is now duplicated in main.cpp and net_processing.cpp, was that intentional?

TheBlueMatt commented at 10:14 pm on December 2, 2016:

Yea, I figured I’d just leave it in both.

sdaftuar approved

sdaftuar commented at 9:23 pm on December 2, 2016: member

Couple nits, but not blockers to merging.

ACK 76faa3cdfedbd3fc91be4ecfff77fc6dc18134fb

sdaftuar commented at 1:03 am on December 3, 2016: member

Fine with me to not address in this pr.

Sent from my iPhone

On Dec 2, 2016, at 5:17 PM, Matt Corallo notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

In src/main.cpp:

@@ -4311,8 +4311,6 @@ void UnloadBlockIndex() pindexBestInvalid = NULL; pindexBestHeader = NULL; mempool.clear();

mapOrphanTransactions.clear();

mapOrphanTransactionsByPrev.clear(); I’d kinda prefer to leave it, actually…adding more extern … mapOrphanTransactions to more files seems super gross, and we already have a check that mapOrphanTransactions{ByPrev} is empty at the end of all tests that use it, so I think its fine.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

theuni commented at 2:05 am on December 3, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/9260/commits/76faa3cdfedbd3fc91be4ecfff77fc6dc18134fb.

My audit process:

Verify the first commit is just a rename (and oneliner header fixup)
- git diff HEAD~1:./main.cpp HEAD:./validation.cpp
Check net_processing.h/cpp against what was removed from main:
- git diff --patience HEAD~2..HEAD~1 -- main.cpp | grep '^-' | sed "s/^-//" > net_processing.cpp
- git diff --patience HEAD~2..HEAD~1 -- main.h | grep '^-' | sed "s/^-//" > net_processing.h
- git diff net_processing.cpp net_processing.h
make sure nothing sneaky was added to main (now validation.h/cpp)
- git diff --patience HEAD~2..HEAD~1 -- main.cpp main.h | grep '^+'
Checked for any static init issues between the newly split files

sipa commented at 2:15 am on December 3, 2016: member

utACK 76faa3cdfedbd3fc91be4ecfff77fc6dc18134fb

My review strategy:

Verify the the changes in the first and third commit by hand (they’re all trivial).
Use git diff --patience e736772c~:src/main.h e736772c:src/main.h, git diff --patience e736772c~:src/main.h e736772c:src/net_processing.h to determine that the second commit’s main.h and net_processing.h only contain code moved or copied from the old main.h.
Use git diff --patience e736772c~:src/main.cpp e736772c:src/main.cpp, git diff --patience e736772c~:src/main.cpp e736772c:src/net_processing.cpp to determine that the second commit’s main.cpp and net_processing.cpp only contain code moved or copied from the old main.cpp.

The only additions are a few header include/define changes, and

0             if (timeNow > pto->nextSendTimeFeeFilter) {
1+                static FeeFilterRounder filterRounder(::minRelayTxFee);
2                 CAmount filterToSend = filterRounder.round(currentFilter);

and

 0-class CMainCleanup
 1+class CNetProcessingCleanup
 2 {
 3 public:
 4-    CMainCleanup() {}
 5-    ~CMainCleanup() {
 6-        // block headers
 7-        BlockMap::iterator it1 = mapBlockIndex.begin();
 8-        for (; it1 != mapBlockIndex.end(); it1++)
 9-            delete (*it1).second;
10-        mapBlockIndex.clear();
11-
12+    CNetProcessingCleanup() {}
13+    ~CNetProcessingCleanup() {
14         // orphan transactions
15         mapOrphanTransactions.clear();
16         mapOrphanTransactionsByPrev.clear();
17     }
18-} instance_of_cmaincleanup;
19+} instance_of_cnetprocessingcleanup;

sipa merged this on Dec 3, 2016

sipa closed this on Dec 3, 2016

sipa referenced this in commit 2efcfa5acf on Dec 3, 2016

TheBlueMatt referenced this in commit 067b85c7d1 on Dec 3, 2016

TheBlueMatt referenced this in commit 9b9324ee49 on Dec 3, 2016

rebroad commented at 4:30 am on December 5, 2016: contributor

Ok, so I have over 100 commits and branches that involve main.cpp. Is there a way I can rebase them all without needed to rewrite them all from scratch please?

Why rename main to validation anyway? It’s just a name and therefore why fix what isn’t broken?

TheBlueMatt commented at 4:41 am on December 5, 2016: member

Commits to the validation side should auto-rebase, but commits to the net_processing side you’ll have to do. If the changes are only to the net_processing side you could dump the diff and edit which file it is applying to.

On December 4, 2016 8:30:22 PM PST, R E Broadley notifications@github.com wrote:

Ok, so I have over 100 commits and branches that involve main.cpp. Is there a way I can rebase them all without needed to rewrite them all from scratch please?

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9260#issuecomment-264765800

sipa referenced this in commit 09c4fd157c on Dec 6, 2016

skang404 commented at 11:16 am on December 15, 2016: none

Could someone comment as to why this renaming was done?

jonasschnelli commented at 1:19 pm on December 15, 2016: contributor

@skang404: The pull request description is indeed a little bit misleading 😊. main.cpp was getting a central container for all sorts of functions and logic. People just added stuff there instead of nicely layer and split stuff into separate files or classes.

This PR finally “removes” main.cpp (I guess if you have a main.cpp is a project large as this one you should consider refactoring some stuff) and splits the functions into a validation and net-processing part.

This was long overdue.

jtimon commented at 7:50 pm on December 15, 2016: contributor

I think @skang404 more specifically refers to renaming main.o to validation.o after separating the network processing code. I really don’t see the point either given that’s not even moved to another directory. I complained on IRC but didn’t want to slow down the separation of net_processing.o.

TheBlueMatt commented at 7:56 pm on December 15, 2016: member

Why not? git magically tracks that rename across rebase/merge, and the stuff there clearly fits into one category after the split.

jtimon commented at 8:14 pm on December 15, 2016: contributor

Why not?

Right, the only disruption comes from includes. But I think the question is “why yes?”. In any case, I think it is too late to revert it (that would mean the same include disruption again in the other direction), so I don’t think there’s much point discussing this.

gladcow referenced this in commit 39c639821d on Mar 5, 2018

gladcow referenced this in commit 3095f88adb on Mar 8, 2018

gladcow referenced this in commit b32ca91b51 on Mar 13, 2018

gladcow referenced this in commit 9059e25f24 on Mar 14, 2018

gladcow referenced this in commit 9d4e2697ff on Mar 15, 2018

gladcow referenced this in commit 7b396b02ff on Mar 15, 2018

gladcow referenced this in commit 6cf070cd48 on Mar 15, 2018

gladcow referenced this in commit 7d966424b9 on Mar 15, 2018

gladcow referenced this in commit b022614bb2 on Mar 24, 2018

gladcow referenced this in commit 1fc226dab5 on Apr 4, 2018

UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018

lateminer referenced this in commit d22d97daba on Jan 3, 2019

andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019

CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019

furszy referenced this in commit 2f4e1caac1 on Sep 30, 2020

DrahtBot locked this on Sep 8, 2021

Mrs Peacock in The Library with The Candlestick (killed main.{h,cpp}) #9260