-
Notifications
You must be signed in to change notification settings - Fork 36.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Run rpc_generateblock.py even with wallet disabled #20808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 4e8803e
Code review and built it with and without wallet
Not a huge deal @stackman27 but we ACK the commit hash of the HEAD commit. In this case it is 4e8803e
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 4e8803e
Ran test with wallet disabled and reviewed code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, this is a good start. You are (I think unintentionally) changing the test behavior.
test/functional/rpc_generateblock.py
Outdated
tx = block['tx'][0] | ||
utxo = {'txid': tx['txid'], 'vout': 0, 'value': tx['vout'][0]['value']} | ||
utxos.append(utxo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove this if you do hash = miniwallet.generate(1)[0]["hash"]
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you still want the utxo
in your list, you can then just do utxos.append(miniwallet.get_utxo())
test/functional/rpc_generateblock.py
Outdated
address = ADDRESS_BCRT1_P2WSH_OP_TRUE | ||
scriptPubKey = hex_str_to_bytes(node.validateaddress(ADDRESS_BCRT1_P2WSH_OP_TRUE)['scriptPubKey']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's frowned upon, but thoughts on doing:
address = miniwallet._address
scriptPubKey = 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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Two minor comments
test/functional/rpc_generateblock.py
Outdated
tx = block['tx'][0] | ||
utxo = {'txid': tx['txid'], 'vout': 0, 'value': tx['vout'][0]['value']} | ||
miniwallet._utxos.append(utxo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
test/functional/rpc_generateblock.py
Outdated
def run_test(self): | ||
node = self.nodes[0] | ||
miniwallet = MiniWallet(node) | ||
address = ADDRESS_BCRT1_P2WSH_OP_TRUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
@property
def address(self):
return self._address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to just make _address
public (remove the underscore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
test/functional/rpc_generateblock.py
Outdated
node.generatetoaddress(110, address) | ||
# Generate some blocks to spend | ||
miniwallet.generate(10) | ||
node.generate(100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
🐙 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". |
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.) |
1e86ff7 test: run rpc-generateblock.py even with wallet disabled (Darius Parvin) Pull request description: Run rpc_generateblock.py test even when the wallet is disabled, as discussed in #20078. This PR picked up from where #20808 left off, started by @ nginocchio. Since then, there have been many improvements to `MiniWallet`, making this PR more straightforward. L23 makes use of `MiniWallet.rescan_utxos()` to add the pre-mined block utxos (when `self.setup_clean_chain` is not set to `True`), rather than generating new blocks during the test. ACKs for top commit: mjdietzx: Tested ACK 1e86ff7 Tree-SHA512: 4285f61516dd53a08004eeea26d58f45b4c1c67f5ca4c94ff1bc9fc7e50f486de2e033a8b4aaf67cb4c33d73aa929362e18dc75d5c7951cbf58120b5fb1de555
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.