test: use MiniWallet for feature_csv_activation.py #21900
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test can now be run even with the Bitcoin Core wallet disabled.
This allows to get rid of the global miniwallet variable and to specify the used node self.nodes[0] at only one place, instead of passing it to every tx creation/send method again and again. Can be reviewed with --ignore-all-space --color-moved=dimmed-zebra
theStack
force-pushed
the
20210507-test-convert_csvtest_miniwallet
branch
from
6505710 to
bd7f27d
Compare
Member
|
Code review ACK bd7f27d |
maflcko
reviewed
May 10, 2021
Member
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
review ACK bd7f27d 🐕
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be 🐕
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhdJwv9EbEK7JyML4wxs0e2BV4oiOiXls92YWe0BO65IzVDNJiLQaJeUJ3lqETg
05pkpy2zweDJSGYpytfLj2wt9rhL4C8REqqhsQfAsY/zWGWCQg4Uml2g4P9mkRDQ
bWg+jr3AsiO367L1k7Ictm9tcLdtDC7C4YoJkzAnumvajbvpt7GRIq18KzHHR6Qb
OSCpxQf14O3muxyMsiRixxPHCsxtKmbSnIiezc9SIzXCs50PTYnFNPLwvIhUMcSc
0mEbna/YwgsJRmr2QXqVaiaJDij4jE2kDB+J5DXbOXvp2+HrzDL93FL7012jDKRG
HiwaHyg0wkjPOs8TdZAsis7lUwKPYHYe7hxX397Uq/bClLvDTX0S1ElcLrHr87o6
6ONs05VfKsl1QOWPeJTJrAl6c6RFE6x7LuGNnWwF6PPPy11dc0/UuqZkjhtFqCDW
hjzFOV46cHuvfkwOZRy8eJgzYchsYGZx8E1f4ho5Q6uzc6UxF96OMdhFvBkJEY7h
8d28W72M
=AHhn
-----END PGP SIGNATURE-----
Timestamp of file with hash d0d7452f0e357ac3817de8619136214b39f29c122badffab8229d6118e90c698 -
maflcko
reviewed
May 10, 2021
| self.extra_args = [[ | ||
| '[email protected]', | ||
| '-addresstype=legacy', | ||
| '-acceptnonstdtxn=1', |
Member
There was a problem hiding this comment.
Would be nice to not set this. Maybe someone can teach the miniwallet how to create pay-to-raw-pubkey or so?
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 10, 2021
bd7f27d refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner) 2eca46b test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in bitcoin#20078. Short reviewers guideline: - Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed. - The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail. - To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis. ACKs for top commit: laanwj: Code review ACK bd7f27d MarcoFalke: review ACK bd7f27d 🐕 Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
May 24, 2021
4bea301 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner) dc7eb64 test: MiniWallet: add P2PK support (Sebastian Falbesoner) Pull request description: This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](bitcoin/bitcoin#21900 (comment)). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed. Possible follow-ups: * Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better * Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?) * Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted) ACKs for top commit: laanwj: Code review ACK 4bea301 Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 25, 2021
4bea301 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner) dc7eb64 test: MiniWallet: add P2PK support (Sebastian Falbesoner) Pull request description: This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](bitcoin#21900 (comment)). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed. Possible follow-ups: * Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better * Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?) * Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted) ACKs for top commit: laanwj: Code review ACK 4bea301 Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Short reviewers guideline:
sign_transactionand its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple.rehash()call is sufficient. Also, generating an addressself.nodeaddress(and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed.MiniWalletcreate a tx with a specific input, we have to call.get_utxo()before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (mark_as_spent=False). With the behaviour on master, the second call to.get_utxo()with the same input would fail.miniwalletis set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can useself.nodes[0]directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis.