test: replace tx hash with txid in rawtransaction test #16078

LongShao007 commented at 12:24 PM on May 23, 2019: contributor

The transaction hash is different from txid for witness transactions, so we should use txid instead of hash.

replace tx hash with txid in test rawtransaction a65dafa8f1

fanquake added the label Tests on May 23, 2019

MarcoFalke commented at 12:54 PM on May 23, 2019: member

ACK a65dafa8f12b3776328ace5cef24cb3fd126cfb1

instagibbs commented at 1:54 PM on May 23, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16078/commits/a65dafa8f12b3776328ace5cef24cb3fd126cfb1

MarcoFalke commented at 2:41 PM on May 23, 2019: member

Please remove the legacy setting with the following diff:

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 6a23891611..4338675270 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -42,7 +42,11 @@ class RawTransactionsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
-        self.extra_args = [["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"]]
+        self.extra_args = [
+            ["-txindex"],
+            ["-txindex"],
+            ["-txindex"],
+        ]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -438,7 +442,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         rawTx = self.nodes[2].createrawtransaction(inputs, outputs)
         rawTxSigned = self.nodes[2].signrawtransactionwithwallet(rawTx)
         assert_equal(rawTxSigned['complete'], True)
-        # 1000 sat fee, ~200 b transaction, fee rate should land around 5 sat/b = 0.00005000 BTC/kB
+        # 1000 sat fee, ~100 b transaction, fee rate should land around 10 sat/b = 0.00010000 BTC/kB
         # Thus, testmempoolaccept should reject
         testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.00001000)[0]
         assert_equal(testres['allowed'], False)
@@ -446,9 +450,9 @@ class RawTransactionsTest(BitcoinTestFramework):
         # and sendrawtransaction should throw
         assert_raises_rpc_error(-26, "absurdly-high-fee", self.nodes[2].sendrawtransaction, rawTxSigned['hex'], 0.00001000)
         # And below calls should both succeed
-        testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.00007000')[0]
+        testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.00070000')[0]
         assert_equal(testres['allowed'], True)
-        self.nodes[2].sendrawtransaction(hexstring=rawTxSigned['hex'], maxfeerate='0.00007000')
+        self.nodes[2].sendrawtransaction(hexstring=rawTxSigned['hex'], maxfeerate='0.00070000')
 
 
 if __name__ == '__main__':

MarcoFalke added this to the milestone 0.19.0 on May 23, 2019

MarcoFalke renamed this:
~~replace tx hash with txid in test rawtransaction~~
test: replace tx hash with txid in rawtransaction test
on May 23, 2019

gmaxwell commented at 1:46 AM on May 24, 2019: contributor

Hash is the stronger of the two-- if hash is the same txid will be the same. Is weakening the test the correct change here? (I have no idea, I just don't see a justification).

I am not suggesting that weakening the tests is bad: overly exact tests have greatly diminished value when their exactitude turns them into hyper-active "something changed" detectors when they would otherwise be more useful as "something isn't right" detectors if constructed more in terms of invariants instead of strict behaviour.

As a plea to contributors: The code itself tells us what it does but it less often tells us why. The essential thing for commit messages and pull requests text to do is to tell us why. Of course, summarizing "what" can be good too. "Why" is also essential to review because if the why doesn't make sense the what probably doesn't matter. Sometimes things are really self apparent, but when in doubt no one was ever hurt by a little more explanation.

sipa commented at 1:55 AM on May 24, 2019: member

@gmaxwell txid is correct here, as it's about arguments to the gettransansaction RPC. It just happens to work with non-witness transactions, as hash equals txid there.

gmaxwell commented at 2:02 AM on May 24, 2019: contributor

@sipa Thanks!

Concept ACK

LongShao007 force-pushed on May 24, 2019

remove parameters -addresstype=legacy in rpc_rawtransaction test 0784af16ef

LongShao007 force-pushed on May 24, 2019

LongShao007 commented at 9:47 AM on May 24, 2019: contributor

I did it @MarcoFalke

MarcoFalke merged this on May 24, 2019

MarcoFalke closed this on May 24, 2019

MarcoFalke referenced this in commit 854ffcae80 on May 24, 2019

MarcoFalke commented at 10:53 AM on May 24, 2019: member

Indeed, it would fail (after git revert a65dafa8f1) using the hash instead of the txid:

Traceback (most recent call last):
...
    self.run_test()
  File "./test/functional/rpc_rawtransaction.py", line 369, in run_test
    assert_equal(self.nodes[0].getrawtransaction(txHash), rawTxSigned['hex'])
test_framework.authproxy.JSONRPCException: No such mempool or blockchain transaction. Use gettransaction for wallet transactions. (-5)

sidhujag referenced this in commit f0b4a70219 on May 25, 2019

jasonbcox referenced this in commit 8244813e38 on Oct 22, 2020

Munkybooty referenced this in commit 2627b1da53 on Oct 17, 2021

Munkybooty referenced this in commit bba09d2393 on Oct 25, 2021

Munkybooty referenced this in commit d23f559586 on Oct 26, 2021

Munkybooty referenced this in commit a9d4ca1f2a on Nov 9, 2021

pravblockc referenced this in commit 7f027ba375 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021