Skip to content

test: Move tx creation to create_self_transfer_multi#26414

Merged
maflcko merged 1 commit intobitcoin:masterfrom
kouloumos:modify-create_self_transfer
Dec 5, 2022
Merged

test: Move tx creation to create_self_transfer_multi#26414
maflcko merged 1 commit intobitcoin:masterfrom
kouloumos:modify-create_self_transfer

Conversation

@kouloumos
Copy link
Contributor

@kouloumos kouloumos commented Oct 28, 2022

Two birds with one stone: replacement of #26278 with simplification of the MiniWallet's transaction creation logic.

Currently the MiniWallet creates simple txns (1 input, 1 output) with create_self_transfer. #24637 introduced create_self_transfer_multi which uses create_self_transfer to create a "transaction template" which then adjusts (copy and mutate inputs and outputs) in order to create more complex multi-input multi-output transactions.
This can more easily lead to issues such as #26278 and is more of a maintenance burden.

This PR simplifies the logic by going the other way around. Now create_self_transfer uses create_self_transfer_multi.
The transaction creation logic has been moved to create_self_transfer_multi which is being called by create_self_transfer to construct the simple case of 1 input 1 output transaction.

@fanquake fanquake added the Tests label Oct 28, 2022
pablomartin4btc
pablomartin4btc reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. It looks cleaner and, as you said, simplified, I'll take a deeper look soon.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 4, 2022 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke
Concept ACK pablomartin4btc, hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26625 (test: Run mempool_packages.py with MiniWallet by MarcoFalke)
  • #25038 (policy: nVersion=3 and Package RBF by glozow)
  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

@DrahtBot DrahtBot mentioned this pull request Nov 4, 2022
Closed
hernanmarino
hernanmarino reviewed Nov 4, 2022
View reviewed changes
Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK. At first sight, everything looks fine. @kouloumos did you have a chance to test the output of these functions before / after the change? Something interesting to try would be to verify the output of the previous/current version of each pair of functions, but I didn't have a chance to do it yet.

@DrahtBot DrahtBot mentioned this pull request Nov 5, 2022
Closed
@kouloumos
Copy link
Contributor Author

kouloumos commented Nov 21, 2022

[...] did you have a chance to test the output of these functions before / after the change? Something interesting to try would be to verify the output of the previous/current version of each pair of functions, but I didn't have a chance to do it yet.

@hernanmarino I'm not sure if I understood what you are trying to say. The individual tests that are using those functions are working properly, which means that the output is the same.

@hernanmarino
Copy link
Contributor

hernanmarino commented Nov 21, 2022 •
edited
Loading

@hernanmarino I'm not sure if I understood what you are trying to say. The individual tests that are using those functions are working properly, which means that the output is the same.

At the moment of my comment I had an idea of doing myself some other tests, but now I have a deeper understanding of the changes, and it looks ok. And as you say, if the tests work, there is no reason to do anything else.

This was referenced Nov 30, 2022
Closed
Merged
@maflcko
Copy link
Member

maflcko commented Dec 5, 2022

ACK 0b78110 👒

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 0b78110f73965fd827748107e0f3497d3352be71 👒
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUirYgwApWdVLOaxuKSKsdyD9mTANyuO+LITvJde9SVxsClw+1A5pLfakqmHlFyL
fJpPCSdZuy4Qxm3lbDIIsPepSS6JV6EJXMhdfHNArFU/gF4R0zgaOSvd3jH/qCJL
Uq9ux5tU3N3H3RHhCIafeqxgY/1re9cGKAPY2/BvUJv6u+mcNd8AG5LjYcpwAk0z
/SVS/uE6bDecpOQrHNAmUK2sUcEHmwEyFAEmxo5bjtN2+qMQCiZQxKbM34WXATdO
p3ABPNx2k16MUcwWmEJKe7jL9Azy+MDaQhCVdrpB6DvO9LNHXuiOA3/b5Ua+2R1h
37CkjJ0ClIaFI55jqu4MbHzsjKnOALYefddvDNjWLdzIogjL7SlM4sTp9hLx0rfL
Lz93MZ0VqbFb/QjtBae85Vb4nTTkW3CLXBvMivMYR129gzv1xrkV5gbx9YV+VSvv
YyjTyyCOu2RDh1xRSnJ2v1RR1xTLLcTn703LBYCsjufdtrPVeiCMDNy5QtU4Fg+J
Z4WXlfTE
=ksp3
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 38cbf43 into bitcoin:master Dec 5, 2022
@kouloumos kouloumos deleted the modify-create_self_transfer branch December 5, 2022 15:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 5, 2022
Merge bitcoin#26414: test: Move tx creation to create_self_transfer_m…
59b077f 
…ulti

0b78110 test: Move tx creation to create_self_transfer_multi (kouloumos)

Pull request description:

  Two birds with one stone: replacement of bitcoin#26278 with simplification of the MiniWallet's transaction creation logic.

  Currently the MiniWallet creates simple txns (1 input, 1 output) with `create_self_transfer`.  bitcoin#24637 introduced `create_self_transfer_multi` **which uses** `create_self_transfer` to create a "transaction template" which then adjusts (copy and mutate inputs and outputs) in order to create more complex multi-input multi-output transactions.
  This can more easily lead to issues such as bitcoin#26278 and is more of a maintenance burden.

  This PR simplifies the logic by going the other way around. Now `create_self_transfer` **uses** `create_self_transfer_multi`.
  The transaction creation logic has been moved to `create_self_transfer_multi` which is being called by `create_self_transfer` to construct the simple case of 1 input 1 output transaction.

ACKs for top commit:
  MarcoFalke:
    ACK 0b78110 👒

Tree-SHA512: 147e577ed5444bee57865bd375b37c9b49d6539e9875c30c2667e70fcba27fe80bcb4552a4e6efb42760d34b40d5dad826883b778eaeefe29425ec081787b4bd
@bitcoin bitcoin locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@hernanmarino hernanmarino hernanmarino left review comments

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kouloumos @DrahtBot @hernanmarino @maflcko @pablomartin4btc @fanquake