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

jonatack · 2020-08-15T10:23:53Z

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:

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

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

test/functional/rpc_net.py

laanwj · 2020-08-15T11:07:27Z

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.

jonatack · 2020-08-15T11:12:49Z

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.

test/functional/rpc_net.py

jnewbery · 2020-08-15T11:53:33Z

src/rpc/net.cpp

+                            {RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last transaction accepted and relayed by this peer"},
+                            {RPCResult::Type::NUM_TIME, "last_block", "The " + UNIX_EPOCH_TIME + " of the last block accepted and relayed by this peer "},


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.

Thanks @jnewbery! I was just reconsidering those as well. Updating.

Unresolving to make the head comment visible for reviewers and future code doc writers.

jonatack · 2020-08-15T13:30:55Z

Updated with @jnewbery feedback (thanks!)

promag

Concept ACK.

promag · 2020-08-16T22:58:29Z

doc/release-notes-19731.md

+------------
+
+- The `getpeerinfo` RPC now has additional `last_block` and `last_transaction`
+  fields that return the UNIX epoch time of the last block and the last valid


nit, last valid block too.

No. nLastBlockTime is updated even if connecting the block fails.

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.

More code documentation ... might be worth adding/updating.

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

More code documentation ... might be worth adding/updating.

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

Agreed!

Right, got confused by fNewBlock.

Improved the documentation in #19857.

promag · 2020-08-16T23:00:49Z

src/rpc/net.cpp

@@ -169,6 +171,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
        obj.pushKV("relaytxes", stats.fRelayTxes);
        obj.pushKV("lastsend", stats.nLastSend);
        obj.pushKV("lastrecv", stats.nLastRecv);
+        obj.pushKV("last_transaction", stats.nLastTXTime);


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

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.

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.

jnewbery · 2020-08-17T09:58:43Z

Code review ACK 5da9621

DrahtBot · 2020-08-20T20:19:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Make timeout mockable and type safe, speed up test #19499 (p2p: Make timeout mockable and type safe, speed up test by MarcoFalke)
rpc, cli: add network in/out connections to getnetworkinfo and -getinfo #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.

theStack

Code Review ACK 5da9621
Just out of curiosity, any reason why you changed the execution order of the tests?

jonatack · 2020-08-24T08:40:17Z

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.

darosior

ACK 5da9621

…ast block/last_transaction in getpeerinfo 5da9621 doc: release note for getpeerinfo last_block/last_transaction (Jon Atack) cfef5a2 test: rpc_net.py logging and test naming improvements (Jon Atack) 21c57ba test: getpeerinfo last_block and last_transaction tests (Jon Atack) 8a560a7 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack) 02fbe3a net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack) Pull request description: 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 bitcoin#19643 (comment). Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549: ```text <jonatack> i was specifically trying to observe and figure out how to test bitcoin#19500 <jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail <jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard <jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently? <sipa> jonatack: nope; i suspect just nobody ever added it <jonatack> sipa: thanks. will propose. ``` The last commit is optional, but I think it would be good to have logging in `rpc_net.py`. ACKs for top commit: jnewbery: Code review ACK 5da9621 theStack: Code Review ACK 5da9621 darosior: ACK 5da9621 Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70

d780293 net: improve nLastBlockTime and nLastTXTime documentation (Jon Atack) Pull request description: Follow-up to #19731 to help alleviate confusion around `nLastBlockTime` and `nLastTXTime`, now also provided by the JSON-RPC API as `last_block` and `last_transaction` in `getpeerinfo` output. Thanks to John Newbery, credited in the commit, and to Dave Harding and Adam Jonas during discussions on how to best explain these in this week's Optech newsletter. ACKs for top commit: practicalswift: ACK d780293 MarcoFalke: ACK d780293 harding: ACK d780293 . The added documentation matches my reading of the code and answers a question I had after seeing #19731 0xB10C: ACK d780293 Tree-SHA512: 72d47cf50a099913c7e4753cb80e11785b26fb66fa3a8b6c382fde4ea725116f3d215f93d32a567246d269768e66159f8dcf017a1bbc6d5f2489a35f81c316fa

…ction in getpeerinfo Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#19731 | core#19731]]. Depends on D9324 and D9330. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9331

…mentation d780293 net: improve nLastBlockTime and nLastTXTime documentation (Jon Atack) Pull request description: Follow-up to bitcoin#19731 to help alleviate confusion around `nLastBlockTime` and `nLastTXTime`, now also provided by the JSON-RPC API as `last_block` and `last_transaction` in `getpeerinfo` output. Thanks to John Newbery, credited in the commit, and to Dave Harding and Adam Jonas during discussions on how to best explain these in this week's Optech newsletter. ACKs for top commit: practicalswift: ACK d780293 MarcoFalke: ACK d780293 harding: ACK d780293 . The added documentation matches my reading of the code and answers a question I had after seeing bitcoin#19731 0xB10C: ACK d780293 Tree-SHA512: 72d47cf50a099913c7e4753cb80e11785b26fb66fa3a8b6c382fde4ea725116f3d215f93d32a567246d269768e66159f8dcf017a1bbc6d5f2489a35f81c316fa

net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats

02fbe3a

jonatack commented Aug 15, 2020

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

jonatack force-pushed the add-eviction-criteria-to-getpeerinfo branch from 5c1ec86 to 7da71af Compare August 15, 2020 11:11

jonatack force-pushed the add-eviction-criteria-to-getpeerinfo branch from 7da71af to 062614f Compare August 15, 2020 11:28

jnewbery reviewed Aug 15, 2020

View reviewed changes

DrahtBot added P2P RPC/REST/ZMQ Tests labels Aug 15, 2020

jonatack added 4 commits August 15, 2020 14:34

rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction

8a560a7

test: getpeerinfo last_block and last_transaction tests

21c57ba

test: rpc_net.py logging and test naming improvements

cfef5a2

doc: release note for getpeerinfo last_block/last_transaction

5da9621

jonatack force-pushed the add-eviction-criteria-to-getpeerinfo branch from 062614f to 5da9621 Compare August 15, 2020 13:28

promag reviewed Aug 16, 2020

View reviewed changes

This was referenced Aug 17, 2020

Add -netinfo peer connections dashboard #19643

Merged

Allow to specify the number of Tor connections and clearnet connections separately #19500

Closed

This was referenced Aug 20, 2020

p2p: Make timeout mockable and type safe, speed up test #19499

Merged

rpc, cli: add network in/out connections to getnetworkinfo and -getinfo #19405

Merged

jonatack mentioned this pull request Aug 21, 2020

net, rpc: expose high bandwidth mode state via getpeerinfo #19776

Merged

theStack approved these changes Aug 23, 2020

View reviewed changes

maflcko removed the Tests label Aug 24, 2020

darosior approved these changes Aug 24, 2020

View reviewed changes

laanwj merged commit 7f609f6 into bitcoin:master Aug 24, 2020

jonatack deleted the add-eviction-criteria-to-getpeerinfo branch August 24, 2020 15:20

jonatack mentioned this pull request Sep 1, 2020

net: improve nLastBlockTime and nLastTXTime documentation #19857

Merged

jonatack mentioned this pull request Sep 20, 2020

net, rpc: expose connection type in getpeerinfo #19883

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		{RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last transaction accepted and relayed by this peer"},
		{RPCResult::Type::NUM_TIME, "last_block", "The " + UNIX_EPOCH_TIME + " of the last block accepted and relayed by this peer "},

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

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

Uh oh!

Conversation

jonatack commented Aug 15, 2020

Uh oh!

Uh oh!

laanwj commented Aug 15, 2020

Uh oh!

jonatack commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 15, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Aug 17, 2020

Uh oh!

DrahtBot commented Aug 20, 2020

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 24, 2020

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Aug 15, 2020 •

edited

Loading

jonatack Aug 17, 2020 •

edited

Loading

jonatack Aug 17, 2020 •

edited

Loading