[docs] fix findFork comment #15659

pull r8921039 wants to merge 1 commits into bitcoin:master from r8921039:fix_findFork_comment changing 1 files +6 −5
  1. r8921039 commented at 10:02 PM on March 24, 2019: contributor

    The return value of findFork is an ancestor of the specified block only when specified block is not on the active chain. When it is on the active chain, the return value is the specified block itself, not an ancestor of it.

  2. gmaxwell commented at 10:14 PM on March 24, 2019: contributor

    Return height of the highest block on the chain that is or has an ancestor

    "is or has an ancestor" doesn't make much sense to me. The function is returning the highest common point, if the block is on the chain, that's obviously itself. The comment in chain.h is correct and I think not confusing: Find the last common block between this chain and a block index entry.

  3. fanquake added the label Docs on Mar 24, 2019
  4. MarcoFalke requested review from ryanofsky on Mar 25, 2019
  5. r8921039 commented at 7:24 AM on March 25, 2019: contributor

    Similarly, the following comment is true only if the locator is not created by a block on the active chain.

        //! Return height of the latest block common to locator and chain, which
        //! is guaranteed to be an ancestor of the block used to create the
        //! locator.
        virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
  6. ryanofsky commented at 12:07 PM on March 25, 2019: member

    @r8921039, I wrote these comments and I can see how they are confusing. I wrote "ancestor" here when I really meant "ancestor-or-self" because it seemed less awkward (similar to how people write ⊂ instead of ⊆ in math for convenience, even though it can be less clear if you don't know the context).

    I think "highest block on the chain that is or has an ancestor of the specified block" is hard to parse, though. Maybe it would be better to change these comments to:

    • findFork: Return height of the specified block if it is on the chain, otherwise return the height of the highest block on chain that's an ancestor of the specified block, or nullopt if there is no common ancestor.
    • findLocatorFork: Return height of the highest block on chain in common with the locator (which will either be the original block used to create the locator, or one of its ancestors.)
  7. r8921039 commented at 5:13 PM on March 25, 2019: contributor

    @ryanofsky Thank you very much for your response and suggestions. The PR is updated accordingly.

  8. ryanofsky approved
  9. ryanofsky commented at 5:40 PM on March 25, 2019: member

    utACK b837d66f6eba294ea4fd996bbdedc53ba9eab97b

  10. MarcoFalke commented at 7:25 PM on March 25, 2019: member
  11. [docs] fix comment: the return value of findFork is _not_ an ancestor when the specified block is on the active chain
    update with suggested comment text from the reviewers
    c968780785
  12. r8921039 force-pushed on Mar 25, 2019
  13. r8921039 commented at 9:31 PM on March 25, 2019: contributor

    @MarcoFalke Yes sir, done as instructed.

  14. promag commented at 9:19 AM on March 26, 2019: member

    utACK c968780, however comment could be shorter.

  15. ryanofsky approved
  16. ryanofsky commented at 4:04 PM on March 29, 2019: member

    utACK c968780785c18bd6d0a8659c9e251ccf8fdc91dc. Only change since last review is squash

  17. r8921039 commented at 6:22 AM on April 6, 2019: contributor

    Would someone be willing to give a bit more love to this PR... Thanks.

  18. ryanofsky commented at 3:04 PM on April 9, 2019: member

    This could just be merged I think. Documentation update with no change in meaning.

  19. r8921039 commented at 5:40 AM on April 10, 2019: contributor

    @MarcoFalke Do we need more utACK to proceed? If you think so, I will try asking on IRC. Thanks.

  20. MarcoFalke merged this on Apr 10, 2019
  21. MarcoFalke closed this on Apr 10, 2019

  22. MarcoFalke referenced this in commit e3a0688f82 on Apr 10, 2019
  23. r8921039 commented at 5:01 PM on April 10, 2019: contributor

    Thanks a lot guys! :)

  24. r8921039 deleted the branch on Apr 15, 2019
  25. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 21:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me