Track best-possible-headers (TheBlueMatt) #13937
Conversation
|
cc @TheBlueMatt, @MarcoFalke, @jamesob as this touches your work cc @jonasschnelli since you reviewed the original PR (@skeees did too, but only the commit that I dropped) |
97eb1f2 to
7b2a938
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| LOCK(cs_main); | ||
| auto it = setBlockIndexHeaderCandidates.rbegin(); | ||
| if (it == setBlockIndexHeaderCandidates.rend()) | ||
| return nullptr; |
There was a problem hiding this comment.
Numerous places call this function but no nullptr check is performed and dereferences are executed on the ptr.
Can this return null in any case where this function is called?
There was a problem hiding this comment.
That seems problematic indeed. There's an assert for this in https://github.com/bitcoin/bitcoin/blame/7b1ab22384d944f638c514f838e25b391204ef67/src/net_processing.cpp#L3308, but not in the calls from validation.cpp:
- in
NotifyHeaderTip(): here it's compared withpindexHeaderOldwhich is anullptrinitially, but would non-null the next time. It's not dereferenced in that function, but it is passed on touiInterface.NotifyHeaderTip. I could add an assert insideif (fNotify) - in
IsCurrentForFeeEstimation(): I could add anassertdirectly above the if statement that deferences it
The comment in InvalidBlockFound suggests that sometimes setBlockIndexHeaderCandidates can be empty, so the above wouldn't be enough. Rather than assert I could explicitly check for nullptr in both functions and deal with them, as well as put a warning where this set is defined.
| height = ::pindexBestHeader->nHeight; | ||
| block_time = ::pindexBestHeader->GetBlockTime(); | ||
| const CBlockIndex* best_header = ::GetBestHeader(); | ||
| if (best_header) { |
There was a problem hiding this comment.
Possible race condition here since we don't lock cs_main? Can the best_head data be deleted between time we call GetBestHeader() and when we use the ptr?
I'm not familiar with the code enough to know the answer to this so just putting it here in case.
54e17d2 to
7b1ab22
Compare
|
Rebased. I added commits to explicitly handle cases where I added a commit to make header count fall back to block height in That leaves
Original commit where I'm not sure if those are safe. Perhaps there should be a guarantee that |
When we find an invalid block, instead of adding BLOCK_FAILED_CHILD to its descendants in FindMostWorkChain, iterate setBlockIndexCandidates to find candidate descendants and mark them as BLOCK_FAILED_CHILD immediately, removing them from setBlockIndexCandidates as we go. This keeps BLOCK_FAILED_MASK entries out of setBlockIndexCandidates. This also adds a few checks to CheckBlockIndex, including one which checks that blocks with BLOCK_FAILED_CHILD are not, themselves, marked invalid, but have an invalid parent. This should be fine for most block indexes, however InvalidateBlock previously violated this. Luckily most users shouldn't be running with -checkblockindex Note that this introduces a bug where a block who's header was received but data was not when a ancestor was found to be invalid will not be marked BLOCK_FAILED_CHILD. Thus, when that block is received, it will be added to setBlockIndexCandidates, violating the new invariant. This is fixed in the next commit.
This mirrors setBlockIndexCandidates but for BLOCK_VALID_TREE instead of BLOCK_VALID_TRANSACTIONS && nTx. There are a few differences between the two to keep the new set practical, see code comments for more.
pindexBestHeader never considers the invalidity of a chain, only the validity of the header chain. Using it for sync estimation and GETHEADERS etc requests in net makes no sense. Instead, use the new setBlockIndexHeaderCandidates to find the best candidate tip.
Instead of allowing net_processing to inform AcceptBlock as to whether a block is interesting enough to commit to disk, use the new setBlockIndexHeadersCandidates to determine if they lead towards a chain with at least the same (or more) work as our current tip. This further decouples the maze in net_processing from our consensus anti-DoS logic.
99adad5 to
bdaf9f9
Compare
|
Rebased. I'll hold back on addressing specific feedback until there's some high level agreement on whether this is useful at all. |
| // to do here. | ||
| setBlockIndexHeaderCandidates.erase(it); | ||
| break; | ||
| parent_present = true; |
There was a problem hiding this comment.
This is dead code. The parent_present = true; is not meant to be after break; in this scope? :-)
| if ((*it)->GetAncestor(pindex->nHeight) == pindex) { | ||
| // pindex is useless - even if there are other tips based on it | ||
| // which we want in setBlockIndexHeaderCandidates, we're not | ||
| // gonna find them here. |
There was a problem hiding this comment.
"going to find them here." :-)
| * | ||
| * Works even if chainActive is empty! | ||
| * | ||
| * If chain_ordered_inertion, we assume that if pindex->pprev was previously a |
There was a problem hiding this comment.
Should be insertion?
| * | ||
| * Note that, unlike setBlockIndexCandidates, we only store "leaf" entries here, as we are not as | ||
| * aggressively prune-able (setBlockIndexCandidates are things which we can, and usually do, try to | ||
| * connect immediately, and thus entries dont stick around for long). Thus, it may be the case that |
| } | ||
| } | ||
| } | ||
| if (!fHasMoreOrSameWork && !parent_of_header_candidate) return true; // We dont think this block leads somewhere interesting |
|
Abandoning for now. Consider it up for grabs, but I suggest getting concept ACKs before trying to rebase. |
|
Since this is marked up for grabs, it might be useful to say what motivations for this change are. It looks like it was brought up in IRC on 12/1 in the context of "SPV sync" but the relationship seems unclear. (I believe "SPV sync" refers to downloading blocks out of order during initial sync so wallet can show unvalidated transactions and be usable before the sync finishes.) |
|
@ryanofsky it started out here: #9483 (comment), so just one step to make it easier to sync headers and fresh blocks first, and then catch up on historical blocks later. |
Rebase of #12138:
Not included:
Noteble rebase changes:
pindex_was_in_chainis still needed due to Only call NotifyBlockTip when chainActive changes #12431, in order to notify the gui about a new block tip, only if the active chain was modifiedqt/clientmodel.cppchanged significantly, because things were moved tointerfaces/node.cppProcessNewBlockbenchI added an additional commit, because the new
!fHasMoreOrSameWork && !parent_of_header_candidatecheck inAcceptBlo 8000 ckcan causesubmitblock(modified in #13439) to mistakingly believe the block is a duplicate.Added debug log statements to
rpc_preciousblock.pythat helped me figure out the above issue.