rpc: Backport getblock verbosity by tecnovert · Pull Request #2779 · PIVX-Project/PIVX

tecnovert · 2022-10-30T20:09:07Z

Changes the boolean 'verbose' parameter of the getblock rpc command to an integer.
Enables getblock output to include full transaction details.

Backported from Bitcoin v15
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.15.0.md

PeterL73

Seems technically sound, only some esthetic changes and backward compatibilty

Sign in to view

+            "2. verbosity              (numeric, optional, default=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data\n"

-            "\nResult (for verbose = true):\n"
+            "\nResult (for verbosity = 1):\n"


PeterL73 · 2022-11-02T18:03:46Z

    { "blockchain",         "getbestblockhash",       &getbestblockhash,       true,  {} },
    { "blockchain",         "getbestsaplinganchor",   &getbestsaplinganchor,   true,  {} },
-    { "blockchain",         "getblock",               &getblock,               true,  {"blockhash","verbose"} },
+    { "blockchain",         "getblock",               &getblock,               true,  {"blockhash","verbosity"} },


To be backward compatible this should be "verbose|verbosity"
(source: https://github.com/bitcoin/bitcoin/pull/8704/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1437)

But then src/rpc/server.cpp would have to be updated as well to recognise "|" as seperator
Like https://github.com/bitcoin/bitcoin/pull/8704/files#diff-019ee7d5e66b74eac42199f64e08cd0e90af4603bb3c105e294665ea4b411219R437-R446

PeterL73 · 2022-11-02T18:04:35Z

+        # Test getblock verbosity
+        block = node.getblock(blockhash=h, verbosity=0)
+        assert(isinstance(block, str))
+
+        block = node.getblock(blockhash=h, verbosity=1)
+        assert(isinstance(block['tx'][0], str))
+
+        block = node.getblock(blockhash=h, verbosity=2)
+        assert('vin' in block['tx'][0])
+
+


Is this functional test necessary? These are the only named arguments being tested

I think it's necessary to test the different options, although it would perhaps make more sense to move it to rpc_blockchain.py

tecnovert · 2022-11-02T19:57:57Z

Thank you for the detailed review.

I've added the missing help text and added back the 'verbose' named argument for backwards compatibility.

PeterL73 · 2022-11-03T12:37:15Z

        assert_equal(node.echo(arg9=None), [None]*10)
        assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]*2 + [3] + [None]*5 + [9])

+


test/functional/rpc_named_arguments.py is now unchanged and could be removed from this PR

PeterL73 · 2022-11-03T13:12:38Z

+        assert(isinstance(node.getblock(blockhash=besthash, verbose=False), str))
+        assert(isinstance(node.getblock(blockhash=besthash, verbosity=0), str))
+        assert(isinstance(node.getblock(besthash, 1)['tx'][0], str))
+        assert(isinstance(node.getblock(besthash, True)['tx'][0], str))
+        assert('vin' in node.getblock(besthash, 2)['tx'][0])


Nice!
I think this covers the basics

Would love to see some stricter tests then just checking if a string is returned
Maybe something like:

assert_is_hex_string(node.getblock(besthash, False)) assert_is_hex_string(node.getblock(besthash, 0)) assert_is_hash_string(node.getblock(besthash, True)['tx'][0]) assert_is_hash_string(node.getblock(besthash, 1)['tx'][0]) assert_is_hash_string(node.getblock(besthash, 2)['tx'][0]['txid'])

PeterL73

Don't know what went wrong, but this remark didn't get added in my last review

Sign in to view

    { "getbalance", 1, "include_watchonly" },
    { "getbalance", 2, "include_delegated" },
    { "getbalance", 3, "include_shield" },
-    { "getblock", 1, "verbose" },


PeterL73 · 2022-11-03T16:52:39Z

+
+        assert_is_hash_string(node.getblock(besthash, 1)['tx'][0])
+        assert_is_hash_string(node.getblock(besthash, True)['tx'][0])
+        assert_is_hex_string(node.getblock(besthash, 2)['tx'][0]['vin'][0]['coinbase']);


test/functional/rpc_blockchain.py:122:88: E703 statement ends with a semicolon ^---- failure generated from test/lint/lint-python.sh

Liquid369

tACK

Upon completion of the last changes, everything looks good now.
I had my own nit of verbose from earlier after compiling but that was all.

PeterL73

tACK b512d94

Fuzzbawls

ACK b512d94

Github-Pull: PIVX-Project#2779 Rebased-From: 715f2a2

Github-Pull: PIVX-Project#2779 Rebased-From: f4943e5

Github-Pull: PIVX-Project#2779 Rebased-From: 8488f7e

Github-Pull: PIVX-Project#2779 Rebased-From: b512d94

c0f6055 [Consensus] Bump v5.5 activation height for mainnet (Fuzzbawls) 200b908 [Consensus] Set v5.5 activation height for mainnet (Fuzzbawls) dc31cef Add functional test for scantxoutset (PeterL73) b452fb6 Remove commented out code (PeterL73) 3fc2e6b Backport scantxoutset and descriptors from BitCoin Core v0.17.2 (PeterL73) 8f5f52d Remove semicolon and add "verbose" back to vRPCConvertParams. (tecnovert) ba310d1 Test for hex strings and remove stray newline from rpc_named_arguments. (tecnovert) 2b244ba Fix missing help text and add backwards compatibility. (tecnovert) cce6951 rpc: Backport getblock verbosity (tecnovert) fd719a2 add include_delegated to vRPCConvertParams (PeterL73) d285af0 use block height from caller (PeterL73) Pull request description: Backports the following PRs to the `5.5` branch: #2772 #2774 #2779 #2780 #2781 #2784 Top commit has no ACKs. Tree-SHA512: b0858e8ce7a39a1a3399fb2cf3eb4c4e77ecd526d9c8ca38fab0dc1ca6d108c962a581c8e481d8ac584a35b8a88f58d9d887166a35aec8fdfb66e8d57232dfa8

rpc: Backport getblock verbosity

715f2a2

PeterL73 suggested changes Nov 2, 2022

View reviewed changes

Fix missing help text and add backwards compatibility.

f4943e5

Fuzzbawls added this to the 5.5.0 milestone Nov 3, 2022

PeterL73 suggested changes Nov 3, 2022

View reviewed changes

Test for hex strings and remove stray newline from rpc_named_arguments.

8488f7e

PeterL73 suggested changes Nov 3, 2022

View reviewed changes

Comment thread src/rpc/client.cpp

{ "getbalance", 1, "include_watchonly" },

{ "getbalance", 2, "include_delegated" },

{ "getbalance", 3, "include_shield" },

{ "getblock", 1, "verbose" },

This comment was marked as resolved.

Sign in to view

PeterL73 reviewed Nov 3, 2022

View reviewed changes

Remove semicolon and add "verbose" back to vRPCConvertParams.

b512d94

Liquid369 approved these changes Nov 3, 2022

View reviewed changes

PeterL73 approved these changes Nov 3, 2022

View reviewed changes

Fuzzbawls added RPC Upstream labels Nov 8, 2022

Fuzzbawls assigned tecnovert Nov 8, 2022

Fuzzbawls added the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Nov 18, 2022

Fuzzbawls approved these changes Nov 18, 2022

View reviewed changes

Fuzzbawls merged commit abb888b into PIVX-Project:master Nov 20, 2022

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Nov 22, 2022

rpc: Backport getblock verbosity

cce6951

Github-Pull: PIVX-Project#2779 Rebased-From: 715f2a2

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Nov 22, 2022

Fix missing help text and add backwards compatibility.

2b244ba

Github-Pull: PIVX-Project#2779 Rebased-From: f4943e5

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Nov 22, 2022

Test for hex strings and remove stray newline from rpc_named_arguments.

ba310d1

Github-Pull: PIVX-Project#2779 Rebased-From: 8488f7e

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Nov 22, 2022

Remove semicolon and add "verbose" back to vRPCConvertParams.

8f5f52d

Github-Pull: PIVX-Project#2779 Rebased-From: b512d94

Fuzzbawls mentioned this pull request Nov 22, 2022

5.5.0 backports #2783

Merged

		assert_equal(node.echo(arg9=None), [None]*10)
		assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]2 + [3] + [None]5 + [9])

Conversation

tecnovert commented Oct 30, 2022

Uh oh!

PeterL73 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

PeterL73 Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

PeterL73 Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

tecnovert Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

tecnovert commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterL73 Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

PeterL73 Nov 3, 2022 • 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!

PeterL73 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

PeterL73 Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

Liquid369 left a comment

Choose a reason for hiding this comment

Uh oh!

PeterL73 left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tecnovert commented Nov 2, 2022 •

edited

Loading

PeterL73 Nov 3, 2022 •

edited

Loading