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

Improve block validity/ConnectBlock() comments #7444

Merged
merged 1 commit into from Feb 3, 2016

Conversation

Projects
None yet
6 participants
@petertodd
Contributor

petertodd commented Jan 31, 2016

Previously didn't make clear that the ContextualCheckBlock* functions meant the block headers as context - not the UTXO set itself - and that ConnectBlock() also did UTXO-related validity checks (in the future we may split that functionality into a separate UTXO-specific contextual check block function).

Also, reordered to put validity checks first for better readability.

Noticed this nit while reviewing #7184

Improve block validity/ConnectBlock() comments
Previously didn't make clear that the ContextualCheckBlock* functions
meant the block headers as context - not the UTXO set itself - and that
ConnectBlock() also did UTXO-related validity checks (in the future we
may split that functionality into a separate UTXO-specific contextual
check block function).

Also, reordered to put validity checks first for better readability.

@laanwj laanwj added the Docs label Feb 1, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 1, 2016

Member

The reordering makes it harder to see what actually changed in the text :(

Member

laanwj commented Feb 1, 2016

The reordering makes it harder to see what actually changed in the text :(

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 1, 2016

Member

Maybe just do it without the reordering?

Member

MarcoFalke commented Feb 1, 2016

Maybe just do it without the reordering?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Feb 2, 2016

Contributor

What can I say, it is just two actual non-comment lines changed. :)

Contributor

petertodd commented Feb 2, 2016

What can I say, it is just two actual non-comment lines changed. :)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 2, 2016

Member

Yes, which would have been easier to see if the move and text change were separate commits.

Anyhow, these are the text changes:

--- a/src/main.h
+++ b/src/main.h
@@ -413,14 +413,18 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus
  *  of problems. Note that in any case, coins may be modified. */
 bool DisconnectBlock(const CBlock& block, CValidationState& state, const CBlockIndex* pindex, CCoinsViewCache& coins, bool* pfClean = NULL);

-/** Apply the effects of this block (with given index) on the UTXO set represented by coins */
+/** Apply the effects of this block (with given index) on the UTXO set represented by coins.
+ *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
+ *  can fail if those validity checks fail (among other reasons). */
 bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins, bool fJustCheck = false);

 /** Context-independent validity checks */
 bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true);
 bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true);

-/** Context-dependent validity checks */
+/** Context-dependent validity checks.
+ *  By "context", we mean only the previous block headers, but not the UTXO
+ *  set; UTXO-related validity checks are done in ConnectBlock(). */
 bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex *pindexPrev);
 bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex *pindexPrev);

ACK petertodd@2f19905

Member

laanwj commented Feb 2, 2016

Yes, which would have been easier to see if the move and text change were separate commits.

Anyhow, these are the text changes:

--- a/src/main.h
+++ b/src/main.h
@@ -413,14 +413,18 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus
  *  of problems. Note that in any case, coins may be modified. */
 bool DisconnectBlock(const CBlock& block, CValidationState& state, const CBlockIndex* pindex, CCoinsViewCache& coins, bool* pfClean = NULL);

-/** Apply the effects of this block (with given index) on the UTXO set represented by coins */
+/** Apply the effects of this block (with given index) on the UTXO set represented by coins.
+ *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
+ *  can fail if those validity checks fail (among other reasons). */
 bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins, bool fJustCheck = false);

 /** Context-independent validity checks */
 bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true);
 bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true);

-/** Context-dependent validity checks */
+/** Context-dependent validity checks.
+ *  By "context", we mean only the previous block headers, but not the UTXO
+ *  set; UTXO-related validity checks are done in ConnectBlock(). */
 bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex *pindexPrev);
 bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex *pindexPrev);

ACK petertodd@2f19905

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik
Contributor

paveljanik commented Feb 2, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 2, 2016

Member

ACK

Member

sipa commented Feb 2, 2016

ACK

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Feb 2, 2016

Contributor

@laanwj Fair enough; I'll split changes like that into two commits in the future.

Contributor

petertodd commented Feb 2, 2016

@laanwj Fair enough; I'll split changes like that into two commits in the future.

@laanwj laanwj merged commit 2f19905 into bitcoin:master Feb 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 3, 2016

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 14, 2016

Why 2f19905 is not contained in master history but 4cdbd42 is ?

jtimon commented on 2f19905 Feb 14, 2016

Why 2f19905 is not contained in master history but 4cdbd42 is ?

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Feb 14, 2016

Owner

@jtimon Odd, 2f19905, shows up in master for me.

Owner

petertodd replied Feb 14, 2016

@jtimon Odd, 2f19905, shows up in master for me.

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)

codablock added a commit to codablock/dash that referenced this pull request Dec 11, 2017

Merge #7444: Improve block validity/ConnectBlock() comments
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)

zkbot added a commit to zcash/zcash that referenced this pull request Mar 9, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 10, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 12, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Mar 12, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 6, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request May 31, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment