BIP352: fix loop range issue in get_pubkey_from_input function #1806

1. 
   famouswizard commented at 8:28 am on March 30, 2025: none

   I fixed an issue in the get_pubkey_from_input function where the loop for extracting the public key from the scriptSig wasn’t correctly handling the range of indices. The loop now starts from the correct index, ensuring that the last 33 bytes of the vin.scriptSig are properly checked against the scriptPubKey hash. This change ensures the function works as intended without going out of bounds or missing the correct data.

   Changes:

   • Adjusted the loop range to correctly shift the window over the last 33 bytes of vin.scriptSig.
   • Added a check to ensure that we don’t go out of bounds when slicing the data.

   This should resolve the issue with incorrectly processed scriptSig data.

2. chore: fix loop range issue in get_pubkey_from_input function 2986cd70fd
3. 
   quapka commented at 1:23 pm on March 30, 2025: contributor

   Is it possible to add, previously failing and now passing, test vectors? I suppose they could go to this file https://github.com/bitcoin/bips/blob/master/bip-0352/send_and_receive_test_vectors.json
4. 
   jonatack commented at 5:51 pm on March 30, 2025: member

   Is it possible to add, previously failing and now passing, test vectors?

   Agree, and please also make separate commits for (1) test coverage, if any is missing, (2) the proposed fix with updated test coverage, (3) code refactoring, and (4) code style and grammar changes.

5. jonatack added the label Proposed BIP modification on Mar 30, 2025
6. murchandamus renamed this:
   chore: fix loop range issue in get_pubkey_from_input function
   BIP352: fix loop range issue in get_pubkey_from_input function
   on Apr 1, 2025
7. 
   jonatack commented at 1:57 pm on April 4, 2025: member

   @famouswizard do you plan to update here, based on the feedback?
8. 
   jonatack commented at 7:00 pm on April 14, 2025: member

   Let’s close this PR if no author update by May 1.
9. jonatack added the label PR Author action required on Apr 14, 2025
10. in bip-0352/reference.py:35 in 2986cd70fd

    31@@ -32,54 +32,51 @@ def get_pubkey_from_input(vin: VinInfo) -> ECPubKey:
    32         # skip the first 3 op_codes and grab the 20 byte hash
    33         # from the scriptPubKey
    34         spk_hash = vin.prevout[3:3 + 20]
    35-        for i in range(len(vin.scriptSig), 0, -1):
    36-            if i - 33 >= 0:
    37+        for i in range(len(vin.scriptSig) - 33, 0, -1):
    

    ---

    ---

    

    murchandamus commented at 6:32 pm on April 15, 2025:

    I’m not sure what issue is being fixed here. Before this loop, we have already determined that we are looking at a P2PKH input. The P2PKH input script consists of a signature and the pubkey. The pubkey bytes are the last 33 bytes of the input script. However, P2PKH is malleable and could have additional junk bytes after the pubkey. This loop starts by reading the last 33 bytes and in each iteration goes forward one byte until it finds the pubkey.

    If I understand this code and the change right, the changed code skips reading the last 33 bytes which means that it breaks support for P2PKH. I may misunderstand what’s going on, if that is so, please elaborate what problem was discovered that this fixes.

11. murchandamus changes_requested
12. 
    murchandamus commented at 6:45 pm on April 15, 2025: contributor

    Please add an explanation to your commit message describing what problem you discovered and how your code change mitigates.

Contributors
famouswizard quapka jonatack murchandamus

Labels
Proposed BIP modification PR Author action required