The PR branch HEAD was fa7eed5b at the time of this review club meeting.
Notes
Following an accidental chain split in 2013 (documented in BIP
50), a
fork warning system was added in PR
2658.
The feature was originally tested via the large reorg test (today found
in test/functional/feature_block.py). However it is no longer possible to
trigger the warning due to changes in the validation logic.
The validation logic to validate and potentially connect a new block is
handled in ProcessNewBlock(), which can be called from ProcessMessage()
in src/net_processing.cpp or from the submitblock RPC. ProcessNewBlock() runs basic
validation sanity checks and stores the block to disk. If the initial checks
pass, it calls ActivateBestChain() to (try to) connect the block to the
best chain.
What are some examples of when a chain split can happen? Which ones should
users be warned about?
ActivateBestChain() consists of two nested loops. What is the condition for
each loop to terminate?
The inner loop calls ActivateBestChainStep(), which disconnects blocks (in
case of a reorg) and connects a batch of up to 32 blocks toward the new tip.
When an invalid block is found, CheckForkWarningConditionsOnNewFork() is
called. Which block from the batch is passed to
CheckForkWarningConditionsOnNewFork()?
ActivateBestChainStep() will also call m_chain.SetTip(), which updates the
global reference to the active chain tip (::ChainActive().Tip()). What is
the maximum block height difference between pindexNewForkTip and pfork?
Is it possible to hit the condition that updates pindexBestForkTip?
What are your thoughts on the approach of the pull request? Should the bug
in the warning system be fixed or should the code be removed? What
alternative places/ways to implement the warning logic can you think of?
<MarcoFalke> Today we are talking about chain splits. What are some examples of when a chain split can happen? Which ones should users be warned about?
<stacie> BIP-50 describes a chain split as an unintended result of switching from Berkely DB to Level DB. Level DB was able to handle blocks with more tx inputs
<stacie> I noticed CBlockIndexWorkComparator returns true if the block passed in as the second parameter is the “better” block (based on the following criteria, in order: if it has more work, was received earlier (nSequenceId), or has a lower pointer address). My question is, is that how comparators typically work in C++? Return true if the second parameter is “better”?
<MarcoFalke> The inner loop calls ActivateBestChainStep(), which disconnects blocks (in case of a reorg) and connects a batch of up to 32 blocks toward the new tip. When an invalid block is found, CheckForkWarningConditionsOnNewFork() is called. Which block from the batch is passed to CheckForkWarningConditionsOnNewFork()?
<MarcoFalke> ActivateBestChainStep() will also call m_chain.SetTip(), which updates the global reference to the active chain tip (::ChainActive().Tip()). What is the maximum block height difference between pindexNewForkTip and pfork?
<MarcoFalke> michaelfolkson: The code is more clear on that. It means the fork tip is within 72 blocks of our current tip, but the fork is at least 7 blocks "heavy" (with pow)
<murch> Given that different difficulties can only appear across diff adjustments, and the adjustment is limited to a factor of four, why a factor of 10?
<jnewbery> imagine we've tried to connect 32 blocks in a loop in ActivateBestChainStep() and the 10th block is invalid. The pindex that we pass to CheckForkWarningConditionsOnNewFork() will be the first block in that batch of 32, which is a valid block
<lightlike> stacie_: I think that starting_tip is not updated once defined (it is the reference we compare to), but m_chain.Tip() ist updated inside of ConnectTip(), which is called from ActivateBestChainStep
<stacie_> catching up now - thank you sipa, trygiorshev and lightlike! That explains it. I didn't dive into the methods that ActivateBestChainStep calls but it makes sense now!
<jnewbery> so we've established that pindexNewForkTip and pfork can be at most one block apart, and the condition is that there's at least 7 block's worth of difference in their total work. Is that possible?
<MarcoFalke> Extra question: if the we passed the highest block of the to-be-connected chain instead, what would the height difference between pfork and pindexNewForkTip be?
<murch> jnewbery: no, because two competing blocks can only differ in difficulty in forks across a difficulty adjustment and a factor 7 won't be possible there
<MarcoFalke> What are your thoughts on the approach of the pull request? Should the bug in the warning system be fixed or should the code be removed? What alternative places/ways to implement the warning logic can you think of?
<michaelfolkson> I have a couple of unrelated questions. What tools are available to the user to react to the warning? Presumably if they became aware of a UASF like scenario they could join it
<jnewbery> MarcoFalke: I think that removing dead code and adding working code can be seen as two separate questions. I don't think "You should fix it" is a reasonable response to someone removing broken code.
<michaelfolkson> In response to your questions MarcoFalke you say in the PR comment "add a fork detection somewhere outside of the ActivateBestChainStep logic (maybe net_processing)."
<fjahr> It doesn't seem like the ideal way to solve this so removing and (possibly) reimplementing seems better than fixing which would make the PR more complicated. But I don't have great ideas how it should be done.
<jnewbery> I tend to think that if it wasn't working, then ripping it out and starting from scratch is safer. It shouldn't be the case, but tweaking code that wasn't working would probably receive less scrutiny in review than implementing something new
<michaelfolkson> I guess in terms of review the most important thing is to check that the code being ripped out isn't used elsewhere. Just for the warning
<jnewbery> Well done everyone. Today was a difficult PR to review. We went really deep into the consensus code. If you managed to follow along for the whole meeting you can give yourself a pat on the back :)