test: Add test for rpc

emilengler commented at 4:36 pm on November 20, 2019: contributor

A test for #12763 Please only review test/function/rpc_whitelist.py. Other stuff should belong to the original PR

fanquake added the label Tests on Nov 20, 2019

JeremyRubin commented at 6:39 pm on November 20, 2019: contributor

The test looks good so far!

Some other conditions to check:

0rpcwhitelist=user1:A,B,C
1rpcwhitelist=user1:B

results in only B being available, not A or C.

Test empty rpcwhitelist

0rpcwhitelist=user1
1rpcwhitelist=user2:

Test trailing comma

0rpcwhitelist=user1:A,

Test the above where whitelistrpcdefault=1 is set. This case is really difficult to do because the testing framework assumes access to the RPCs for setup. So you can set up setting the args for the test-framework’s rpc credential to be sufficient, but then testing the case where there are no credentialed users seems… hard! But maybe we’re OK with not covering that case via test.

It also looks like there is a new method (since I wrote the original test sketch?) called “add_options”, which you could override to add the command line args… but what’s written works so if it ain’t broke…

DrahtBot commented at 7:18 pm on November 20, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16698 (Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
#16673 (Relog configuration args on debug.log rotation by LarryRuane)
#16115 (On bitcoind startup, write config args to debug.log by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

emilengler commented at 7:47 pm on November 20, 2019: contributor

@JeremyRubin The test was written from scratch with a method taken from rpc_users.py I will add your suggestions now

JeremyRubin commented at 8:31 pm on November 20, 2019: contributor

Ah yeah – I just mean that I think the add_options method appears to be new – newer than rpc_users too I guess? Not sure!

JeremyRubin commented at 8:44 pm on November 20, 2019: contributor

BTW (maybe a minority opinion here) but those linters are super pesky.

You need to fix the typos on “permissions” in order for the tests to run.

emilengler commented at 8:49 pm on November 20, 2019: contributor

@JeremyRubin Ok, will change that. I also added the exceptions mentioned in your first comment. See f012f12

emilengler commented at 8:52 pm on November 20, 2019: contributor

BTW where you’ve found the type you meant in the comment? Any line number please?

MarcoFalke commented at 9:01 pm on November 20, 2019: member

The linters fail because trailing white space was added. Trailing white space is causing issues with git and many editors. The type can be left as is if you want to.

emilengler commented at 9:06 pm on November 20, 2019: contributor

Oh now I understand what you mean :D Should be fixed now

fanquake commented at 9:19 pm on November 20, 2019: member

@emilengler You need need to squash your commits. If you’re fixing things like typos and whitespace, you’ll always need to squash, so it’s best to just do it before pushing to GitHub.

emilengler force-pushed on Nov 20, 2019

emilengler commented at 9:21 pm on November 20, 2019: contributor

@fanquake Done

emilengler force-pushed on Nov 20, 2019

in test/functional/rpc_whitelist.py:78 in e3825f59dd outdated

72+            i = 0
73+            while i < len(permissions):
74+                if permissions[i] == '':
75+                    permissions.pop(i)
76+
77+                i += 1

JeremyRubin commented at 9:48 pm on November 20, 2019:

just use import re; filter(lambda x: x=='', re.split(" |,", user[2]))?

in test/functional/rpc_whitelist.py:79 in e3825f59dd outdated

74+                if permissions[i] == '':
75+                    permissions.pop(i)
76+
77+                i += 1
78+            self.log.info("[" + user[0] + "]: Testing a permitted permission")
79+            assert_equal(200, rpccall(self.nodes[0], user, permissions[randint(0, len(permissions) - 1)]).status)

JeremyRubin commented at 9:49 pm on November 20, 2019:

I like what you’re thinking here with picking a random permission (especially if we eventually make this scale out to test a larger set), but I think for functional tests we value the determinism & repeatability a bit more. So maybe as a compromise, test everything in permissions, and then test all of the disallowed (until in the future they are too many).

emilengler commented at 4:17 pm on November 21, 2019:

Ok

emilengler commented at 5:32 pm on November 21, 2019:

Done

JeremyRubin commented at 10:43 pm on November 21, 2019:

I think you left a few instances lower down where you still do random :)

emilengler commented at 6:09 pm on November 22, 2019:

Oh right, forgot the strange tests

emilengler commented at 6:47 pm on November 22, 2019:

Done

JeremyRubin changes_requested

JeremyRubin commented at 9:52 pm on November 20, 2019: contributor

Looking better! Still need to test rpcwhitelistdefault=1. Tagging @jnewbery who might have some ideas on how to do that?

Maybe one more test to add:

rpcwhitelist=user1:A,A

Just make sure that A is permitted/doesn’t crash when named twice

MarcoFalke commented at 9:58 pm on November 20, 2019: member

It might be possible to adjust def wait_for_rpc_connection to accept the 403 error code as acceptable signal that indicates the rpc thread is running.

emilengler force-pushed on Nov 21, 2019

emilengler force-pushed on Nov 22, 2019

emilengler commented at 6:50 pm on November 22, 2019: contributor

The test is done. The only problem is to test rpcwhitelistdefault=1 There are two possible ways:

The easy way: Just add another test (file)
The hard way: Restart bitcoind with other args during the test. Any ideas on how to do this @MarcoFalke?

emilengler force-pushed on Nov 22, 2019

JeremyRubin commented at 10:18 pm on November 22, 2019: contributor

You don’t need a new file, you can just do a new subclass of the test framework in that file and call one after the other in Main.

It will be a bit tough because you can’t actually stop a node using RPC with rpcwhitelistdefault=1 and no auth with stop permitted. So you’d need to write a bit of code to kill by PID.

emilengler commented at 2:05 am on November 23, 2019: contributor

You don’t need a new file, you can just do a new subclass of the test framework in that file and call one after the other in Main.

Tried that and failed, will try it again tomorrow

JeremyRubin commented at 5:13 am on December 6, 2019: contributor

@emilengler checking in here – stuck?

emilengler commented at 2:37 pm on December 6, 2019: contributor

@JeremyRubin The test works fine. The only problem is testing rpcwhitelistenable=1. The problem is that I haven’t found a way to restart bitcoind. It would need to be separate test.

JeremyRubin commented at 3:56 pm on December 6, 2019: contributor

I’m OK with that; great work preparing these tests. @MarcoFalke given the strong concept ACKs+utacks/lgtms on the original PR, now with @emilengler’s work on tests I think this is in a mergeable state. Do you prefer for me to squash the existing branch (I don’t want to make people have to re-ack the squash needlessly)?

emilengler commented at 0:43 am on December 7, 2019: contributor

@emilengler’s work on tests I think this is in a mergeable state. Do you prefer for me to squash the existing branch (I don’t want to make people have to re-ack the squash needlessly)?

I’m a bit afraid that mine will cause merge issues then. Otherwise I will rebase it from your updated branch.

Add RPC Whitelist Feature from #12248 7414d3820c

JeremyRubin commented at 9:17 pm on December 11, 2019: contributor

@emilengler you can rebase

test: Add test for rpc_whitelist d868d09017

emilengler force-pushed on Dec 12, 2019

emilengler commented at 7:22 pm on December 12, 2019: contributor

@JeremyRubin Should be done now

emilengler commented at 1:18 pm on December 13, 2019: contributor

Got merged into master by 2081442c421cc4376e5d7839f68fbe7630e89103

emilengler closed this on Dec 13, 2019

DrahtBot locked this on Dec 16, 2021

test: Add test for rpc_whitelist #17536

Conflicts