Take the committed script type from the script, not from which PSBT fields are set - #2
Merged
Conversation
…ields are set The unified message commits a byte naming the script type, which domain separates the four so a signature made for one can never verify as another. It was derived from field presence: witness_script or witness_utxo being set meant WITNESS_V0. BIP174 allows a legacy input to be carried as witness_utxo, and consensus can only read that output as BARE. So the same P2PKH UTXO committed WITNESS_V0 through one carrier and BARE through the other, one of the two signatures could not verify on chain, and two signers given different carriers of one PSBT disagreed. It also broke multisig interop with the other implementations, which all classify from the spent scriptPubKey. Classified from the script now, in both PSBT and PSBTView. The legacy digest selection is untouched: it is upstream's, and only the unified message commits the byte to consensus.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The unified message commits a byte naming the script type, which domain separates the four so a signature made for one can never verify as another. It was derived from which PSBT fields were present rather than from the spent output's script:
BIP174 allows a legacy input to be carried as
witness_utxo, and theorchain short-circuits on that before the actual script is ever consulted.What went wrong
The same P2PKH output committed a different byte depending on how the PSBT carried it, demonstrated by execution:
Consensus can only read that output as
BARE, so the second signature cannot verify on chain. Two signers handed different carriers of one PSBT also disagreed with each other, and both differed from the other implementations of this message, which classify from the spentscriptPubKey.The fix
Classify from the script, in both
PSBTandPSBTView. The two clauses that already tested the real script are what remains, which matches the rule the reference implementation uses: witness types arep2wpkhandp2wshnatively, or wrapped where the redeem script is one of them; everything else is bare.The legacy digest selection is deliberately untouched. It is upstream's, and only the unified message commits this byte to consensus.
Verified
The regression test fails against the previous code and passes against this one. Both carriers now produce the identical digest, and it is the
BAREone that was already correct, so the case that moved is the one that was wrong.Wrapped segwit is covered separately, to pin that dropping the presence clauses does not send P2SH-P2WPKH to
BARE.Worth noting for future review: the PSBT versus PSBTView agreement test does not catch this. Both implementations carried the same defect and agreed with each other throughout. Only comparing the two carriers of one UTXO exposes it.
155 tests pass.
On chain
A Knots regtest node past activation, spending the same P2PKH output through each carrier, signed by this library and finalized by it:
non_witness_utxo0x210x21witness_utxo0x21The digest also matches Knots' own
test_framework.script.UnifiedSignatureHash, which is written from the specification rather than by mirroring an implementation, for the legacy input through both carriers and for a P2SH-P2WPKH input that must stayWITNESS_V0.Harness:
embit_legacy_carrier_e2e.py.