test: Replace getmempoolentry with testmempoolaccept in MiniWallet #20876

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2101-testMiniWalletNoSubmit changing 1 files +5 −5

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 . 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 12: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 12: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

Contributors

Labels

Linked (view graph)

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

test: Replace getmempoolentry with testmempoolaccept in MiniWallet #20876

Conflicts