[doc] Fix comment in FindForkInGlobalIndex #12951

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2018-04-11-findforkinglobalindex-doc changing 1 files +2 −1

jamesob commented at 6:17 PM on April 11, 2018: member

The comment erroneously implies that we're searching chainActive for the first block common to locator, but we're using the parameter chain.
MarcoFalke added the label Docs on Apr 11, 2018
MarcoFalke commented at 6:44 PM on April 11, 2018: member

C.f. 6db83db3eb96809da3e680464b152f82785e38
sipa commented at 6:53 PM on April 11, 2018: member

utACK
sdaftuar commented at 7:11 PM on April 11, 2018: member

utACK, but if we're going to improve the comment here, some explanation of what a CBlockLocator is supposed to look like might be helpful as well? (I guess the main point being that it's supposed to have more recent block hashes first.)

jamesob commented at 7:48 PM on April 11, 2018: member

@sdaftuar something like

diff --git a/src/validation.cpp b/src/validation.cpp
index c48f9fd..3fcbbab 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -263,7 +263,8 @@ CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& loc
 {
     AssertLockHeld(cs_main);

-    // Find the first block the caller has in the main chain
+    // Find the latest block common to locator and chain - we expect that
+    // locator.vHave is sorted descending by height.
     for (const uint256& hash : locator.vHave) {
         CBlockIndex* pindex = LookupBlockIndex(hash);
         if (pindex) {

sdaftuar commented at 7:53 PM on April 11, 2018: member

Looks good to me, thanks!

[doc] Fix comment in FindForkInGlobalIndex

The comment erroneously implies that we're searching `chainActive` for the
first block common to `locator`, but we're using the parameter `chain`.

0ef7b403d0

jamesob force-pushed on Apr 11, 2018
jamesob commented at 9:10 PM on April 11, 2018: member

Pushed.
promag commented at 9:19 AM on April 12, 2018: member

ACK 0ef7b40.

All calls to FindForkInGlobalIndex set chain = activeChain, but this comment is obvious more correct.
jnewbery commented at 1:15 PM on April 12, 2018: member

ACK 0ef7b403d0a203ae2251ea0b9b926c7f41d80d1d
jimpo commented at 5:47 AM on April 16, 2018: contributor

ACK 0ef7b40
fanquake commented at 6:37 AM on April 16, 2018: member

utACK 0ef7b40
fanquake commented at 6:37 AM on April 16, 2018: member

@sdaftuar Would you like to ACK the final change?
laanwj merged this on Apr 16, 2018
laanwj closed this on Apr 16, 2018
laanwj referenced this in commit 6df0c6cb41 on Apr 16, 2018
sdaftuar commented at 12:46 PM on April 16, 2018: member

Post-merge utACK :)
PastaPastaPasta referenced this in commit 8250928a1e on Jul 17, 2020
PastaPastaPasta referenced this in commit 7ecac10253 on Jul 17, 2020
PastaPastaPasta referenced this in commit fad4723a93 on Jul 19, 2020
PastaPastaPasta referenced this in commit fbc1bfd1d5 on Jul 21, 2020
jasonbcox referenced this in commit 96909c5381 on Sep 28, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels