[Core] Split some classes and const definitions out of main #1209

barrystyle · 2019-12-18T13:45:41Z

In reference to #1098

random-zebra · 2019-12-22T13:58:20Z

Thanks @barrystyle for doing this work! This is really great.
I agree with the plan set in 1098, to do this overhaul step, by step in successive pull request (and use 1098 as a reference for tracking).
On that note, I think this one is ready to be reviewed and merged.
But before being able to do that, the unit tests and the CMake builds need to be updated.
For the former, you should add #include "consensus/tx_verify.h" in the following files

script_P2SH_tests.cpp
transaction_tests.cpp
sighash_tests.cpp

For the latter, instead, it should be sufficient to add ./src/consensus/tx_verify.cpp in CMakeLists.txt (in the COMMON_SOURCES group).

barrystyle · 2019-12-22T14:05:12Z

:) Cheers, i did wonder why travis failed.. and now i'm enlightened, will fix these up now.

random-zebra

Just couple notes about variables not being used anymore (or yet).

src/consensus/consensus.h

src/amount.cpp

src/amount.h

src/chainparams.cpp

src/chainparams.h

barrystyle · 2019-12-23T12:04:37Z

Excellent; I did notice the issue with statically declaring the coinbase maturity (after the fact as always) - it being a perfect entrypoint for the next commit. I was hoping to introduce the Consensus namespace to mitigate this..

Fuzzbawls

simple change to get CI builds working

src/Makefile.am

src/chainparams.cpp

barrystyle · 2020-01-13T13:48:15Z

Am I able to get this rebuilt? It seems to have worked last time.. just hit a timeout.

Fuzzbawls · 2020-01-15T07:57:41Z

this actually needs a rebase now after recent merges to fix merge conflicts.

git fetch origin # assuming the 'origin' remote is this https://github.com/PIVX-Project/PIVX.git repo
git pull --rebase origin master # same assumption as above

Note: if this repo is setup as a remote other than origin, substitute it's name in the above commands

note that the src/test/zerocoin_chain_tests.cpp file has been removed, and you will need to make an additional change to src/qt/transactionrecord.cpp after all conflicts are resolved:

Index: src/qt/transactionrecord.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/qt/transactionrecord.cpp	(revision 877708a3967fcb275d0e08987beef7256fb65a04)
+++ src/qt/transactionrecord.cpp	(date 1579074390286)
@@ -462,7 +462,7 @@
     int depth = 0;
     bool isTrusted = wtx.IsTrusted(depth, fConflicted);
     const bool isOffline = (GetAdjustedTime() - wtx.nTimeReceived > 2 * 60 && wtx.GetRequestCount() == 0);
-    int nBlocksToMaturity = (wtx.IsCoinBase() || wtx.IsCoinStake()) ? std::max(0, (Params().COINBASE_MATURITY() + 1) - depth) : 0;
+    int nBlocksToMaturity = (wtx.IsCoinBase() || wtx.IsCoinStake()) ? std::max(0, (Params().GetConsensus().CoinbaseMaturity() + 1) - depth) : 0;
 
     status.countsForBalance = isTrusted && !(nBlocksToMaturity > 0);
     status.cur_num_blocks = chainHeight;

feel free to ask for assistance if you encounter difficulties during the rebase.

barrystyle · 2020-01-15T17:12:53Z

Rebased and squashed; it seems to have pulled your last commit in for some reason.. easy fix?

eca9806013da49274d9b3f8b40c09002d8503f05 Use new function name for coinbase maturity; after rebase over 4.0.2 dec93ff3480b85dd01b56ea65bc191f28a06c670 Correct build config 3553a3e3f9d8cad19c7e3af029355e71b1fd6944 Move all zerocoin-related checks/functions to the consensus folder 8612a8ad6377a257910834896ec29f3fb4372903 Swap to newer method for genesis calculation; introduce Consensus namespace 3f19ccc5987a4acd4bfcd23070865828c50f4401 Missed includes and entry in CMakeLists.txt allowing tests to run c6924f3ecbac8d075ebba80e2dfac8e17bdfbdf4 Move transaction checks out to consensus/tx_verify.cpp 2534662c944af71e200aa54f536b4312e4ba2a29 Move CDiskTxPos/CBlockUndo to txdb.h/undo.h respectively 2ab2e18c307631b04ecad5a490ee044068e0a5fd Move CValidationState class/messages to consensus folder 19d5af718667ecafed5a48127f85c82102099c9f Split some classes and const definitions out

barrystyle · 2020-01-18T10:38:03Z

:D resolved.. howto mergeing?? aha

Fuzzbawls · 2020-01-19T00:51:03Z

Looks like something is messing with the regression testing here

barrystyle · 2020-01-22T09:53:42Z

Ready for merge?

@barrystyle

…of main f578a68 Move CValidationState to consensus subdir (Fuzzbawls) 083cc4d Use AbortNode instead of state.Abort (Fuzzbawls) ad77d3d Move consensus constants to consensus.h (Fuzzbawls) c210d20 Move CBlockTemplate from main.h to miner.h (Fuzzbawls) 4d8b9ce Move CBlockFileInfo class from main.h to chain.h (Fuzzbawls) 668ac61 Refactor nMaxMoneyOut back to amount.h (Fuzzbawls) Pull request description: This is the first part of #1209 that moves the CBlockFileInfo and CValidationState classes (and their supporting constants) out of `main.cpp(.h)` @barrystyle's original commits have been further split to assist in review-ability ease. I left the refactor of `nMaxMoneyOut` and `CBlockTemplate` included here as they are rather straight forward, however they each now are in their own respective commits. ACKs for top commit: random-zebra: utACK f578a68 furszy: utACK f578a68 Tree-SHA512: 4433aacde02be0479d6c8ac3d6d4d725c4ed5518c82a412539e791f62b63516f042828c0407a697b4018c8f92965425f05837293a38d6da890a98bdf0e4e9dfe

@barrystyle

…espectively fe2af5e Move CDiskTxPos/CBlockUndo to txdb.h/undo.h respectively (barrystyle) Pull request description: This is PR is the second part of breaking up #1209 into individual narrow-focused PRs. Original commit was cherry-picked from @barrystyle's initial pre-squashed branch (reference branch is at https://github.com/Fuzzbawls/PIVX/commits/pr-1209) Here `CDiskTxPos` and `CBlockUndo` are moved out of `main.h` into more appropriate files, namely `txdb.h` and `undo.h`. ACKs for top commit: furszy: utACK fe2af5e random-zebra: utACK fe2af5e and merging... Tree-SHA512: 57c7d55da931338b5212f412cadca639f2e1a9b90ea47749bf7e0c981af1091d2cb065cb8e1a6592db94b06085e59f6271b259b4a328ded8afcaf9d7bed089cf

@barrystyle

…erify.cpp 1e0bc16 Move transaction checks out to consensus/tx_verify.cpp. (barrystyle) Pull request description: This is PR is the third part of breaking up #1209 into individual narrow-focused PRs. Original commit(s) were cherry-picked from @barrystyle's initial pre-squashed branch and then re-squashed (reference branch is at https://github.com/Fuzzbawls/PIVX/commits/pr-1209) Input commits: Fuzzbawls@90950f1 Fuzzbawls@10b7566 --------------- This Moves the basic (non-zerocoin) transaction validation checks away from main.h(cpp) and into the `consensus` subdir. Code move only, no changes to functionality. This PR conflicts minorly with #1319, and will need to be rebased once that PR is merged, but reviewing is possible now as the conflicts are mostly just matter-of-fact conflicts due to both PRs touching a common file. Relies on: - [x] #1319 ACKs for top commit: furszy: utACK 1e0bc16 random-zebra: utACK 1e0bc16 and merging... Tree-SHA512: aa82d6f473d7ff6b65f04e590eb5dcfbcb5c1f22635bc22f0204ef3c23adcca99f95fc16d0b838c7358d6b832a4c55d43bf5f255902ea7fea4f5ff6216ea1423

@barrystyle

23c9b6c Swap to newer method for genesis calculation; introduce Consensus namespace.. (barrystyle) Pull request description: This is PR is the forth part of breaking up #1209 into individual narrow-focused PRs. Original commit(s) were cherry-picked from @barrystyle's initial pre-squashed branch and then re-squashed (reference branch is at https://github.com/Fuzzbawls/PIVX/commits/pr-1209) Input Commits: Fuzzbawls@041c9ee Fuzzbawls@5d7e61c Fuzzbawls@cf696e4 Fuzzbawls@18ab8ef Fuzzbawls@0a8338b Note: I didn't retain any transitional or "fixup" commits, so this PR is a single commit which takes the overall changeset of the above commits, along with some cleanup of unused/unneeded code that was in the original overall changeset (not-implemented BIP9 chain parameters, for example). ------------------- This PR does two things: 2. Establishes the `Consensus` namespace skeleton, with a minimum number of chain parameters moved into the new namespace. Additional PRs should be opened after this is merged that handle the moving of other consensus-specific chain parameters in a concise and narrow-focused manner. 2. Refactors the method for initializing the genesis block on each chain (Mainnet, Testnet, Regtest) to be a function call rather than an in-class manual crafting of the block. ACKs for top commit: random-zebra: ACK 23c9b6c furszy: ACK 23c9b6c Tree-SHA512: dae6e1d887ade18855c9f1d16f625ec6d6e1cab890fe50eb045835798bab5df55475cdb6ec2e41326e48c568649b13f5e13adc49d21cba097feda2193fd28970

furszy · 2020-02-22T14:58:00Z

can we close this? I think that it was done in other PRs.

random-zebra · 2020-02-22T18:06:57Z

Yep, closed.

bd49362 [Refactor] Move zerocoin checks out of main (Fuzzbawls) Pull request description: This is PR is the final part of breaking up #1209 into individual narrow-focused PRs. For this one, the original commit had too many merge conflicts, so it was easier/faster for me to just start from a clean slate and re-do the commit. (reference branch is at https://github.com/Fuzzbawls/PIVX/commits/pr-1209) Reference Commit: Fuzzbawls@547ca6c This moves all zerocoin checking methods out of main.cpp/h into two new files in the `consensus` subdir: zerocoin_verify.cpp/h ACKs for top commit: random-zebra: utACK bd49362 furszy: rebase utACK bd49362. Tree-SHA512: 2ea66411d17bd7a6659243540ff060b7613377bff09633a4d743ccd78fe03efa146c93ff05e68f2347e3202ef5d22c14ca336abb35cc0b7007814e490c0517d6

barrystyle mentioned this pull request Dec 18, 2019

Split main.cpp into net_processing/validation as per upstream #1098

Closed

random-zebra assigned barrystyle Dec 21, 2019

random-zebra reviewed Dec 23, 2019

View reviewed changes

src/consensus/consensus.h Outdated Show resolved Hide resolved

src/amount.cpp Outdated Show resolved Hide resolved

src/amount.h Outdated Show resolved Hide resolved

src/chainparams.cpp Outdated Show resolved Hide resolved

src/chainparams.h Outdated Show resolved Hide resolved

random-zebra changed the title ~~Split some classes and const definitions out.~~ [Core] Split some classes and const definitions out of main Dec 23, 2019

random-zebra added Backport Cleanup labels Dec 23, 2019

random-zebra added this to the 4.1.0 milestone Dec 23, 2019

Fuzzbawls requested changes Jan 3, 2020

View reviewed changes

src/Makefile.am Outdated Show resolved Hide resolved

random-zebra suggested changes Jan 3, 2020

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

src/chainparams.cpp Outdated Show resolved Hide resolved

barrystyle requested a review from random-zebra January 3, 2020 19:01

Further rework/includes required after 4.0.2 rebase

5a44e28

barrystyle added 2 commits January 19, 2020 18:49

Ensure all additional environment settings are sane (testnet/regtest).

27db884

Remove training wheels from chainparams.cpp for genesis creation.

63f3d79

Fuzzbawls mentioned this pull request Jan 24, 2020

[Refactor] Move CBlockFileInfo and CValidationState out of main #1302

Merged

This was referenced Feb 1, 2020

[Refactor] Move CDiskTxPos/CBlockUndo to txdb.h/undo.h respectively #1319

Merged

[Refactor] Move transaction checks out to consensus/tx_verify.cpp #1320

Merged

random-zebra removed the Backport label Feb 7, 2020

random-zebra added the Upstream label Feb 7, 2020

Fuzzbawls mentioned this pull request Feb 11, 2020

[Refactor] Introduce Consensus namespace #1328

Merged

Fuzzbawls mentioned this pull request Feb 19, 2020

[Refactor] Move zerocoin checks out of main #1342

Merged

random-zebra closed this Feb 22, 2020

lyricidal mentioned this pull request Sep 16, 2021

[Upstream] [Refactor] Define constant string variable for currency unit PRCYCoin/PRCYCoin#119

Merged

[Core] Split some classes and const definitions out of main #1209

[Core] Split some classes and const definitions out of main #1209

Uh oh!

Conversation

barrystyle commented Dec 18, 2019

Uh oh!

random-zebra commented Dec 22, 2019

Uh oh!

barrystyle commented Dec 22, 2019

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barrystyle commented Dec 23, 2019

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barrystyle commented Jan 13, 2020

Uh oh!

Fuzzbawls commented Jan 15, 2020

Uh oh!

barrystyle commented Jan 15, 2020

Uh oh!

barrystyle commented Jan 18, 2020

Uh oh!

Fuzzbawls commented Jan 19, 2020

Uh oh!

barrystyle commented Jan 22, 2020

Uh oh!

furszy commented Feb 22, 2020

Uh oh!

random-zebra commented Feb 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants