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

add function to determine script-type for all inputs in a psbt #6984

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

rage-proof
Copy link
Contributor

Currently it's only possible to set the script-type for the inputs in a psbt for wallet owned inputs. For other imported inputs(payjoin) they stay 'unknown'.
This PR adds a function to determine the inputs-types by checking if a script(in scriptpubkey or redeem-script) matches a scripts pattern.

I wanna use this function in this PR: #6804 to validate equality of input-types in the payjoin.


general question:
Why is the naming of the script-types different than in most other application/BIPs? If I'm right, in Electrum composed names are in the form: [inner/encapsulated-type]-outer (p2wpkh-p2sh), but in the BIP's the naming is p2sh-p2wpkh. Do I understand it correctly?

@SomberNight
Copy link
Member

Why is the naming of the script-types different than in most other application/BIPs? If I'm right, in Electrum composed names are in the form: [inner/encapsulated-type]-outer (p2wpkh-p2sh), but in the BIP's the naming is p2sh-p2wpkh. Do I understand it correctly?

Specifically which BIPs use either naming convention?

@rage-proof
Copy link
Contributor Author

Specifically which BIPs use either naming convention?

BIP-174(psbt) for example has Test Vectors and their description uses the later form.

@SomberNight
Copy link
Member

SomberNight commented Jan 31, 2021

Well our nomenclature predates BIP174; I think it predates any BIP that has one :)
There isn't really a fundamental reason to prefer one over the other; we use what we use for historical reasons: these are the names we arbitrarily came up with years ago.
Note that us changing them would be a breaking change, as we use these for WIF keys.

@rage-proof
Copy link
Contributor Author

Thanks for the explanation.

electrum/transaction.py Outdated Show resolved Hide resolved
if match_script_against_template(decoded, SCRIPTPUBKEY_TEMPLATE_P2WPKH):
return 'p2wpkh'
if match_script_against_template(decoded, SCRIPTPUBKEY_TEMPLATE_P2WSH):
return 'p2wsh'
Copy link
Member

Choose a reason for hiding this comment

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

Note that in many other places in transaction.py, a script_type of "p2sh" means p2sh-multisig, while a "p2wsh" means p2wsh-multisig.
I guess something will have to be done about that sooner or later anyway... Maybe it's ok to ignore that for now.
However, this is something to keep in mind if you are planning on comparing these script_type strings to determine if certain inputs use similar scripts.

@SomberNight
Copy link
Member

SomberNight commented Feb 3, 2021

Note that in many other places in transaction.py, a script_type of "p2sh" means p2sh-multisig, while a "p2wsh" means p2wsh-multisig.
I guess something will have to be done about that sooner or later anyway...

Ah, actually this might already become a problem here.

It seems to me that is_complete will consider non-ismine inputs that this PR marks as e.g. "p2sh" to be complete, and the code will try to finalise them. Even (non-ismine) inputs that are multisig can get incorrectly finalised because num_sig is not set for them correctly.

def is_complete(self) -> bool:
if self.script_sig is not None and self.witness is not None:
return True
if self.is_coinbase_input():
return True
if self.script_sig is not None and not self.is_segwit():
return True
signatures = list(self.part_sigs.values())
s = len(signatures)
# note: The 'script_type' field is currently only set by the wallet,
# for its own addresses. This means we can only finalize inputs
# that are related to the wallet.
# The 'fix' would be adding extra logic that matches on templates,
# and figures out the script_type from available fields.
if self.script_type in ('p2pk', 'p2pkh', 'p2wpkh', 'p2wpkh-p2sh'):
return s >= 1
if self.script_type in ('p2sh', 'p2wsh', 'p2wsh-p2sh'):
return s >= self.num_sig
return False

def finalize(self) -> None:
def clear_fields_when_finalized():
# BIP-174: "All other data except the UTXO and unknown fields in the
# input key-value map should be cleared from the PSBT"
self.part_sigs = {}
self.sighash = None
self.bip32_paths = {}
self.redeem_script = None
self.witness_script = None
if self.script_sig is not None and self.witness is not None:
clear_fields_when_finalized()
return # already finalized
if self.is_complete():
self.script_sig = bfh(Transaction.input_script(self))
self.witness = bfh(Transaction.serialize_witness(self))
clear_fields_when_finalized()

self.num_sig = 0 # type: int # num req sigs for multisig

I think the easiest fix would be to limit the detection added in this PR to only
p2pkh, p2wpkh-p2sh, p2wpkh.

The longer term fix would be

  • to split up p2sh et al to p2sh-ms and p2sh-unknown (or "p2sh-generic"? or just "p2sh"?) -- probably also change them to an Enum or similar instead of string literals
  • and then adapt the detection to specifically match for multisig templates, and also set num_sig

@rage-proof
Copy link
Contributor Author

Oh, interesting case. I agree this is a good solution for now.

I think the easiest fix would be to limit the detection added in this PR to only
p2pkh, p2wpkh-p2sh, p2wpkh.

This are the only types defined for Payjoin anyway.

@SomberNight SomberNight merged commit cb39777 into spesmilo:master Feb 4, 2021
@SomberNight
Copy link
Member

Thanks.

@SomberNight SomberNight added the topic-transactions 📑 related to logic in transaction.py label Feb 4, 2021
@rage-proof rage-proof deleted the script_type_psbt branch February 7, 2021 11:56
rage-proof added a commit to rage-proof/electrum that referenced this pull request Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-transactions 📑 related to logic in transaction.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants