Bugfix: QA: Run tests with UPnP disabled #16560

1. 
   luke-jr commented at 11:17 pm on August 6, 2019: member

   Needed for builds configured with –enable-upnp-default
2. Bugfix: QA: Run tests with UPnP disabled

   Needed for builds configured with --enable-upnp-default

   be192e6734
3. fanquake added the label Tests on Aug 6, 2019
4. 
   promag commented at 1:35 am on August 7, 2019: member

   Not quite a bugfix?

   Anyway, without this change:

   0./configure  ... --enable-upnp-default
   1...
   2checking whether to build with support for UPnP... yes
   3checking whether to build with UPnP enabled by default... yes
   4...
   5
   6make
   7...
   

   And the following fails:

   0test/functional/feature_config_args.py
   

   Tested ACK be192e6734d4bd59debbd63bb9b135284494d113

5. 
   practicalswift commented at 11:08 am on August 7, 2019: contributor

   Thanks for finding and addressing this issue @luke-jr – we should take the security of our users seriously.

   Concept ACK @promag If running the tests opens up ports in our users’ firewalls needlessly then that definitely sounds like a bug to me :-)

6. in test/functional/test_framework/test_node.py:94 in be192e6734

   90@@ -91,6 +91,7 @@ def __init__(self, i, datadir, *, rpchost, timewait, bitcoind, bitcoin_cli, cove
   91             "-debugexclude=libevent",
   92             "-debugexclude=leveldb",
   93             "-uacomment=testnode%d" % i,
   94+            "-upnp=0",
   

   ---

   ---

   

   MarcoFalke commented at 11:12 am on August 7, 2019:

   See the comment in line 84

   common args are set in the config file (see initialize_datadir). The args here are only to set the datadir and debug log options.

   ---

   

   promag commented at 0:03 am on August 19, 2019:

   Agree, looks like all tests use the generated bitcoin.conf.
7. 
   promag commented at 9:32 pm on August 7, 2019: member

   @practicalswift I mean it’s not a bug to the end user.
8. 
   laanwj commented at 11:01 am on August 8, 2019: member

   Yes, this seems the correct fix ACK be192e6734d4bd59debbd63bb9b135284494d113 (though yeah, better specify it in the configuration file instead)
9. 
   hebasto commented at 3:13 pm on August 10, 2019: member

   ACK be192e6734d4bd59debbd63bb9b135284494d113

   Verified that after ./configure --enable-upnp-default && make the test/functional/feature_config_args.py fails on master (e47e36cb4975b332a4d9552bc73b031ab8fd6ab1).

   With this PR the test/functional/feature_config_args.py passes.

   Nit: agree with the suggestion to move -upnp option to the bitcoin.conf: https://github.com/bitcoin/bitcoin/blob/e47e36cb4975b332a4d9552bc73b031ab8fd6ab1/test/functional/test_framework/util.py#L289-L305

10. fanquake added the label Waiting for author on Aug 11, 2019
11. 
    fanquake commented at 2:16 am on August 19, 2019: member

    I’ve cherry-picked this change into #16646, but modified it to use bitcoin.conf.
12. fanquake closed this on Aug 19, 2019

    ---

13. laanwj referenced this in commit 27ee0cc5a6 on Aug 19, 2019
14. laanwj removed the label Waiting for author on Oct 24, 2019
15. linuxsh2 referenced this in commit b7e4be7f61 on Aug 11, 2021
16. DrahtBot locked this on Dec 16, 2021

Contributors
luke-jr promag practicalswift MarcoFalke laanwj hebasto fanquake

Labels
Tests