test: Run rpc_generateblock.py even with wallet disabled

nginocchio commented at 0:11 am on December 31, 2020: none

This PR enables one more of the non-wallet functional tests (rpc_generateblock.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.

fanquake added the label Tests on Dec 31, 2020

stackman27 commented at 8:11 pm on January 3, 2021: contributor

ACK 4e8803eCode review and built it with and without wallet

michaelfolkson commented at 8:39 pm on January 3, 2021: contributor

Not a huge deal @stackman27 but we ACK the commit hash of the HEAD commit. In this case it is 4e8803e22f0cd8a71f7f81f67ed0137a667b63ce

When giving an ACK, specify the commits reviewed by appending the commit hash of the HEAD commit.

For more details on the review process see this @jonatack doc: https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core

stackman27 commented at 10:06 pm on January 3, 2021: contributor

Not a huge deal @stackman27 but we ACK the commit hash of the HEAD commit. In this case it is 4e8803e

When giving an ACK, specify the commits reviewed by appending the commit hash of the HEAD commit.

For more details on the review process see this @jonatack doc: https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core

oooh i see. Thank you!

in test/functional/rpc_generateblock.py:54 in 4e8803e22f outdated

50@@ -51,37 +51,34 @@ def run_test(self):
51         assert_equal(len(block['tx']), 1)
52         assert_equal(block['tx'][0]['vout'][0]['scriptPubKey']['addresses'][0], combo_address)
53 
54-        # Generate 110 blocks to spend
55-        node.generatetoaddress(110, address)
56+        # Generate some blocks to spend

michaelfolkson commented at 5:59 pm on January 4, 2021:

nit: Any reason you bumped up the blocks generated from 110 to 200?

nginocchio commented at 7:11 pm on January 4, 2021:

I bumped it from 110 blocks because I was getting premature spend when using 110 blocks. I think its because the utxos are sorted by value so their confirmations are somewhat arbitrary in the self._utxos. 200 seemed to be a quick and dirty fix.

mjdietzx commented at 9:27 pm on January 4, 2021:

Yeah, you’re right that it’s bc the utxos are sorted by value in MiniWallet and a lot of them will be immature coin bases. I think what you have is good. But what I’ve usually done, and seen done before, is generating the utxos you need to spend with mini_wallet.generate(), and then generating 100 block with node, node.generate(100) so that all the utxos in MiniWallet are mature and good to go

stackman27 commented at 10:29 pm on January 4, 2021:

I pretty much approached with a similar style as described by @mjdietzx in mempool_limit.py as well to prevent premature spend error

nginocchio commented at 10:35 pm on January 4, 2021:

ya that certainly seems like a more coherent approach

michaelfolkson commented at 6:04 pm on January 4, 2021: contributor

ACK 4e8803e22f0cd8a71f7f81f67ed0137a667b63ce

Ran test with wallet disabled and reviewed code.

in test/functional/rpc_generateblock.py:84 in 4e8803e22f outdated

87-        raw1 = node.createrawtransaction([{'txid':txid, 'vout':0}],[{address:0.9999}])
88-        signed_raw1 = node.signrawtransactionwithwallet(raw1)['hex']
89-        txid1 = node.sendrawtransaction(signed_raw1)
90-        raw2 = node.createrawtransaction([{'txid':txid1, 'vout':0}],[{address:0.999}])
91-        signed_raw2 = node.signrawtransactionwithwallet(raw2)['hex']
92-        assert_raises_rpc_error(-25, 'TestBlockValidity failed: bad-txns-inputs-missingorspent', node.generateblock, address, [signed_raw2, txid1])

mjdietzx commented at 9:22 pm on January 4, 2021:

It doesn’t seem totally related to what’s being tested here, but one thing I noticed while reviewing:

Do you know why, in the original code, rpc_generateblock.py sometimes does sendrawtransaction and other times does not, before generateblock?

ie here signed_raw2 was never submitted to the node with sendrawtransaction, while txid1 was.

nginocchio commented at 10:25 pm on January 4, 2021:

I am not entirely sure, but my assumption while reading through the code was that it was to save a line of code.

glozow commented at 11:13 pm on January 5, 2021:

sendrawtransaction submits the tx to mempool, while createrawtransaction + signrawtransaction merely creates the raw tx objects. Here, in the original test, tx1 is submitted and tx2 is not. It’s calling generateblock() with the whole raw tx for tx2 and just the txid for tx1.

So, actually, your implementation doesn’t really match what was there before. Be careful - generateblock accepts hexstrings that can represent either txids (when the tx is already in mempool) or raw transactions.

mjdietzx commented at 9:23 pm on January 4, 2021: contributor

Concept ACK

taurus228 commented at 10:59 pm on January 4, 2021: none

.

in test/functional/rpc_generateblock.py:70 in 1816feeddd outdated

73         self.log.info('Generate block with raw tx')
74-        utxos = node.listunspent(addresses=[address])
75-        raw = node.createrawtransaction([{'txid':utxos[0]['txid'], 'vout':utxos[0]['vout']}],[{address:1}])
76-        signed_raw = node.signrawtransactionwithwallet(raw)['hex']
77-        hash = node.generateblock(address, [signed_raw])['hash']
78+        txid = miniwallet.send_self_transfer(from_node=node)['txid']

MarcoFalke commented at 9:56 am on January 5, 2021:

This will add the tx to the mempool, which is not wanted. the raw tx should only be passed to the miner

michaelfolkson commented at 4:29 pm on January 5, 2021:

By “not wanted” you mean unnecessary @MarcoFalke? I only ask for my own understanding as the test passes in its current form.

MarcoFalke commented at 6:23 am on January 6, 2021:

Transactions for the miner are picked from the mempool, usually. This test should check that the tx can be supplied in raw form, not taking it from the mempool. Also, it tests that a miner can mine a tx that is not in the mempool.

in test/functional/rpc_generateblock.py:76 in 1816feeddd outdated

80+        hash = node.generateblock(address, [rawtx])['hash']
81         block = node.getblock(hash, 1)
82         assert_equal(len(block['tx']), 2)
83         txid = block['tx'][1]
84-        assert_equal(node.gettransaction(txid)['hex'], signed_raw)
85+        assert_equal(node.getrawtransaction(txid, False, hash), rawtx)

MarcoFalke commented at 9:57 am on January 5, 2021:

would be nice to use named arguments

MarcoFalke commented at 9:58 am on January 5, 2021: member

Concept ACK. Make sure to squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits after addressing the feedback

nginocchio force-pushed on Jan 5, 2021

in test/functional/rpc_generateblock.py:59 in 54bb428711 outdated

68@@ -51,37 +69,43 @@ def run_test(self):
69         assert_equal(len(block['tx']), 1)
70         assert_equal(block['tx'][0]['vout'][0]['scriptPubKey']['addresses'][0], combo_address)
71 
72-        # Generate 110 blocks to spend
73-        node.generatetoaddress(110, address)
74+        # Generate some blocks to spend
75+        miniwallet.generate(100)
76+        node.generate(100)

glozow commented at 11:14 pm on January 5, 2021:

You can keep this as just 10. It’ll fix your premature spend error; 10 outputs is enough for this test.

0miniwallet.generate(10)

(oops, I put the suggestion on the wrong line - I mean only generate 10 on the miniwallet)

glozow commented at 11:16 pm on January 5, 2021: member

Concept ACK, this is a good start. You are (I think unintentionally) changing the test behavior.

nginocchio force-pushed on Jan 9, 2021

in test/functional/rpc_generateblock.py:27 in e5b7f5fd26 outdated

57+        scriptPubKey = hex_str_to_bytes(node.validateaddress(ADDRESS_BCRT1_P2WSH_OP_TRUE)['scriptPubKey'])
58+        utxos = []
59 
60         self.log.info('Generate an empty block to address')
61-        address = node.getnewaddress()
62         hash = node.generateblock(output=address, transactions=[])['hash']

mjdietzx commented at 6:49 pm on January 10, 2021:

Can you do hash = miniwallet.generate(1)[0]["hash"] here? That way the coinbase will be appended to miniwallet._utxos in the way you’re doing it and you won’t have to duplicate logic

nginocchio commented at 7:54 pm on January 10, 2021:

I thought of doing something along the lines of that but shouldn’t I be using generateblock explicitly in this test?

in test/functional/rpc_generateblock.py:62 in e5b7f5fd26 outdated

61-        address = node.getnewaddress()
62         hash = node.generateblock(output=address, transactions=[])['hash']
63         block = node.getblock(blockhash=hash, verbose=2)
64+        tx = block['tx'][0]
65+        utxo = {'txid': tx['txid'], 'vout': 0, 'value': tx['vout'][0]['value']}
66+        utxos.append(utxo)

mjdietzx commented at 6:50 pm on January 10, 2021:

You should be able to remove this if you do hash = miniwallet.generate(1)[0]["hash"] instead

mjdietzx commented at 6:57 pm on January 10, 2021:

If you still want the utxo in your list, you can then just do utxos.append(miniwallet.get_utxo())

in test/functional/rpc_generateblock.py:54 in e5b7f5fd26 outdated

52-
53     def run_test(self):
54         node = self.nodes[0]
55+        miniwallet = MiniWallet(node)
56+        address = ADDRESS_BCRT1_P2WSH_OP_TRUE
57+        scriptPubKey = hex_str_to_bytes(node.validateaddress(ADDRESS_BCRT1_P2WSH_OP_TRUE)['scriptPubKey'])

mjdietzx commented at 7:03 pm on January 10, 2021:

I know it’s frowned upon, but thoughts on doing:

0address = miniwallet._address
1scriptPubKey = miniwallet._scriptPubKey

Otherwise we’re just duplicating that logic, and we can get rid of a bunch of imports. Maybe it’s worth considering adding a getter for address and scriptPubKey to MiniWallet (although I don’t see anything wrong just accessing the private vars)

in test/functional/rpc_generateblock.py:102 in e5b7f5fd26 outdated

101 
102         self.log.info('Generate block with txid')
103-        txid = node.sendtoaddress(address, 1)
104+        utxo = utxos.pop(0)
105+        rawtx = create_miniwallet_rawtx(utxo=utxo, scriptPubKey=scriptPubKey)
106+        txid = node.sendrawtransaction(rawtx)

mjdietzx commented at 7:08 pm on January 10, 2021:

I’m not seeing how this is different than miniwallet.send_self_transfer(...)

nginocchio commented at 8:01 pm on January 10, 2021:

You’re right, it really isn’t. It mostly just comes down to the blocks generated by generateblock not being appended to self._utxos. So what it comes down to is what the proper approach for appending to self._utxos is.

in test/functional/rpc_generateblock.py:107 in e5b7f5fd26 outdated

107         hash = node.generateblock(address, [txid])['hash']
108-        block = node.getblock(hash, 1)
109+        block = node.getblock(hash, 2)
110+        tx = block['tx'][0]
111+        utxo = {'txid': block['tx'][1]['txid'], 'vout': 0, 'value': block['tx'][1]['vout'][0]['value']}
112+        utxos.append(utxo)

mjdietzx commented at 7:10 pm on January 10, 2021:

If you did miniwallet.send_self_transfer(…) as suggested above, you could get rid of this. Again, if you want the utxo in the list of utxos you are maintaining, you could do: utxos.append(miniwallet.get_utxo())

in test/functional/rpc_generateblock.py:33 in e5b7f5fd26 outdated

28+    satoshi_round,
29+    hex_str_to_bytes,
30+)
31+from test_framework.wallet import MiniWallet
32+
33+def create_miniwallet_rawtx(utxo, scriptPubKey):

mjdietzx commented at 7:15 pm on January 10, 2021:

I think @MarcoFalke is setting you up for a layup here #20876. Then @glozow suggests “was just thinking that it’d be nice to have a just-create-don’t-send option for the MiniWallet. What do you think of a bool option?”

I think this is the PR where this is needed. So I’d consider rebasing on-top of #20876, and adding this option to send_self_transfer as described here. Then you can utilize this extended functionality and ditch create_miniwallet_rawtx

thoughts?

nginocchio commented at 7:30 pm on January 10, 2021:

Yes I didn’t like my approach but I wasn’t sure I should touch the miniwallet class itself to add a create_raw_transaction function. So I think your suggestion is a nice solution.

stackman27 commented at 7:41 pm on January 10, 2021:

I did a similar thing in mempool_limit by adding a prepare_tx method in MiniWallet Class. link: https://github.com/bitcoin/bitcoin/pull/20874/files#diff-7932a60a9127fd22d10d367526fd7b987f9647ce017595f8b1af5c32d5db0083R58 To be honest, it was just to avoid repetition, but I can see how it could be helpful for both of us

stackman27 commented at 7:56 pm on January 10, 2021:

nit: shouldn’t the name of the method be create_and_sign_miniwallet_rawtx?

MarcoFalke commented at 7:59 am on January 11, 2021:

Agree with @mjdietzx . Please rebase and add the bool option to #20876

in test/functional/rpc_generateblock.py:55 in e5b7f5fd26 outdated

53     def run_test(self):
54         node = self.nodes[0]
55+        miniwallet = MiniWallet(node)
56+        address = ADDRESS_BCRT1_P2WSH_OP_TRUE
57+        scriptPubKey = hex_str_to_bytes(node.validateaddress(ADDRESS_BCRT1_P2WSH_OP_TRUE)['scriptPubKey'])
58+        utxos = []

mjdietzx commented at 7:18 pm on January 10, 2021:

I’m not convinced that we need to maintain our own utxo set. After some re-working (ie adding “a just-create-don’t-send option for the MiniWallet.”) I’m wondering if this can go away

nginocchio commented at 7:45 pm on January 10, 2021:

Yes it probably can go away. My only reasoning for maintaining this list was because I wasn’t exactly sure how I could get MiniWallet access to the utxos created from generateblock, and even if it did, Miniwallet (in its current state) doesn’t have the option to create a transaction not submitted to mempool.

nginocchio force-pushed on Jan 12, 2021

DrahtBot commented at 8:13 am on January 12, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
#21014 (test: Run mempool_accept.py even with wallet disabled by stackman27)
#20889 (test: ensure any failing method in MiniWallet leaves no side-effects by mjdietzx)
#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)
#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/rpc_generateblock.py:31 in 69e47b1f88 outdated

30-        address = node.getnewaddress()
31         hash = node.generateblock(output=address, transactions=[])['hash']
32         block = node.getblock(blockhash=hash, verbose=2)
33+        tx = block['tx'][0]
34+        utxo = {'txid': tx['txid'], 'vout': 0, 'value': tx['vout'][0]['value']}
35+        miniwallet._utxos.append(utxo)

mjdietzx commented at 6:47 pm on January 12, 2021:

Is this necessary? I don’t see why we need this Coinbase in our utxos. I don’t see us specifically spending it, and if you remove this it gets rid of the potential problem of appending to mini wallets private _utxos

nginocchio commented at 7:06 pm on January 12, 2021:

10 blocks isn’t enough for the test because the 10 generated by MiniWallet are sent to sit in the mempool not to be mined. So I can up the generated blocks to 11 and remove that code, or do this approach.

mjdietzx commented at 7:20 pm on January 12, 2021:

Is there any reason you need this specific utxo for this test? If you need 11, I’d say generate 11 blocks where you need them.

but the 10 generated by MiniWallet are definitely mined. ie if you synced another node, you’d see those blocks with the coinbases (utxos) you are spending. they aren’t just in the mempool

if I’m missing something please lmk, definitely possible

nginocchio commented at 7:42 pm on January 12, 2021:

I do not need that specific utxo for the test. I guess I should have elaborated more on my point. What I meant was that those 10 blocks are generated via generate and then are sent via send_self_transfer, however, those transactions are never confirmed. So while those new utxos are present within miniwallet generating a confirmed transaction based on them (because they are unconfirmed) isn’t possible. Please correct me if I am wrong. Its probably just a better approach to generate 11.

mjdietzx commented at 7:47 pm on January 12, 2021:

Ah yeah, so the transactions you send with send_self_transfer are in the mempool. However, you could still spend them even though they are only in the mempool. And you could also generate a block, and all the transactions in the mempool would then be mined/confirmed (although I don’t think this is necessary).

I’d recommend generating 11 blocks for miniwallet if you need to send 11 utxos. Otherwise the 11th will pick one of the previous send_self_transfer utxos to spend (ie one of the transactions in the mempool is the parent of that new “child” txn)

in test/functional/rpc_generateblock.py:24 in 69e47b1f88 outdated

22-        self.skip_if_no_wallet()
23-
24     def run_test(self):
25         node = self.nodes[0]
26+        miniwallet = MiniWallet(node)
27+        address = ADDRESS_BCRT1_P2WSH_OP_TRUE

mjdietzx commented at 6:53 pm on January 12, 2021:

nit: I’m not sure if this really matters, but would it be better to use miniwallet._address? I know they are hard-coded to the same value rn @MarcoFalke what’s you’re opinion on this, would it be beneficial to add a getter to MiniWallet?

0[@property](/bitcoin-bitcoin/contributor/property/)
1def address(self):
2  return self._address

MarcoFalke commented at 4:58 pm on February 16, 2021:

I think it is fine to just make _address public (remove the underscore)

mjdietzx commented at 6:53 pm on January 12, 2021: contributor

This is looking great! Two minor comments

in test/functional/test_framework/wallet.py:67 in 69e47b1f88 outdated

54@@ -55,8 +55,8 @@ def get_utxo(self, *, txid=''):
55             index = self._utxos.index(utxo)
56         return self._utxos.pop(index)
57 
58-    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
59-        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
60+    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None,  submit_tx=True):

stackman27 commented at 8:46 pm on January 12, 2021:

I’m just wondering whether rpc_generateblock.py requires a flat_fee or a calculated fee? Because, if i’m not mistaken you used flat_fee in previous commits

nginocchio commented at 9:12 pm on January 12, 2021:

The fee was irrelevant to rpc_generateblock.py so I used a flat fee to reduce the amount of code in the helper function create_miniwallet_rawtx. create_miniwallet_rawtx now no longer needed in place of send_self_transfer.

in test/functional/test_framework/wallet.py:85 in 69e47b1f88 outdated

72@@ -73,7 +73,8 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
73 
74         tx_info = from_node.testmempoolaccept([tx_hex])[0]
75         self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
76-        from_node.sendrawtransaction(tx_hex)
77+        if submit_tx:

stackman27 commented at 8:47 pm on January 12, 2021:

could this be something like if not submit_tx: return tx because that would help mempool_limit.py as well without much changes?

mjdietzx commented at 4:39 pm on January 14, 2021:

You can also rebuild the tx from the hex field that this returns as-is I think

0from test_framework.messages import CTransaction, ToHex
1hex = wallet.send_self_transfer(...)["hex"]
2tx = FromHex(CTransaction(), hex)

stackman27 commented at 8:53 pm on January 15, 2021:

I see thanks :)

stackman27 commented at 8:49 pm on January 12, 2021: contributor

I added few comments on wallet.py to make sure we’re both on the same track, since we’re both modifying it for the same reason :)

in test/functional/rpc_generateblock.py:59 in 69e47b1f88 outdated

53@@ -51,37 +54,35 @@ def run_test(self):
54         assert_equal(len(block['tx']), 1)
55         assert_equal(block['tx'][0]['vout'][0]['scriptPubKey']['addresses'][0], combo_address)
56 
57-        # Generate 110 blocks to spend
58-        node.generatetoaddress(110, address)
59+        # Generate some blocks to spend
60+        miniwallet.generate(10)
61+        node.generate(100)

MarcoFalke commented at 4:58 pm on February 16, 2021:

Generating blocks takes a few seconds in valgrind, so I am thinking if this test may benefit from using miniwallet.scan_blocks (to be introduced in #21200)

test: Run rpc_generateblock.py even with wallet disabled 4a6bb452ff

add submit transaction to network boolean in MiniWallet f40a01a9b5

make address public c4fb414825

nginocchio force-pushed on Mar 1, 2021

DrahtBot commented at 2:22 pm on March 29, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Mar 29, 2021

MarcoFalke commented at 7:32 am on July 20, 2021: member

Are you still working on this?

MarcoFalke added the label Up for grabs on Aug 27, 2021

MarcoFalke closed this on Aug 27, 2021

jsarenik commented at 7:28 pm on November 18, 2021: none

Are you still working on this?

I know someone who is working on this and hopefully they will write here soon. (Just wanted to leave a trace in the PR by this message, sorry for spam.)

DariusParvin commented at 5:33 pm on November 19, 2021: contributor

Thanks @jsarenik :) I am working on this issue. I am mostly finished but will go through it a few more times (reviewing the comments here) before submitting a PR. Work in progress is here

MarcoFalke removed the label Up for grabs on Nov 20, 2021

MarcoFalke removed the label Needs rebase on Nov 20, 2021

MarcoFalke referenced this in commit 368831371d on Nov 21, 2021

DrahtBot locked this on Nov 20, 2022

test: Run rpc_generateblock.py even with wallet disabled #20808

Conflicts