walletprocesspsbt can generate unparseable PSBT #14473

1. 
   gwillen commented at 8:31 am on October 13, 2018: contributor

   Starting with the following unsigned transaction:

   0cHNidP8BAHMCAAAAAXg6mTOjO3d9K0RiVOr87oYRYhYB/Bpomi66nXc85ebsAAAAAAD9////Avs+EQAAAAAAF6kU4lwd88Ejsg5sG0iVOiL2/c05MHKHoIYBAAAAAAAXqRSxG32ZtPml0iJQGDVxp82wjHBD+4cs7RUAAAEBIHZOFQAAAAAAF6kUP2yi0rx2ke1Lmdk5gsR+WIvukb+HAQQWABSL8jhHTVLizeYj0cS1ClE18i8jbCIGAnwLKYOB2pXhdmi+i21xj17Np67DQdv7RPiIVKqOrp++ENZSlGkAAACAAQAAgAAAAIAAAQAWABSLPsswJus4oRP6Zq1BmWcxI5hNQiICAgi7iTon5Hy59yU96uKT12AU4/bhxQy7CeEdkfg5sZLoENZSlGkAAACAAQAAgAEAAIAAAQAWABQ6Ram/bbw29IPN92ylIqYKhU1ThyICA3oH7xQc22oDM2dG6g/VZ05JLscr/hEQKUI4fTMKvUjRENZSlGkAAACAAAAAgAAAAIAA
   

   I first call:

   0walletprocesspsbt "cHNidP8BAHMCAAAAAXg6mTOjO3d9K0RiVOr87oYRYhYB/Bpomi66nXc85ebsAAAAAAD9////Avs+EQAAAAAAF6kU4lwd88Ejsg5sG0iVOiL2/c05MHKHoIYBAAAAAAAXqRSxG32ZtPml0iJQGDVxp82wjHBD+4cs7RUAAAEBIHZOFQAAAAAAF6kUP2yi0rx2ke1Lmdk5gsR+WIvukb+HAQQWABSL8jhHTVLizeYj0cS1ClE18i8jbCIGAnwLKYOB2pXhdmi+i21xj17Np67DQdv7RPiIVKqOrp++ENZSlGkAAACAAQAAgAAAAIAAAQAWABSLPsswJus4oRP6Zq1BmWcxI5hNQiICAgi7iTon5Hy59yU96uKT12AU4/bhxQy7CeEdkfg5sZLoENZSlGkAAACAAQAAgAEAAIAAAQAWABQ6Ram/bbw29IPN92ylIqYKhU1ThyICA3oH7xQc22oDM2dG6g/VZ05JLscr/hEQKUI4fTMKvUjRENZSlGkAAACAAAAAgAAAAIAA" true ALL true
   

   Which generates:

   0cHNidP8BAHMCAAAAAXg6mTOjO3d9K0RiVOr87oYRYhYB/Bpomi66nXc85ebsAAAAAAD9////Avs+EQAAAAAAF6kU4lwd88Ejsg5sG0iVOiL2/c05MHKHoIYBAAAAAAAXqRSxG32ZtPml0iJQGDVxp82wjHBD+4cs7RUAAAEBIHZOFQAAAAAAF6kUP2yi0rx2ke1Lmdk5gsR+WIvukb+HAQcXFgAUi/I4R01S4s3mI9HEtQpRNfIvI2wBCGsCRzBEAiBRE2oCVM2cMWAjt9c17fFUkhcDQmc4aQVDGHp1wnvHbwIgGrV1hMs6ZAbOQL1Dqn06rWOFV3LKzVDglS3v2BQswsABIQJ8CymDgdqV4XZovottcY9ezaeuw0Hb+0T4iFSqjq6fvgABABYAFIs+yzAm6zihE/pmrUGZZzEjmE1CIgICCLuJOifkfLn3JT3q4pPXYBTj9uHFDLsJ4R2R+DmxkugQ1lKUaQAAAIABAACAAQAAgAABABYAFDpFqb9tvDb0g833bKUipgqFTVOHIgIDegfvFBzbagMzZ0bqD9VnTkkuxyv+ERApQjh9Mwq9SNEQ1lKUaQAAAIAAAACAAAAAgAA=
   

   Which is “complete: true” and parses fine. But then if I run walletprocesspsbt on the result AGAIN (don’t ask why … or do, but this should be idempotent):

   0walletprocesspsbt "cHNidP8BAHMCAAAAAXg6mTOjO3d9K0RiVOr87oYRYhYB/Bpomi66nXc85ebsAAAAAAD9////Avs+EQAAAAAAF6kU4lwd88Ejsg5sG0iVOiL2/c05MHKHoIYBAAAAAAAXqRSxG32ZtPml0iJQGDVxp82wjHBD+4cs7RUAAAEBIHZOFQAAAAAAF6kUP2yi0rx2ke1Lmdk5gsR+WIvukb+HAQcXFgAUi/I4R01S4s3mI9HEtQpRNfIvI2wBCGsCRzBEAiBRE2oCVM2cMWAjt9c17fFUkhcDQmc4aQVDGHp1wnvHbwIgGrV1hMs6ZAbOQL1Dqn06rWOFV3LKzVDglS3v2BQswsABIQJ8CymDgdqV4XZovottcY9ezaeuw0Hb+0T4iFSqjq6fvgABABYAFIs+yzAm6zihE/pmrUGZZzEjmE1CIgICCLuJOifkfLn3JT3q4pPXYBTj9uHFDLsJ4R2R+DmxkugQ1lKUaQAAAIABAACAAQAAgAABABYAFDpFqb9tvDb0g833bKUipgqFTVOHIgIDegfvFBzbagMzZ0bqD9VnTkkuxyv+ERApQjh9Mwq9SNEQ1lKUaQAAAIAAAACAAAAAgAA=" true ALL true
   

   I get the following:

   0cHNidP8BAHMCAAAAAXg6mTOjO3d9K0RiVOr87oYRYhYB/Bpomi66nXc85ebsAAAAAAD9////Avs+EQAAAAAAF6kU4lwd88Ejsg5sG0iVOiL2/c05MHKHoIYBAAAAAAAXqRSxG32ZtPml0iJQGDVxp82wjHBD+4cs7RUAAAEAigIAAAABLi4h8ViD8SvykywwbO+Tv4GCnfVXwX8TIvfUV85wZgkAAAAAFxYAFNiGEusp8/GHaPxQSib0noXXSyxe/f///wJ2ThUAAAAAABepFD9sotK8dpHtS5nZOYLEfliL7pG/h6CGAQAAAAAAF6kUsRt9mbT5pdIiUBg1cafNsIxwQ/uHF5gVAAEHFxYAFIvyOEdNUuLN5iPRxLUKUTXyLyNsAQhrAkcwRAIgURNqAlTNnDFgI7fXNe3xVJIXA0JnOGkFQxh6dcJ7x28CIBq1dYTLOmQGzkC9Q6p9Oq1jhVdyys1Q4JUt79gULMLAASECfAspg4HaleF2aL6LbXGPXs2nrsNB2/tE+IhUqo6un74AAQAWABSLPsswJus4oRP6Zq1BmWcxI5hNQiICAgi7iTon5Hy59yU96uKT12AU4/bhxQy7CeEdkfg5sZLoENZSlGkAAACAAQAAgAEAAIAAAQAWABQ6Ram/bbw29IPN92ylIqYKhU1ThyICA3oH7xQc22oDM2dG6g/VZ05JLscr/hEQKUI4fTMKvUjRENZSlGkAAACAAAAAgAAAAIAA
   

   Which is still reported as “complete: true”, but when I try to decode or finalize it:

   TX decode failed PSBT is not sane.: unspecified iostream_category error (code -22)

   I would expect walletprocesspsbt [foo] true ALL true to be idempotent, or at least produce output that’s parseable.

   This is all testnet and I have attached the wallet.dat in case it’s useful (it only has two UTXOs and very few keys.).

   I am going to continue trying to debug this, but I figured since I haven’t gotten it yet I should document it. Please let me know if someone else starts looking into it.

2. fanquake added the label RPC/REST/ZMQ on Oct 13, 2018
3. 
   gwillen commented at 9:09 am on October 13, 2018: contributor

   I’m seeing that the first time we sign, we drop the redeem_script field from the input (as well as the bip32derivs field), while adding final_scriptSig and final_scriptwitness. Then if we’re asked to sign again, suddenly we convert the input from a witness_utxo into a non_witness_utxo, which causes the consistency check to fail (since the final_scriptwitness is still present.)

   I faintly suspect that the fact that we’re dropping the redeem_script perhaps causes us to then be unable to figure out that we’re looking at a P2WPKH-in-P2SH, and mistakenly fall back to something else undesirable.

   I’m not sure whether we should be dropping redeem_script, but we probably should not be attempting to sign an input that’s already signed, which is how we mess it up.

4. 
   gwillen commented at 9:26 am on October 13, 2018: contributor

   Aha, found it:

   • Every time we FillPSBT, we grab both the witness and non-witness versions of the utxo representation (the output only, versus the whole transaction), and put both of them into the PSBT structure, counting on some later processing to drop the redundant one.
   • When FillPSBT calls SignPSBTInput, the latter will always drop the non-witness representation if both are present, preferring the witness one.
   • But if the input is already signed, SignPSBTInput will return early, before performing that drop, leaving both representations present (an invalid PSBT.)
   • Then when serializing, PSBTInput::Serialize covers up the mistake by dropping one of the representations – but it always drops the witness version, leaving the non-witness one.
   • The resulting PSBT, with a final_scriptwitness but a non_witness_utxo, is invalid.

   I suspect some refactoring will be involved in the best fix for this, since the blame is distributed in several places (and it would be good to avoid other possible errors like this, which will require understanding what sorts of invariants are meant to be maintained across various functions – which I’m not sure was precisely considered the first time around.) @achow101, as the original author, any thoughts?

   Some possible fixes: (1) to have SignPSBTInput always drop the redundant representation, (2) to have Serialize always drop the non-witness instead of the witness representation for consistency, (3) allow a final_scriptwitness with a non_witness_utxo (this merely includes redundant information, right?)

   I don’t like (2) because it’s arbitrary. I don’t like (3) alone because it’s nonintuitive that re-signing a signed transaction causes it to change, even if the resulting transaction is still valid. I think (1) would be reasonable as a quick fix.

5. 
   sipa commented at 4:06 pm on October 13, 2018: member

   I would think that the easiest fix is just not ever touching an input which has a final_scriptsig or final_scriptwitness. Since such inputs are final, we shouldn’t be bothering to add UTXOs etc.
6. 
   achow101 commented at 5:51 pm on October 13, 2018: member

   I agree with @sipa. If there is a final_scriptsig or final_script_witness for that input, we should skip processing of that input entirely.
7. 
   gwillen commented at 6:40 am on October 14, 2018: contributor

   I agree that seems like an easy fix, but:

   • Can we enforce an invariant that PartiallySignedTransaction only ever has a witness_utxo or a non_witness_utxo? That seems cleaner to me than the current situation where we sometimes have both and count on something to clean one up later, hoping it’s the right one.
   • Failing that, could we at least enforce such an invariant by the time we’re serializing, and have the serializer throw an exception, or assert or something, if it sees both fields, rather than arbitrarily guessing which one to discard (since as we’ve seen it has no reasonable way to guess right, and guessing wrongly just causes downstream problems?)
8. gwillen referenced this in commit 6f44aa81bf on Oct 27, 2018
9. gwillen referenced this in commit 6b9a154f8f on Oct 30, 2018
10. gwillen referenced this in commit 547d203ffa on Oct 30, 2018
11. gwillen referenced this in commit 72f2c6be0c on Oct 30, 2018
12. gwillen referenced this in commit 2f40f52068 on Oct 30, 2018
13. gwillen referenced this in commit 565500508a on Nov 1, 2018
14. gwillen referenced this in commit e13fea975d on Nov 1, 2018
15. sipa closed this on Nov 10, 2018

    ---

16. sipa referenced this in commit b30c62d4b9 on Nov 10, 2018
17. jfhk referenced this in commit 29df5b1b1b on Nov 14, 2018
18. jfhk referenced this in commit 2f79e52426 on Nov 14, 2018
19. sipa referenced this in commit 40279f711a on Nov 22, 2018
20. sipa referenced this in commit 3c390829c0 on Nov 22, 2018
21. sipa referenced this in commit 2091dc9859 on Nov 22, 2018
22. sipa referenced this in commit f672137c23 on Nov 22, 2018
23. JeremyRubin referenced this in commit 94872bd057 on Nov 24, 2018
24. JeremyRubin referenced this in commit 8796f091ae on Nov 24, 2018
25. HashUnlimited referenced this in commit d3709b2ab2 on Nov 26, 2018
26. HashUnlimited referenced this in commit 7c33f680b5 on Nov 26, 2018
27. sipa referenced this in commit 1e928e9a97 on Dec 3, 2018
28. sipa referenced this in commit 9f1d6f2062 on Dec 3, 2018
29. sipa referenced this in commit db445d4e5a on Dec 3, 2018
30. sipa referenced this in commit ff56bb9b44 on Dec 3, 2018
31. MarcoFalke referenced this in commit 5d12143c73 on Dec 5, 2018
32. kallewoof referenced this in commit d91380384d on Oct 4, 2019
33. DrahtBot locked this on Sep 8, 2021
34. knst referenced this in commit 5aaf5ae6fa on Feb 17, 2023
35. knst referenced this in commit ee438b8f30 on Feb 20, 2023
36. PastaPastaPasta referenced this in commit 0615c9f175 on Mar 19, 2023

Contributors
gwillen sipa achow101

Labels
RPC/REST/ZMQ