test: Run mempool_accept.py even with wallet disabled by stackman27 · Pull Request #21014 · bitcoin/bitcoin

stackman27 · 2021-01-26T18:49:41Z

This is a PR proposed in #20078

This PR enables one more of the non-wallet functional tests (mempool_accept.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead.

This PR also includes changes on wallet.py to accommodate some of the features used in mempool_accept.py like explicitly stating nSequence for RBF and sending tx with max fee = 0

michaelfolkson · 2021-01-26T19:02:09Z

Concept ACK

Can you squash your commits @stackman27?

stackman27 · 2021-01-26T21:00:57Z

Concept ACK

Can you squash your commits @stackman27?

Yes! Should be fine now :)

DrahtBot · 2021-01-27T01:25:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Speed up mempool_spend_coinbase.py #21762 (test: Speed up mempool_spend_coinbase.py by MarcoFalke)
test: Run feature_cltv with MiniWallet #21754 (test: Run feature_cltv with MiniWallet by MarcoFalke)
test: run mempool_reorg.py even with wallet disabled #21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
test: Run mempool_limit.py even with wallet disabled #20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

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.

test/functional/mempool_accept.py

test/functional/test_framework/wallet.py

maflcko

left some comments

test/functional/mempool_accept.py

DariusParvin

Looks good! The only thing is I think having a separate create_self_transfer method would be more straightforward than having a send_tx flag in send_self_transfer. With the send_tx flag, it also becomes a bit ambiguous whether the new utxo has been appended to the list of spendable utxos when send_tx = False.

test/functional/mempool_accept.py

test/functional/test_framework/wallet.py

test/functional/mempool_accept.py

DariusParvin · 2021-02-15T02:08:05Z

test/functional/test_framework/wallet.py

It seems like it might be misleading to have a method whose name is send_self_transfer, but then have a flag to say send = False.

Instead, maybe it would be more straightforward to have a separate method create_self_transfer, that returns the tx hex. This function could be used within send_self_transfer to create the transaction before broadcasting it.

I'm about to submit a PR for another test where I had a go at doing it this way.

That totally makes sense! or maybe we can simply rename the method to something that mimics it's accurate behavior, something in the lines of sign_or_send_self_transfer. Would love to hear @MarcoFalke's thoughts on this

test/functional/test_framework/wallet.py

test/functional/mempool_accept.py

glozow

A few comments
Also on style - from PEP8: "Don't use spaces around the = sign when used to indicate a keyword argument, or when used to indicate a default value for an unannotated function parameter"

glozow · 2021-03-03T03:19:49Z

test/functional/test_framework/wallet.py

        return self._utxos.pop(index)

-    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
+    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), seq_num = 0xffffffff, from_node, utxo_to_spend=None, send_tx = True, nVersion = 1, default_fee = False):


Since it's optional

Suggested change

def send_self_transfer(self, *, fee_rate=Decimal("0.003"), seq_num = 0xffffffff, from_node, utxo_to_spend=None, send_tx = True, nVersion = 1, default_fee = False):

def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, send_tx=True, nVersion=1, seq_num=0xffffffff, default_fee=False):

glozow · 2021-03-03T03:26:28Z

test/functional/test_framework/wallet.py

-        self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
-        from_node.sendrawtransaction(tx_hex)
+        if send_tx:
+            if default_fee:


This should be called bypass_maxfeerate. But really, since it's a parameter for sendrawtransaction, why isn't the caller doing a send_tx=False and then submitting it themselves?

So this was following Darius's comment. I could only append tx in self.utxos after sending a transaction. If I try to simply send transaction from somewhere else, I wouldn't be able to append it in the internal wallet.py list

glozow · 2021-03-03T03:28:01Z

test/functional/test_framework/wallet.py

-        tx.vin = [CTxIn(COutPoint(int(utxo_to_spend['txid'], 16), utxo_to_spend['vout']))]
+        tx.vin = [CTxIn(COutPoint(int(utxo_to_spend['txid'], 16), utxo_to_spend['vout']), nSequence = seq_num)]
        tx.vout = [CTxOut(int(send_value * COIN), self._scriptPubKey)]
+        tx.nVersion = 2


You're accepting a nVersion param but not using it?

oops my bad 😅

glozow · 2021-03-03T03:36:29Z

test/functional/mempool_accept.py

-            inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
-            outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
-        ))['hex']
+        fee = Decimal('0.00000672')


You're changing the fee?

Yea cause the previous fee of fee = Decimal('0.000007') was too low to pass the mempool test. I manually calculated the fee here fee_rate = Decimal('0.00007672') - fee,, to get the best option

glozow · 2021-03-03T03:39:33Z

test/functional/mempool_accept.py

        self.log.info('Start with empty mempool, and 200 blocks')
        self.mempool_size = 0
+        miniwallet.generate(10)
+        node.generate(190)


Assuming this is done to mature the coinbases... is there any reason to do more than 100?

Not really, just wanted to keep 200 blocks to mimic the current code

DrahtBot · 2021-04-29T07:10:22Z

🐙 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".}

maflcko · 2021-07-18T13:53:50Z

Are you still working on this? Otherwise someone else could pick this up.

maflcko · 2021-07-20T18:36:39Z

Picked up in #22509

DrahtBot added the Tests label Jan 26, 2021

stackman27 force-pushed the diswallet_mempoolaccept branch from e585692 to ce45bd6 Compare January 26, 2021 21:00

This was referenced Jan 27, 2021

test: Run mempool_limit.py even with wallet disabled #20874

Closed

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

Closed

stackman27 force-pushed the diswallet_mempoolaccept branch from ce45bd6 to 4501022 Compare January 27, 2021 19:40

0xB10C reviewed Jan 28, 2021

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 2, 2021

test: ensure any failing method in MiniWallet leaves no side-effects #20889

Closed

stackman27 force-pushed the diswallet_mempoolaccept branch from 4501022 to 27f078c Compare February 5, 2021 17:10

amitiuttarwar reviewed Feb 7, 2021

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

test/functional/test_framework/wallet.py Outdated Show resolved Hide resolved

stackman27 force-pushed the diswallet_mempoolaccept branch from 27f078c to 582d57c Compare February 7, 2021 15:05

maflcko reviewed Feb 8, 2021

View reviewed changes

stackman27 force-pushed the diswallet_mempoolaccept branch from 582d57c to f37a517 Compare February 9, 2021 16:15

DrahtBot mentioned this pull request Feb 11, 2021

test: convert feature_bip68_sequence.py to use MiniWallet #21144

Closed

DariusParvin reviewed Feb 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Feb 15, 2021

test: run mempool_reorg.py even with wallet disabled #21178

Merged

maflcko reviewed Feb 16, 2021

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

test:minwallet:changes to accomodate mempool_accept

4cd36f6

stackman27 force-pushed the diswallet_mempoolaccept branch from 91c2848 to 311a6f7 Compare February 19, 2021 16:21

test:run mempool_accept even when wallet is disabled

5ed2bde

stackman27 force-pushed the diswallet_mempoolaccept branch from 311a6f7 to 5ed2bde Compare March 1, 2021 16:14

glozow reviewed Mar 3, 2021

View reviewed changes

This was referenced Apr 22, 2021

test: Run feature_cltv with MiniWallet #21754

Merged

test: Speed up mempool_spend_coinbase.py #21762

Merged

DrahtBot added the Needs rebase label Apr 29, 2021

sriramdvt mentioned this pull request Jul 20, 2021

test: Use MiniWallet in mempool_accept.py #22509

Closed

fanquake closed this Jul 21, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	def send_self_transfer(self, *, fee_rate=Decimal("0.003"), seq_num = 0xffffffff, from_node, utxo_to_spend=None, send_tx = True, nVersion = 1, default_fee = False):
	def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, send_tx=True, nVersion=1, seq_num=0xffffffff, default_fee=False):

Conversation

stackman27 commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelfolkson commented Jan 26, 2021

Uh oh!

stackman27 commented Jan 26, 2021

Uh oh!

DrahtBot commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DariusParvin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stackman27 Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 29, 2021

Uh oh!

maflcko commented Jul 18, 2021

Uh oh!

maflcko commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

stackman27 commented Jan 26, 2021 •

edited

Loading

DrahtBot commented Jan 27, 2021 •

edited

Loading

stackman27 Feb 19, 2021 •

edited

Loading