Improve createrawtransaction functional tests #11877

promag · 2017-12-12T15:01:37Z

This was motivated by the Invalid parameter, duplicated address test.

Credit to @laanwj for multidict implementation.

promag · 2017-12-12T15:05:33Z

laanwj · 2017-12-12T15:06:34Z

test/functional/rawtransactions.py

+        assert_raises_rpc_error(-5, "Invalid Bitcoin address", self.nodes[0].createrawtransaction, [], {'foo': 0})
+        assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].createrawtransaction, [], {address: 'foo'})
+        assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].createrawtransaction, [], {address: -1})
+        assert_raises_rpc_error(-8, "Invalid parameter, duplicated address: %s" % address, self.nodes[0].createrawtransaction, [], multidict([(address, 1), (address, 1)]))


So there is already a check for duplicated address?
That's great!

Yes, createrawtransaction and sendmany RPC's check for duplicated address.

Indeed! I hadn't expected a check here because passing this is tricky in JSON object format.

laanwj · 2017-12-12T15:09:51Z

utACK 0df137b

jonasschnelli

utACK 0df137b8c719a904c792240a0e373ca38c334443

jnewbery

Tested ACK. Great coverage - thanks @promag . One request for an additional comment.

jnewbery · 2017-12-12T22:06:43Z

test/functional/rawtransactions.py

@@ -15,6 +15,16 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import *

+
+class multidict(dict):


Can you add a comment for this please? Something like:

"""Dictionary that allows duplicate keys. Constructed with a list of (key, value) tuples. When dumped by the json module, will output invalid json with repeated keys, eg: >>> json.dumps(multidict([(1,2),(1,2)]) '{"1": 2, "1": 2}' Used to test calls to rpc methods with invalid repeated keys in the json object."""

Yeah submitting JSON objects in deterministic order and with potentially duplicated keys is the only thing it's good for, it's not quite a traditional multidict implementation.

maflcko · 2017-12-13T00:11:00Z

test/functional/rawtransactions.py

+        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
+
+        # Test `createrawtransaction` invalid extra parameters
+        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')


nit: My personal style preference is to use named args when passing unnamed parameters. Not sure what others think.

In this case I'm not sure, because that will throw Unknown named parameter. But for other cases you mean?

assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, inputs=[], outputs={}, locktime=0, replaceable=False)

Ah, right. Won't work in this case.

maflcko · 2017-12-13T00:13:05Z

Concept ACK.

maflcko · 2017-12-13T15:23:08Z

According to what others reviewed, only change should be adding a comment, so trivial to re-ACK. (ping @jnewbery )

jnewbery · 2017-12-13T16:07:43Z

Tested ACK 88af502. Comment looks good 🙂

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

@laanwj

88af502 test: Add createrawtransaction functional tests (João Barbosa) 27c6199 test: Add multidict to support dictionary with duplicate key (laanwj) (João Barbosa) 320669a rpc: Validate replaceable type in createrawtransaction (João Barbosa) Pull request description: This was motivated by the `Invalid parameter, duplicated address` test. Credit to @laanwj for `multidict` implementation. Tree-SHA512: a87139ae11004b73b467db1e8a072b75e23a0622b173a5668eed383b3575d8abc709817ddd2dfdc53f55afc90750fb61331199ad5de38c1ef6d482f2bc220f74

rpc: Validate replaceable type in createrawtransaction

320669a

laanwj reviewed Dec 12, 2017

View reviewed changes

laanwj mentioned this pull request Dec 12, 2017

[rpc] createrawtransaction: Accept sorted outputs #11872

Merged

laanwj added the Tests label Dec 12, 2017

jonasschnelli approved these changes Dec 12, 2017

View reviewed changes

jnewbery reviewed Dec 12, 2017

View reviewed changes

maflcko reviewed Dec 13, 2017

View reviewed changes

promag added 2 commits December 13, 2017 14:51

test: Add multidict to support dictionary with duplicate key (laanwj)

27c6199

test: Add createrawtransaction functional tests

88af502

promag force-pushed the 2017-12-createrawtransaction branch from 0df137b to 88af502 Compare December 13, 2017 14:52

laanwj merged commit 88af502 into bitcoin:master Dec 13, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Improve createrawtransaction functional tests #11877

Improve createrawtransaction functional tests #11877

Uh oh!

Conversation

promag commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 12, 2017

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

jnewbery commented Dec 13, 2017

Uh oh!

Uh oh!

promag commented Dec 12, 2017 •

edited

Loading

promag commented Dec 12, 2017 •

edited

Loading