test: Check RPC argument mapping

laanwj commented at 1:19 pm on July 6, 2017: member

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:

0* Checking consistency between dispatch tables and vRPCConvertParams
1ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
2ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
3WARNING: 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:

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

laanwj added the label RPC/REST/ZMQ on Jul 6, 2017

laanwj added the label Tests on Jul 6, 2017

jonasschnelli commented at 1:29 pm on July 6, 2017: contributor

Nice. Concept ACK.

jnewbery commented at 1:45 pm on July 6, 2017: member

Concept ACK. Will review later.

in contrib/devtools/check-rpc-mappings.py:60 in 603159515f outdated

55+                    in_rpcs = False
56+                elif '{' in line and '"' in line:
57+                    m = re.search('{ *("[^"]*"), *("[^"]*"), *&([^,]*), *(true|false), *{([^}]*)} *},', line)
58+                    if not m:
59+                        print('No match: %s' % line)
60+                        exit(1)

practicalswift commented at 2:43 pm on July 6, 2017:

Nit: Use sys.exit(…)

laanwj commented at 2:46 pm on July 6, 2017:

Why?

practicalswift commented at 3:32 pm on July 6, 2017:

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.”

laanwj commented at 5:54 pm on July 6, 2017:

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

promag commented at 9:43 am on July 7, 2017: member

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 commented at 10:05 am on July 7, 2017: member

@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 commented at 10:14 am on July 7, 2017: member

Thanks for pointing that out. @laanwj could this be executed in pre-commit hook?

laanwj commented at 11:40 am on July 7, 2017: member

@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 commented at 2:15 pm on July 7, 2017: member

@laanwj another PR or care to add it here?

laanwj commented at 7:00 am on July 8, 2017: member

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” notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/10753#issuecomment-313694531, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutvPib9u0jiX_HkyKAtsFAGdNzyfZks5sLj2cgaJpZM4OPn4k .

ryanofsky commented at 10:15 am on July 8, 2017: member

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.

laanwj closed this on Jul 8, 2017

promag commented at 11:26 am on July 8, 2017: member

@laanwj reason?

jnewbery commented at 1:03 pm on July 10, 2017: member

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

laanwj commented at 2:07 pm on July 10, 2017: member

@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 commented at 2:16 pm on July 10, 2017: member

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.

laanwj reopened this on Jul 10, 2017

in contrib/devtools/check-rpc-mappings.py:28 in 603159515f outdated

23+def parse_string(s):
24+    assert(s[0] == '"')
25+    assert(s[-1] == '"')
26+    return s[1:-1]
27+
28+def make_string(s):

jnewbery commented at 2:35 pm on July 10, 2017:

nit: unused, please remove

jnewbery commented at 5:46 pm on September 7, 2017:

still unused, please remove

in contrib/devtools/check-rpc-mappings.py:68 in 603159515f outdated

63+                    if args_str:
64+                        args = [RPCArgument(parse_string(x.strip()).split('|'), idx) for idx,x in enumerate(args_str.split(','))]
65+                    else:
66+                        args = []
67+                    cmds.append(RPCCommand(name, args))
68+    assert(not in_rpcs) # If this assertion is hit, something went wrong with parsing the C++ file: update the regexps

jnewbery commented at 2:46 pm on July 10, 2017:

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

0assert 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.

jnewbery commented at 5:49 pm on September 7, 2017:

please remove parens from assert

in contrib/devtools/check-rpc-mappings.py:43 in 603159515f outdated

36+class RPCArgument:
37+    def __init__(self, names, idx):
38+        self.names = names
39+        self.idx = idx
40+
41+def process_commands(fname):

jnewbery commented at 3:23 pm on July 10, 2017:

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

jnewbery commented at 3:30 pm on July 10, 2017: member

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 commented at 11:14 pm on August 9, 2017: member

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

laanwj commented at 6:59 am on August 10, 2017: member

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.

MarcoFalke referenced this in commit 60dd9cc470 on Aug 28, 2017

laanwj force-pushed on Sep 7, 2017

laanwj commented at 4:16 pm on September 7, 2017: member

Rebased and updated for new table syntax without safemode flag.

theuni commented at 5:08 pm on September 7, 2017: member

Concept ACK.

sipa commented at 5:38 pm on September 7, 2017: member

Concept ACK

in contrib/devtools/check-rpc-mappings.py:2 in 9b47f080a2 outdated

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

jnewbery commented at 5:46 pm on September 7, 2017:

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

laanwj commented at 8:18 pm on September 7, 2017:

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

jnewbery commented at 8:33 pm on September 7, 2017:

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)

in contrib/devtools/check-rpc-mappings.py:5 in 9b47f080a2 outdated

0@@ -0,0 +1,165 @@
1+#!/usr/bin/python3
2+'''
3+Check RPC argument consistency.
4+'''
5+import sys, os, re

jnewbery commented at 5:46 pm on September 7, 2017:

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

in contrib/devtools/check-rpc-mappings.py:10 in 9b47f080a2 outdated

 5+import sys, os, re
 6+from collections import defaultdict
 7+
 8+# Source files (relative to root) to scan for dispatch tables
 9+SOURCES = [
10+"src/rpc/server.cpp",

jnewbery commented at 5:46 pm on September 7, 2017:

nit: indentation for continuation lines.

in contrib/devtools/check-rpc-mappings.py:35 in 9b47f080a2 outdated

34+        self.args = args
35+
36+class RPCArgument:
37+    def __init__(self, names, idx):
38+        self.names = names
39+        self.idx = idx

jnewbery commented at 5:47 pm on September 7, 2017:

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

in contrib/devtools/check-rpc-mappings.py:58 in 9b47f080a2 outdated

53+            else:
54+                if line.startswith('};'):
55+                    in_rpcs = False
56+                elif '{' in line and '"' in line:
57+                    m = re.search('{ *("[^"]*"), *("[^"]*"), *&([^,]*), *{([^}]*)} *},', line)
58+                    if not m:

jnewbery commented at 5:48 pm on September 7, 2017:

Perhaps replace with an assert:

assert m, "No match: %s" % line

in contrib/devtools/check-rpc-mappings.py:88 in 9b47f080a2 outdated

83+            else:
84+                if line.startswith('};'):
85+                    in_rpcs = False
86+                elif '{' in line and '"' in line:
87+                    m = re.search('{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
88+                    if not m:

jnewbery commented at 5:49 pm on September 7, 2017:

perhaps replace with an assert?

in contrib/devtools/check-rpc-mappings.py:122 in 9b47f080a2 outdated

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

jnewbery commented at 5:51 pm on September 7, 2017:

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

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

etc

in contrib/devtools/check-rpc-mappings.py:161 in 9b47f080a2 outdated

156+            if argname in IGNORE_DUMMY_ARGS: # these are testing or dummy, don't warn for them
157+                continue
158+            print('WARNING: conversion mismatch for argument named %s (%s)' %
159+                    (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
160+
161+    exit(errors > 0)

jnewbery commented at 5:51 pm on September 7, 2017:

use sys.exit()

in contrib/devtools/check-rpc-mappings.py:165 in 9b47f080a2 outdated

160+
161+    exit(errors > 0)
162+
163+if __name__ == '__main__':
164+    main()
165+

jnewbery commented at 5:51 pm on September 7, 2017:

Remove trailing blank line

in contrib/devtools/check-rpc-mappings.py:1 in 9b47f080a2 outdated

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

jnewbery commented at 5:54 pm on September 7, 2017:

For portability, use:

0#!/usr/bin/env python3

Also add copyright notice

jnewbery commented at 5:57 pm on September 7, 2017: member

A bunch of nits inline. I’ve implemented them here: https://github.com/jnewbery/bitcoin/commit/b73ce0a785aa8d6ea69078ae8b7222534f6d01c7. Feel free to take and squash

jtimon commented at 7:53 pm on September 7, 2017: contributor

Concept ACK. If somebody writes a unittest alternative later, it is easy to replace one for the other, not a good reason to stop this.

laanwj force-pushed on Sep 7, 2017

laanwj commented at 8:28 pm on September 7, 2017: member

Addressed @jnewbery’s nits and updated for .travis.yml conflict.

test: Check RPC argument mapping

Parse the dispatch tables from the server implementation files,
and the conversion table from the client.

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).

77aa9e59ea

in .travis.yml:48 in eef090fb23 outdated

44@@ -45,7 +45,7 @@ install:
45 before_script:
46     - if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/commit-script-check.sh $TRAVIS_COMMIT_RANGE; fi
47     - if [ "$CHECK_DOC" = 1 ]; then contrib/devtools/check-doc.py; fi
48-    - unset CC; unset CXX

jnewbery commented at 8:32 pm on September 7, 2017:

Is this removal intended? How is it related to this PR?

laanwj commented at 8:33 pm on September 7, 2017:

Sigh, rebase fail

laanwj force-pushed on Sep 7, 2017

jnewbery commented at 11:33 pm on September 7, 2017: member

Tested ACK 77aa9e59ea0e6a000a0aea5dec4ef9585356147d

It’s definitely working:

0ERROR: estimaterawfee argument 0 is named nblocks in vRPCConvertParams but ['conf_target'] in dispatch table
1WARNING: conversion mismatch for argument named conf_target ([('estimaterawfee', False), ('sendmany', True), ('sendtoaddress', True)])

laanwj commented at 11:35 pm on September 7, 2017: member

Thanks for testing. Yep, we should merge #11267 first to not break the tests.

MarcoFalke referenced this in commit c377feaad8 on Sep 12, 2017

MarcoFalke commented at 5:41 pm on September 13, 2017: member

utACK 77aa9e59ea0e6a000a0aea5dec4ef9585356147d

MarcoFalke merged this on Sep 13, 2017

MarcoFalke closed this on Sep 13, 2017

MarcoFalke referenced this in commit 7fcd61b261 on Sep 13, 2017

mempko referenced this in commit d98f235b23 on Sep 28, 2017

MarcoFalke referenced this in commit e92b7287c1 on Oct 3, 2017

PastaPastaPasta referenced this in commit 062561b702 on Sep 19, 2019

PastaPastaPasta referenced this in commit 9b09ca6f7d on Sep 23, 2019

PastaPastaPasta referenced this in commit 94b9944a06 on Sep 24, 2019

PastaPastaPasta referenced this in commit 3375f6a77c on Sep 24, 2019

codablock referenced this in commit 654c78fa29 on Sep 25, 2019

PastaPastaPasta referenced this in commit 94d1d28c54 on Nov 19, 2019

PastaPastaPasta referenced this in commit 3e9d3cc88f on Nov 21, 2019

PastaPastaPasta referenced this in commit a2bcdb9d1d on Dec 9, 2019

PastaPastaPasta referenced this in commit 7085b31845 on Dec 21, 2019

PastaPastaPasta referenced this in commit f64887dfe5 on Jan 1, 2020

PastaPastaPasta referenced this in commit bd89676e44 on Jan 2, 2020

PastaPastaPasta referenced this in commit a0a3d17e9a on Jan 2, 2020

PastaPastaPasta referenced this in commit 2c7c504b42 on Jan 2, 2020

PastaPastaPasta referenced this in commit dd89d1844b on Jan 2, 2020

PastaPastaPasta referenced this in commit c7aa2d6a6f on Jan 2, 2020

PastaPastaPasta referenced this in commit e411886030 on Jan 2, 2020

PastaPastaPasta referenced this in commit 561ec27683 on Jan 3, 2020

PastaPastaPasta referenced this in commit a14339e4f4 on Jan 4, 2020

PastaPastaPasta referenced this in commit eb4fece37d on Jan 4, 2020

PastaPastaPasta referenced this in commit c442648c19 on Jan 10, 2020

PastaPastaPasta referenced this in commit 55d0336348 on Jan 10, 2020

PastaPastaPasta referenced this in commit 3ca3c65897 on Jan 10, 2020

PastaPastaPasta referenced this in commit 872204e0a2 on Jan 12, 2020

barrystyle referenced this in commit cdb52ecec9 on Jan 22, 2020

ckti referenced this in commit 5ac21eb8cb on Mar 28, 2021

ckti referenced this in commit f18f4dc2bf on Mar 28, 2021

gades referenced this in commit 339f96b813 on Jun 24, 2021

DrahtBot locked this on Sep 8, 2021

test: Check RPC argument mapping #10753