MOVEONLY: Split Transaction and Block from core

jtimon commented at 0:37 am on October 18, 2014: contributor

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 commented at 1:25 am on October 18, 2014: member

Missing some files.

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

jtimon commented at 2:22 am on October 18, 2014: contributor

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

jtimon closed this on Oct 18, 2014

sipa commented at 2:33 am on October 18, 2014: member

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 commented at 4:04 am on October 18, 2014: contributor

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 commented at 4:13 am on October 18, 2014: contributor

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 commented at 4:15 am on October 18, 2014: contributor

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

jtimon commented at 5:26 pm on October 18, 2014: contributor

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 commented at 5:34 pm on October 18, 2014: contributor

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

jtimon reopened this on Oct 18, 2014

jtimon force-pushed on Oct 18, 2014

jtimon commented at 6:11 pm on October 18, 2014: contributor

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 commented at 6:18 pm on October 18, 2014: member

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 commented at 6:35 pm on October 18, 2014: contributor

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.

in src/txcompressor.cpp: in 4b3b7b59e7 outdated

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

Diapolo commented at 7:52 pm on October 18, 2014:

Nit: Why only 2013?

in src/txcompressor.h: in 4b3b7b59e7 outdated

0@@ -0,0 +1,41 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2013 The Bitcoin developers

Diapolo commented at 7:53 pm on October 18, 2014:

Same here :).

in src/undo.h: in 4b3b7b59e7 outdated

0@@ -0,0 +1,71 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2013 The Bitcoin developers

Diapolo commented at 7:53 pm on October 18, 2014:

And here ^^.

jtimon commented at 7:46 am on October 19, 2014:

This is annoying. If you fix that in the whole project I will ACK.

sipa commented at 7:59 am on October 19, 2014:

ACK, me too.

Diapolo commented at 10:38 am on October 19, 2014:

I’m currently not going to add anymore pulls to this project, as I want to see how code looks like if no cleanup pulls are made, which (speaking of my pulls) only cause controversial discussions on uncontroversial changes. That said I only watch for such things and give nits/comments.

jtimon force-pushed on Oct 19, 2014

sipa commented at 8:09 am on October 19, 2014: member

Can you move CBlockUndo to undo as well?

in src/core/block.h: in 35b2c1b9af outdated

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

Diapolo commented at 10:37 am on October 19, 2014:

Nit: Rename and edit end comment?

in src/core/transaction.h: in 35b2c1b9af outdated

103@@ -109,8 +104,6 @@ class CTxIn
104     std::string ToString() const;
105 };
106 
107-

Diapolo commented at 10:40 am on October 19, 2014:

I also like these :), but don’t understand why no one is yelling, when you do whitespace changes.

Diapolo commented at 10:50 am on October 19, 2014: none

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

jtimon force-pushed on Oct 20, 2014

jtimon commented at 2:08 am on October 20, 2014: contributor

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.

jtimon force-pushed on Oct 20, 2014

sipa commented at 2:54 am on October 20, 2014: member

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 force-pushed on Oct 20, 2014

jtimon renamed this:
~~MOVEONLY: Separate CTransaction and dependencies from core~~
MOVEONLY: Split Transaction and Block from core
on Oct 21, 2014

theuni commented at 5:35 pm on October 21, 2014: member

Agree with the changes in general. Might be easier to drop the Amount change from here and do that as a separate pull. I’ll verify the code movement if/when there are a few more ACKs.

jtimon commented at 7:41 pm on October 21, 2014: contributor

I think the Amount constants belong in amount. Doing the Amount change here makes the PR more readable IMO, since otherwise you would move CFeeRate to core/transaction only to move it later to amount.

jtimon force-pushed on Oct 21, 2014

jtimon commented at 7:56 pm on October 21, 2014: contributor

Fixed alphabetial nits. Since nobody had verified the moveonly and clang changes yet I just rebased instead of piling up nit commits.

laanwj added the label Improvement on Oct 22, 2014

theuni commented at 11:13 pm on October 22, 2014: member

Verified move-only with one exception:

0-static const int64_t COIN = 100000000;
1-static const int64_t CENT = 1000000;
2+static const CAmount COIN = 100000000;
3+static const CAmount CENT = 1000000;

That seems fine to me, any reason it wasn’t done already?

theuni commented at 11:14 pm on October 22, 2014: member

clang changes verified as well. Note that some objects aren’t exactly equal because assert line-numbers changed.

jtimon commented at 11:53 pm on October 22, 2014: contributor

Great, thanks for verifying. Yes, those two int64_t -> CAmount were the non-move-only changes. Apparently they got missed in #4234. Re: misplaced squash? Probably, I rebased this too many times I guess. Sadly I have to rebase and the verification becomes outdated, but I can fix the CFeeRate::ToString() leak.

theuni commented at 11:56 pm on October 22, 2014: member

Go ahead, I’ve got your branch cached. I can just verify that it remains identical to your new push.

jtimon force-pushed on Oct 23, 2014

jtimon commented at 0:12 am on October 23, 2014: contributor

Rebased solving the CFeeRate::ToString() mistake. Ready for verification again.

theuni commented at 0:21 am on October 23, 2014: member

Heh, rebasing on top of master kinda defeats the purpose :p. I re-did the rebase locally though, and the diff is clean.

Verified move-only.

Edit: spoke too soon, looks like the rebase on master caused a build problem.

jtimon commented at 0:53 am on October 23, 2014: contributor

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 commented at 4:27 am on October 23, 2014: member

ACK

sipa commented at 12:10 pm on October 27, 2014: member

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 commented at 12:14 pm on October 27, 2014: member

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.

jtimon force-pushed on Oct 27, 2014

MOVEONLY: Move CFeeRate and Amount constants to amount.o eda3733091

MOVEONLY: Separate CTransaction and dependencies from core 4a3587d8db

MOVEONLY: separate CTxUndo out of core 999a2ab41e

MOVEONLY: Move script/compressor out of script and put CTxOutCompressor (from

core) with it

561e9e9de9

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

jtimon force-pushed on Oct 27, 2014

jtimon commented at 12:57 pm on October 27, 2014: contributor

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 commented at 1:00 pm on October 27, 2014: member

@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 commented at 1:08 pm on October 27, 2014: member

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 commented at 1:11 pm on October 27, 2014: contributor

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

sipa commented at 1:12 pm on October 27, 2014: member

Sure, a later PR sounds good.

jtimon commented at 3:19 pm on October 27, 2014: contributor

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

in src/undo.h: in 99f41b9cf7

0@@ -0,0 +1,71 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2013 The Bitcoin 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 H_BITCOIN_TXUNDO
7+#define H_BITCOIN_TXUNDO
8+
9+#include "compressor.h"

TheBlueMatt commented at 10:59 pm on October 27, 2014:

You have a trailing space here.

TheBlueMatt commented at 11:01 pm on October 27, 2014: member

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA1
 2
 3ACK commithash 99f41b9cf7b8e039cea75500a905498a1f6969f3 (-trailing space)
 4-----BEGIN PGP SIGNATURE-----
 5Version: GnuPG v2
 6
 7iQIcBAEBAgAGBQJUTs7GAAoJEIm7uGY+LmXOSsYP/2iM4ZuhSAowA7brZAwrzJCE
 8YvuhbiY5pVS0OyRtiudWpsHhmQVhB/Be19+YltOZduNo8fAPVRJ68yL/Y8fIlXAQ
 9ljy/UTNRMlbqKZB9I2W87VHP0aTZy7XlPlVwMP60JjOtXkNVVNzDh19Kytf2ukKP
10lEWo3UcxM2LTpI1YmXQxW9cRhkjgmY7DFQ6raC60b7BIZhgmRVJQRHC1c+iDJDF1
11yuIF39fFNS/SyCsdKmzq76Hj0BH2wX8T5+LWbxP1AkFjKmeN0Sxq2W1Z3Bdyj/qd
12dVFchpdWrVvmfXKh23L6dO6ZgCz523hDXBaRspLWZWPTK5VMiyQoDdulq+2luK2/
13odgSJ/K8oHKpJJcVgsak0JSAW88Aa7sRJZm8QgWi5Ru/vvFob81CCfw3fbjIDpz/
146yTv+XVaxbyODZ51iDwcmFzday6cA8o9L43DMbTvNIoN6S2rY3PAZIMN1oYhINQt
15ex+CFyV2esP9taZfI8y7JTgsIz5oj5xGGsefqv9zr7QuGZthKxx/cTdzdGdEWCgl
16FuEqisIQosLgO4nnx3Q5iRMhHoXNY0SLwrVJT1C07bzi+g8U5alSj+VgGqLa3i1I
17gQXpe4bMOfiCbfPDmcu5z+SBIQO2mGMAG7ctzJ9K8QUsXJBXQnPHVkU0QC9id4de
18XTpcHMUtWk3Nv6fFTkVo
19=NWC1
20-----END PGP SIGNATURE-----

sipa commented at 8:19 am on October 28, 2014: member

@laanwj comments?

laanwj commented at 12:07 pm on October 28, 2014: member

utACK

sipa merged this on Oct 28, 2014

sipa closed this on Oct 28, 2014

sipa referenced this in commit 723c752636 on Oct 28, 2014

luke-jr commented at 1:35 am on October 30, 2014: member

 0$ git pull
 1remote: Counting objects: 255, done.
 2remote: Compressing objects: 100% (163/163), done.
 3remote: Total 255 (delta 131), reused 149 (delta 92)
 4Receiving objects: 100% (255/255), 385.22 KiB | 62.00 KiB/s, done.
 5Resolving deltas: 100% (131/131), done.
 6From git://github.com/bitcoin/bitcoin
 7   2ffdf21..723c752  master     -> origin/master
 8Updating 2ffdf21..723c752
 9error: The following untracked working tree files would be overwritten by merge:
10    src/core
11    src/core
12    src/core
13    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 commented at 1:38 am on October 30, 2014: contributor

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

jtimon commented at 10:56 am on October 30, 2014: contributor

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 commented at 11:20 am on October 30, 2014: member

datatypes +1

furszy referenced this in commit 128978d45b on Jul 16, 2020

DrahtBot locked this on Sep 8, 2021

MOVEONLY: Split Transaction and Block from core #5100