test: replace tx hash with txid in rawtransaction test

[LongShao007]

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

[maflcko]

ACK a65dafa

[instagibbs]

utACK a65dafa

[maflcko]

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__':

[gmaxwell]

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]

@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]

@sipa Thanks!

Concept ACK

[LongShao007]

I did it @MarcoFalke

[maflcko]

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)