Inconsistent Descriptor Space Parsing #28943

1. 
   Pttn commented at 4:42 pm on November 26, 2023: contributor

   Is there an existing issue for this?

   • I have searched the existing issues

   Current behaviour

   Sometimes, it is possible to have trailing spaces in a Descriptor and sometimes not. For example:

   • getdescriptorinfo "wsh(multi(2, KzLnMBHHwTs7uUo9UfUxCboj5Urvigx2v7t6iAqhBk2PM4fBMcig , L1S2VZsNYexzyH76CPi1EvrNSoEHsiT12UaR4BTiycmCKqRnSqLY ))" Output:

   0{
   1  "descriptor": "wsh(multi(2,03147a8994e451cb10891688c9db15aea6d62a2691c781678bf7fb3a36e58ccca6,0366b8ab537286e57187eaa8eff803621271500ec496e1e03ef40b9cca6c7a1d1c))#x3mm5nm9",
   2  "checksum": "xxgjqes0",
   3  "isrange": false,
   4  "issolvable": true,
   5  "hasprivatekeys": true
   6}
   

   • getdescriptorinfo "wsh( multi(2, KzLnMBHHwTs7uUo9UfUxCboj5Urvigx2v7t6iAqhBk2PM4fBMcig , L1S2VZsNYexzyH76CPi1EvrNSoEHsiT12UaR4BTiycmCKqRnSqLY ) )" Output:

   0A function is needed within P2WSH (code -5)
   

   • getdescriptorinfo "wsh(multi(2, 03147a8994e451cb10891688c9db15aea6d62a2691c781678bf7fb3a36e58ccca6, 0366b8ab537286e57187eaa8eff803621271500ec496e1e03ef40b9cca6c7a1d1c))" Output:

   0Multi: key ' 03147a8994e451cb10891688c9db15aea6d62a2691c781678bf7fb3a36e58ccca6' is not valid (code -5)
   

   Expected behaviour

   Consistence: either never allow spaces, or always allow them, but not both.

   Steps to reproduce

   Open Bitcoin-Qt Console and run the commands above.

   Relevant log output

   No response

   How did you obtain Bitcoin Core

   Compiled from source

   What version of Bitcoin Core are you using?

   master@5f9fd11680af43b0d0d80a6aa4f315aa04afeac4

   Operating system and version

   Debian 12

   Machine specifications

   No response

2. 
   fanquake commented at 10:10 am on November 27, 2023: member

   cc @achow101 @Sjors
3. fanquake added the label Descriptors on Nov 27, 2023
4. 
   Sjors commented at 1:23 pm on November 28, 2023: member

   I think we should disallow them, though BIP 380 doesn’t say anything about it.
5. 
   achow101 commented at 3:29 pm on November 28, 2023: member

   Agree that we should probably just disallow spaces. @sipa any thoughts?
6. 
   sipa commented at 4:22 pm on November 28, 2023: member

   Agree, let’s outlaw spaces (or alternatively, treat spaces as a higher-level thing that’s permitted by RPC layer, and strip them before parsing).
7. 
   sipa commented at 4:24 pm on November 28, 2023: member

   Is it possible someone has a descriptor wallet which has imported descriptors that include spaces?
8. 
   achow101 commented at 5:59 pm on November 28, 2023: member

   Is it possible someone has a descriptor wallet which has imported descriptors that include spaces?

   I don’t think that will matter since we rebuild the string when writing to the wallet rather than using what was provided. This should eliminate any spaces.

9. 
   furszy commented at 0:29 am on November 29, 2023: member

   I don’t think that will matter since we rebuild the string when writing to the wallet rather than using what was provided. This should eliminate any spaces.

   While not necessarily incorrect, this also mean that we are storing the descriptor with a checksum that differs from the one provided by the user.

   or alternatively, treat spaces as a higher-level thing that’s permitted by RPC layer, and strip them before parsing

   This would require decoupling the checksum verification from the parsing code, moving it up to the RPC level (or an util file), and executing it prior to stripping the spaces. Thoughts on this?

10. 
    sipa commented at 11:03 pm on November 29, 2023: member

    This would require decoupling the checksum verification from the parsing code, moving it up to the RPC level (or an util file), and executing it prior to stripping the spaces. Thoughts on this?

    If we’re not worried about invalidating existing checksummed descriptors that have spaces, we don’t need to do that. The checksum would just implicitly be defined over the space-stripped descriptor.

Contributors
Pttn fanquake Sjors achow101 sipa furszy

Labels
Descriptors