test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction #22437

Maybe:

            # 2. invalid parameters - supply txid and string "Flase" (intentionally misspelled "False").

?

I agree that it's unclear. Changed to the following in b0bf8c996034a4:

-            # 6. invalid parameters - supply txid and string "Flase"
-            assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txId, "Flase")
+            # 6. invalid parameters - supply txid and invalid boolean values (strings) for verbose
+            for value in ["True", "False"]:
+                assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid=txId, verbose=value)

This is suspicious to me. Isn't this always true given that gottx is in hex?

This assert tests that the in_active_chain field is absent as expected when the blockhash argument is not passed...see src/rpc/rawtransaction.cpp::getrawtransaction() or bitcoin-cli help getrawtransaction

Just to make myself more clear: L140 ends with ['hex'] so presumably gottx is a string, so testing on L142 whether gottx does not contain in_active_chain seems to be always true.

But maybe I just didn't get your reply. Anyway, if I got it wrong, feel free to just ignore my comment.

Thank you for re-explaining. You are right! Fixing. Thank you!

Updated commit 4e83843f03dad710a0fad7a706e2618a7a69a6b3 to fix. Thanks @kiminuo!

Question: Is this intentionally only on 4 nodes instead of 6? If it is, maybe sync_peers should be called slightly differently not to give the wrong impression.

Question: Is this intentionally only on 4 nodes instead of 6?

Yes, as only nodes 0 to 3 have a mempool. The last two nodes are -blocksonly and sync_mempools() won't work on them. Added a comment in efe13c3b77 to clarify this.

-        self.sync_mempools()
+        self.sync_mempools(self.nodes[0:3])  # nodes 0 to 3 have a mempool

Not originally your code, but variables named txid and txId are very easy to confuse, I think.

+1. Maybe something like txid1, txid2?

I initially planned to respond that this was out of scope, then saw this very issue had tripped me up (https://github.com/bitcoin/bitcoin/pull/22437#discussion_r684653116). Agree! Done in 10a3db049ce2858 when the name is reused or reassigned in the same test function. Doing this also revealed an unused txId assignment; fixed.

I would expect txId instead of txid here. Or can you explain this please?

Good catch! This illustrates why using txid and txId variable names in the same test isn't ideal. Fixed in commit "Test src/validation::GetTransaction() with -blocksonly".

Is it necessary here to have node[2] create 1 block first?

Well spotted! Thanks to the transactions in the next lines, it's not needed and might be an example of the accumulated layers of change in this test file. Removed in commit 8f5c6ba69000b with an explanation in the commit message.

This doesn't seem like getrawtransaction() tests. Maybe test 5 and 6 should be moved into createrawtransaction() test group?

Good idea! Done in a3d8f790633b89a.

I am not sure whether having two txid and txId transactions helps here, given both of them are confirmed. Because of that we are also not getting a test for only mempool transactions.

So maybe we can have a txid confirmed in a block, and then have txId in the mempool to check for both the scenario?

Will make the purpose of two transactions clear.

Also, the names are confusing, something more distinct would be helpful.

Interesting. We use and need all three transactions, AFAICT. Updated the names in the last commit to txid, txid2, and txid3. The test still passes if we sync_blocks() for the second and third transactions instead of sync_all(), but I'd rather not mess with this here.

Took a fresh look this morning, good call! The first conditional in CTransactionRef GetTransaction() was not necessarily being exercised by the tests:

    if (mempool && !block_index) {
        CTransactionRef ptx = mempool->get(hash);
        if (ptx) return ptx;
    }

Added this assert in 7f7e64e30376a2 that covers it:

             # 1. valid parameters - supply txid along with various valid values for verbose
+            if n == 0 or n == 3:
+                # test with a tx in mempool, with and without -txindex
+                tx_in_mempool = self.nodes[n].sendtoaddress(self.nodes[n + 1].getnewaddress(), 0.1)
+                self.nodes[n].getrawtransaction(tx_in_mempool)

Verified by removing that code in GetTransaction() to ensure the new assert fails without it and passes with it.

It seems to me that sync_peers() is not doing anything and all the nodes get the blocks with generate(). This is probably because generate() already broadcasts the blocks in p2p.

Done

Moving the in-mempool transaction tests commit after the simplification commit revealed that removing the self.sync_peers() would be flakey without the presence of the in-mempool transaction. This could be fragile, so no longer removing it in the simplification commit.

What's the reason for having this as a variable instead of passing the literal to assert_raises_rpc_error as an arg?

This variable is invoked twice below in this test, so it clarifies that we check for the same message and also allows reducing the line length to under 120; previously for instance, both lines did not fit in the GitHub display without scrolling (and scrolling is an annoyance with a file or diff of this length, as you have to navigate to the bottom, scroll, then navigate back to the lines).

Thanks for clarifying!

in c7327a6e90: Why is this test removed and re-added in the next commit? I think it would be easier to review if it was kept.

Probably a rebase oversight during the various changes.

Not addressed in the latest force push? The first two commits aren't squashed?

Thanks for having a look. Didn't change any commits, only dropped the last ones to ~preserve the previous review ACKs.

reviewing that two commits are squashed should be as easy as checking that other commits are dropped, no?

squashed

c7327a6e90: Don't all nodes have txindex, except n=3? At least the extra_args would suggest that.

Yes, kept node 0 as it was already in use for the -txindex case, and added node 3 to test without -txindex. Didn't seem worth adding iterating through nodes 1 and 2.

(this does indeed work:)

-        for n in [0, 3, 4, 5]:
+        for n in range(self.num_nodes):
             self.log.info(
-                f"Test getrawtransaction {'with' if n == 0 or n == 5 else 'without'} -txindex,"
+                f"Test getrawtransaction {'with' if n <= 2 or n == 5 else 'without'} -txindex,"
                 f" {'with' if n >= 4 else 'without'} -blocksonly"
             )
             # 1. valid parameters - supply txid along with various valid values for verbose
-            if n == 0 or n == 3:
+            if n <= 3:
                 # with a transaction in the mempool, with and without -txindex
                 tx_in_mempool = self.nodes[n].sendtoaddress(self.nodes[n + 1].getnewaddress(), 0.5)
                 self.nodes[n].getrawtransaction(tx_in_mempool)
-            if n == 0 or n == 5:
+            if n <= 2 or n == 5:
                 # with -txindex

No need to change. I missed that this was only picking two nodes.

75d1e4cb14a4bc293a17e516598a1b6b81dcac4a: If you rebase, you can use self.generate(nodes[node], blocks)

75d1e4c: If you rebase, you can use self.generate(nodes[node], blocks)

Thanks. Dropped the commit.

0e4cb3a4bbbf464376b5cad78e83328444bdcf71: The forth node has a mempool, too?

How does -blocksonly even affect the test, given that all txs are mined into blocks anyway?

If this line remains self.sync_mempools(), the test fails with AssertionError: Mempool sync timed out.

The last commit adds an in-mempool tx.

The last commit adds an in-mempool tx.

The first tx will be mined into a block immediately after. The second test isn't run on the -blocksonly nodes, unless I am missing something.

In general -blocksonly only skips incoming transactions, so anything you can test with the option is also possible by simply querying a txid that doesn't exist at all.

Do you think it would be better coverage to run the in-mempool test on the -blocksonly nodes as well...

@@ -88,7 +88,8 @@ class RawTransactionsTest(BitcoinTestFramework):
         for amount in [1.5, 1.0, 5.0]:
             self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), amount)
-        self.nodes[0].sendtoaddress(self.nodes[3].getnewaddress(), 1)
+        for n in [3, 4, 5]:
+            self.nodes[0].sendtoaddress(self.nodes[n].getnewaddress(), 1)
         self.generate_and_sync(node=0, blocks=5)
@@ -109,16 +110,22 @@ class RawTransactionsTest(BitcoinTestFramework):
             "No such mempool transaction. Use -txindex or provide a block hash to enable"
             " blockchain transaction queries. Use gettransaction for wallet transactions."
         )
+        err_msg_2 = "No such mempool or blockchain transaction. Use gettransaction for wallet transactions."
         for n in [0, 3, 4, 5]:
             self.log.info(
                 f"Test getrawtransaction {'with' if n == 0 or n == 5 else 'without'} -txindex,"
                 f" {'with' if n >= 4 else 'without'} -blocksonly"
             )
-            # 1. valid parameters - supply txid along with various valid values for verbose
+            # With a transaction in the mempool
+            tx_in_mempool = self.nodes[n].sendtoaddress(self.nodes[n].getnewaddress(), 0.5)
             if n == 0 or n == 3:
-                # with a transaction in the mempool, with and without -txindex
-                tx_in_mempool = self.nodes[n].sendtoaddress(self.nodes[n + 1].getnewaddress(), 0.5)
+                # with and without -txindex
                 self.nodes[n].getrawtransaction(tx_in_mempool)
+            else:
+                # -blocksonly, with and without -txindex
+                assert_raises_rpc_error(-5, err_msg if n == 4 else err_msg_2, self.nodes[n].getrawtransaction, tx_in_mempool)

or do you prefer dropping the -blocksonly nodes and testing the other nodes with a non-existing tx.

Unless I am missing something, the goal of this test is to cover GetTransaction, which itself is unaffected by the blocksonly option. Thus, there is no need to test this option. (Otherwise, there would also be reason to test with mocktime, minrelaytxfee, ...)

The -blocksonly nodes allow us to add missing test coverage for rpc getrawtransaction error cases like in my #22437 (review) above. We could maybe pass non-existing txids instead, but it seems better to test actual -blocksonly behavior.

I have a branch to add that coverage and some other improvements, and rather than increase the size of this pull and invalidate the existing review, I can propose it as a follow-up.

it seems better to test actual -blocksonly behavior.

blocksonly is a network option, so it seems confusing to test it's behaviour inside a raw transaction test.

My thought was that -blocksonly effectively disables mempool functionality, which for end-to-end testing relates to how getrawtransaction behaves when a node operator running -blocksonly calls getrawtransaction, which is valid with either txindex and/or passing blockhash if the tx is in a block, so it seems good to have explicit coverage that serves as a regression test and sanity check. Passing a wrong txid could be added as well, where relevant, to verify the same outcome.

-blocksonly effectively disables mempool functionality

The behaviour of the mempool module isn't affected by blocksonly at all. (All txs that are submitted to the mempool are added/rejected in the same way regardless of blocksonly.) blocksonly is purely a net-processing option to disable incoming tx relay from network peers. It doesn't affect (in)validity of txs submitted to the mempool. For example, when the wallet or rpc submits a tx, it will still be added to the mempool.

As part of this test, if the goal is to get a fully functioning, but empty mempool, it might be easier to run the tests that you want to run at the beginning with an empty mempool. (Or by restarting a node with -nopersistmempool to empty the mempool). Or as mentioned previously by picking a txid that is not currently in the mempool.

If there was an option to completely disable the mempool (i.e. never initialize the class), then it indeed would make a lot of sense to check for that code path.

In testing it is important to be able to draw units/modules and assume that the modules don't interact with each other, other than over their defined interface. This allows to test units/modules separate from each other. If they had to be tested in combination it would lead to path explosion in any real software project, thus making it harder to test meaningful.

Moreover, if the behaviour of one module is required for testing another module, it will make it harder to change the behaviour of that module and only touching the tests of that module.

Or put another way: If -blockonly was relevant to test here, why is -minrelaytxfee (or any other setting) not relevant to test here?

I agree from a unit test point of view, but for end-to-end testing these interactions can be good to test. For example, I think I've found a parameter interaction that doesn't work as documented (not sure yet, need to look further). That seems like good coverage to have if someone proposes it, it is reviewed, and ready to use. The asmap/addrman interaction is another example. What to do here? Lots of time invested so far. Sure, I could spend even more time to rework this without the coverage and hope for everyone to review it again. Or just drop the work starting with "Test...with -blocksonly" and maybe pick up the rest later in a follow-up.

The asmap/addrman interaction is another example

I think asmap is part of the addrman module, so testing those together in a test makes sense.

Thank u

                                   Traceback (most recent call last):
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/test_framework.py", line 132, in main
                                       self.run_test()
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/rpc_rawtransaction.py", line 84, in run_test
                                       self.getrawtransaction_tests()
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/rpc_rawtransaction.py", line 107, in getrawtransaction_tests
                                       assert_equal(self.nodes[n].getrawtransaction(txId), rawTxSigned['hex'])
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/coverage.py", line 49, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/authproxy.py", line 144, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: No such mempool transaction. Use -txindex or provide a block hash to enable blockchain transaction queries. Use gettransaction for wallet transactions. (-5)

not sure how this could happen, though...

Looks like the test isn't testing anything, as the tx is never included in a block.

If the tx is included, the test will fail:

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 96691b2686..b648012413 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -99,7 +99,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         rawTx = self.nodes[1].createrawtransaction([{'txid': txid, 'vout': vout}], {self.nodes[1].getnewaddress(): 9.999})
         rawTxSigned = self.nodes[1].signrawtransactionwithwallet(rawTx)
         txId = self.nodes[1].sendrawtransaction(rawTxSigned['hex'])
-        self.generate(self.nodes[0], 1)
+        self.generateblock(self.nodes[0], output=self.nodes[0].getnewaddress(), transactions=[rawTxSigned['hex']])
 
         for n in [0, 3]:
             self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex")

Thanks, was working on the part 2 follow-up to this, will have a look.

#24113