Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate Consensus::CheckTxInputs and GetSpendHeight in CheckInputs #6061

Merged
merged 1 commit into from Jun 10, 2015

Conversation

@jtimon
Copy link
Member

jtimon commented Apr 25, 2015

A simple refactor as preparation for moving consensus to code for transaction validation.

Consensus shouldn't depend on std::vector<CScriptCheck> *pvChecks.

This is part of #6051 but can be merged independently.

@sipa
Copy link
Member

sipa commented Apr 28, 2015

Code change looks good to me, but why is CheckInputs not consensus, while CheckTxInputs is?

@jtimon
Copy link
Member Author

jtimon commented Apr 28, 2015

CheckInputs uses the CScriptCheck class which consensus doesn't need. I later plan to create Consensus::CheckTxInputsScripts that does the remaining and it's used directly instead of CheckInputs in some places (in miner and AcceptToMemPool, but not in AcceptBlock).
The following branch is outdated, but you can get an idea here jtimon@ddd505a

@laanwj laanwj added the Refactoring label May 12, 2015
src/main.h Outdated
@@ -507,4 +507,10 @@ extern CCoinsViewCache *pcoinsTip;
/** Global variable that points to the active block tree (protected by cs_main) */
extern CBlockTreeDB *pblocktree;

/**
* While checking, GetBestBlock() refers to the parent block. (protected by cs_main)

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 10, 2015

Member

This comment does not actually describe what the function does :)

This comment has been minimized.

Copy link
@jtimon

jtimon Jun 10, 2015

Author Member

Yeah, I just moved the comments from the cpp instead of moving the implementation of the new function. I've just added a line to the doc. Should I leave the rest here or move it back to the implementation?

…ight in CheckInputs
@jtimon jtimon force-pushed the jtimon:consensus_inputs branch from 69f64fe to eb83719 Jun 10, 2015
@jtimon
Copy link
Member Author

jtimon commented Jun 10, 2015

By the way, @laanwj (or anyone) feel free to propose additional edits to GetSpendHeight's documentation.

@laanwj laanwj merged commit eb83719 into bitcoin:master Jun 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj
Copy link
Member

laanwj commented Jun 10, 2015

ACK

laanwj added a commit that referenced this pull request Jun 10, 2015
eb83719 Consensus: Refactor: Separate Consensus::CheckTxInputs and GetSpendHeight in CheckInputs (Jorge Timón)
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Jun 17, 2020
…ht in CheckInputs

e27420e Separate Consensus::CheckTxInputs and GetSpendHeight in CheckInputs (furszy)

Pull request description:

  Coming from bitcoin#6061

  Refactor needed for an upcoming work, no functional changes.

  Had to do something little bit dirty to be able to get Consensus::Params from inside the Consensus namespace (struct `Params` name clashes with global method `Params()`) and not have any functional change there.

  Point of discussion for a later PR: could be moved to a function argument or check if there is another workaround to distinguish between the name clash.

ACKs for top commit:
  random-zebra:
    utACK e27420e
  Fuzzbawls:
    utACK e27420e

Tree-SHA512: 953921659a7ab41d954a8110d2c1b6911bc44b2b458ffbfd97f01262e4c946444bb6537081f34c4ba3ff6f5fca5e803b1d99a5fc84ff902f42a1c8f2956ff4d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.