Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Return block time in getblockchaininfo #22407

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

promag
Copy link
Member

@promag promag commented Jul 5, 2021

Return tip time in getblockchaininfo, for some use cases this can save a call to getblock.

@theStack
Copy link
Contributor

theStack commented Jul 5, 2021

Concept ACK

@naumenkogs
Copy link
Member

ACK b17bb4c.
I can indeed see how this is useful.

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, could use a test asserting on the value, needs this change for the CI to be green:

diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index 90715cae26..24dc668ea0 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -93,6 +93,7 @@ class BlockchainTest(BitcoinTestFramework):
             'pruned',
             'size_on_disk',
             'softforks',
+            'time',
             'verificationprogress',
             'warnings',

@theStack
Copy link
Contributor

theStack commented Jul 9, 2021

@naumenkogs: I think you ACKed a commit not belonging to this PR?

Happy to review and test as soon as the CI is happy (see #22407 (review)) :)

@promag promag force-pushed the 2021-07-getblockchaininfo-time branch from 9e73be1 to c86e001 Compare July 11, 2021 10:51
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c86e001

Does this need a release note?

@naumenkogs
Copy link
Member

Sorry for acking the wrong hash.
ACK c86e001.

@0xB10C
Copy link
Contributor

0xB10C commented Jul 12, 2021

ACK c86e001

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK c86e001.

Tests in test/functional/rpc_blockchain.py also check out successfully.

@theStack I think this change should have a release note like getnetworkinfo did when the connections_in & connections_out fields were added in v0.20.0. It could be something like:

`getblockchaininfo` now returns a new `time` field, that provides the chain tip time. (#22407)

@kristapsk
Copy link
Contributor

kristapsk commented Jul 12, 2021

Concept ACK, but agree with @Zero-1729 that this needs release note.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK c86e001

I would add a release note for this, the one proposed here is good.

Additionally, I would add an assert for this as suggested here: #22407 (review)

diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index 24dc668ea..f7290ff22 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -99,6 +99,8 @@ class BlockchainTest(BitcoinTestFramework):
         ]
         res = self.nodes[0].getblockchaininfo()
 
+        assert isinstance(res['time'], int)
+
         # result should have these additional pruning keys if manual pruning is enabled
         assert_equal(sorted(res.keys()), sorted(['pruneheight', 'automatic_pruning'] + keys))

@promag promag force-pushed the 2021-07-getblockchaininfo-time branch from c86e001 to 20edf4b Compare July 20, 2021 09:51
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 20edf4b

Copy link
Contributor

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 20edf4b

Now includes a release note in doc/release-notes.md and the proposed assert in the functional test.

@naumenkogs
Copy link
Member

ACK 20edf4b

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 20edf4b

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 20edf4b

@maflcko maflcko merged commit 458d6ac into bitcoin:master Jul 21, 2021
@promag promag deleted the 2021-07-getblockchaininfo-time branch July 21, 2021 08:23
maflcko pushed a commit that referenced this pull request Jul 21, 2021
2ce7f95 doc: clean out release notes post branch-off (fanquake)

Pull request description:

  Should probably be done, given we've already had a PR add a release note for 23.0 (#22407).

ACKs for top commit:
  MarcoFalke:
    ACK 2ce7f95

Tree-SHA512: 1c524b55daecb8c69e110759b82a5caf31080ea7abbae39ecfefe0107786199623499c9950dedd8e72594a55b072eda93801ee787e896a3f242cde73d8a984f6
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
20edf4b rpc: Return block time in getblockchaininfo (João Barbosa)

Pull request description:

  Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.

ACKs for top commit:
  naumenkogs:
    ACK 20edf4b
  theStack:
    re-ACK 20edf4b
  0xB10C:
    ACK 20edf4b
  kristapsk:
    ACK 20edf4b
  Zero-1729:
    re-ACK 20edf4b

Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jul 30, 2021
…d assert on time and mediantime

ef5e930 test: update logging and docstring in rpc_blockchain.py (Jon Atack)
d548dc7 test: replace magic values by constants in rpc_blockchain.py (Jon Atack)
78c3610 test: assert on mediantime in getblockheader and getblockchaininfo (Jon Atack)
0a9129c test: assert on the value of getblockchaininfo#time (Jon Atack)

Pull request description:

  Follow-up to #22407 improving test coverage per bitcoin/bitcoin#22407 (review).

ACKs for top commit:
  tryphe:
    untested ACK ef5e930

Tree-SHA512: f746d56f430331bc6a2ea7ecd27b21b06275927966aacf1f1127d8d5fdfd930583cabe72e23df3adb2e005da904fc05dc573b8e5eaa2f86e0e193b89a17a5734
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants