qa: Add getrawtransaction in_active_chain=False test #11838

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1712-qaRpcRawTx changing 2 files +15 −11

MarcoFalke commented at 4:03 PM on December 6, 2017: member

#10275 accidentally forgot to add a test for in_active_chain==False.

This adds a test and also removes the special casing of blockhash.IsNull(), which makes no sense imo.
qa: Add getrawtransaction in_active_chain=False test fa4c16d2e7
MarcoFalke added the label Tests on Dec 6, 2017
MarcoFalke added the label RPC/REST/ZMQ on Dec 6, 2017
promag commented at 10:46 PM on December 6, 2017: member

removes the special casing of blockhash.IsNull(), which makes no sense

Agree, ParseHashV throws.

in test/functional/rawtransactions.py:81 in fa4c16d2e7

  77 | +        # Undo the blocks and check in_active_chain
  78 | +        self.nodes[0].invalidateblock(block1)
  79 | +        gottx = self.nodes[0].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
  80 | +        assert_equal(gottx['in_active_chain'], False)
  81 | +        self.nodes[0].reconsiderblock(block1)
  82 | +        assert_equal(self.nodes[0].getbestblockhash(), block2)

promag commented at 10:57 PM on December 6, 2017:

Nit, IMO this check is not really necessary in the context of this test (it's testing reconsiderblockworked). The same could be done above after invalidateblock(block1).

But I guess you want to be sure the state is the same for the remaining script.

promag commented at 10:58 PM on December 6, 2017: member

ACK fa4c16d.
kallewoof commented at 1:52 AM on December 7, 2017: member

utACK fa4c16d2e72a8ec1032da49a68c9913c2595dbfe
laanwj commented at 4:37 PM on December 7, 2017: member

utACK fa4c16d
laanwj merged this on Dec 7, 2017
laanwj closed this on Dec 7, 2017
laanwj referenced this in commit 3e50024120 on Dec 7, 2017

in src/rpc/rawtransaction.cpp:157 in fa4c16d2e7

 154 | @@ -155,14 +155,12 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
 155 |  
 156 |      if (!request.params[2].isNull()) {
 157 |          uint256 blockhash = ParseHashV(request.params[2], "parameter 3");

jnewbery commented at 6:49 PM on December 7, 2017:

nit: suggest you add a comment here to say that ParseHashV() will throw if called with a string which isn't a 64 length hex strong.

in test/functional/rawtransactions.py:75 in fa4c16d2e7

  71 | @@ -72,7 +72,13 @@ def run_test(self):
  72 |          assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, True)
  73 |          assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, "foobar")
  74 |          assert_raises_rpc_error(-8, "parameter 3 must be of length 64", self.nodes[0].getrawtransaction, tx, True, "abcd1234")
  75 | -        assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
  76 | +        assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "0000000000000000000000000000000000000000000000000000000000000000")

jnewbery commented at 6:57 PM on December 7, 2017:

nit: prefer assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "0" * 64) to make it clear that this is a valid length 64 hex string

in test/functional/rawtransactions.py:79 in fa4c16d2e7

  71 | @@ -72,7 +72,13 @@ def run_test(self):
  72 |          assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, True)
  73 |          assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, "foobar")
  74 |          assert_raises_rpc_error(-8, "parameter 3 must be of length 64", self.nodes[0].getrawtransaction, tx, True, "abcd1234")
  75 | -        assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
  76 | +        assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "0000000000000000000000000000000000000000000000000000000000000000")
  77 | +        # Undo the blocks and check in_active_chain
  78 | +        self.nodes[0].invalidateblock(block1)
  79 | +        gottx = self.nodes[0].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
  80 | +        assert_equal(gottx['in_active_chain'], False)

jnewbery commented at 7:02 PM on December 7, 2017:

nit: slight preference for assert not gottx['in_active_chain']

it'd also be nice to test the positive case (call getrawtransaction with a blockhash in the current chain and assert gottx['in_active_chain']

jnewbery commented at 7:04 PM on December 7, 2017: member

tested ACK fa4c16d2e72a8ec1032da49a68c9913c2595dbfe. A few nits inline.
MarcoFalke deleted the branch on Feb 22, 2018
PastaPastaPasta referenced this in commit 6893b12c29 on Feb 13, 2020
PastaPastaPasta referenced this in commit 7ec4952dbd on Feb 27, 2020
PastaPastaPasta referenced this in commit 6cf2ea19b2 on Feb 27, 2020
ckti referenced this in commit 1b2db678a3 on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10275 [rpc] Allow fetching tx directly from specified block in getrawtransaction