Fix of TODO regarding explicitly having to add the 00 byte for non segwit inputs in segwit transactions

[abhigit-saha]

This PR aims to solve the TODO where explicitly having to add an empty TxWitnessInput was necessary for compatibility.
Changes:

• I have created a new method create_witness that checks whether the length of the witnesses is same as inputs.
• If it is not, then, since witnesses are added after inputs, I add empty TxWitnessInput that make the length equal, and then update the one corresponding to the txin_index.
• Changed the example spend_multi_input_p2tr_and_p2pkh to use create_witness instead of manually changing the witness array.

On running the updated version of the spend_multi_input_p2tr_and_p2pkh.py example and comparing it to before the implementation yields the same output, suggesting that the implementation is correct. Kindly review it and let me know if any changes are needed.

Before:

After:

[abhigit-saha]

Hey @j1warren I have fixed the formatting issues. Since this was an issue that was listed in the TODO file, I do think it is worth fixing. Kindly let me know if you want anything from the specific function itself, or any specific code style I should change in order to make it more consistent!

[karask]

@abhigit-saha This is not as easy as it looks.

Your loop for appending witnesses will add witness every time you set_witness. Would your code work for inputs: legacy, segwit, legacy, legacy, segwit ?

This probably needs a more holistic approach to be solved. Do we need to specify TxInput's as segwit or not on instantiatkion? If yes, this would require a lot of changes to automatically flag the Transaction as segwit if at least one of the inputs is segwit. Then we can check the TxInputs to add the witness automatically. I think there are other ways as well with pros and cons. I am not sure what to select, which is why I left it for later.

I would suggest to propose a solution in detail, together with how it would be implemented (which files will be changed, pros and cons, etc.) and then proceed to implementation.

[abhigit-saha]

Hi @karask I had thought about this and yes I think it would work. My code basically only sets witnesses when the set_witness function is called (which imperatively means that the user is trying to set the witness for a segwit input at index txin_index). The user won't be using set_witness for non segwit inputs (to abstract away the logic of adding empty witnesses). This is not the same as just creating a wrapper function for setting witnesses[i] = witness kind of logic.

The full flow:
The user would call set_witness for vin i (suppose) to set the witness for that segwit input. Then the function would loop over all the previous inputs and add empty witnesses for those (only add the required number of witnesses, since it could happen that previously there was a witness set in a segwit input, and then there were multiple legacy inputs, so I add only the witnesses for those legacy inputs). Should I work on a separate test file/ edit the current one to justify this? Meanwhile I hope this explanation made some sense

[karask]

I had a more holistic change in mind, however it wasn't captured in the TODO file so I can understand your rationale. I updated the TODO to be more specific (I always intended to create issues out of the TODO file which was more for myself).

For now, your solution would make the user code cleaner so I decided to merge it.

EDIT: However, several tests break with the change. So you need to resolve this before merging.

[abhigit-saha]

Hi @karask sorry for the late response. I really have no clue why the tests were failing, I figured it probably was a git issue, considering the function I had implemented wasn't really used anywhere aside from an example. Maybe some part of the code wasn't updated when I pushed an earlier version of the code?

I have tested it now locally and it seems to work, you can test it yourself