Implements the changes to BIP 174 proposed in https://github.com/bitcoin/bips/pull/849 and https://github.com/bitcoin/bips/pull/784
Implements PSBT_GLOBAL_VERSION, PSBT_GLOBAL_PROPRIETARY, PSBT_IN_PROPRIETARY, PSBT_OUT_PROPRIETARY, and PSBT_GLOBAL_XPUB. The PSBT_GLOBAL_XPUB changes are merged in from #16463.
Also includes the test vectors added to BIP 174 for these fields.
A number of additional changes to keypath and xpub serialization are made to support PSBT_GLOBAL_XPUB.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--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.
463 | @@ -462,8 +464,9 @@ struct PartiallySignedTransaction 464 | break; 465 | } 466 | 467 | - // First byte of key is the type 468 | - unsigned char type = key[0]; 469 | + // Type is compact size uint at beginning of key 470 | + VectorReader skey(s.GetType(), s.GetVersion(), key, 0); 471 | + uint64_t type = ReadCompactSize(skey);
Manually inspected that all the fields with constant types are now being written with compact size, but I would like to be more systematic somehow in the future.
cursory review ACK
571 | @@ -496,6 +572,35 @@ struct PartiallySignedTransaction 572 | } 573 | break; 574 | } 575 | + case PSBT_GLOBAL_VERSION: 576 | + { 577 | + if (version) { 578 | + throw std::ios_base::failure("Duplicate Key, version already provided"); 579 | + } else if (key.size() != 1) { 580 | + throw std::ios_base::failure("Global verion key is more than one byte type");
version*
Fixed
510 | @@ -500,6 +511,21 @@ struct PartiallySignedTransaction 511 | } 512 | break; 513 | } 514 | + case PSBT_GLOBAL_VERSION: 515 | + { 516 | + if (m_version) { 517 | + throw std::ios_base::failure("Duplicate Key, version already provided"); 518 | + } else if (key.size() != 1) { 519 | + throw std::ios_base::failure("Global version key is more than one byte type");
ec06a2e9ff3a59a903b301a8b539f467a8d57077: BIP174 defines it as <32-bit uint>. So we should check key.size() > 4? Not that it matters much, since we trip over any version > 0.
The version is in the value, not the key.
Ah yes. A better error message might be: "Global version key may not contain key data" (if you need to touch it)
This is the same wording that we use elsewhere for this kind of error, so I'm going to leave it as is.
686 | @@ -668,6 +687,36 @@ struct PartiallySignedTransaction 687 | } 688 | break; 689 | } 690 | + case PSBT_GLOBAL_XPUB: 691 | + { 692 | + if (key.size() != BIP32_EXTKEY_WITH_VERSION_SIZE + 1) {
23b45a66f3343fdf5315a8673ed898202fddf182: I assume that + 1 is here because <xpub> is encoded as CompactSize and is always shorter than 253 bytes (i.e. it's always 78 bytes)? Still it seems cleaner to just deserialise it, rather than adding 1 everywhere.
The + 1 is for the type which is always encoded as a single byte.
I wish <keylen> would have been defined without the key type (i.e. keydatalen), but let's not overhaul the BIP :-)
tACK 052d8c7
It would be nice to see testing by someone who actually uses the proprietary fields.
Followup suggestions:
global_xpubs field for (a simple subset of) descriptors wallets.Note to other reviewers: the change to CompactSize in f90d63b does not make this standard backward incompatible, because all existing key types are less than 253. The new propreitary types (PSBT_IN_PROPRIETARY, etc) are safely set to 0xFC (252). See also https://github.com/bitcoin/bips/pull/1115
994 | @@ -995,6 +995,13 @@ static RPCHelpMan decodepsbt() 995 | {RPCResult::Type::ELISION, "", "The layout is the same as the output of decoderawtransaction."}, 996 | }}, 997 | {RPCResult::Type::NUM, "psbt_version", "The PSBT version number. Not to be confused with the unsigned transaction version"}, 998 | + {RPCResult::Type::OBJ, "proprietary", "The global proprietary map",
I think this should be inside an RPCResult::Type::ARR?
Yes, done.
546 | @@ -546,6 +547,9 @@ struct PSBTOutput 547 | struct PartiallySignedTransaction 548 | { 549 | std::optional<CMutableTransaction> tx; 550 | + // We use a vector of CExtPubKey in the event that there happens to be the same KeyOriginInfos for different CExtPubKeys 551 | + // Note that this map swaps the key and values from the serialization 552 | + std::map<KeyOriginInfo, std::set<CExtPubKey>> m_xpubs;
This comment may be out of date? You are using a set-valued map rather than a vector.
Fixed.
re-utACK bf9c50a modulo RPCResult::Type::ARR
Shouldn't "The global proprietary map" for inputs and outputs also be an RPCResult::Type::ARR? And maybe label them "The input proprietary map" and "The output proprietary map" respectively.
Shouldn't "The global proprietary map" for inputs and outputs also be an
RPCResult::Type::ARR? And maybe label them "The input proprietary map" and "The output proprietary map" respectively.
Done
ACK 7f066240654608743eee165149f7036f43791922
Here is a rebase: https://github.com/darosior/bitcoin/tree/bip174-extensions .
I also have a branch implementing the various hash preimage input types, since it is needed for Miniscript signing support. Here it is rebased on this one since i figured it might make sense to have other input types implementations in this PR as well.
re-ACK 198b080f1f5ddc7b698f6f6f3def4511ab2b4b83
34 | @@ -34,7 +35,12 @@ 35 | "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6UwpyN+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLrCwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m80GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/jnF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4OFyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkzgHNEZPhPKrMAAAAAAQMEAQAAAAAAAA==", 36 | "cHNidP8BAKACAAAAAqsJSaCMWvfEm4IS9Bfi8Vqz9cM9zxU4IagTn4d6W3vkAAAAAAD+////qwlJoIxa98SbghL0F+LxWrP1wz3PFTghqBOfh3pbe+QBAAAAAP7///8CYDvqCwAAAAAZdqkUdopAu9dAy+gdmI5x3ipNXHE5ax2IrI4kAAAAAAAAGXapFG9GILVT+glechue4O/p+gOcykWXiKwAAAAAAAEA3wIAAAABJoFxNx7f8oXpN63upLN7eAAMBWbLs61kZBcTykIXG/YAAAAAakcwRAIgcLIkUSPmv0dNYMW1DAQ9TGkaXSQ18Jo0p2YqncJReQoCIAEynKnazygL3zB0DsA5BCJCLIHLRYOUV663b8Eu3ZWzASECZX0RjTNXuOD0ws1G23s59tnDjZpwq8ubLeXcjb/kzjH+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQEgAOH1BQAAAAAXqRQ1RebjO4MsRwUPJNPuuTycA5SLx4cBBBYAFIXRNTfy4mVAWjTbr6nj3aAfuCMIACICAurVlmh8qAYEPtw94RbN8p1eklfBls0FXPaYyNAr8k6ZELSmumcAAACAAAAAgAIAAIAAIgIDlPYr6d8ZlSxVh3aK63aYBhrSxKJciU9H2MFitNchPQUQtKa6ZwAAAIABAACAAgAAgAA=", 37 | "cHNidP8BAFUCAAAAASeaIyOl37UfxF8iD6WLD8E+HjNCeSqF1+Ns1jM7XLw5AAAAAAD/////AaBa6gsAAAAAGXapFP/pwAYQl8w7Y28ssEYPpPxCfStFiKwAAAAAAAEBIJVe6gsAAAAAF6kUY0UgD2jRieGtwN8cTRbqjxTA2+uHIgIDsTQcy6doO2r08SOM1ul+cWfVafrEfx5I1HVBhENVvUZGMEMCIAQktY7/qqaU4VWepck7v9SokGQiQFXN8HC2dxRpRC0HAh9cjrD+plFtYLisszrWTt5g6Hhb+zqpS5m9+GFR25qaAQEEIgAgdx/RitRZZm3Unz1WTj28QvTIR3TjYK2haBao7UiNVoEBBUdSIQOxNBzLp2g7avTxI4zW6X5xZ9Vp+sR/HkjUdUGEQ1W9RiED3lXR4drIBeP4pYwfv5uUwC89uq/hJ/78pJlfJvggg71SriIGA7E0HMunaDtq9PEjjNbpfnFn1Wn6xH8eSNR1QYRDVb1GELSmumcAAACAAAAAgAQAAIAiBgPeVdHh2sgF4/iljB+/m5TALz26r+En/vykmV8m+CCDvRC0prpnAAAAgAAAAIAFAACAAAA=", 38 | - "cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==" 39 | + "cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==", 40 | + "cHNidP8B+wQAAAAAAQB1AgAAAAEmgXE3Ht/yhek3re6ks3t4AAwFZsuzrWRkFxPKQhcb9gAAAAAA/v///wLT3/UFAAAAABl2qRTQxZkDxbrChodg6Q/VIaRmWqdlIIisAOH1BQAAAAAXqRQ1RebjO4MsRwUPJNPuuTycA5SLx4ezLhMAAAEA/aUBAQAAAAABAomjxx6rTSDgNxu7pMxpj6KVyUY6+i45f4UzzLYvlWflAQAAABcWABS+GNFSqbASA52vPafeT1M0nuy5hf////+G+KpDpx3/FEiJOlMKcjfva0YIu7LdLQFx5jrsakiQtAEAAAAXFgAU/j6e8adF6XTZAsQ1WUOryzS9U1P/////AgDC6wsAAAAAGXapFIXP8Ql/2eAIuzSvcJxiGXs4l4pIiKxy/vhOLAAAABepFDOXJboh79Yqx1OpvNBn1semo50FhwJHMEQCICcSviLgJw85T1aDEdx8qaaJcLgCX907JAIp8H+KXzokAiABizjX3NMU5zTJJ2vW+0D2czJbxLqhRMgA0vLwLbJ2XAEhA9LhVnSUG61KmWNyy4fhhW02UmBtmFYv45xenn5BPyEFAkgwRQIhANErhS2F3Nlh0vX0q2YGVN9u7cx5TAwzzlzDCf+1/OWNAiBnM4qODhclwZf7GoivWfUeROQlWyAWfIaEAxwF0fJZKgEhAiO3K+7wll0Qvgd47+zWH8rG95pOoWk5M4BzRGT4TyqzAAAAAAAAAA==", 41 | + "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAD/AAAAaoF/AKqqgABqgABAP2lAQEAAAAAAQKJo8ceq00g4Dcbu6TMaY+ilclGOvouOX+FM8y2L5Vn5QEAAAAXFgAUvhjRUqmwEgOdrz2n3k9TNJ7suYX/////hviqQ6cd/xRIiTpTCnI372tGCLuy3S0BceY67GpIkLQBAAAAFxYAFP4+nvGnRel02QLENVlDq8s0vVNT/////wIAwusLAAAAABl2qRSFz/EJf9ngCLs0r3CcYhl7OJeKSIiscv74TiwAAAAXqRQzlyW6Ie/WKsdTqbzQZ9bHpqOdBYcCRzBEAiAnEr4i4CcPOU9WgxHcfKmmiXC4Al/dOyQCKfB/il86JAIgAYs419zTFOc0ySdr1vtA9nMyW8S6oUTIANLy8C2ydlwBIQPS4VZ0lButSpljcsuH4YVtNlJgbZhWL+OcXp5+QT8hBQJIMEUCIQDRK4UthdzZYdL19KtmBlTfbu3MeUwMM85cwwn/tfzljQIgZzOKjg4XJcGX+xqIr1n1HkTkJVsgFnyGhAMcBdHyWSoBIQIjtyvu8JZdEL4HeO/s1h/KxveaTqFpOTOAc0Rk+E8qswAAAAAF/AKqqgEBqwAABfwCqqoCAawA", 42 | + "cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==",
Actually, i think there is a duplicate between line 38 and 41 (my bad).
Removed the duplicate.
re-utACK e75e52f7a2a82e24b20f59255cf39809342fcf58
re-ACK e75e52f7a2a82e24b20f59255cf39809342fcf58
SerializeToVector, UnserializeFromVector, DeserializeHDKeypaths, and SerializeHDKeypaths
were in sign.h where PSBT was originally implemented. Since all of the PSBT serialization
has moved to its own file, these functions should follow.
CExtPubKey does not store the version bytes for the extended public key.
We store these so that a CExtPubKey can be serialized and deserialized with
the same version bytes.
Code review ACK 81521173ba830ca1dea204e83897482f8970e519
