Improve block validity/ConnectBlock() comments #7444
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Member
|
The reordering makes it harder to see what actually changed in the text :( |
Member
|
Maybe just do it without the reordering? |
Contributor
Author
|
What can I say, it is just two actual non-comment lines changed. :) |
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); |
Contributor
Member
|
ACK |
Contributor
Author
|
@laanwj Fair enough; I'll split changes like that into two commits in the future. |
laanwj
added a commit
that referenced
this pull request
Feb 3, 2016
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Dec 11, 2017
2f19905 Improve block validity/ConnectBlock() comments (Peter Todd)
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Mar 10, 2018
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
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
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.
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
Oct 23, 2018
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Oct 24, 2018
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
Oct 25, 2018
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 5, 2019
Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - 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, and various other functions in `main.cpp` have `const CChainParams&` arguments added. Part of #2074.
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Apr 9, 2019
Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - 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, and various other functions in `main.cpp` have `const CChainParams&` arguments added. Part of #2074.
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
May 24, 2019
Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - 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, and various other functions in `main.cpp` have `const CChainParams&` arguments added. Part of #2074.
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
May 28, 2019
Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - 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, and various other functions in `main.cpp` have `const CChainParams&` arguments added. Part of #2074.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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