net, rpc: expose nLastBlockTime/nLastTXTime as last block/last_transaction in getpeerinfo #19731

pull jonatack wants to merge 5 commits into bitcoin:master from jonatack:add-eviction-criteria-to-getpeerinfo changing 5 files +52 −17
  1. jonatack commented at 10:23 am on August 15, 2020: member

    This PR adds inbound peer eviction criteria nLastBlockTime and nLastTXTime to CNodeStats and CNode::copyStats, which then allows exposing them in the next commit as last_transaction and last_block Unix Epoch Time fields in RPC getpeerinfo.

    This may be useful for writing missing eviction tests. I’d also like to add lasttx and lastblk columns to the -netinfo dashboard as described in #19643 (comment).

    Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549:

    0<jonatack> i was specifically trying to observe and figure out how to test [#19500](/bitcoin-bitcoin/19500/)
1<jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail
2<jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
3<jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
4<sipa> jonatack: nope; i suspect just nobody ever added it
5<jonatack> sipa: thanks. will propose.

    The last commit is optional, but I think it would be good to have logging in rpc_net.py.

  2. net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats 02fbe3ae0b
  3. in test/functional/rpc_net.py:166 in 5c1ec869cb outdated 
    162+        if peer_info[1][0]['last_transaction'] == 0:
163+            assert_equal(peer_info[1][0]['last_transaction'], 0)
164+            assert_approx(peer_info[1][1]['last_transaction'], time_now, vspan=60)
165+        else:
166+            assert_equal(peer_info[1][1]['last_transaction'], 0)
167+            assert_approx(peer_info[1][0]['last_transaction'], time_now, vspan=60)
    jonatack commented at 10:29 am on August 15, 2020:
    Note, the assert_approx assertions pass locally for me with vspan values < 5 (seconds). Made them 60 to leave a wide margin for avoiding data races. There shouldn’t be any cost to doing so; the values under test should either be 0 or the current unix time.
  4. laanwj commented at 11:07 am on August 15, 2020: member

    ACK on the C++ code change

    I’ll leave it up to @MarcoFalke to review the test, it might be a time dependency like this without using mocking makes the test too brittle / time dependent.

  5. jonatack force-pushed on Aug 15, 2020
  6. jonatack commented at 11:12 am on August 15, 2020: member

    ACK on the C++ code change

    I’ll leave it up to @MarcoFalke to review the test, it might be a time dependency like this without using mocking makes the test too brittle / time dependent.

    Agree, I’m not sure here either. Edit: loosened the tests to not be time-dependent.

  7. jonatack force-pushed on Aug 15, 2020
  8. in test/functional/rpc_net.py:157 in 062614fd16 outdated 
    153+            self.nodes[1].generate(1)
154+            self.sync_all()
155+        time_now = int(time.time())
156         peer_info = [x.getpeerinfo() for x in self.nodes]
157+        # Verify last_block and last_transaction values.
158+        for x in range(self.num_nodes):
    jnewbery commented at 11:45 am on August 15, 2020:
    Consider using itertools.product() rather than a triple-nested for loop here. It’d also be helpful if there were more meaningful variable names than x and y.
    jonatack commented at 1:28 pm on August 15, 2020:
    Good idea – done. It’s much nicer now.
  9. in src/rpc/net.cpp:104 in 062614fd16 outdated 
     99@@ -100,6 +100,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
100                             {RPCResult::Type::BOOL, "relaytxes", "Whether peer has asked us to relay transactions to it"},
101                             {RPCResult::Type::NUM_TIME, "lastsend", "The " + UNIX_EPOCH_TIME + " of the last send"},
102                             {RPCResult::Type::NUM_TIME, "lastrecv", "The " + UNIX_EPOCH_TIME + " of the last receive"},
103+                            {RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last transaction accepted and relayed by this peer"},
104+                            {RPCResult::Type::NUM_TIME, "last_block", "The " + UNIX_EPOCH_TIME + " of the last block accepted and relayed by this peer "},
    jnewbery commented at 11:53 am on August 15, 2020:

    I don’t think ‘accepted and relayed by’ is quite right:

    For last_transaction, it’s the time of the last transaction that we received and accepted into our mempool. I think the time of the last valid transaction received from this peer is accurate. For last_block, it’s the time of the last block that we received and saved to disk (even if we don’t connect the block or it eventually fails connection). I think the time of the last block received from this peer is fine here.

    jonatack commented at 12:05 pm on August 15, 2020:
    Thanks @jnewbery! I was just reconsidering those as well. Updating.
    jonatack commented at 10:22 am on August 17, 2020:
    Unresolving to make the head comment visible for reviewers and future code doc writers.
  10. DrahtBot added the label P2P on Aug 15, 2020
  11. DrahtBot added the label RPC/REST/ZMQ on Aug 15, 2020
  12. DrahtBot added the label Tests on Aug 15, 2020
  13. rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction 8a560a7d57
  14. test: getpeerinfo last_block and last_transaction tests 21c57bacda
  15. test: rpc_net.py logging and test naming improvements cfef5a2c98
  16. doc: release note for getpeerinfo last_block/last_transaction 5da96210fc
  17. jonatack force-pushed on Aug 15, 2020
  18. jonatack commented at 1:30 pm on August 15, 2020: member
    Updated with @jnewbery feedback (thanks!)
  19. in doc/release-notes-19731.md:5 in 5da96210fc 
    0@@ -0,0 +1,6 @@
1+Updated RPCs
2+------------
3+
4+- The `getpeerinfo` RPC now has additional `last_block` and `last_transaction`
5+  fields that return the UNIX epoch time of the last block and the last valid
    promag commented at 10:58 pm on August 16, 2020:
    nit, last valid block too.
    jnewbery commented at 9:54 am on August 17, 2020:
    No. nLastBlockTime is updated even if connecting the block fails.
    jonatack commented at 10:09 am on August 17, 2020:

    Thanks @jnewbery. I found figuring out their exact definition to be the most interesting part of this PR. More precise code documentation than

    • net.h Block and TXN accept times
    • net.cpp::AttemptToEvictConnection recently sent us transactions/recently sent us blocks

    might be worth adding/updating.

    jnewbery commented at 10:15 am on August 17, 2020:

    More code documentation … might be worth adding/updating.

    Certainly! Although it doesn’t have to be in this PR.

    jonatack commented at 10:17 am on August 17, 2020:

    More code documentation … might be worth adding/updating.

    Certainly! Although it doesn’t have to be in this PR.

    Agreed!

    promag commented at 11:57 pm on August 20, 2020:
    Right, got confused by fNewBlock.
    jonatack commented at 4:01 pm on September 1, 2020:
    Improved the documentation in #19857.
  20. in src/rpc/net.cpp:174 in 5da96210fc 
    170@@ -169,6 +171,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
171         obj.pushKV("relaytxes", stats.fRelayTxes);
172         obj.pushKV("lastsend", stats.nLastSend);
173         obj.pushKV("lastrecv", stats.nLastRecv);
174+        obj.pushKV("last_transaction", stats.nLastTXTime);
    promag commented at 11:00 pm on August 16, 2020:
    nit, I don’t have an alternative but last_transaction suggests it returns the actual transaction, not a timestamp.
    jonatack commented at 10:00 am on August 17, 2020:

    nit, I don’t have an alternative but last_transaction suggests it returns the actual transaction, not a timestamp.

    I considered adding _time to the names, but we already have timestamp fields both with and without “time” (conntime, lastsend, lastrecv), so I went for the shorter variants unless everyone prefers last_transaction_time and last_block_time instead.

    jnewbery commented at 10:04 am on August 17, 2020:
    I’m fine with either, although I think last_block_time could still be confusing, since one interpretation could be the timestamp in the block.
  21. promag commented at 11:01 pm on August 16, 2020: member
    Concept ACK.
  22. jnewbery commented at 9:58 am on August 17, 2020: member
    Code review ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
  23. DrahtBot commented at 8:19 pm on August 20, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19499 (p2p: Make timeout mockable and type safe, speed up test by MarcoFalke)
    • #19405 (rpc, cli: add network in/out connections to getnetworkinfo and -getinfo by jonatack)

    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.

  24. theStack approved
  25. theStack commented at 10:46 pm on August 23, 2020: member
    Code Review ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257 Just out of curiosity, any reason why you changed the execution order of the tests?
  26. MarcoFalke removed the label Tests on Aug 24, 2020
  27. jonatack commented at 8:40 am on August 24, 2020: member

    Just out of curiosity, any reason why you changed the execution order of the tests?

    Good question. It mattered for an earlier version of the test and no longer matters now, and I forgot to revert to the previous order. I don’t think there was anything critical or sacred about the previous order, but I don’t mind reverting that if someone thinks it’s better to do so.

  28. darosior approved
  29. darosior commented at 10:53 am on August 24, 2020: member
    ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
  30. laanwj merged this on Aug 24, 2020
  31. laanwj closed this on Aug 24, 2020
  32. jonatack deleted the branch on Aug 24, 2020
  33. sidhujag referenced this in commit 29ed917b69 on Aug 24, 2020
  34. fanquake referenced this in commit c17a003758 on Sep 2, 2020
  35. Fabcien referenced this in commit d923449b16 on Mar 22, 2021
  36. PastaPastaPasta referenced this in commit 84fda17304 on Sep 17, 2021
  37. PastaPastaPasta referenced this in commit d109cccf0b on Sep 24, 2021
  38. kittywhiskers referenced this in commit 6602c7ef63 on Oct 12, 2021
  39. DrahtBot locked this on Feb 15, 2022


jonatack laanwj jnewbery promag DrahtBot theStack darosior

Labels
RPC/REST/ZMQ P2P

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: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me