test: Use MiniWallet in rpc_rawtransaction.py #25044

        self.requires_wallet = self.is_wallet_compiled()

Interesting. I think the goal of this change it to not run the test at all to avoid wasting resources if a specific wallet type is requested, but not available?

However, the test will still run twice when the wallet isn't compiled: https://cirrus-ci.com/task/5291414786408448?logs=ci#L3574

I think it might be better to fix this by removing this whole method and also consolidate the two tasks into one:

$ git grep 'rpc_rawtransaction.py'
test/functional/test_runner.py:    'rpc_rawtransaction.py --legacy-wallet',
test/functional/test_runner.py:    'rpc_rawtransaction.py --descriptors',

I think the goal of this change it to not run the test at all to avoid wasting resources if a specific wallet type is requested, but not available?

It's more that without this function if you try to run the test with --descriptors but without having sqlite compiled (or, viceversa, legacy but no bdb) the whole test fails...

❯ ./configure --without-bdb
❯ make -j 9
❯ ./test/functional/rpc_rawtransaction.py --legacy-wallet
2022-05-07T21:43:41.365000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7w6fc454
2022-05-07T21:43:41.641000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.setup()
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 295, in setup
    self.setup_network()
  File "/home/daniela/Developer/bitcoin/./test/functional/rpc_rawtransaction.py", line 72, in setup_network
    super().setup_network()
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 389, in setup_network
    self.setup_nodes()
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 416, in setup_nodes
    self.import_deterministic_coinbase_privkeys()
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 433, in import_deterministic_coinbase_privkeys
    self.init_wallet(node=i)
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_framework.py", line 440, in init_wallet
    n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/test_node.py", line 753, in createwallet
    return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/daniela/Developer/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Compiled without bdb support (required for legacy wallets) (-4)
2022-05-07T21:43:41.694000Z TestFramework (INFO): Stopping nodes
2022-05-07T21:43:41.800000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_7w6fc454
2022-05-07T21:43:41.800000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_7w6fc454/test_framework.log
2022-05-07T21:43:41.801000Z TestFramework (ERROR): 
2022-05-07T21:43:41.801000Z TestFramework (ERROR): Hint: Call /home/daniela/Developer/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_7w6fc454' to consolidate all logs
2022-05-07T21:43:41.801000Z TestFramework (ERROR): 
2022-05-07T21:43:41.801000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-05-07T21:43:41.801000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2022-05-07T21:43:41.801000Z TestFramework (ERROR):

I think it might be better to fix this by removing this whole method and also consolidate the two tasks into one:

I've given a bit of thought to this, but I really don't know how I would do that... self.signrawtransactionwithwallet_tests still has to use the Core wallet, shouldn't we test it with both the legacy and the descriptor wallet?

You can execute the subtests conditionally

if sqlite:
  do_sqlite_test()
test()

I'm sorry, I really can't figure out how to do this without massively changing the test_framework.py code. To me it seems that you can't in a single run test both with the descriptors and the legacy wallet...

Right. This should only be possible when both the descriptor wallet and legacy wallet are compiled (and this test is slightly rewritten).

However, I think it is fine to simply run just one (either sqlite or bdb).

This means that any sqlite test case line of code will need to be guarded by a check that checks for sqlite (and the same for bdb). This should not need any framework changes.

Unless of course the test is largely monolithic (most lines depend on previous lines), so conditional execution is not straightforward.

Let me know if you are stuck on this and I can take a closer look myself.

I'm working on it, I'll let you know, thanks 😄

Should be ok now :)

Why are those two calls needed?

Those two were already in the test:

https://github.com/bitcoin/bitcoin/blob/fcf6c8f4eb217763545ede1766831a6b93f583bd/test/functional/rpc_rawtransaction.py#L75-L80

But yeah, you're right, I can just drop them:

• It seems to me that generating from nodes[2] was never really needed
• Generating from nodes[0] is not needed anymore (I'm already doing it a couple of lines above, to make sure that all the self.wallet utxos are mature)

No, you are adding them twice? Whereas before they were only there once?

Yeah, I was doing self.generate(self.nodes[0], COINBASE_MATURITY + 1) twice without a real reason :)

It should be better now, thanks!

nit:

from test_framework.wallet import (
    MiniWallet,
    getnewdestination,
)

(that way, potential merge conflicts can be resolved easier)

Updated, thanks :)

        if self.requires_wallet:

maybe you can avoid calls to is_wallet_compiled() since you defined a var for it in set_test_params:

if self.is_wallet_compiled():
    self.requires_wallet = True

Oh, right! I just updated it, thank you :)

nit here and line 337 below

        # Fee rate is 0.00100000 BTC/kvB

nit, consistency with your changes

        txId = tx['txid']

nit, not sure how obvious it is to readers that getnewdestination()[2] is an address without looking up the function, so (here and just above) this might be clearer

-            outputs = {getnewdestination()[2]: 1}
-            rawtx = self.nodes[0].createrawtransaction(inputs, outputs)
+            address = getnewdestination()[2]
+            rawtx = self.nodes[0].createrawtransaction(inputs=inputs, outputs={address: 1})

If this is unclear, it might be clearer to change this everywhere wheregetnewdestination is used (in a separate pull)?

Right, and/or maybe have it return a dict: getnewdestination()['address']

Fixed, thanks! From a quick git grep it seems to me the usage of getnewdestination is clear in the other tests:

test/functional/feature_coinstatsindex.py:    getnewdestination,
test/functional/feature_coinstatsindex.py:        reorg_blocks = self.generatetoaddress(index_node, 2, getnewdestination()[2])
test/functional/interface_rest.py:    getnewdestination,
test/functional/interface_rest.py:        txid, _ = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=int(0.1 * COIN))
test/functional/interface_rest.py:        txid, _ = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=int(0.1 * COIN))
test/functional/p2p_filter.py:    getnewdestination,
test/functional/p2p_filter.py:        block_hash = self.generatetoscriptpubkey(getnewdestination()[1])
test/functional/p2p_filter.py:        self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=7 * COIN)
test/functional/p2p_filter.py:            txid, _ = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=getnewdestination()[1], amount=7 * COIN)
test/functional/p2p_filter.py:            block_hash = self.generatetoscriptpubkey(getnewdestination()[1])
test/functional/rpc_createmultisig.py:    getnewdestination,
test/functional/rpc_createmultisig.py:            self.final = getnewdestination()[2]
test/functional/rpc_createmultisig.py:        pk0 = getnewdestination()[0].hex()
test/functional/rpc_createmultisig.py:        pk1 = getnewdestination()[0].hex()
test/functional/rpc_createmultisig.py:        pk2 = getnewdestination()[0].hex()
test/functional/rpc_rawtransaction.py:    getnewdestination,
test/functional/rpc_rawtransaction.py:            address = getnewdestination()[2]
test/functional/rpc_rawtransaction.py:            address = getnewdestination()[2]
test/functional/rpc_rawtransaction.py:        address = getnewdestination()[2]
test/functional/rpc_rawtransaction.py:        address2 = getnewdestination()[2]
test/functional/rpc_scantxoutset.py:    getnewdestination,
test/functional/rpc_scantxoutset.py:        pubk1, spk_P2SH_SEGWIT, addr_P2SH_SEGWIT = getnewdestination("p2sh-segwit")
test/functional/rpc_scantxoutset.py:        pubk2, spk_LEGACY, addr_LEGACY = getnewdestination("legacy")
test/functional/rpc_scantxoutset.py:        pubk3, spk_BECH32, addr_BECH32 = getnewdestination("bech32")
test/functional/test_framework/wallet.py:def getnewdestination(address_type='bech32'):

nit, since you are touching this maybe move address2 to where it is first used

         address = getnewdestination()[2]
-        address2 = getnewdestination()[2]
         assert_raises_rpc_error(-1, "JSON value is not an array as expected", self.nodes[0].createrawtransaction, [], 'foo')
         self.nodes[0].createrawtransaction(inputs=[], outputs={})  # Should not throw for backwards compatibility
         self.nodes[0].createrawtransaction(inputs=[], outputs=[])
@@ -246,6 +242,7 @@ class RawTransactionsTest(BitcoinTestFramework):
             self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=[{address: 99}]),
         )
         # Two outputs
+        address2 = getnewdestination()[2]
         tx = tx_from_hex(self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)])))

hmm. Does this mean this test won't be run even if the bdb is compiled, unless the user specifies --legacy-wallet?

I think it would be better to just run this test if bdb is compiled, no?

was this addressed?

I didn't see this comment, sorry!

If you don't specify any flag, the test_framework will pick self.options.descriptors for you, preferring BDB if it's compiled:

https://github.com/bitcoin/bitcoin/blob/66bb4df4105945b907a81da0886a1206070554e1/test/functional/test_framework/test_framework.py#L215-L224

If you specify --descriptors instead, the test won't be run.

So, the test will be run if the user doesn't specify any flag and bdb is compiled, or if the user specifies --legacy-wallet

Hmm. I guess it is not possible to just run the test with the wallet that was compiled, since this will break if both bdb and sqlite are compiled.

An alternative would be to duplicate the test here once for sqlite and once with bdb? Otherwise, a user will need to run the whole test twice just to test both wallet implementations.

Another alternative would be to move this to one of the wallet tests?

I just realized I can create the wallets manually inside the tests:

    def signrawtransactionwithwallet_tests(self):
        for descriptors in [True, False]:
            if not descriptors and not self.is_bdb_compiled():
                pass
            if descriptors and not self.is_sqlite_compiled():
                pass
            wallet_name = "descriptors" if descriptors else "legacy"
            self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=descriptors)
            wallet = self.nodes[0].get_wallet_rpc(wallet_name)
            for type in ["bech32", "p2sh-segwit", "legacy"]:
                # etc...

This tests all the wallets that's compiled for, but I'm not sure if it's clean - maybe moving those in the wallet tests, as you suggested, is cleaner?

Yeah, it might be cleaner, but a bit more work.

Not exactly an immediate solution, but is there a reason that signrawtransactionwithwallet_tests are not part of rpc_signrawtransaction.py? From my understanding, this issue will also come up for anyone tackling that non-wallet tests convertion, so maybe an other alternative would be to move this to rpc_signrawtransaction.py?

Yeah, since rpc_signrawtransaction.py can't be converted to the MiniWallet (as all the tests in the file require the Core wallet), we could go with moving both signrawtransactionwithwallet_tests and raw_multisig_transaction_legacy_tests, I like it :)

I think that part of rpc_signrawtransaction.py can be converted, similar to how rpc_signmessage.py was converted in #22641. So by moving signrawtransactionwithwallet_tests we'll need to deal with this issue only once (while imo moving the test to a more suitable location).

I don't think that is neccesary to move raw_multisig_transaction_legacy_tests as it doesn't need more work, it just needs the previous not self.options.descriptors conditional clause instead of self.requires_wallet.

Done in 76f0542a2ae4fd9af7f29d84d89eac1817d47871

I think you can keep this test as-is and just remove this line and replace the call to getnewaddress

Right, thanks!

Is there a reason that this is not from_node=self.nodes[0]? As that would remove the need for self.sync_all(). Or alternatively generate the block from node[1].

As that would remove the need for self.sync_all()

I think it would still be needed, as we're calling getrawtransaction below, and we want every node to be aware of the tx (sync_all syncs both the mempools and the blocks)

EDIT: Oh, no, you're right. generate calls sync_all anyways, so sending from node[0] would need a sync_all less. Updating the code :)

Looks like the test could use renaming and the logging updated In the first commit? maybe something like this

diff --git a/test/functional/rpc_signrawtransaction.py b/test/functional/rpc_signrawtransaction.py
index 29cb116793..8da2cfa72b 100755
--- a/test/functional/rpc_signrawtransaction.py
+++ b/test/functional/rpc_signrawtransaction.py
@@ -334,10 +334,10 @@ class SignRawTransactionsTest(BitcoinTestFramework):
         assert_equal(signed["complete"], True)
         self.nodes[0].sendrawtransaction(signed["hex"])
 
-    def signrawtransactionwithwallet_tests(self):
+    def test_signing_with_missing_prevtx_info(self):
         txid = "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000"
         for type in ["bech32", "p2sh-segwit", "legacy"]:
-            self.log.info(f"Test signrawtransactionwithwallet with missing prevtx info ({type})")
+            self.log.info(f"Test signing with missing prevtx info ({type})")
             addr = self.nodes[0].getnewaddress("", type)
             addrinfo = self.nodes[0].getaddressinfo(addr)
             pubkey = addrinfo["scriptPubKey"]
@@ -393,7 +393,7 @@ class SignRawTransactionsTest(BitcoinTestFramework):
         self.test_fully_signed_tx()
         self.test_signing_with_csv()
         self.test_signing_with_cltv()
-        self.signrawtransactionwithwallet_tests()
+        self.test_signing_with_missing_prevtx_info()

$ test/functional/rpc_signrawtransaction.py 
2022-05-30T13:39:10.050000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ukdy99c_
2022-05-30T13:39:10.954000Z TestFramework (INFO): Test valid raw transaction with one input
2022-05-30T13:39:10.958000Z TestFramework (INFO): Test script verification errors
2022-05-30T13:39:10.969000Z TestFramework (INFO): Test signing transaction to P2SH-P2WSH addresses without wallet
2022-05-30T13:39:12.667000Z TestFramework (INFO): Test with a P2PKH script as the witnessScript
2022-05-30T13:39:12.822000Z TestFramework (INFO): Test with a P2PK script as the witnessScript
2022-05-30T13:39:12.970000Z TestFramework (INFO): Test OP_1NEGATE (0x4f) satisfies BIP62 minimal push standardness rule
2022-05-30T13:39:12.972000Z TestFramework (INFO): Test correct error reporting when trying to sign a locked output
2022-05-30T13:39:13.519000Z TestFramework (INFO): Test signing a fully signed transaction does nothing
2022-05-30T13:39:14.504000Z TestFramework (INFO): Test signing a transaction containing a fully signed CSV input
2022-05-30T13:39:15.920000Z TestFramework (INFO): Test signing a transaction containing a fully signed CLTV input
2022-05-30T13:39:16.368000Z TestFramework (INFO): Test signing with missing prevtx info (bech32)
2022-05-30T13:39:16.458000Z TestFramework (INFO): Test signing with missing prevtx info (p2sh-segwit)
2022-05-30T13:39:16.558000Z TestFramework (INFO): Test signing with missing prevtx info (legacy)
2022-05-30T13:39:16.699000Z TestFramework (INFO): Stopping nodes
2022-05-30T13:39:16.904000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_ukdy99c_ on exit
2022-05-30T13:39:16.904000Z TestFramework (INFO): Tests successful

Also in the first commit

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index fecb8310b9..54357ccd31 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -7,7 +7,6 @@
 Test the following RPCs:
    - getrawtransaction
    - createrawtransaction
-   - signrawtransactionwithwallet
    - sendrawtransaction
    - decoderawtransaction
 """

Edit: hm, there are still a few in raw_multisig_transaction_legacy_tests() so perhaps never mind

Edit: hm, there are still a few in raw_multisig_transaction_legacy_tests() so perhaps never mind

Yeah, I'd say it's better to leave it :)