karask
Issue when parsing an unsigned segwit transaction with Transaction.from_raw()
Hi & thanks for your bitcoinutils package, very handy ! :+1: But I think I've spotted an issue.
Although Transaction.from_raw() is able to parse a signed or unsigned legacy tx, it is only able to correctly parse signed segwit txs because it assumes the presence of witnesses in the tx payload.
But unsigned segwit tx payloads do not have any witness field.
This PR proposes to fix that by checking the number of remaining bytes between the end of the last output and the end of the tx payload and by skipping the witnesses parsing if that number is not greater than 4 (i.e. last tx field i.e. locktime length).
Hi @karask and @hcleonis,
I noticed there are merge conflicts preventing this PR from being resolved. Since I don't have direct write access to the repository, I'm bringing this to your attention.
Could one of the repository maintainers:
- Pull the latest changes from the master branch
- Resolve the merge conflicts manually
- Update the pull request
The proposed fix looks promising for handling unsigned SegWit transactions, but the merge conflicts need to be addressed first.
Alternatively, @hcleonis might want to:
- Rebase their branch on the latest master
- Force push the updated branch
- This often resolves merge conflicts automatically
@karask, as the repository owner, your guidance would be appreciated on how to proceed with merging this fix.
Hi @JAGADISHSUNILPEDNEKAR Thanks for reporting these merge conflicts. I'm not surprised as I should have pushed some tests for this PR months ago now as @karask requested but I didn't have time to do it (shame on me). I'll try and push a rebased version during the coming end of week.
Hi @hcleonis and @karask,
I'm following up on this issue regarding parsing unsigned segwit transactions. The proposed fix to check remaining bytes between the end of the last output and the end of the transaction payload seems like a solid approach.
@hcleonis - Thank you for offering to rebase and update the PR with tests. Having comprehensive tests would indeed help validate both the existing functionality and the new fix.
As a contributor to the project, I'm interested in seeing this issue resolved. Once the PR is updated with tests and the merge conflicts are addressed, I believe this would be ready for @karask to review.
If there's anything I can help with in the meantime, please let me know.
Best regards, JAGADISHSUNILPEDNEKAR