Make PsbtSigHashType use the same formatting as other *SigHashTypes by dr-orlovsky · Pull Request #898 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2022-03-22T08:00:18Z

The newly introduced PsbtSigHashType uses very different serde formatting from previously used EcdsaSigHashType; for instance it does not output human-readable sighash. This is especially obvious when printing out PSBT as JSON/YAML object and is a breaking change from the 0.27. Serde human-readable implementation requires Display/FromStr, which were also absent.

tcharding

This PR highlights that we have some non-uniformity with the serde for the three sighash types

EcdsaSigHashType uses strings (e.g. SIGHASH_ALL)
SchnorrSigHashType uses the variant name, this is almost certainly a bug (example below)
PsbtSigHashType uses strings for EcdsaSigHashType inner type and hex strings for SchnorrSigHashType and non-standard sighash value.

Example serializatiion of SchnorrSigHashType:

        let ty = SchnorrSigHashType::SinglePlusAnyoneCanPay;
        let ser =serde_json::to_string(&ty).unwrap();
        println!("{}", ser);

Outputs: "SinglePlusAnyoneCanPay"

Perhaps this PR should fix all that? (I swear I did this already but I cannot find the patches in my tree.)

EDIT: I found it, changes to SchnorrSigHashType are in the open RC-fix PR: 609a04a

tcharding · 2022-03-23T00:26:25Z

src/util/psbt/map/input.rs

I get what you are doing with the REVERT-ME in commit brief message but I get the feeling that, since this is the only use of this, that the code will just get fixed and the commit will be left in there. I also just don't like the idea of having a commit with REVERT-ME in the brief message, it is going to scream at any dev that sees it making them check whats going on.

So what is the suggested way of doing this?

src/util/psbt/map/input.rs

dr-orlovsky · 2022-03-23T10:40:50Z

@tcharding force-pushed version which uses your proposal for FromStr and removes FIXME commit

sanket1729

Did a superficial review, left one comment. Will do a detailed review after that is addressed. It might be worth looking at how bitcoin core psbt RPCs deal with sighash types

sanket1729 · 2022-03-23T17:32:30Z

src/util/psbt/map/input.rs

What happens in the case when we have SchnorrSigHashType::Default

This is not related to this PR and a question to how ecdsa_hash_ty function is working. Will check later, but I assume the correct behaviour should be to return ALL value.

I think this is related to this PR. The display for PsbtSigHashType should cover the 00 value case which is Default. Note that this variant is not present in EcdsaSigHashType

I believe it gets output as 0x00 - I wondered the same thing :)

tcharding

LGTM (by the way, I am explicitly not acking in a way that triggers github approve while I'm still new to the project. All my reviews should be considered 'soft' reviews until I'm more experienced).

tcharding · 2022-03-24T00:15:18Z

src/blockdata/transaction.rs

I was copying your work over in #903 and I used unrecognized instead of misspelled_ty because at another place in the code this error could be caused by a malformed hex string, 'misspelled_ty' doesn't seem to fit that use case.

35b682d Implement Display/FromStr for SchnorrSigHashType (Tobin Harding) 46c4164 Improve SigHashTypeParseError field (Tobin Harding) c009210 Use full path for String in macro (Tobin Harding) Pull request description: Implement Display/FromStr for SchnorrSigHashType We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and use them in the `serde_string_impl` macro to implement ser/de. Mirror this logic in `SchnorrSigHashType`. Patch 1 and 2 are preparatory patches for patch 3. ## Notes to reviewers This PR has some conflicts with #898 but is pushing in the same direction, I'm happy to let 898 go in first and rebase on top. ACKs for top commit: sanket1729: ACK 35b682d. Thanks, much easier to review now that the diff is small dr-orlovsky: ACK 35b682d Tree-SHA512: 481f192a3064ff39acf8904737dfb25b54ef128a37e0ca765ebb39138edac772d4f01ed10aa98ff185a8ed5668d64fa5d5957206b920ffe87950cafcf5a3b516

dr-orlovsky · 2022-03-24T06:25:14Z

Re-based on top of merged #903 and optimized the code to support schnorr sighash types

tcharding · 2022-03-24T23:41:31Z

Now to/from string doesn't roundtrip.

I think we want the following test to pass, right?

    #[test]
    fn roundtrip_psbt_sighash_type() {
        let sighash = PsbtSigHashType { inner: 0xffffffff };
        let s = format!("{}", sighash);
        let back = PsbtSigHashType::from_str(&s).unwrap();

        assert_eq!(back, sighash)
    }

The original version you had but with the schnorr optimizations works.

#[allow(deprecated)] // TODO: Swap `trim_left_matches` for `trim_start_matches` once MSRV >= 1.30.
impl FromStr for PsbtSigHashType {
    type Err = SigHashTypeParseError;

    #[inline]
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        // We accept strings of form: "SIGHASH_ALL".
        // (schnorr sighash type is a super set of ecdsa sighash type.)
        if let Ok(ty) = SchnorrSigHashType::from_str(s) {
            return Ok(ty.into());
        }

        // We accept non-standard sighash values.
        if let Ok(inner) = u32::from_str_radix(s.trim_left_matches("0x"), 16) {
            return Ok(PsbtSigHashType { inner });
        }

        Err(SigHashTypeParseError{ unrecognized: s.to_owned() })
    }
}

src/util/psbt/map/input.rs

dr-orlovsky · 2022-03-26T01:39:14Z

@tcharding thank you for catching this... Overall PSBT lacks unit tests and it was my bad that I didn't wrote one for added functionality :(

Updated with more tests and the problem fixed.

dr-orlovsky · 2022-03-27T11:13:05Z

Failing test shows ambiguity in handling Schnorr-specific sighash types in PSBT sighash type. So, should we support parsing PsbtSigHashType from SIGHASH_DEFAULT and SIGHASH_RESERVED strings, or we must require them to be represented only with SIGHASH_ALL and 0xFF?

sanket1729 · 2022-03-27T19:05:03Z

@dr-orlovsky, is this really needed for release? We can this formatting next release.

dr-orlovsky · 2022-03-27T20:39:43Z

@sanket1729 RC1 introduced buggy code which will require major release to fix, i.e. at least half of year :(

dr-orlovsky · 2022-03-27T20:45:33Z

I force-pushed the way I propose to handle the ambiguity described in #898 (comment)

sanket1729 · 2022-03-27T21:53:45Z

@dr-orlovsky What is the bug here? Why can't PsbtSigHashType have different formatting?

I agree that uniform encoding is good to have, but is it a bug if this is different formatting?

tcharding · 2022-03-27T23:23:41Z

src/util/psbt/map/input.rs

Suggested change

fn psbt_sighash_type_schnorr_nostd() {

fn psbt_sighash_type_schnorr_nonstd() {

Non-std, right?

tcharding · 2022-03-27T23:32:22Z

I agree that uniform encoding is good to have, but is it a bug if this is different formatting?

I appreciate you trying to expedite the release @sanket1729, if I understand correctly, @dr-orlovsky is working under the assumption that our release process is a little messed up at the moment and is worried that the 0.29 release won't be for another <insert long time frame here> months :)

Perhaps we should remember 'perfect is the enemy of good' in relation to our processes right now, lets keep moving forwards but be forgiving of the current situation.

Is that a fair interpretation of both of your positions?

sanket1729 · 2022-03-27T23:43:34Z

@tcharding , it is a fair characterization of m situation. 0.28 is a breaking release, we did a breaking change in changing EcdsaSigHashType to PstbSigHashType. There is no obligation(although a good to have) to have the same serde formatting for PsbtSigHashType as EcdsaSigHashType.

sanket1729 · 2022-03-27T23:45:08Z

AFAIU this PR does not work for the most common case of SigHashType::Default.

sanket1729 · 2022-03-27T23:45:45Z

src/util/psbt/map/input.rs

I would like to see this work for Default too.

This is the topic I raised above: what to do with SchnorrSigHash-specific types which must not be used with pre-taproot inputs (#898 (comment))

dr-orlovsky · 2022-03-28T07:58:02Z

@sanket1729 my point is that wallets using JSON/YAML encoding of PSBTs (for instance I have one) will break backward compatibility when we will change this encoding in the next release. So this has to be a major release, i.e. it will take a long time. It is a bug since previously we had a different serde encoding, so we are breaking the compatibility - to break it again in the next version (introduction of a breaking incorrect behavior subjected to the future breaking changes = bug).

sanket1729 · 2022-03-28T08:32:05Z

JSON/YAML encoding of PSBTs (for instance I have one) will break backward compatibility when we will change this encoding in the next release.

This is unrelated to the issue, but I think this might be difficult for rust-bitcoin to guarantee. Fields name changes and lots of auto-derives break things constantly. I think you are misusing psbts if you are reliant on some other serde format for compatibility. Psbt is already compatible with all others that don't even understand serde/rust using consensus encoding (consensus::encode and consensus::decode). You should be using the most common psbt bas64 encoding. You can be sure that all major/minor versions of rust-bitcoin will stay true to consensus encoding.

So, should we support parsing PsbtSigHashType from SIGHASH_DEFAULT and SIGHASH_RESERVED string

I think we should do those as strings for now.

so we are breaking the compatibility

The point of breaking release is that structure of the objects change and the serde implementation will change! Here it is possible to fix it in a (somewhat) backward-compatible way. But this is not always possible and we cannot offer this guarantee.

sanket1729

Ignore my previous comment about serializing DEFAULT and RESERVED, I think what I commented now is a better solution to deal with this.

sanket1729 · 2022-03-28T09:06:07Z

src/util/psbt/map/input.rs

I think this requires it's own custom code as this is the unification of both sighash types. You can write FromStr accordingly. We should not serialize RESERVED (as there is no such thing). And error on things that are not strings

impl fmt::Display for PsbtSigHashType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let s = match self.as_u32() { 0 => "SIGHASH_DEFAULT" 1 => "SIGHASH_ALL", .. => "SIGHASH_NONE", .. => "SIGHASH_SINGLE", ..=> "SIGHASH_ALL|SIGHASH_ANYONECANPAY", .. => "SIGHASH_NONE|SIGHASH_ANYONECANPAY", ..... }; f.write_str(s) } }

dr-orlovsky · 2022-03-28T09:45:14Z

This is unrelated to the issue, but I think this might be difficult for rust-bitcoin to guarantee. Fields name changes and lots of auto-derives break things constantly. I think you are misusing psbts if you are reliant on some other serde format for compatibility. Psbt is already compatible with all others that don't even understand serde/rust using consensus encoding (consensus::encode and consensus::decode). You should be using the most common psbt bas64 encoding. You can be sure that all major/minor versions of rust-bitcoin will stay true to consensus encoding.

I understand your point and is well aware of Base64 and binary PSBT encodins as the main way to maintain the compatibility. But there are much more usecases, which can't work with these encodings and will require serde:

Inspecting PSBT in human-readable way and using this inspection in tests - or, in my software one may export PSBT in YAML/JSON, use some UI to edit it and then re-build PSBT from the edited version.
Providing JSON PSBT data to web interface from the server, relying on the rust PSBT processing on the backend (or the same for FFI between rust and wallet UI)
I also know that @JeremyRubin was using JSON encoding in his work on Sapio and it may break things there.

Regarding the question of whether serde serialization change is a breaking change requiring major version update, in previous PRs affecting it we had this discussion and AFAIR @apoelstra and @Kixunil pointed out that fact (I was not aware of it before). Not sure I will be able to find that place though...

dr-orlovsky · 2022-03-28T09:48:26Z

@sanket1729
Actually I assume that this PR covers another bug dealing with the failed FromStr/Display roundtrips in SchnorrSigHashType, which clearly should be addressed one or other way before the release: 4cf2dcf

JeremyRubin · 2022-03-28T11:13:41Z

For psbt I believe I try to always do it as a consensus encode string because usually I want the tooling outside of rust to be able to work with it. My preferred solution would be to always do psbts, even for JSON, as consensus encode.

…

On Mon, Mar 28, 2022, 2:48 AM Dr. Maxim Orlovsky ***@***.***> wrote: @sanket1729 <https://github.com/sanket1729> Actually I assume that this PR covers another bug dealing with the failed FromStr/Display roundtrips in SchnorrSigHashType: 4cf2dcf <4cf2dcf> — Reply to this email directly, view it on GitHub <#898 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGYN62JYY7SXV2BVAWXWV3VCF57LANCNFSM5RKBH3NA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

apoelstra · 2022-03-28T13:14:55Z

Yes, changing serde serialization is a breaking change and will require a major rev. But as others have said, the serde serialization of PSBT really doesn't matter. There is a consensus encoding and that's what people should be using if they expect consistency (and if they expect to interoperate with any non-rust-bitcoin project).

Probably the serde encoding should just encode the PSBT as either a base64 string or a big byte blob..

dr-orlovsky · 2022-03-28T14:36:18Z

Probably the serde encoding should just encode the PSBT as either a base64 string or a big byte blob..

I will be concept NACK this since w/o serde there will be no simple way of converting PSBT into JSON or YAML for inspection/UI tasks

Personally I am fine with having breaking changes even each two weeks, I am updating my code constantly. My issue was more about not waiting for 1/2 year+ for simple things to get done because we have to wait for the major release.

dr-orlovsky · 2022-03-28T15:01:03Z

@sanket1729, @tcharding fixed according to your suggestions

JeremyRubin · 2022-03-28T15:04:55Z

I think you can still have a to/from Json method if you need a human readable thing. Also for human readable things the bitcoin js stuff can read a psbt for you and then you can access it programmatically. You can also throw the psbt at core and do decodepsbt or analyzepsbt. But in general, I think the as a Json serialization is useless for interop with most things, and we should drop it in favor of one that integrates with any environment that can parse a psbt.

…

On Mon, Mar 28, 2022, 7:36 AM Dr. Maxim Orlovsky ***@***.***> wrote: Probably the serde encoding should just encode the PSBT as either a base64 string or a big byte blob.. I will be concept NACK this since w/o serde there will be no simple way of converting PSBT into JSON or YAML for inspection/UI tasks Personally I am fine with having breaking changes even each two weeks, I am updating my code constantly. My issue was more about not waiting for 1/2 year+ for simple things to get done because we have to wait for the major release. — Reply to this email directly, view it on GitHub <#898 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGYN637ZT47BPX44JFDX7TVCG7W5ANCNFSM5RKBH3NA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dr-orlovsky · 2022-03-28T15:09:22Z

I think you can still have a to/from Json method if you need a human
readable thing.

If serde implementation parses to Base58 string, this would not help.

You can also throw the psbt at core and do decodepsbt or analyzepsbt.

I personally work with YAML from command-line a lot (bitcoin core's JSON is terrible for humans), and for me having command-line tool presenting given PSBT from Base58 like this is very useful:

an example of PSBT YAML

unsigned_tx:
  version: 2
  lock_time: 0
  input:
    - previous_output: "c96ec66b95ab23bbc287d698c5e63f141dcd75b7728d00f5a89e1f2d93a9424f:1"
      script_sig: ""
      sequence: 4294967295
      witness: []
  output:
    - value: 10000
      script_pubkey: 5120137b87fb9b00c8e09f1b91a9379c98eef1774a568f8ad8514079aaa3941666f3
    - value: 9666
      script_pubkey: 5120566b18c07d063d114a572735c4705f4aa007486b016463d4ffae278d225c3a91
version: 0
xpub:
  tpubDCLfi9JceW8Er2qcaK7gBudjXPrS31VCf7wiiPFLV2s9ptKLkaGxnQutTtdsJoqgbRxWhBDMHJEZ6XsGfC26QhMZEoRYfdCFS3qhaXskUeD:
    - 877c9433
    - "m/86'/1/0'"
  tpubDCLfi9JceW8FJEw6RpfrCtByfaKCHsS9VHdrvB97ZkfS2wKbxtygMxW4MraMuPV5avsDkvae5t36MMwtRuJ5btAfQh7NxKGpmNxzwSm15ZF:
    - 877c9433
    - "m/86'/1/10'"
proprietary: []
unknown: []
inputs:
  - non_witness_utxo:
      version: 2
      lock_time: 2190833
      input:
        - previous_output: "8c7d5031f9e1e9ef691aacedd20a5a8e6dd79882b569581c331a38c0ba17491c:0"
          script_sig: ""
          sequence: 4294967294
      output:
        - value: 464333
          script_pubkey: 0014862b51f53fa54235a0c5c5e1fb256d73dc90fffc
        - value: 20000
          script_pubkey: 5120f23d25a56919ba6c873a727a2699c8764fffaa2f5fb70df2f5febbec86b9fe8f
    witness_utxo: ~
    partial_sigs: {}
    sighash_type:
      inner: 1
    redeem_script: ~
    witness_script: ~
    bip32_derivation: []
    final_script_sig: ~
    final_script_witness: ~
    ripemd160_preimages: {}
    sha256_preimages: {}
    hash160_preimages: {}
    hash256_preimages: {}
    tap_key_sig:
      sig: 987a59ed7b7a57efa602b1ed86551f4bf4759f5198adc006b0043b045091813d870a253a45a0253c55acb77f71f3d3b90b05ea02a5989fdf7bb74535a8d0afb3
      hash_ty: All
    tap_script_sigs:
      - - - 6332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388
          - 27686a1d5598f23ac8884676a4ffd6b742a9936abcc10fae26aecd09776af574
        - sig: 7e5e4521edccbe61bec1370b61fbd3d39ae8be4229af6f8c428faf7a02f1a4432b520ffefa6431482aa51b76e6dda8518a2aefa4ca90464b91f8ee1800ddff4d
          hash_ty: All
      - - - 6332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388
          - 3b17906c53e5308858f3e5a78f1ca33ef014cb84096f608b35983822bc5867da
        - sig: 90f9f9651a0d6780a6ee2786868dbf546b99fef527fa0b1c3e92e7c36dc05f4006f22944bd0544724d55c0b43c091745d67a759cc015c4ec8b43ab2b8aea0d6f
          hash_ty: All
      - - - 6332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388
          - d7cc5bca95baca4b690daa28d0efe18ae82fe4cc1ce44814ee2be098b7f1510b
        - sig: 7ad2b4fa33e8c9fe02e1e7db89068ca5171f1173bc876451d1d71f7519a1fbe8aedd4c5b93e202b2dafa8eb3ae17def57f4f9b0ec9c9a5c93caf4ab541d94b55
          hash_ty: All
    tap_scripts:
      - - leaf_version: 192
          output_key_parity: 1
          internal_key: d99571989a8fcd762e3bd5b3e864ee81727fbff82660c4a57b5de501b4ba8ed4
          merkle_branch:
            - 27686a1d5598f23ac8884676a4ffd6b742a9936abcc10fae26aecd09776af574
            - 3b17906c53e5308858f3e5a78f1ca33ef014cb84096f608b35983822bc5867da
        - - 206332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388ac20cff309356e57796533f8c22ec6b9625e0b465498f123a8169b2a9d8e2c278255ba20dec28c52648655a6d711525503bbfebaa8c12a332e3c9ffd82064ff166af698cba529d5ab1
          - 192
      - - leaf_version: 192
          output_key_parity: 1
          internal_key: d99571989a8fcd762e3bd5b3e864ee81727fbff82660c4a57b5de501b4ba8ed4
          merkle_branch:
            - 9732abae6c1729c3bb135a2f52977265f290c8d8fba6377c7a0e0cc5eece5567
        - - 206332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388ac20cff309356e57796533f8c22ec6b9625e0b465498f123a8169b2a9d8e2c278255ba20dec28c52648655a6d711525503bbfebaa8c12a332e3c9ffd82064ff166af698cba539c
          - 192
      - - leaf_version: 192
          output_key_parity: 1
          internal_key: d99571989a8fcd762e3bd5b3e864ee81727fbff82660c4a57b5de501b4ba8ed4
          merkle_branch:
            - d7cc5bca95baca4b690daa28d0efe18ae82fe4cc1ce44814ee2be098b7f1510b
            - 3b17906c53e5308858f3e5a78f1ca33ef014cb84096f608b35983822bc5867da
        - - 206332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388ac20cff309356e57796533f8c22ec6b9625e0b465498f123a8169b2a9d8e2c278255ba20dec28c52648655a6d711525503bbfebaa8c12a332e3c9ffd82064ff166af698cba519d0164b1
          - 192
    tap_key_origins:
      - - 6332fd4c53a053bccc53a1d82fa2e1e2276ce609c0200d3ced4cd74a22653388
        - - - 27686a1d5598f23ac8884676a4ffd6b742a9936abcc10fae26aecd09776af574
            - 3b17906c53e5308858f3e5a78f1ca33ef014cb84096f608b35983822bc5867da
            - d7cc5bca95baca4b690daa28d0efe18ae82fe4cc1ce44814ee2be098b7f1510b
          - - fcedac08
            - m/0/0
      - - d99571989a8fcd762e3bd5b3e864ee81727fbff82660c4a57b5de501b4ba8ed4
        - - []
          - - 4d94875d
            - m/0/0
    tap_internal_key: d99571989a8fcd762e3bd5b3e864ee81727fbff82660c4a57b5de501b4ba8ed4
    tap_merkle_root: 94b1f8ca0e387b63d25fce87354e841f18d2a97c1027aaee7e3072fa40233b1e
    proprietary: []
    unknown: []
outputs:
  - redeem_script: ~
    witness_script: ~
    bip32_derivation: []
    tap_internal_key: ~
    tap_tree: ~
    tap_key_origins: []
    proprietary: []
    unknown: []
  - redeem_script: ~
    witness_script: ~
    bip32_derivation: []
    tap_internal_key: 9342cfe214556af10e9b34549458af0d937603c68dd6e8b95f005095b26df7da
    tap_tree:
      branch:
        - hash: 2b15aae72ed4ac365785d21c56cf435a43633ba2af84e4ed3c0b4d26897bbcf2
          leaves:
            - script: 2071beef4035dca001a9ab71983b860d273b1a2c8173d00656b8bbaf497e6e444dac207339326d461857494b87bbe2cc9b3e31ccff1d7844a2bdfcb6c5c2e9727098aeba20ad21352f1ca6eef7db5fa23f6e573eb5c823da6c7755c61c652a45d3349d059aba519d0164b1
              ver: 192
              merkle_branch:
                - 2409b7478a75b0350b4710b5eca54056fde8c22ba035eb209e9a1e4e659783f1
                - 9653be7568281a9657d57478560cb2638acf80c98124f0318adaedb16d63b02c
            - script: 2071beef4035dca001a9ab71983b860d273b1a2c8173d00656b8bbaf497e6e444dac207339326d461857494b87bbe2cc9b3e31ccff1d7844a2bdfcb6c5c2e9727098aeba20ad21352f1ca6eef7db5fa23f6e573eb5c823da6c7755c61c652a45d3349d059aba529d5ab1
              ver: 192
              merkle_branch:
                - 304ad4baaba8c42012939da18cd35fded9d15fb24e8af33e364cf02fb340a5ce
                - 9653be7568281a9657d57478560cb2638acf80c98124f0318adaedb16d63b02c
            - script: 2071beef4035dca001a9ab71983b860d273b1a2c8173d00656b8bbaf497e6e444dac207339326d461857494b87bbe2cc9b3e31ccff1d7844a2bdfcb6c5c2e9727098aeba20ad21352f1ca6eef7db5fa23f6e573eb5c823da6c7755c61c652a45d3349d059aba539c
              ver: 192
              merkle_branch:
                - ac035f11d4fdce47af56e62818ad3f11c7b6a2c5bcefc435787148f2c08318bf
    tap_key_origins: []
    proprietary: []
    unknown: []

sanket1729

ACK 992857a. This is much better

sanket1729 · 2022-03-28T16:48:08Z

Might make sense to add a similar PR to rus-bitcoin.

https://github.com/rust-bitcoin/rust-secp256k1/pull/369/files

apoelstra

ACK 992857a

apoelstra · 2022-03-28T17:32:44Z

Sure, though I again point out that "serde encoding" is not defined by us and can't possibly be "the same as consensus encoding".

JeremyRubin · 2022-03-28T17:41:22Z

still doesn't seem like good default behavior if it's what you want.

i would recommend doing this then:

struct HumanPSBT(Psbt);

And either manually implementing Serialize/Deserialize OR doing a TryFrom serde field with a JSON.

That way if you ever want the humanPSBT impl, just wrap the type in your struct / wrap before serializing if just sending a PSBT itself.

but i don't think we want this in most situations, to the point where I definitely have conversion structs in places where i have to stringify psbts before serdeing, and that should be the default.

In general, we should aim to use external stable serializations where they exist, and PSBT is such a case.

dr-orlovsky added bug trivial Obvious, easy and quick to review (few lines or doc-only...) RC fix labels Mar 22, 2022

dr-orlovsky added this to the 0.28.0 milestone Mar 22, 2022

tcharding reviewed Mar 23, 2022

View reviewed changes

tcharding removed the trivial Obvious, easy and quick to review (few lines or doc-only...) label Mar 23, 2022

tcharding mentioned this pull request Mar 23, 2022

Re-work sighash type conversion methods #796

Merged

sanket1729 reviewed Mar 23, 2022

View reviewed changes

tcharding mentioned this pull request Mar 24, 2022

Improve SchnorrSigHashType #903

Merged

tcharding reviewed Mar 24, 2022

View reviewed changes

src/util/psbt/map/input.rs Outdated Show resolved Hide resolved

tcharding reviewed Mar 27, 2022

View reviewed changes

sanket1729 reviewed Mar 27, 2022

View reviewed changes

sanket1729 reviewed Mar 28, 2022

View reviewed changes

Support SIGHASH_RESERVED in SchnorrSigHashType::from_u8

7cdcdaa

dr-orlovsky requested review from sanket1729 and tcharding March 28, 2022 15:01

dr-orlovsky added 2 commits March 28, 2022 17:03

PsbtSigHashType Display and FromStr implementation

5be1cdb

PsbtSighashType unit tests

992857a

sanket1729 approved these changes Mar 28, 2022

View reviewed changes

apoelstra approved these changes Mar 28, 2022

View reviewed changes

apoelstra merged commit b32d403 into rust-bitcoin:master Mar 28, 2022

sanket1729 mentioned this pull request Mar 28, 2022

Witness human-readable serde #899

Closed

	fn psbt_sighash_type_schnorr_nostd() {
	fn psbt_sighash_type_schnorr_nonstd() {

Conversation

dr-orlovsky commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-orlovsky commented Mar 23, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Mar 24, 2022

Uh oh!

tcharding commented Mar 24, 2022

Uh oh!

Uh oh!

dr-orlovsky commented Mar 26, 2022

Uh oh!

dr-orlovsky commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Mar 27, 2022

Uh oh!

dr-orlovsky commented Mar 27, 2022

Uh oh!

dr-orlovsky commented Mar 27, 2022

Uh oh!

sanket1729 commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 27, 2022

Uh oh!

sanket1729 commented Mar 27, 2022

Uh oh!

sanket1729 commented Mar 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Mar 22, 2022 •

edited

Loading

tcharding left a comment •

edited

Loading

dr-orlovsky commented Mar 27, 2022 •

edited

Loading

sanket1729 commented Mar 27, 2022 •

edited

Loading

dr-orlovsky commented Mar 28, 2022 •

edited

Loading

sanket1729 commented Mar 28, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading

dr-orlovsky commented Mar 28, 2022 •

edited

Loading

dr-orlovsky commented Mar 28, 2022 •

edited

Loading