Add bitcoin-cli -stdin and -stdinrpcpass functional tests #11125

pull promag wants to merge 6 commits into bitcoin:master from promag:2017-08-stdinrpcpass-functional-test changing 4 files +65 −16
  1. promag commented at 5:47 PM on August 24, 2017: member

    This patch adds tests for bitcoin-cli options -stdin (#7550) and -stdinrpcpass #10997.

  2. promag force-pushed on Aug 24, 2017
  3. laanwj added the label Tests on Aug 24, 2017
  4. in test/functional/test_framework/test_node.py:142 in 7162087cc2 outdated 
     135 | @@ -136,3 +136,7 @@ def node_encrypt_wallet(self, passphrase):
 136 |              time.sleep(0.1)
 137 |          self.rpc = None
 138 |          self.rpc_connected = False
 139 | +
 140 | +    def cli(self, args):
 141 | +        """Start the cli."""
 142 | +        return subprocess.Popen(['bitcoin-cli'] + args, stderr=self.stderr, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
    laanwj commented at 6:30 PM on August 24, 2017:

    We need a way to override what bitcoin-cli to use, akin to BITCOIND environment variable.

    Which reminds me, didn't we have another PR to add bitcoin-cli testing support at some point? (if so, might be better to do it on top of that)

    MarcoFalke commented at 8:13 PM on August 24, 2017:

    You might be able to hack something up based on TestNodeCLI from current master branch.

    jnewbery commented at 8:16 PM on August 24, 2017:

    This should already be possible with #10798 merged

  5. laanwj commented at 6:30 PM on August 24, 2017: member

    Concept ACK

  6. jnewbery commented at 6:32 PM on August 24, 2017: member

    Concept ACK, but I recommend you build on top of #10798 which already adds bitcoin-cli interface to TestNode.

  7. promag commented at 8:16 PM on August 24, 2017: member

    I'll refactor now that #10798 is merged.

  8. promag force-pushed on Aug 25, 2017
  9. promag force-pushed on Aug 25, 2017
  10. promag renamed this:
    Add bitcoin-cli -stdinrpcpass functional test
    Add bitcoin-cli -stdin and -stdinrpcpass functional tests
    on Aug 25, 2017
  11. promag commented at 3:16 AM on August 25, 2017: member

    @jnewbery take a look at f275cf6, it allows to pass -stdin and -stdinrpcpass. Also, TestNodeCLI instances are only created when necessary.

    Updated PR title and description accordingly.

  12. promag force-pushed on Aug 25, 2017
  13. jnewbery commented at 3:42 PM on August 25, 2017: member

    See commit here for an alternative way of doing this: https://github.com/jnewbery/bitcoin/commit/5bfd5e0d47f89f4c2e9becf266876c0c6cc62787

    A few advantages:

    • bitcoin-cli command line arguments are passed in as a string and fed through to subprocess, rather than being parsed by python
    • It avoids the cludgy input magic key
    • it maintains the -datadir argument, which I think we want to keep (without that, bitcoin-cli won't have access to the rpcport value in the bitcoin.conf file).

    I've proposed that as a test for #10871. Let me know what you think.

  14. promag commented at 6:02 PM on August 25, 2017: member

    It avoids the cludgy input magic key @jnewbery how do you pass the stdin content?

  15. jnewbery commented at 6:08 PM on August 25, 2017: member

    ah sorry, I just looked at the commit at https://github.com/bitcoin/bitcoin/commit/f275cf65018681689e711af6bca202125557c434 and misunderstood how input was used. I'll re-review.

  16. MarcoFalke commented at 7:54 PM on August 25, 2017: member

    utACK 5b4e3391f6a7dc9e7148eaee05e285f200f46927

  17. in test/functional/bitcoin_cli.py:26 in 5b4e3391f6 outdated 
      23 | -
  24 |          assert_equal(cli_get_info, rpc_get_info)
  25 |  
  26 | +        self.log.info("Test -stdinrpcpass option")
  27 | +        user, password = get_auth_cookie(self.nodes[0].datadir)
  28 | +        block_count = self.nodes[0].cli(rpcuser=user, rpcport=rpc_port(0), stdinrpcpass=None).getblockcount(input=password)
    laanwj commented at 7:49 AM on August 28, 2017:

    I would prefer to make a temporary, empty datadir here instead of make it use the default. The tests should not touch ~/.bitcoin.

    jnewbery commented at 2:50 PM on August 28, 2017:

    It seems weird to me that input is an argument to the RPC, rather than an argument to the bitcoin-cli instance.

    It seems possible that an RPC will have input as a parameter. Rather less likely that bitcoin-cli will add an argument called input.

  18. promag commented at 7:55 AM on August 28, 2017: member

    Agree!

  19. in test/functional/test_framework/test_node.py:148 in 5b4e3391f6 outdated 
     143 | +        """Creates a configurable cli for the node.
 144 | +
 145 | +        If arguments is empty then -datadir is set to match the node
 146 | +        """
 147 | +        args = ["-" + str(key) + ("=" + str(value) if value else "") for (key, value) in kwargs.items()]
 148 | +        if not args:
    jnewbery commented at 2:21 PM on August 28, 2017:

    I think you should add an error here. If args is specified, then datadir must be one of the args (otherwise bitcoin-cli will look in ~/.bitcoin).

    laanwj commented at 2:56 PM on August 28, 2017:

    Another option would be to set datadir=self.datadir if it isn't overridden otherwise. As you say, there is no reason the argument should ever be missing.

    jnewbery commented at 6:37 PM on August 29, 2017:

    I think either approach is fine. Making it an error to not include it in args makes it more explicit to the test writer (ie datadir doesn't get set magically for them).

  20. jnewbery commented at 2:53 PM on August 28, 2017: member

    Looks generally good to me. A couple of nits inline.

  21. in test/functional/test_framework/test_node.py:57 in 5b4e3391f6 outdated 
      50 | @@ -51,8 +51,6 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo
  51 |          self.extra_args = extra_args
  52 |          self.args = [self.binary, "-datadir=" + self.datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(mocktime), "-uacomment=testnode%d" % i]
  53 |  
  54 | -        self.cli = TestNodeCLI(os.getenv("BITCOINCLI", "bitcoin-cli"), self.datadir)
    MarcoFalke commented at 7:50 PM on August 28, 2017:

    Thinking more about this, I think we should keep TestNode.cli to be the corresponding interface to TestNode.rpc. The logic to hack a different datadir can be placed in the test that checks for -stdinrpcpass. This makes the TestNode code easier to understand, as the datadir overwrite is rarely needed... Likely only once?

    jnewbery commented at 6:43 PM on August 29, 2017:

    I also think I prefer keeping TestNode.cli as a class (and making it callable as per https://github.com/jnewbery/bitcoin/commit/5bfd5e0d47f89f4c2e9becf266876c0c6cc62787), but I'm happy to be convinced the other way. @MarcoFalke - I'm not sure I fully understand your comment though. As things currently stand with this PR, the interface for changing datadir is to add an argument to TestNode.cli(). How would you propose to make that different?

    MarcoFalke commented at 7:11 AM on August 30, 2017:

    In python every member is public, so changing the datadir is as easy as self.nodes[0].cli.datadir = './new' # Overwrite datadir, so we pass in rpcuser and password through stdin

    jnewbery commented at 2:01 PM on August 30, 2017:

    yes, that works. I think I still prefer making TestNodeCLI callable and adding datadir as an argument.

    MarcoFalke commented at 2:10 PM on August 30, 2017:

    It is not like we need to pass in anything to TestNodeCLI. The datadir change is only needed for the test in this pull.

    jnewbery commented at 2:15 PM on August 30, 2017:

    Have you looked at https://github.com/jnewbery/bitcoin/commit/5bfd5e0d47f89f4c2e9becf266876c0c6cc62787 ? Passing arguments to TestNodeCLI seems like a good way to pass bitcoin-cli command line arguments.

    (I'm not arguing forcefully for the change in https://github.com/jnewbery/bitcoin/commit/5bfd5e0d47f89f4c2e9becf266876c0c6cc62787 and I'm happy to be shown a different way to do this)

    MarcoFalke commented at 2:25 PM on August 30, 2017:

    I did not.

    This also help with selecting the rpcwallet, so making it callable is more useful than I thought.

  22. in test/functional/test_framework/test_node.py:169 in f275cf6501 outdated 
     162 | @@ -155,13 +163,16 @@ def dispatcher(*args, **kwargs):
 163 |  
 164 |      def send_cli(self, command, *args, **kwargs):
 165 |          """Run bitcoin-cli command. Deserializes returned string as python object."""
 166 | -
 167 | +        input = None
 168 | +        if "input" in kwargs:
 169 | +            input = kwargs["input"]
 170 | +            del kwargs["input"]
    ryanofsky commented at 6:24 PM on August 29, 2017:

    In commit "[test] Add support for custom arguments to TestNodeCLI"

    You could replace these four lines with input = kwargs.pop("input", None), or change the function signature to def send_cli(self, command, *args, input=None, **kwargs) to get the same result.

    Probably though it is better just to pass input to the TestNodeCLI constructor as John suggests.

  23. MarcoFalke commented at 7:41 PM on September 5, 2017: member

    @promag Are you still working on this. I think what is missing is setting up the temp datadir and addressing @ryanofsky feedback..

  24. promag force-pushed on Sep 6, 2017
  25. promag force-pushed on Sep 6, 2017
  26. promag force-pushed on Sep 6, 2017
  27. promag commented at 3:59 PM on September 6, 2017: member

    Thanks all for the great feedback. These are the relevant changes:

    • keep -datadir
    • input=... moved to .cli(), although -input is not bitcoin-cli argument 🙄
    • added tests for invalid authentication
  28. promag force-pushed on Sep 6, 2017
  29. in test/functional/test_framework/util.py:80 in ad730f55b1 outdated 
      75 | +        kwds**: named arguments for the function.
  76 | +    """
  77 | +    try:
  78 | +        fun(*args, **kwds)
  79 | +    except CalledProcessError as e:
  80 | +        if (returncode is not None) and (returncode != e.returncode):
    jnewbery commented at 7:14 PM on September 6, 2017:

    You can remove (returncode is not None) here and (output is not None) below. The only reason I allowed assert_raises_jsonrpc() callers to not specify returncode or output was to be backwards compatible with existing tests which didn't specify those. As far as I'm concerned, tests should always assert return codes and error messages if they're expecting.

    (we can probably remove the same from assert_raises_jsonrpc() as well in a separate PR now)

    promag commented at 7:56 PM on September 6, 2017:

    Great, will do.

  30. in test/functional/test_framework/util.py:10 in ad730f55b1 outdated 
       6 | @@ -7,6 +7,7 @@
   7 |  from base64 import b64encode
   8 |  from binascii import hexlify, unhexlify
   9 |  from decimal import Decimal, ROUND_DOWN
  10 | +from subprocess import CalledProcessError
    jnewbery commented at 7:15 PM on September 6, 2017:

    nit: my personal preference is for imports to be sorted by module name (ie from subprocess... should be between import re and import time)

    promag commented at 7:52 PM on September 6, 2017:

    My bad, that's also my preference.

  31. in test/functional/test_framework/util.py:61 in ad730f55b1 outdated 
      57 | @@ -57,18 +58,44 @@ def assert_raises_message(exc, message, fun, *args, **kwds):
  58 |      else:
  59 |          raise AssertionError("No exception raised")
  60 |  
  61 | +def assert_raises_process(returncode, output, fun, *args, **kwds):
    jnewbery commented at 7:17 PM on September 6, 2017:

    nit: perhaps assert_raises_process_error()?

    promag commented at 7:52 PM on September 6, 2017:

    No problem.

  32. in test/functional/bitcoin_cli.py:28 in ad730f55b1 outdated 
      20 | @@ -21,5 +21,15 @@ def run_test(self):
  21 |  
  22 |          assert_equal(cli_get_info, rpc_get_info)
  23 |  
  24 | +        user, password = get_auth_cookie(self.nodes[0].datadir)
  25 | +
  26 | +        self.log.info("Test -stdinrpcpass option")
  27 | +        assert_equal(0, self.nodes[0].cli('-rpcuser=%s' % user, '-stdinrpcpass', input=password).getblockcount())
  28 | +        assert_raises_process(1, "incorrect rpcuser or rpcpassword", self.nodes[0].cli('-rpcuser=%s' % user, '-stdinrpcpass', input="xpto").echo)
    jnewbery commented at 7:19 PM on September 6, 2017:

    Thanks for teaching me some Portuguese: https://pt.wikipedia.org/wiki/X.P.T.O. 🙂

  33. jnewbery commented at 7:27 PM on September 6, 2017: member

    A few nits inline. Tested ACK ad730f55b182549c5e95d6cc09be4474a1c3152a

  34. promag force-pushed on Sep 6, 2017
  35. laanwj assigned laanwj on Sep 6, 2017
  36. Fix style in -stdin and -stdinrpcpass handling 7696841329
  37. [test] Improve assert_raises_jsonrpc docstring e1274947d4
  38. [test] Add support for custom arguments to TestNodeCLI 5c18a84b9a
  39. [test] Add assert_raises_process_error to assert process errors 232e3e8471
  40. [test] Replace check_output with low level version ce379b47b9
  41. [test] Add bitcoin-cli -stdin and -stdinrpcpass functional tests 29e1dfbd97
  42. promag force-pushed on Sep 6, 2017
  43. laanwj merged this on Sep 6, 2017
  44. laanwj closed this on Sep 6, 2017
  45. laanwj referenced this in commit 645a7ecc0b on Sep 6, 2017
  46. MarcoFalke referenced this in commit 812c870043 on Oct 3, 2017
  47. MarcoFalke referenced this in commit e0bfd28de2 on Oct 3, 2017
  48. MarcoFalke referenced this in commit e38211f5e8 on Oct 3, 2017
  49. MarcoFalke referenced this in commit 2b97b36e5d on Oct 3, 2017
  50. MarcoFalke referenced this in commit de1d8c04ab on Oct 3, 2017
  51. PastaPastaPasta referenced this in commit 74801efebf on Sep 23, 2019
  52. PastaPastaPasta referenced this in commit a1b2915923 on Sep 24, 2019
  53. PastaPastaPasta referenced this in commit 502b1c0387 on Nov 19, 2019
  54. PastaPastaPasta referenced this in commit 7329569cda on Nov 21, 2019
  55. PastaPastaPasta referenced this in commit 82035124bf on Dec 9, 2019
  56. PastaPastaPasta referenced this in commit e9355f9eec on Jan 1, 2020
  57. PastaPastaPasta referenced this in commit 871903f0d2 on Jan 2, 2020
  58. PastaPastaPasta referenced this in commit 946eb1599f on Jan 2, 2020
  59. PastaPastaPasta referenced this in commit 8aeb9e2f86 on Jan 2, 2020
  60. PastaPastaPasta referenced this in commit 9ae12e04e2 on Jan 2, 2020
  61. PastaPastaPasta referenced this in commit 39c2079743 on Jan 2, 2020
  62. PastaPastaPasta referenced this in commit eac4ea76e4 on Jan 2, 2020
  63. PastaPastaPasta referenced this in commit 7214f07e78 on Jan 3, 2020
  64. PastaPastaPasta referenced this in commit 3b1696a7e6 on Jan 4, 2020
  65. PastaPastaPasta referenced this in commit 09e4b00321 on Jan 12, 2020
  66. PastaPastaPasta referenced this in commit 1e1a79a68f on Jan 12, 2020
  67. PastaPastaPasta referenced this in commit 5de97013f2 on Jan 12, 2020
  68. ckti referenced this in commit 6ae78314c7 on Mar 28, 2021
  69. DrahtBot locked this on Sep 8, 2021
Contributors
promaglaanwjMarcoFalkejnewberyryanofsky
Labels
Tests
Linked (view graph)
#7550 rpc: Input-from-stdin mode for bitcoin-cli#10997 RPC: Add option -stdinrpcpass to bitcoin-cli to allow RPC password to be read from standard input
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me