qa: Support using bitcoind in fedpeg test #415

stevenroose · 2018-09-24T16:32:33Z

This ports the functionality from pegging.py to the feature_fedpeg.py
test. The following two commands are not equivalent:

$ ./qa/rpc-tests/pegging.py dir/to/bitcoin

$ ./qa/pull-tester/rpc-tests.py feature_fedpeg --parent_bitcoin \
    --parent_binpath=bin/to/bitcoin/bitcoind

jtimon · 2018-09-24T16:49:05Z

qa/rpc-tests/feature_fedpeg.py

@@ -47,9 +49,30 @@ def __init__(self):
        self.setup_clean_chain = True
        self.num_nodes = 4

-    def setup_network(self, split=False):
+    def add_options(self, parser):
+        parser.add_option("--bitcoinddir", dest="bitcoinddir", default="",


perhaps use --binpath or something directly instead of appending bitcoind in the test?
That way we could also use another elements binary, perhaps an older one to test compatibility or a newer one for testing rebases.
For that we would need to decouple another option --parent_is_bitcoinlike or something of the sort.

Changed to --parent_bitcoin and --parent_binpath= :)

jtimon · 2018-09-24T16:52:38Z

qa/rpc-tests/feature_fedpeg.py

+        ] for n in range(2)]
+
+        binary = os.path.join(self.options.bitcoinddir, "bitcoind")
+        self.nodes = start_nodes(2, self.options.tmpdir, self.extra_args[:2], binary=binary, chain="regtest")


there's still no -chain=regtest option in normal bitcoind binaries, so in https://github.com/ElementsProject/elements/blob/elements-0.14.1/qa/rpc-tests/test_framework/util.py#L343 you will need to replace '-chain='+chain with just '-regtest' if chain == 'regtest'

Yeah I wanted to try. Is this why it fails? I thought it might just ignore it.

instagibbs · 2018-09-24T18:07:03Z

qa/rpc-tests/feature_fedpeg.py

-        self.nodes = start_nodes(2, self.options.tmpdir, self.extra_args[:2], chain='parent')
+        chain = None
+        if self.options.parent_bitcoin:
+            if self.options.parent_binpath == "":


Shouldn't this be filtered in rpc-tests.py as well? This way a top-level arg there could trigger this, or not, safely.

I don't understand your point..

rpc-tests.py will run all tests in sequence. I'd rather catch this argument error up front instead of waiting for this to be run.

Oh, you can pass these arguments to rpc-tests.py and they are globally set for all tests??

That was my hope, yes

Should be able to just make it some global(?)

Euhm, it seems that that's not possible anyway. If you run rpc-tests.py with those arguments, all other tests will complain that they don't know the arguments:

create_cache.py: error: no such option: --parent_bitcoin

Also, I think for now it makes more sense to only use those flags when running this test specifically..

jtimon · 2018-09-24T19:59:37Z

qa/rpc-tests/test_framework/util.py

-    args = [ binary, '-chain='+chain, "-datadir="+datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-mocktime="+str(get_mocktime()) ]
+    args = [ binary, "-datadir="+datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-mocktime="+str(get_mocktime()) ]
+    if chain != "regtest": #TODO(stevenroose) remove if unnecessary or when bitcoind has -chain
+        args.append("-chain="+chain)


why not?

# Special case for regtest. Remove if bitcoind gets -chain if chain == 'regtest': args.append('-regtest') else: args.append('-chain=' + chain)

Or perhaps just

args.append('-regtest' if chain == 'regtest' else: '-chain=' + chain)

or some more pythonic way to do it (haven't tried this one, but I know there's something like the '?' keyword in C, java and friends).

Yeah I made it conditional and put a todo to remove it :)

stevenroose · 2018-09-25T10:52:00Z

Like @jtimon pointed out, this also allows to use a custom elements binary for the parent chain. F.e. any liquid binary.

jtimon · 2018-09-25T16:00:09Z

qa/rpc-tests/feature_fedpeg.py

+        parser.add_option("--parent_bitcoin", dest="parent_bitcoin", default=False, action="store_true",
+                          help="Parent nodes are Bitcoin")
+
+    def start_node(self, idx):


I'm not so sure this adds a lot of readibility, you could just use:

start_node(n, self.options.tmpdir, self.extra_args[n], chain='sidechain')

in one place and

binary = self.options.parent_binpath if self.options.parent_binpath != "" else None chain = "regtest" if self.options.parent_bitcoin else "parent" start_node(n, self.options.tmpdir, self.extra_args[n], binary=binary, chain=chain)

In the other, at least for the sidechains case seems unnecessarily complicated.

I also use it to restart the node lateron.

jtimon · 2018-09-25T16:02:44Z

qa/rpc-tests/feature_fedpeg.py

            raise Exception("Peg-in with non-matching claim_script should fail.")
        except JSONRPCException as e:
-            print(e.error["message"])
+            print('ERROR:', e.error)


Are you sure removing ['message'] makes the printed error more clear?

It's a bit too noisy imo.

instagibbs · 2018-09-25T16:06:09Z

let's set printtoconsole=0 for both bitcoind/elementsd, otherwise 0.17+ nodes make a ton of noise on the test.

instagibbs · 2018-09-25T16:02:11Z

qa/rpc-tests/feature_fedpeg.py

+                          help="Parent nodes are Bitcoin")
+
+    def start_node(self, idx):
+        is_parent = idx < 2


this isn't very readable, add a comment or make it more explicit in code?

or just get rid of the function as I propose above

instagibbs · 2018-09-25T16:04:31Z

qa/rpc-tests/feature_fedpeg.py

    def setup_network(self, split=False):
+        if self.options.parent_bitcoin and self.options.parent_binpath == "":
+            raise "Can't run with --parent_bitcoin without specifying --parent_binpath"


Unexpected exception caught during testing: TypeError('exceptions must derive from BaseException',)

Can we handle this?

Just put the quote in Exception()

instagibbs · 2018-09-25T16:08:30Z

Let's also fix up our handling of 0.17 validateaddress ala #416

instagibbs · 2018-09-25T18:23:26Z

I just realized this test isn't setting fedpegscript, which results in the peg-in witness stuff being a kind of a no-op... let's set it in the PR. It can just literally be the script in pegging.py:

fedpeg_key="cPxqWyf1HDGpGFH1dnfjz8HbiWxvwG8WXyetbuAiw4thKXUdXLpR"
fedpeg_script="512103dff4923d778550cc13ce0d887d737553b4b58f4e8e886507fc39f5e447b2186451ae"

no need to import it or anything, just set it.

instagibbs · 2018-09-25T18:29:39Z

Here's some suggested fixes: instagibbs@af19891

instagibbs · 2018-09-25T18:32:23Z

this too: instagibbs@d084d36

jtimon · 2018-09-26T00:18:02Z

Can we squash "qa: Fix bug in feature_fedpeg test script"? it isn't really fixing anything, it works the same before and after the change.
Also, more suggestions https://github.com/jtimon/elements/tree/pr-415
Feel free to take some but not others.

jtimon · 2018-09-26T00:21:33Z

Do we really need rpc: "reject non-hex strings for claim_script" here?
We're not calling createrawpegin from the tests (perhaps we should).
Also, it would be nice to test that new error, but it seems orthogonal to the PR.

instagibbs · 2018-09-26T00:22:44Z

@jtimon yes we are, claimpegin calls the raw version.

jtimon · 2018-09-26T00:48:08Z

Ok, either way I had to call it directly from the test to get the 'Given claim_script is not hex' exception. Updated https://github.com/jtimon/elements/tree/pr-415 with that.

instagibbs · 2018-09-26T15:36:26Z

qa/rpc-tests/feature_fedpeg.py

-        parent_pubkey = parent.validateaddress(parent_chain_addr)["pubkey"]
-        parent_chain_p2sh_addr = parent.createmultisig(1, [parent_pubkey])["address"]
-        self.test_pegout(parent_chain_p2sh_addr, sidechain)
+        # print("Test pegout P2SH")


read the commits, perhaps better read the commit tittles in the branch without the squashes: https://github.com/jtimon/elements/tree/pr-415

please don't disable swaths of tests when it's very possible to retain them

…ack -chain

This ports the functionality from pegging.py to the feature_fedpeg.py test. The following two commands are not equivalent: $ ./qa/rpc-tests/pegging.py dir/to/bitcoin $ ./qa/pull-tester/rpc-tests.py feature_fedpeg --parent_bitcoin \ --parent_binpath=bin/to/bitcoin/bitcoind

jtimon · 2018-09-26T17:13:59Z

Updated https://github.com/jtimon/elements/tree/pr-415 with a squashed version that works with 0.17.0

jtimon · 2018-09-26T17:57:00Z

Tested ACK 498877e

jtimon · 2018-09-26T18:00:32Z

oh, and I guess you can add an extra commit getting rid of pegging.py

stevenroose · 2018-09-26T18:29:38Z

Done :)

instagibbs · 2018-09-26T18:29:45Z

qa/rpc-tests/feature_fedpeg.py

-            self.extra_args.append([
-                # '-printtoconsole',
+            args = [
+                "-printtoconsole",


nit: can we set this to 0?

I added a commit to do this. If you want me to squash it with the others, let me know..

You commented out, not put to 0. Bitcoin Core 0.17 defaults to 1.

instagibbs · 2018-09-26T18:33:01Z

@stevenroose Sorry I mean explicitly set to 0. Future Core version are 1 by default.

instagibbs · 2018-09-26T18:58:48Z

We have no coverage of the cookie auth connection to parent node in this test.

instagibbs · 2018-09-26T19:20:03Z

tACK 9545ca3

9545ca3 qa: Disable -printtoconsole in feature_fedpeg (Steven Roose) 1451e86 qa: Remove pegging.py in favor of feature_fedpeg.py (Steven Roose) 498877e QA: Fix feature_fedpeg.py to work with 0.17.0 (Jorge Timón) a7736f8 QA: Add test to get 'Given claim_script is not hex' error (Jorge Timón) 35364b6 rpc: reject non-hex strings for claim_script (Gregory Sanders) 07c617b qa: Support using bitcoind in fedpeg test (Steven Roose) 729969d QA: Special case for regtest, expected to be used for binaries that lack -chain (Jorge Timón)

instagibbs · 2018-09-26T19:33:12Z

I'll follow-up with cookie auth check

stevenroose force-pushed the fedpeg-bitcoind branch from 6fdd17f to 28b89bf Compare September 24, 2018 16:44

jtimon reviewed Sep 24, 2018

View reviewed changes

stevenroose force-pushed the fedpeg-bitcoind branch from 28b89bf to 66b0299 Compare September 24, 2018 17:04

stevenroose changed the title ~~WIP feature_fedpeg with bitoind~~ WIP feature_fedpeg with bitcoind Sep 24, 2018

stevenroose force-pushed the fedpeg-bitcoind branch 4 times, most recently from d6660ac to 4e2ac62 Compare September 24, 2018 17:22

instagibbs reviewed Sep 24, 2018

View reviewed changes

stevenroose force-pushed the fedpeg-bitcoind branch from 4e2ac62 to 1aeed4b Compare September 24, 2018 19:37

jtimon reviewed Sep 24, 2018

View reviewed changes

stevenroose force-pushed the fedpeg-bitcoind branch from 1aeed4b to fe6e6d6 Compare September 25, 2018 09:02

stevenroose changed the title ~~WIP feature_fedpeg with bitcoind~~ qa: Support using bitcoind in fedpeg test Sep 25, 2018

jtimon reviewed Sep 25, 2018

View reviewed changes

stevenroose force-pushed the fedpeg-bitcoind branch from fe6e6d6 to 94c4a27 Compare September 25, 2018 16:06

instagibbs reviewed Sep 25, 2018

View reviewed changes

stevenroose mentioned this pull request Sep 25, 2018

Fix peg functional test compatability with Core 0.17 #416

Closed

stevenroose force-pushed the fedpeg-bitcoind branch from 94c4a27 to e380a7f Compare September 25, 2018 16:36

stevenroose force-pushed the fedpeg-bitcoind branch from e380a7f to 42ab8ed Compare September 25, 2018 19:23

stevenroose force-pushed the fedpeg-bitcoind branch from 42ab8ed to e6d1dec Compare September 26, 2018 15:15

instagibbs reviewed Sep 26, 2018

View reviewed changes

jtimon and others added 5 commits September 26, 2018 19:05

QA: Special case for regtest, expected to be used for binaries that l…

729969d

…ack -chain

rpc: reject non-hex strings for claim_script

35364b6

QA: Add test to get 'Given claim_script is not hex' error

a7736f8

QA: Fix feature_fedpeg.py to work with 0.17.0

498877e

stevenroose force-pushed the fedpeg-bitcoind branch from e6d1dec to 498877e Compare September 26, 2018 17:17

instagibbs reviewed Sep 26, 2018

View reviewed changes

qa: Remove pegging.py in favor of feature_fedpeg.py

1451e86

stevenroose force-pushed the fedpeg-bitcoind branch from c858a77 to 9545ca3 Compare September 26, 2018 19:16

qa: Disable -printtoconsole in feature_fedpeg

9545ca3

instagibbs merged commit 9545ca3 into ElementsProject:elements-0.14.1 Sep 26, 2018

instagibbs mentioned this pull request Sep 26, 2018

Test mainchainrpccookie args in functional tests #420

Closed

stevenroose mentioned this pull request Sep 27, 2018

Integrate pegging.py in qa/rpc-tests/feature_fedpeg.py #127

Closed

stevenroose deleted the fedpeg-bitcoind branch March 25, 2019 14:12

qa: Support using bitcoind in fedpeg test #415

qa: Support using bitcoind in fedpeg test #415

Conversation

stevenroose commented Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 25, 2018 • edited Loading

instagibbs commented Sep 25, 2018

instagibbs commented Sep 25, 2018

instagibbs commented Sep 25, 2018

jtimon commented Sep 26, 2018

jtimon commented Sep 26, 2018

instagibbs commented Sep 26, 2018

jtimon commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Sep 26, 2018

jtimon commented Sep 26, 2018

jtimon commented Sep 26, 2018

stevenroose commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 26, 2018

instagibbs commented Sep 26, 2018

instagibbs commented Sep 26, 2018

instagibbs commented Sep 26, 2018

stevenroose commented Sep 24, 2018 •

edited

Loading

jtimon Sep 24, 2018 •

edited

Loading

instagibbs commented Sep 25, 2018 •

edited

Loading