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

Document PSBT roles and limitation #1100

Merged
merged 1 commit into from Jul 17, 2022

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jul 15, 2022

The READEME claims rust-bitcoin supports PSBT finalization, but really needs rust-miniscript for that. I think we should make this clear in this crate's PSBT examples as well.

Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional.

We should probably add it to the README here or something. If I needed to deal with scripts or witnesses in any way beyond reserializing them or hashing scriptpubkeys into addresses, I'd immediately turn to Miniscript. You basically can't work on scripts without it, unless you are doing something very specific.

Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.

Not to be too glib, but suppose we had a finalizer that just always returned an error, and was documented not to finalize any transactions. Would that meet your expectations? It is hard to do much more without Miniscript.

Originally posted by apoelstra in #630 (comment)

@apoelstra
Copy link
Member

Do we really only support PSBT v0? Otherwise this looks good to me.

@DanGould
Copy link
Contributor Author

We only support v0 for now. Any non zero version number errors as per bip-174 version numbers spec

if pair.key.key.is_empty() {
// there can only be one version
if version.is_none() {
let vlen: usize = pair.value.len();
let mut decoder = Cursor::new(pair.value);
if vlen != 4 {
return Err(encode::Error::ParseFailed("Wrong global version value length (must be 4 bytes)"))
}
version = Some(Decodable::consensus_decode(&mut decoder)?);
// We only understand version 0 PSBTs. According to BIP-174 we
// should throw an error if we see anything other than version 0.
if version != Some(0) {
return Err(encode::Error::ParseFailed("PSBT versions greater than 0 are not supported"))
}
} else {
return Err(Error::DuplicateKey(pair.key).into())
}
} else {
return Err(Error::InvalidKey(pair.key).into())
}

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 896ca42

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 896ca42

@apoelstra apoelstra merged commit 2ba759e into rust-bitcoin:master Jul 17, 2022
@DanGould DanGould deleted the readme-psbt branch July 18, 2022 02:31
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
896ca42 Document PSBT roles and limitation (DanGould)

Pull request description:

  The READEME claims rust-bitcoin supports PSBT finalization, but really needs rust-miniscript for that. I think we should make this clear in this crate's PSBT examples as well.

  > > Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional.
  >
  > We should probably add it to the README here or something. If I needed to deal with scripts or witnesses in any way beyond reserializing them or hashing scriptpubkeys into addresses, I'd immediately turn to Miniscript. You basically can't work on scripts without it, unless you are doing something very specific.
  >
  > >Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.
  >
  > Not to be too glib, but suppose we had a finalizer that just always returned an error, and was documented not to finalize any transactions. Would that meet your expectations? It is hard to do much more without Miniscript.
  >

  _Originally posted by apoelstra in rust-bitcoin/rust-bitcoin#630 (comment)

ACKs for top commit:
  apoelstra:
    ACK 896ca42
  tcharding:
    ACK 896ca42

Tree-SHA512: e71a65b8c04134d9b3406ea76bb915fa116e4a961f9f6cb24350422f9d550cba26a630e02f9ba352fae63076926532bc4bf2d1001488666a05f18d7774ddda9e
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants