Skip to content

test: Actually set sequence in MiniWallet create_self_transfer_multi #26278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
maflcko wants to merge 1 commit into from
Closed

test: Actually set sequence in MiniWallet create_self_transfer_multi #26278

maflcko wants to merge 1 commit into from

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 7, 2022

Can be tested with:

diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
index de8eedc10e..25b630bb00 100644
--- a/test/functional/test_framework/wallet.py
+++ b/test/functional/test_framework/wallet.py
@@ -284,6 +284,7 @@ class MiniWallet:
             self._bulk_tx(tx, target_weight)
 
         txid = tx.rehash()
+        assert [i.nSequence for i in tx.vin]==sequence
         return {
             "new_utxos": [self._create_utxo(
                 txid=txid,

@DrahtBot DrahtBot added the Tests label Oct 7, 2022
@kouloumos
Copy link
Contributor

kouloumos commented Oct 28, 2022

Not such a big of an issue (as I believe other tests would break if that was the case) but this test scenario is not tested since #25522 where this was introduced.

I believe that the current logic of how create_self_transfer_multi uses create_self_transfer in order to create a simple transaction template to then adjust (copy and adapt) based on its arguments can more easily lead to this kind of issues and is more of a maintenance burden.

Looking into this, I've came up with a change in logic, where create_self_transfer uses create_self_transfer_multi. I think this simplifies the code and (if I am not missing something) could be a replacement to this PR.
You can see this change in kouloumos@0b78110 (modify-create_self_transfer branch).

@maflcko
Copy link
Member Author

maflcko commented Oct 28, 2022

Sounds good

@kouloumos kouloumos mentioned this pull request Oct 28, 2022
Merged
@maflcko maflcko closed this Oct 28, 2022
@maflcko maflcko deleted the 2210-test-seq-🕕 branch October 28, 2022 14:34
maflcko pushed a commit that referenced this pull request Dec 5, 2022
Merge #26414: test: Move tx creation to create_self_transfer_multi
38cbf43 
0b78110 test: Move tx creation to create_self_transfer_multi (kouloumos)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    ACK 0b78110 👒

Tree-SHA512: 147e577ed5444bee57865bd375b37c9b49d6539e9875c30c2667e70fcba27fe80bcb4552a4e6efb42760d34b40d5dad826883b778eaeefe29425ec081787b4bd
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 Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maflcko @kouloumos @DrahtBot