The decodepsbt incorrectly truncates the (32 bit) sighash type field before looking up its human friendly name. It's not dangerous, as such a signature would be invalid, but potentially confusing.
Fix that and add a test.
I plan to use SighashToStr in another pull request to render an error message for invalid sighash type field values, but it seemed worth fixing in a standalone PR.
The PSBT sighash type field is a 32 bit unsigned integer in BIP 174,
signed in PSBTInput, and it is not validated when deserialized.
decodepsbt incorrectly truncates this field before looking up its
name. Fix that and add a test.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36113.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | winterrdog, jeanpablojp, rkrux, achow101 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Approach ACK
Makes sense to have the guard in SighashToStr rather than at the call site.
561 | + self.log.info("Test that decodepsbt rejects invalid trailing bytes in the sighash type field") 562 | + node = self.nodes[0] 563 | + psbt = PSBT.from_base64(node.createpsbt([{"txid": "00" * 32, "vout": 0}], [{"data": "00"}])) 564 | + # The first byte of this sighash type is ALL, but the type itself is not 565 | + psbt.i[0].map[PSBT_IN_SIGHASH_TYPE] = (0x101).to_bytes(4, "little") 566 | + assert_equal(node.decodepsbt(psbt.to_base64())["inputs"][0]["sighash"], "")
Nothing is rejected here, decodepsbt accepts the PSBT and returns an empty string. The guard has two halves and this case only trips > 0xff, so nothing in rpc_psbt.py covers the sighash_type < 0 || half. Dropping it leaves the file green. A value like 0xffffff01 would cover it, since it lands negative in PSBTInput, which keeps the field as an int.
Nothing is rejected here
"rejects" isn't the best word, but I couldn't find a better term.
sighash_type < 0
Not sure if it's worth covering, other than for code coverage.
The real fix for negative values is to use uint32_t everywhere, since that's how the BIP defines it, but that's too much churn.
use
uint32_teverywhere, since that's how the BIP defines it,
nit: Correct! I was just wondering why you chose to go with a signed integer.
🤔 Won't that introduce more confusion since it deviates from what is in the BIP?
A signed integer is consistent with the rest of the codebase, which annoyingly didn't follow the BIP from the start. I looked at how much code churn is involved in fixing that throughout the code, and it's not worth it imo. The code comment hopefully makes it clear.
Concept ACK
tACK 1fca81960abcd5331765842776909d4787929cea
successfully built and tested on this toolchain: FreeBSD 15.0/clang++-19/x86_64. LGTM
ACK 1fca81960abcd5331765842776909d4787929cea
lgtm ACK 1fca81960abcd5331765842776909d4787929cea
Fixes a non-dangerous but potentially confusing case.
342 | +std::string SighashToStr(int32_t sighash_type) 343 | { 344 | - const auto& it = mapSigHashTypes.find(sighash_type); 345 | + // Signatures encode the sighash type in a single byte, but the PSBT field 346 | + // for it is a 32 bit unsigned integer in BIP 174 (signed in PSBTInput) 347 | + if (sighash_type < 0 || sighash_type > 0xff) return "";
Ideally would have preferred to throw here instead of silently ignoring. But that might be a bit overkill I suppose in this case. Shall we update the doc as well mentioning that an incorrect sighash value will be ignored?
It might be worth a followup PR to decide how to handle unrecognized sighash types. They could come from malformed input, but also theoretically from future soft forks. As well as the distinction between absent and unknown.
ACK 1fca81960abcd5331765842776909d4787929cea