Improve createrawtransaction functional tests #11877

promag commented at 3:01 PM on December 12, 2017: member

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

Credit to @laanwj for multidict implementation.

rpc: Validate replaceable type in createrawtransaction 320669a363

promag commented at 3:05 PM on December 12, 2017: member

Relates to #11872 (https://github.com/bitcoin/bitcoin/pull/11872#issuecomment-351024184).

in test/functional/rawtransactions.py:86 in 0df137b8c7 outdated

  72 | +        assert_raises_rpc_error(-3, "Expected type object", self.nodes[0].createrawtransaction, [], 'foo')
  73 | +        assert_raises_rpc_error(-8, "Data must be hexadecimal string", self.nodes[0].createrawtransaction, [], {'data': 'foo'})
  74 | +        assert_raises_rpc_error(-5, "Invalid Bitcoin address", self.nodes[0].createrawtransaction, [], {'foo': 0})
  75 | +        assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].createrawtransaction, [], {address: 'foo'})
  76 | +        assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].createrawtransaction, [], {address: -1})
  77 | +        assert_raises_rpc_error(-8, "Invalid parameter, duplicated address: %s" % address, self.nodes[0].createrawtransaction, [], multidict([(address, 1), (address, 1)]))

laanwj commented at 3:06 PM on December 12, 2017:

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

promag commented at 3:08 PM on December 12, 2017:

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

laanwj commented at 3:09 PM on December 12, 2017:

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

laanwj commented at 3:09 PM on December 12, 2017: member

utACK 0df137b

laanwj added the label Tests on Dec 12, 2017

jonasschnelli approved

jonasschnelli commented at 6:47 PM on December 12, 2017: contributor

utACK 0df137b8c719a904c792240a0e373ca38c334443

in test/functional/rawtransactions.py:19 in 0df137b8c7 outdated

  14 | @@ -15,6 +15,16 @@
  15 |  from test_framework.test_framework import BitcoinTestFramework
  16 |  from test_framework.util import *
  17 |  
  18 | +
  19 | +class multidict(dict):

jnewbery commented at 10:06 PM on December 12, 2017:

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."""

laanwj commented at 5:53 AM on December 13, 2017:

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.

promag commented at 2:52 PM on December 13, 2017:

Done.

jnewbery commented at 10:07 PM on December 12, 2017: member

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

in test/functional/rawtransactions.py:66 in 0df137b8c7 outdated

  48 | @@ -39,6 +49,41 @@ def run_test(self):
  49 |          self.nodes[0].generate(5)
  50 |          self.sync_all()
  51 |  
  52 | +        # Test `createrawtransaction` required parameters
  53 | +        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction)
  54 | +        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
  55 | +
  56 | +        # Test `createrawtransaction` invalid extra parameters
  57 | +        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')

MarcoFalke commented at 12:11 AM on December 13, 2017:

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

promag commented at 9:52 AM on December 13, 2017:

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)

MarcoFalke commented at 3:06 PM on December 13, 2017:

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

MarcoFalke commented at 12:13 AM on December 13, 2017: member

Concept ACK.

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

test: Add createrawtransaction functional tests 88af5028ad

promag force-pushed on Dec 13, 2017

MarcoFalke commented at 3:23 PM on December 13, 2017: member

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

jnewbery commented at 4:07 PM on December 13, 2017: member

Tested ACK 88af5028ad3de71c8b86b50cb1c6bdd57c1ba6e5. Comment looks good :slightly_smiling_face:

laanwj merged this on Dec 13, 2017

laanwj closed this on Dec 13, 2017

laanwj referenced this in commit d4991c0cbb on Dec 13, 2017

PastaPastaPasta referenced this in commit 65e657a56c on Jan 17, 2020

PastaPastaPasta referenced this in commit 6fb08431bb on Jan 22, 2020

PastaPastaPasta referenced this in commit 40994d0142 on Jan 22, 2020

PastaPastaPasta referenced this in commit c3469855dc on Jan 29, 2020

PastaPastaPasta referenced this in commit 42a198ec53 on Jan 29, 2020

PastaPastaPasta referenced this in commit 132ad56ca8 on Jan 29, 2020

PastaPastaPasta referenced this in commit 212496c202 on Jan 31, 2020

ckti referenced this in commit cb14997f52 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021