Skip to content
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.

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

psbt: always use non witness serialization format #1842

Merged
merged 1 commit into from
Apr 18, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Apr 12, 2022

Fixes #1841.

BIP-0174 states that the transaction must be in the old serialization
format (without witnesses).

https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#specification (see first entry in table).

cc @Roasbeef, @positiveblue

Fixes lightningnetwork/lnd#6386

@coveralls
Copy link

coveralls commented Apr 12, 2022

Pull Request Test Coverage Report for Build 2161078427

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 54.135%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcutil/psbt/psbt.go 5 6 83.33%
Files with Coverage Reduction New Missed Lines %
btcutil/psbt/psbt.go 1 71.59%
Totals Coverage Status
Change from base Build 2148764512: 0.004%
Covered Lines: 24795
Relevant Lines: 45802

💛 - Coveralls

@guggero
Copy link
Collaborator Author

guggero commented Apr 12, 2022

This requires a new tag (btcutil/psbt/v1.1.3) after merge.


// BIP-0174 states: "The transaction must be in the old serialization
// format (without witnesses)."
err = msgTx.DeserializeNoWitness(bytes.NewReader(value))
Copy link
Member

Choose a reason for hiding this comment

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

Should we continue to try to decode the witness version and fall back to non-witness? Since it's possible there's old software out there encoding wit the witness version, we'd potentially break compatibility across lnd version for even btcutil versions with this change as is.

It seems the most important thing here is for us to encode using the new witness version, AFAICT. So if we just serialize w/o the witness, then things are compatible across the prior versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add the fallback. Though I don't think this change will create a compatibility issue. A PSBT packet with an unsigned TX that has a witness set cannot be serialized, the library will complain. And if you call msgTx.Serialize() with a transaction that has no witness, the old format will be used. Therefore the change to the serialization is purely to be more explicit about it. The actual important change is that we always read the TX expecting it to be in the non-witness format.

BIP-0174 states that the transaction must be in the old serialization
format (without witnesses).
@guggero guggero force-pushed the psbt-serialization-fix branch from 5991989 to eb2eeaf Compare April 13, 2022 12:13
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🛰

@Roasbeef Roasbeef merged commit 8c5bfee into btcsuite:master Apr 18, 2022
@Roasbeef
Copy link
Member

The tag btcutil/psbt/v1.1.3 has been pushed.

@guggero guggero deleted the psbt-serialization-fix branch April 19, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants