test: Check RPC argument mapping #10753

laanwj · 2017-07-06T13:19:12Z

Parse the dispatch tables from the server implementation files, and the conversion table from the client (see #10751).

Perform the following consistency checks:

Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.
Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.
All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

Any of these results in an error.

It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

This test is added to travis and run when CHECK_DOC. Currently fails with the following output:

* Checking consistency between dispatch tables and vRPCConvertParams
ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])

~~Be consistent in calling transactions "replaceable" for Opt-In RBF #10698 fixes the first ERROR~~
[rpc] fix verbose argument for getblock in bitcoin-cli #10747 fixes the second ERROR, as well as the WARNING

Update: #10698 was merged, leaving:

* Checking consistency between dispatch tables and vRPCConvertParams
ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])

jonasschnelli · 2017-07-06T13:29:38Z

Nice.
Concept ACK.

jnewbery · 2017-07-06T13:45:12Z

Concept ACK. Will review later.

practicalswift · 2017-07-06T14:43:12Z

contrib/devtools/check-rpc-mappings.py

+                    m = re.search('{ *("[^"]*"), *("[^"]*"), *&([^,]*), *(true|false), *{([^}]*)} *},', line)
+                    if not m:
+                        print('No match: %s' % line)
+                        exit(1)


Nit: Use sys.exit(…)

According to the Python documentation exit(…) should be used in the interactive interpreter shell but not in programs:

The site module (which is imported automatically during startup, except if the -S command-line option is given) adds several constants to the built-in namespace. They are useful for the interactive interpreter shell and should not be used in programs.

Links:

sys.exit(…): "Exit from Python."

exit(…): "raise SystemExit with the specified exit code. […] should not be used in programs."

Agree then! I never knew that. Assumed they were simply aliasses.

promag · 2017-07-07T09:43:19Z

IMO we could somehow share the same data between the server table and the client table so that none of this would be necessary. Don't take me wrong, but parsing code feels bad practice. Beside that, keeping these tables in sync could very well be avoided. I could work in an alternative, unless I'm missing something.

laanwj · 2017-07-07T10:05:46Z

@promag That discussion is in #10751 (comment)

And yes, in the long run there are absolutely better solutions (ideally the cli would have none of this information and requests it from the server), but in the short term I think adding this check is good.

Anyhow I don't care deeply if this doesn't get merged, I use this script myself, and @TheBlueMatt requested some way to check this in #10751 so I thought it'd be useful for some other people too...

promag · 2017-07-07T10:14:53Z

Thanks for pointing that out.

@laanwj could this be executed in pre-commit hook?

laanwj · 2017-07-07T11:40:33Z

@laanwj could this be executed in pre-commit hook?

Sure, would be a matter of adding the line to execute it to the .git/hooks/pre-commit or .git/hooks/pre-push script.

promag · 2017-07-07T14:15:54Z

@laanwj another PR or care to add it here?

laanwj · 2017-07-08T07:00:40Z

You have to do it locally. Hooks are not part of the repository (for security reasons, among others.) Shipping some examples of hooks would work but that's definitely out of scope here.

…

On Jul 7, 2017 4:15 PM, "João Barbosa" ***@***.***> wrote: @laanwj <https://github.com/laanwj> another PR or care to add it here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10753 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHutvPib9u0jiX_HkyKAtsFAGdNzyfZks5sLj2cgaJpZM4OPn4k> .

ryanofsky · 2017-07-08T10:15:21Z

I think this might be simpler and more robust if it were written as a c++ unit test that just accesses the global variables directly, instead of a python tool that has to parse c++ with regexes.

promag · 2017-07-08T11:26:47Z

@laanwj reason?

jnewbery · 2017-07-10T13:03:32Z

@laanwj did you close this because you're planning to propose an alternative?

laanwj · 2017-07-10T14:07:17Z

@jnewbery No, I don't have the time nor motivation to work on this (it works for me), but I'm looking forward to a better alternative as no one seems to like this solution.

jnewbery · 2017-07-10T14:16:13Z

I like it! At the very least I like it better than no tests at all.

Running code beats suggestions of better ways to do it without any code.

If you change your mind, I'm happy to review.

jnewbery

A few nits inline.

I think it'd simplify the implementation if you got rid of the RPCCommand and RPCArgument classes and returned tuples from the process_commands() and process_mappings() functions. The different format of the return values makes some of the logic in main() a little fiddly.

However, this does the job, and I don't think it's worth the time optimizing this, so ACK 603159515f1fa2b9aa0c4f37c6726803b7c39ded (once #10747 is merged).

I also recommend running a linter such as flake8 over python modules before committing, since removing all warnings can sometimes uncover bugs. Rather than fill this review with nits, I've pushed a branch here that fixes them: https://github.com/jnewbery/bitcoin/tree/pr10753 . Feel free to take/leave/squash as you see fit.

If @ryanofsky or @promag want to implement a C++ unit test, I'm happy to review.

jnewbery · 2017-07-10T14:35:09Z

contrib/devtools/check-rpc-mappings.py

+    assert(s[-1] == '"')
+    return s[1:-1]
+
+def make_string(s):


nit: unused, please remove

still unused, please remove

jnewbery · 2017-07-10T14:46:15Z

contrib/devtools/check-rpc-mappings.py

+                    else:
+                        args = []
+                    cmds.append(RPCCommand(name, args))
+    assert(not in_rpcs) # If this assertion is hit, something went wrong with parsing the C++ file: update the regexps


assert is a statement, not a function, so these parens are unnecessary. In fact, this would be better expressed as:

assert not in_rpcs, "Something went wrong with parsing the C++ file: update the regexps"

since that delivers the error text to the user at the point of failure.

please remove parens from assert

jnewbery · 2017-07-10T15:23:30Z

contrib/devtools/check-rpc-mappings.py

+        self.names = names
+        self.idx = idx
+
+def process_commands(fname):


I think it'd simplify implementation in main() if process_commands() and process_mappings() both returned a set of tuples (command, index, argname).

jnewbery · 2017-08-09T23:14:39Z

@laanwj - do you still plan to work on this? If not, do you mind if I take over?

laanwj · 2017-08-10T06:59:54Z

I'll get to this after 0.15.0. There's not much to be done here but I'm completely distracted at the moment.

7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b

laanwj · 2017-09-07T16:16:02Z

Rebased and updated for new table syntax without safemode flag.

theuni · 2017-09-07T17:08:17Z

Concept ACK.

sipa · 2017-09-07T17:38:10Z

Concept ACK

jnewbery

A bunch of nits inline. I've implemented them here: jnewbery@b73ce0a. Feel free to take and squash

jnewbery · 2017-09-07T17:46:07Z

contrib/devtools/check-rpc-mappings.py

@@ -0,0 +1,165 @@
+#!/usr/bin/python3
+'''


nit: One-line docstrings should be on one line.

Meh, I personally prefer them on multiple lines, makes it easier to extend them later.

Thanks for changing it anyway! (for context, I picked up the preference for one-line docstrings from https://www.python.org/dev/peps/pep-0257/#id16)

jnewbery · 2017-09-07T17:46:21Z

contrib/devtools/check-rpc-mappings.py

+'''
+Check RPC argument consistency.
+'''
+import sys, os, re


nit: one import per line, sorted by name of module

jnewbery · 2017-09-07T17:46:46Z

contrib/devtools/check-rpc-mappings.py

+
+# Source files (relative to root) to scan for dispatch tables
+SOURCES = [
+"src/rpc/server.cpp",


nit: indentation for continuation lines.

jnewbery · 2017-09-07T17:46:58Z

contrib/devtools/check-rpc-mappings.py

+    assert(s[-1] == '"')
+    return s[1:-1]
+
+def make_string(s):


still unused, please remove

jnewbery · 2017-09-07T17:47:25Z

contrib/devtools/check-rpc-mappings.py

+class RPCArgument:
+    def __init__(self, names, idx):
+        self.names = names
+        self.idx = idx


suggestion: add self.convert = False for self-documentation

jnewbery · 2017-09-07T17:49:26Z

contrib/devtools/check-rpc-mappings.py

+                    in_rpcs = False
+                elif '{' in line and '"' in line:
+                    m = re.search('{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
+                    if not m:


perhaps replace with an assert?

jnewbery · 2017-09-07T17:51:08Z

contrib/devtools/check-rpc-mappings.py

+        if argname not in rargnames:
+            print('ERROR: %s argument %i is named %s in vRPCConvertParams but %s in dispatch table' % (cmdname, argidx, argname, rargnames), file=sys.stderr)
+            errors += 1
+


Suggestion: add prints for each check (like you have for the first one):

print('* Checking for conflicts in vRPCConvertParams conversion')

etc

jnewbery · 2017-09-07T17:51:29Z

contrib/devtools/check-rpc-mappings.py

+            print('WARNING: conversion mismatch for argument named %s (%s)' %
+                    (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
+
+    exit(errors > 0)


use sys.exit()

jnewbery · 2017-09-07T17:51:40Z

contrib/devtools/check-rpc-mappings.py

+
+if __name__ == '__main__':
+    main()
+


Remove trailing blank line

jnewbery · 2017-09-07T17:54:12Z

contrib/devtools/check-rpc-mappings.py

@@ -0,0 +1,165 @@
+#!/usr/bin/python3


For portability, use:

#!/usr/bin/env python3

Also add copyright notice

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan) Pull request description: Parse the dispatch tables from the server implementation files, and the conversion table from the client (see #10751). Perform the following consistency checks: - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work. - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work. - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work. Any of these results in an error. It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted, another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error). This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output: ``` * Checking consistency between dispatch tables and vRPCConvertParams ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False] WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)]) ``` - ~#10698 fixes the first ERROR~ - #10747 fixes the second ERROR, as well as the WARNING Update: #10698 was merged, leaving: ``` * Checking consistency between dispatch tables and vRPCConvertParams ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False] WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)]) ``` Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a

7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin/bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b

7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b

*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS 77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan) Pull request description: Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751). Perform the following consistency checks: - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work. - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work. - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work. Any of these results in an error. It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted, another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error). This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output: ``` * Checking consistency between dispatch tables and vRPCConvertParams ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False] WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)]) ``` - ~bitcoin#10698 fixes the first ERROR~ - bitcoin#10747 fixes the second ERROR, as well as the WARNING Update: bitcoin#10698 was merged, leaving: ``` * Checking consistency between dispatch tables and vRPCConvertParams ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False] WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)]) ``` Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a

7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b

5acd82d rpc: make estimatesmartfee argument naming consistent with documentation (Wladimir J. van der Laan) 24697c4 rpc: update cli for estimatefee argument rename (Wladimir J. van der Laan) Pull request description: The first argument of `estimaterawfee` was renamed from `nblocks` to `conf_target` in 06bcdb8. Update the client-side table as well. This makes bitcoin#10753 pass again. Tree-SHA512: 107c0072a45e0f4e083dc803d534973e6bd4c005e62337a867815d7c98ab1c21d97b7a495c32763883975cbbb001b80003001a6709b7d9bdd81ce4d441b667be