test: Replace getmempoolentry with testmempoolaccept in MiniWallet

MarcoFalke commented at 4:01 pm on January 7, 2021: member

This is a refactor to not use the return value of sendrawtransaction and getmempoolentry with the goal that submitting the tx to the mempool will become optional.

MarcoFalke added the label Refactoring on Jan 7, 2021

MarcoFalke added the label Tests on Jan 7, 2021

DrahtBot commented at 7:52 pm on January 7, 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:

#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: Replace getmempoolentry with testmempoolaccept in MiniWallet faabc26a61

MarcoFalke force-pushed on Jan 8, 2021

in test/functional/test_framework/wallet.py:76 in faabc26a61

70@@ -71,9 +71,9 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
71         tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
72         tx_hex = tx.serialize().hex()
73 
74-        txid = from_node.sendrawtransaction(tx_hex)
75-        self._utxos.append({'txid': txid, 'vout': 0, 'value': send_value})
76-        tx_info = from_node.getmempoolentry(txid)
77+        tx_info = from_node.testmempoolaccept([tx_hex])[0]
78+        self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
79+        from_node.sendrawtransaction(tx_hex)

stackman27 commented at 9:11 am on January 8, 2021:

I just have a conceptual question here. Why would we wanna send tx when we know it’s going to fail? for instance I ran this code to check for failure and success and got markoPR2

. Is it to retrieve the error reason? If so, since reject-reason is a part of the tx_info object can’t we just return the reject-reason rather than raising an exception?

MarcoFalke commented at 9:46 am on January 8, 2021:

None of the code paths currently send a tx that we know is going to fail. Otherwise, sendrawtransaction would throw.

mjdietzx commented at 4:50 pm on January 8, 2021:

This makes me notice a potentially unwanted side-effect in send_self_transfer. If a test case is catching an exception in send_self_transfer, self._utxos.append({'txid': txid, 'vout': 0, 'value': send_value}) is happening before some rpc calls and assertions that could potentially fail. This could cause some confusing behavior in the case an exception does get thrown, and self._utxos is left modified

Not related to this PR imo as this behavior was already present, but do you think it would be useful to do self._utxos.append({'txid': txid, 'vout': 0, 'value': send_value}) at the very end of this method, when we know it will succeed, in another PR?

mjdietzx commented at 4:53 pm on January 8, 2021:

Well, I guess this would only apply if there are cases where testmempoolaccept succeeds but sendrawtransaction fails

in test/functional/test_framework/wallet.py:79 in faabc26a61

79+        from_node.sendrawtransaction(tx_hex)
80         assert_equal(tx_info['vsize'], vsize)
81-        assert_equal(tx_info['fee'], fee)
82-        return {'txid': txid, 'wtxid': tx_info['wtxid'], 'hex': tx_hex}
83+        assert_equal(tx_info['fees']['base'], fee)
84+        return {'txid': tx_info['txid'], 'wtxid': from_node.decoderawtransaction(tx_hex)['hash'], 'hex': tx_hex}

mjdietzx commented at 4:55 pm on January 8, 2021:

Unrelated to this PR: What are your thoughts of returning the entire tx_info here? There have been a few cases where that would have been nice when I worked with MiniWallet. If you think it’s useful I could open a PR for that

glozow commented at 0:10 am on January 10, 2021:

@mjdietzx What fields are you thinking of in particular?

Also I’m pretty sure you can get wtxid with tx.calc_sha256(with_witness=True)

MarcoFalke commented at 9:17 am on January 10, 2021:

Also I’m pretty sure you can get wtxid with tx.calc_sha256(with_witness=True)

sha256 here is is an int, not a hex encoding of how txids are represented in Bitcoin (with endian switched for some reason)

MarcoFalke commented at 9:19 am on January 10, 2021:

If you think it’s useful I could open a PR for that

Sure, let’s do that in the PR where the return value is needed.

mjdietzx commented at 8:45 pm on January 8, 2021: contributor

ACK faabc26a61873b2cd0390a21df571fe53c893c11

glozow commented at 0:04 am on January 10, 2021: member

Concept ACK hehe, 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?

MarcoFalke commented at 9:19 am on January 10, 2021: member

What do you think of a bool option?

Sure, but let’s do that in the PR that needs the feature. (To not introduce dead code branches right away)

MarcoFalke merged this on Jan 11, 2021

MarcoFalke closed this on Jan 11, 2021

MarcoFalke deleted the branch on Jan 11, 2021

sidhujag referenced this in commit da64cb0d57 on Jan 11, 2021

DrahtBot locked this on Aug 16, 2022

test: Replace getmempoolentry with testmempoolaccept in MiniWallet #20876

Conflicts