[docs] PeerLogicValidation interface #12603

jamesob commented at 7:40 PM on March 5, 2018: member

Adds docs for PeerLogicValidation's public interface and two related functions.

fanquake added the label Docs on Mar 5, 2018

practicalswift commented at 8:25 PM on March 5, 2018: contributor

While you are changing this file, consider fixing this delete-non-virtual-dtor warning (clang-7):

In file included from init.cpp:12:
In file included from ./addrman.h:9:
In file included from ./netaddress.h:13:
In file included from ./serialize.h:16:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/memory:80:
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/bits/unique_ptr.h:78:2: warning: delete called on non-final 'PeerLogicValidation' that has virtual functions but non-virtual
      destructor [-Wdelete-non-virtual-dtor]
        delete __ptr;
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/bits/unique_ptr.h:268:4: note: in instantiation of member function 'std::default_delete<PeerLogicValidation>::operator()'
      requested here
          get_deleter()(__ptr);
          ^
init.cpp:76:38: note: in instantiation of member function 'std::unique_ptr<PeerLogicValidation, std::default_delete<PeerLogicValidation> >::~unique_ptr' requested here
std::unique_ptr<PeerLogicValidation> peerLogic;
                                     ^
1 warning generated.

In file included from test/test_bitcoin.cpp:5:
In file included from ./test/test_bitcoin.h:8:
In file included from ./chainparamsbase.h:8:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/memory:80:
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/bits/unique_ptr.h:78:2: warning: delete called on non-final 'PeerLogicValidation' that has virtual functions but non-virtual
      destructor [-Wdelete-non-virtual-dtor]
        delete __ptr;
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/7.2.0/../../../../include/c++/7.2.0/bits/unique_ptr.h:268:4: note: in instantiation of member function 'std::default_delete<PeerLogicValidation>::operator()'
      requested here
          get_deleter()(__ptr);
          ^
test/test_bitcoin.cpp:63:15: note: in instantiation of member function 'std::unique_ptr<PeerLogicValidation, std::default_delete<PeerLogicValidation> >::~unique_ptr' requested here
TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
              ^
1 warning generated.

jamesob commented at 8:27 PM on March 5, 2018: member

@practicalswift thanks. Yeah, there're other code changes I'd like to make as well but I figured I'd keep this PR docs only for ease of review.

in src/net_processing.h:45 in 4ef01d394f outdated

  41 | @@ -42,13 +42,18 @@ class PeerLogicValidation : public CValidationInterface, public NetEventsInterfa
  42 |  public:
  43 |      explicit PeerLogicValidation(CConnman* connman, CScheduler &scheduler);
  44 |  
  45 | +    /** Evict orphan txn pool entries (EraseOrphanTx) based on a newly connected block */

laanwj commented at 8:29 PM on March 5, 2018:

Right now the formalations here confuse me a bit, because it's a function called BlockConnected which does something completely different. I think it'd make sense to mention in these comment which interface is implemented (e.g. CValidationInterface block connected), along with what the specific implementation does (which you added).

practicalswift commented at 8:34 PM on March 5, 2018: contributor

@jamesob Great! Feel free to ping me in your follow-up PR and I'll verify it against clang-7 :-)

jamesob force-pushed on Mar 5, 2018

jamesob commented at 8:45 PM on March 5, 2018: member

@laanwj thanks for the feedback, incorporated. @practicalswift generous offer -- will do.

laanwj commented at 9:10 PM on March 5, 2018: member

Thanks, looks good to me now ACK https://github.com/bitcoin/bitcoin/pull/12603/commits/46975bd2375dcbd932cf825c1b075b9b2ddc555f

randolf approved

randolf commented at 6:04 AM on March 6, 2018: contributor

High quality comments in code, like these, are always helpful.

practicalswift commented at 6:05 AM on March 6, 2018: contributor

ACK 46975bd2375dcbd932cf825c1b075b9b2ddc555f

in src/net_processing.cpp:406 in 46975bd237 outdated

 400 | @@ -401,6 +401,11 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
 401 |      }
 402 |  }
 403 |  
 404 | +/**
 405 | + * When a peer sends us a valid block, instruct it to announce blocks to us
 406 | + * using CMPCTBLOCK if possible. Trim lNodesAnnouncingHeaderAndIDs to a desired

ryanofsky commented at 3:30 PM on March 6, 2018:

I didn't get how the second sentence related at first. Could perhaps link it to the first part by saying something like "instruct it to announce blocks to us by adding nodeid to the end of the lNodesAnnouncingHeaderAndIDs list (and dropping the first node in the list if it would reach the max size)."

in src/net_processing.h:46 in 46975bd237 outdated

  41 | @@ -42,13 +42,34 @@ class PeerLogicValidation : public CValidationInterface, public NetEventsInterfa
  42 |  public:
  43 |      explicit PeerLogicValidation(CConnman* connman, CScheduler &scheduler);
  44 |  
  45 | +    /**
  46 | +     * Evict orphan txn pool entries (EraseOrphanTx) based on a newly connected block.

ryanofsky commented at 3:45 PM on March 6, 2018:

Could also mention that this saves the time of the last tip update.

in src/net_processing.h:58 in 46975bd237 outdated

  61 | +     */
  62 |      void BlockChecked(const CBlock& block, const CValidationState& state) override;
  63 | +    /**
  64 | +     * Maintain state about the best-seen block and fast-announce a compact block to compatible peers.
  65 | +     *
  66 | +     * Overridden from CValidationInterface.

ryanofsky commented at 4:02 PM on March 6, 2018:

You might want to consider splitting up these comments between the .cpp and .h files. The "Overridden from CValidationInterface" parts (which apply to these methods as a group) seem most useful here. But the other parts seem more useful for understanding the code inside in the methods, so maybe they should be moved to the implementation where they would be more visible and less likely to get out of date.

jamesob commented at 7:06 PM on March 6, 2018:

Yep, definitely agree here. Will fix.

fanquake requested review from sipa on Mar 6, 2018

ryanofsky commented at 7:02 PM on March 6, 2018: member

utACK 46975bd2375dcbd932cf825c1b075b9b2ddc555f. Looks good. I left a few additional suggestions, but this is a pure improvement already as it is.

jamesob force-pushed on Mar 6, 2018

Add documentation to PeerLogicValidation interface and related functions b7cd08b717

jamesob force-pushed on Mar 6, 2018

jamesob commented at 7:44 PM on March 6, 2018: member

Thanks for the comments, @ryanofsky. Incorporated and pushed.

ryanofsky commented at 8:44 PM on March 6, 2018: member

utACK b7cd08b717b57376d23ea550da60b00f239556db. Only changes since previous review were what was suggested.

I think this PR is ready to be merged. It has no code changes (comments only) and already a few acks.

laanwj merged this on Mar 6, 2018

laanwj closed this on Mar 6, 2018

laanwj referenced this in commit 85424d79ed on Mar 6, 2018

PastaPastaPasta referenced this in commit e13d883096 on Jun 13, 2020

PastaPastaPasta referenced this in commit 8cb071b46a on Jun 13, 2020

PastaPastaPasta referenced this in commit b55fcaad6a on Jun 14, 2020

PastaPastaPasta referenced this in commit 288e3b9818 on Jun 17, 2020

PastaPastaPasta referenced this in commit 5e6f63037c on Jun 17, 2020

PastaPastaPasta referenced this in commit d22f917da2 on Jun 17, 2020

PastaPastaPasta referenced this in commit 8adade97a4 on Jun 17, 2020

MarcoFalke locked this on Sep 8, 2021