karask / python-bitcoin-utils

Library to interact with the Bitcoin network. Ideal for low-level learning and experimenting.
MIT License
262 stars 99 forks source link

Modified ControlBlock Class #85

Closed vking45 closed 2 months ago

vking45 commented 2 months ago

Based on the TODO file, it was mentioned that the ControlBlock class needed to be modified so that it does not automatically construct the merkle path (expects it as input).

I modified the class so that it's new constructor looks as below -

def __init__(self, pubkey: PublicKey, scripts: None | list[list[Script]], merkle_path: bytes, is_odd=False)

I am now taking all the scripts as input in form of a List of Lists of Scripts, it can be None as well in case that we are only spending out a single script in the transaction. The merkle_path is being taken as bytes in input which was the required task.

I also modified the files under the examples folder so that they are now consistent with new ControlBlock class.

Finally, I made sure that the test_p2tr_txs.py ran without throwing any errors and made necessary modification pertaining to the ControlBlock in that test.

karask commented 2 months ago

Hi @vking45

To spend an alternative taproot path we need to pass all the necessary spending paths so that the merkle root can be reconstructed. In the examples I do that manually (e.g. in spend_p2tr_three_scripts_by_script_path.py I manually construct/use leaf_a and leaf_c to pass as merkle proof (to prove the leaf_b is in the merkle root). The order in which I passed leaf_a and leaf_c matters because it specifies the structure of the tree.

This task would require to pass all the leafs (all_leafs in the example) and have an algorithm determine leaf_a, leaf_c and their order (so that we don't have to construct it manually in the examples).

I hope the above helps

vking45 commented 2 months ago

Thank you for your prompt response on the PR, I have understood your point and will be making the necessary changes ASAP.

vking45 commented 2 months ago

Hey @karask

I have made the changes that you expected. I created a _generate_merkle_path function which calculates the merkle path on its own by taking a list of lists of taproots and the target index (of which you want to calculate a path for) as input.

The modified ControlBlock constructor looks as def __init__(self, pubkey: PublicKey, scripts: None | list[list[Script]], index: int, is_odd=False)

The index of the taproot is simply defined as the number of the leaf node it is starting from left. An Example -

                 TB_ABC
                  /    \
                 /      \
              TB_AB      \
               / \        \
              /   \        \
            TL_A TL_B     TL_C

For the above image indices are as follow : A -> 0 B -> 1 C -> 2

I have made the necessary changes in the example files and test files so that everything is working as it should.

Let me know if there is still something wrong or if you would like to do things differently.

karask commented 2 months ago

Hi @vking45

Yes, that is the idea. The only issue is that now only list of lists is supported. Ideally we want arbitrary level of depth for the tree; ie. recursive e.g.:

             TB_ABCDE
              /      \
            /          \
       TB_AB         TB_CDE
      /    \        /    \
    /        \    /        \
TL_A        TL_B TB_CD     TL_E
                 /  \          
               /      \         
             TL_C     TL_D     

all_scripts = [ [A, B], [ [C, D], E ] ]

You can find some example code to understand and use as base: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki

vking45 commented 2 months ago

@karask

I am sorry for that overlook in logic on my part. I have fixed the _generate_merkle_path function and it is now working for an arbitrary depth. I am using the recursive approach for this and I hope this aligns with the requirement. Let me know if there is something missing.

karask commented 2 months ago

That was quick. Good job.