util: add psbt combiner role #19907

1. 
   dr-orlovsky commented at 12:16 pm on September 7, 2020: none

   Current BIP 174 spec defines combiner role, which is absent from the current Bitcoin Core implementation. This PR adds support for it.

   The role can be easily detected by the fact that PSBT contains only some signatures, but not all of them.

   This PR adds new PSBTRole type, corresponding naming and next role, analysis for COMBINER as described above, and support for Qt PSBT interface, re-using existing SIGNER option.

2. fanquake added the label PSBT on Sep 7, 2020
3. 
   achow101 commented at 4:06 pm on September 7, 2020: member

   Note that Bitcoin Core does implement the combiner role as combinepsbt. This PR is adding COMBINER to analyzepsbt.

   I’m not sure that combiner is something that should be added to analyzepsbt. Everything that can be parallelized and thus involves combiner can also be done serially without combiner. You can’t know whether the user is doing a parallel process or a serial process.

4. 
   benthecarman commented at 8:56 pm on September 7, 2020: contributor

   Concept NACK

   As @achow101 points out, this is only adding COMBINER to analyzepsbt which doesn’t make sense imo. It’s impossible to know if the user just needs to sign again or combine. Having COMBINE could be confusing to users if they are in the processing of signing, whereas having SIGNER is clear that signing still needs to take place.

   Maybe just an addition to need_sig_text to say that the sigs can come from combining PSBTs would be a better option.

5. 
   dr-orlovsky commented at 11:39 am on September 8, 2020: none

   What about renaming SIGNER role into SIGNER_COMBINER, if they can’t be always distinguished? Because if the combiner role is missed from the code, while mentioned in the docs, it is also confusing (I was confused when I was studying PSBT implementation where it has gone). And, then, we can adjust the message as well.

   Also, it’s not clear to me who to classify the situation where some (but not all) signatures are present? With what you have wrote, this could happen when a user started adding signatures in sequential mode, but has not completed this process, i.e. when (s)he is signer - and also with a parallel process when (s)he is combiner. If this is true, I propose to change the current implementation to match this logic (right now it was that the signer role is detected only when none of the signatures are present, otherwise the user classified as updater, which is incorrect).

6. 
   achow101 commented at 3:18 pm on September 8, 2020: member

   Combiners can also be involved after updating. It is possible to have a PSBT that has been partially updated. In a parallel process, such as PSBT would go to a combiner, then over to signers to be signed. In a serial process, it would go to the next updater. So making a SIGNER_COMBINER role would also be incorrect.
7. 
   dr-orlovsky commented at 5:35 pm on September 8, 2020: none

   Ok, but right now if some PSBT has a partial signatures, it is classified as UPDATER, which is also wrong.
8. 
   sipa commented at 5:42 pm on September 8, 2020: member

   Ok, but right now if some PSBT has a partial signatures, it is classified as UPDATER, which is also wrong.

   That sounds wrong. It should say signer?

9. 
   dr-orlovsky commented at 8:04 pm on September 8, 2020: none

   Correct me if I am wrong, but I understand the logic of PSBT in the following way: if there at least one signature is present, this is SIGNER or COMBINER. If none, this is UPDATER, and this can’t be COMBINER (since when PSBTs are combines, each one of them must have at least one signature).
10. 
    sipa commented at 8:06 pm on September 8, 2020: member

    Combiners can merge more than just signatures. So you could have done the updating done in parallel by multiple participants too, have then merge the results.
11. Add PSBTRole::COMBINER and respective PSBT analysis

    Current BIP 174 spec defines combiner role, which can be easily
    detected by the fact that PSBT contains only some signatures, but
    not all of them.
    
    This PR adds new PSBTRole type, corresponding naming and next role,
    analysis for COMBINER as described above, and support for Qt PSBT
    interface, re-using existing SIGNER option.

    ff68a7da21
12. dr-orlovsky force-pushed on Sep 8, 2020
13. 
    dr-orlovsky commented at 9:19 pm on September 8, 2020: none

    I haev force-pushed updated version w/o Combiner role but other potentially required changes
14. 
    sipa commented at 9:21 pm on September 8, 2020: member

    Can you first demonstrate the bug? The original code looks correct to me.
15. 
    dr-orlovsky commented at 11:03 pm on September 8, 2020: none

    You are right, I was confused and thought it checks presence of all signatures, not pubkeys. But now I see that if all pubkeys are present and nothing other then signatures is missed, then it evaluates to Signer, which is correct. Closing now
16. dr-orlovsky closed this on Sep 8, 2020

    ---

17. DrahtBot locked this on Feb 15, 2022

Contributors
dr-orlovsky achow101 benthecarman sipa

Labels
PSBT