MOVEONLY: Split Transaction and Block from core #5100

jtimon · 2014-10-18T00:37:46Z

The main goal is to separate minimal core dependencies for script/interpreter. CFeeRate shouldn't be there, but that won't be move only.
This should help with #4692 and it's related to #4981 among other PRs

theuni · 2014-10-18T01:25:12Z

Missing some files.

Any reason not to go ahead and move CTxOutCompressor/CTxInUndo/CTxUndo while you're at it, as discussed on IRC?

jtimon · 2014-10-18T02:22:55Z

Ok, I'll do a more complete PR, closing for now.

sipa · 2014-10-18T02:33:59Z

Can you move CTxOutCompressor either along with CTransaction (it doesn't depend on anything block, right?), or directly to coins.h.
Can you move the remaining part to a coins/block.h? coins.h and coins/transaction.h is a bit strange.

jtimon · 2014-10-18T04:04:15Z

I don't want to put compressor in core/transaction since script/interpreter doesn't need it.
undotx depends on txcompressor but not on the rest of coins.o

That means txcompressor goes on its own (maybe take script/compressor with it?)

I don't like to have compressor in chain
I don't want to be responsible for putting more stuff in main

Does that mean txundo goes on its own as well?
I think so, but please debate 3 and 4.

gmaxwell · 2014-10-18T04:13:35Z

logically the compressor stuff is part of coins-- its the fancy, locally specific highly compressed seralization used only in the utxo set representation. Is there any reason it can't just go into coins.cpp/coins.h?

Undo seem logically best placed with chain.* though it's a bit different since undo data (like the compressor) is only locally significant, most of chain.* is actually locally specific storage stuff for the chain in any case.

jtimon · 2014-10-18T04:15:30Z

Never mind, coins.o also depends on CTxInUndo so they both belong in their own files.

jtimon · 2014-10-18T17:26:08Z

logically the compressor stuff is part of coins-- its the fancy, locally
specific highly compressed seralization used only in the utxo set
representation. Is there any reason it can't just go into coins.cpp/coins.h?

Txundo depends on it but not in the rest of coins.
@theuni also mentioned that moving it to coins caused a circular dependency
with main.
Additionally, script/compressor could be moved there too, since it's the
only placed where it's used.

Undo seem logically best placed with chain.* though it's a bit different
since undo data (like the compressor) is only locally significant, most of
chain.* is actually locally specific storage stuff for the chain in any
case.

CChain and CTxUndo don't depend on each other, why should they go together?
About moving it to main...I am of the opinion that main should be
dismembered in many small pieces and this wouldn't help to move in that
direction.
I'm generally in favor of many small files over a few big ones. It is more
readable to me and it's also more flexible for later refactors.

jtimon · 2014-10-18T17:34:52Z

Oh, and coins depends on CTxInUndo but not in chain or main.

jtimon · 2014-10-18T18:11:29Z

Mhmm, blockundo can get out of main too.
Should txundo and blockundo go together in undo.h or each separated (ie undotx and undoblock) so that coins doesn't have to include undoblock ?

sipa · 2014-10-18T18:18:25Z

So undo data (CTxInUndo, CTxUndo, CBlockUndo) is is not directly depended on by chain, but CBlockIndex (in chain) contains references to where undo data is on disk, which makes it sort-of related. I'm fine with a separate file too.

To avoid coins from depending on undo (or chain), CCoins::Spend could become a method (or function) on CTxInUndo. I think that method doesn't logically belong in coins anyway.

jtimon · 2014-10-18T18:35:29Z

Ok, so here's the plan:

Decouple coins from undo as suggested by @sipa
Move txundo to undo
Move TxCompressor and script/compressor to coins
Move blockundo to undo

I also want to decouple txout from feerate and move it out of core/transaction (to policy/fee ?) but I'll leave that for a later PR.

Diapolo · 2014-10-18T19:52:46Z

src/txcompressor.cpp

@@ -0,0 +1,59 @@
+// Copyright (c) 2012-2013 The Bitcoin developers


Nit: Why only 2013?

sipa · 2014-10-19T08:09:42Z

Can you move CBlockUndo to undo as well?

Diapolo · 2014-10-19T10:37:07Z

src/core/block.h

+// Distributed under the MIT/X11 software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_CORE_H


Nit: Rename and edit end comment?

Diapolo · 2014-10-19T10:50:50Z

Suggestion: After this is confirmed to be move-only etc. create a last pull, which does the clang-formatting!?

jtimon · 2014-10-20T02:08:24Z

I ended up starting with moving CFeeRate and the Amount constants to amount.o so I had to rebase and I included the nits. I added a last clang commit as suggested.
CBlockUndo depends on CDiskBlockPos, so I'll leave that for another PR.
There's something that is not purely move-only, so someone reviewing the move-only should notice.

sipa · 2014-10-20T02:54:26Z

An alternative would be moving the undo stuff to chain.h (it's about auxiliary data about chain validation). I prefer to have all undo stuff together :)

jtimon · 2014-10-23T00:53:24Z

I needed to rebase on top of master due to some minor include conflicts.
Then I needed to fix some missing includes: git is smart but not enough to figure includes by itself.
The latest couple of commits should solve it.

theuni · 2014-10-23T04:27:30Z

ACK

sipa · 2014-10-27T12:10:50Z

The clang fix included here makes the result much harder to review. Can you remove that? We'll do the clang fixes all at once before release, without mixing it with other changes. Sorry, @Diapolo - we'll get to it!

sipa · 2014-10-27T12:14:01Z

Also, feel free to rebase & squash. As long as all commits are move-only, it's easy to check with git diff merge~:src..merge:dest.

…or (from core) with it

jtimon · 2014-10-27T12:57:40Z

Left the clang change out. Not sure if it's worth it to make a separate PR though. Aren't we applying clang to everything just before 0.10 ?

sipa · 2014-10-27T13:00:05Z

@jtimon yes, that's what I meant - let's not bother with clang fixes now, as we'll do it all at once right before the 0.10 release.

sipa · 2014-10-27T13:08:19Z

ACK - verified move-only. Do you plan on moving CBlockUndo to undo.h as well (after turning its read/write methods into main.cpp functions) ?

jtimon · 2014-10-27T13:11:50Z

sipa Since too many of my open PRs depend this I plan to do that in a later PR.

sipa · 2014-10-27T13:12:33Z

Sure, a later PR sounds good.

jtimon · 2014-10-27T15:19:31Z

Done, CBlockUndo is moved to undo.h as part of #5111.

TheBlueMatt · 2014-10-27T22:59:41Z

src/undo.h

+#ifndef H_BITCOIN_TXUNDO
+#define H_BITCOIN_TXUNDO
+
+#include "compressor.h" 


You have a trailing space here.

TheBlueMatt · 2014-10-27T23:01:42Z

sipa · 2014-10-28T08:19:10Z

@laanwj comments?

laanwj · 2014-10-28T12:07:14Z

utACK

99f41b9 MOVEONLY: core.o -> core/block.o (jtimon) 561e9e9 MOVEONLY: Move script/compressor out of script and put CTxOutCompressor (from core) with it (jtimon) 999a2ab MOVEONLY: separate CTxUndo out of core (jtimon) 4a3587d MOVEONLY: Separate CTransaction and dependencies from core (jtimon) eda3733 MOVEONLY: Move CFeeRate and Amount constants to amount.o (jtimon)

luke-jr · 2014-10-30T01:35:29Z

$ git pull
remote: Counting objects: 255, done.
remote: Compressing objects: 100% (163/163), done.
remote: Total 255 (delta 131), reused 149 (delta 92)
Receiving objects: 100% (255/255), 385.22 KiB | 62.00 KiB/s, done.
Resolving deltas: 100% (131/131), done.
From git://github.com/bitcoin/bitcoin
   2ffdf21..723c752  master     -> origin/master
Updating 2ffdf21..723c752
error: The following untracked working tree files would be overwritten by merge:
    src/core
    src/core
    src/core
    src/core

I think it's a bad practice to name files/directories simply "core" (this breaks automatic coredumps on many systems) - can we call it something else?

gmaxwell · 2014-10-30T01:38:29Z

On Unix-like systems core is a semi-magical file name since coredumps get written to it.

jtimon · 2014-10-30T10:56:26Z

I suspect this wouldn't be a problem if we didn't had executables at /src but...
What about "datatypes", "core_structs", "serializable" or something along these lines?
We should have bikeshed before but we need to do it now as well...

luke-jr · 2014-10-30T11:20:24Z

datatypes +1

9670a84 Move UndoWriteToDisk() and UndoReadFromDisk() to anon namespace (random-zebra) 4fe609f Decouple CBlockUndo from CDiskBlockPos (jtimon) 9703b0e Decouple miner.o and txmempool.o from CTxUndo (random-zebra) cdc8cdc Decouple CCoins from CTxInUndo (jtimon) Pull request description: Backport bitcoin#5111 Original description: > Built on top of bitcoin#5100. > coins.o doesn't need to depend on undo.h (Decouple CCoins from CTxInUndo) > miner.o neither (Decouple miner.o from CTxUndo) > Finally, CBlockUndo can be moved from main.h to undo.h if it is first decoupled from CDiskBlockPos. ACKs for top commit: furszy: utACK 9670a84 Fuzzbawls: re-utACK 9670a84 Tree-SHA512: d77110525f50a3085013ca9cb87550ff786ee06c5b938a9002fe3a434c7a35a425238d1b623a01c26bebb5075c7e1bea9bc2ff4638c0c1190df15196b7097e55

jtimon closed this Oct 18, 2014

jtimon reopened this Oct 18, 2014

jtimon force-pushed the core branch 2 times, most recently from d0fe1cb to 5a3ff87 Compare October 18, 2014 17:39

Diapolo reviewed Oct 18, 2014
View reviewed changes

src/txcompressor.cpp

@@ -0,0 +1,59 @@

// Copyright (c) 2012-2013 The Bitcoin developers

Copy link

Diapolo Oct 18, 2014

Choose a reason for hiding this comment

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

Nit: Why only 2013?

jtimon force-pushed the core branch 2 times, most recently from 13d22e9 to 4f8d319 Compare October 19, 2014 07:41

Diapolo reviewed Oct 19, 2014
View reviewed changes

jtimon force-pushed the core branch 2 times, most recently from 9c5f844 to 3004ad3 Compare October 20, 2014 02:02

jtimon force-pushed the core branch from 3004ad3 to 09bc944 Compare October 20, 2014 02:23

jtimon force-pushed the core branch from 09bc944 to 1d0f7a0 Compare October 20, 2014 22:17

jtimon force-pushed the core branch from 066f4c7 to a4d4331 Compare October 27, 2014 12:33

jtimon added 5 commits October 27, 2014 13:54

MOVEONLY: Move CFeeRate and Amount constants to amount.o

eda3733

MOVEONLY: Separate CTransaction and dependencies from core

4a3587d

MOVEONLY: separate CTxUndo out of core

999a2ab

MOVEONLY: Move script/compressor out of script and put CTxOutCompress…

561e9e9

…or (from core) with it

MOVEONLY: core.o -> core/block.o

99f41b9

jtimon force-pushed the core branch from a4d4331 to 99f41b9 Compare October 27, 2014 12:56

TheBlueMatt reviewed Oct 27, 2014
View reviewed changes

src/undo.h

#ifndef H_BITCOIN_TXUNDO

#define H_BITCOIN_TXUNDO

#include "compressor.h"

Copy link

Contributor

TheBlueMatt Oct 27, 2014

Choose a reason for hiding this comment

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

You have a trailing space here.

sipa merged commit 99f41b9 into bitcoin:master Oct 28, 2014

random-zebra mentioned this pull request Jul 13, 2020

[Core] Only include undo.h from main.cpp PIVX-Project/PIVX#1746

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -0,0 +1,59 @@
		// Copyright (c) 2012-2013 The Bitcoin developers

MOVEONLY: Split Transaction and Block from core #5100

MOVEONLY: Split Transaction and Block from core #5100

Uh oh!

Conversation

jtimon commented Oct 18, 2014

Uh oh!

theuni commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

sipa commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

gmaxwell commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

sipa commented Oct 18, 2014

Uh oh!

jtimon commented Oct 18, 2014

Uh oh!

Diapolo Oct 18, 2014

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 19, 2014

Uh oh!

Diapolo Oct 19, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Oct 19, 2014

Uh oh!

jtimon commented Oct 20, 2014

Uh oh!

sipa commented Oct 20, 2014

Uh oh!

jtimon commented Oct 23, 2014

Uh oh!

theuni commented Oct 23, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

jtimon commented Oct 27, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

jtimon commented Oct 27, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

jtimon commented Oct 27, 2014

Uh oh!

TheBlueMatt Oct 27, 2014

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 27, 2014

Uh oh!

sipa commented Oct 28, 2014

Uh oh!

laanwj commented Oct 28, 2014

Uh oh!

luke-jr commented Oct 30, 2014

Uh oh!

gmaxwell commented Oct 30, 2014

Uh oh!

jtimon commented Oct 30, 2014

Uh oh!

luke-jr commented Oct 30, 2014

Uh oh!

Uh oh!