Avoid producing 72 byte signatures (low-R)

external/libngu

@@ -1 +1 @@
-Subproject commit 74b373c2b7c92e6e903be22da773bad3f0daa09b
+Subproject commit ddfd47644962fe20301466199da90a6f292732af

releases/ChangeLog.md

@@ -1,4 +1,4 @@
-## 4.1.2 - Jun XX, 2021
+## 4.1.2 - July XX, 2021
 
 - Enhancement: Shows QR code with BIP-85 derived entropy value if you press (3) while
   value shown on-screen. Thanks to [@opennoms](https://twitter.com/openoms) for idea.
@@ -8,7 +8,11 @@
     - Seed words, during picking process (before the quiz)
     - Stored seed words: Advanced > Danger Zone > Seed Functions > View Seed Words
     - TXID of just-signed transaction (64 hex digits)
-    - Backup password (12 words) which is only case safe to save as a photo on your phone!
+    - Backup password (12 words) which is the only safe QR to save as a photo on your phone!!!
+- Enhancement: We now grind a nonce so that our signatures are always 71 bytes or shorter.
+  This may save a byte or two in transaction size (and miner fees), and makes our signatures
+  identical to those produced by BitcoinCore, improving anonymity on-chain. Thanks to
+  [@craigraw](https://twitter.com/craigraw) for detecting this.
 
 ## 4.1.1 - April 30, 2021
 

shared/auth.py

@@ -147,7 +147,7 @@ def sign_message_digest(digest, subpath, prompt):
         sv.register(pk)
 
         dis.progress_bar_show(.75)
-        rv = ngu.secp256k1.sign(pk, digest).to_bytes()
+        rv = ngu.secp256k1.sign(pk, digest, 0).to_bytes()
 
     dis.progress_bar_show(1)
 

shared/psbt.py

@@ -1517,21 +1517,30 @@ class psbtObject(psbtProxy):
                 #print(" digest %s" % b2a_hex(digest).decode('ascii'))
 
                 # Do the ACTUAL signature ... finally!!!
-                result = ngu.secp256k1.sign(pk, digest).to_bytes()
+
+                # We need to grind sometimes to get a positive R
+                # value that will encode (after DER) into a shorter string.
+                # - saves on miner's fee (which might be expected/required)
+                # - blends in with Bitcoin Core signatures which do this
+                for retry in range(10):
+                    result = ngu.secp256k1.sign(pk, digest, retry).to_bytes()
+
+                    # convert signature to DER format
+                    #assert len(result) == 65
+                    r = result[1:33]
+                    s = result[33:65]
+                    der_sig = ser_sig_der(r, s, inp.sighash)
+
+                    if len(der_sig) <= 71:
+                        # odds of needing retry: just under 50% I think
+                        break
 
                 # private key no longer required
                 stash.blank_object(pk)
                 stash.blank_object(node)
                 del pk, node, pu, skp
 
-                #print("result %s" % b2a_hex(result).decode('ascii'))
-
-                # convert signature to DER format
-                assert len(result) == 65
-                r = result[1:33]
-                s = result[33:65]
-
-                inp.added_sig = (which_key, ser_sig_der(r, s, inp.sighash))
+                inp.added_sig = (which_key, der_sig)
 
                 success.add(in_idx)
 

shared/usb.py

@@ -644,7 +644,7 @@ class USBHandler:
             pk = sv.node.privkey()
             sv.register(pk)
 
-            signature = ngu.secp256k1.sign(pk, self.session_key).to_bytes()
+            signature = ngu.secp256k1.sign(pk, self.session_key, 0).to_bytes()
 
             assert len(signature) == 65
 

testing/README.md

@@ -21,9 +21,19 @@ None of this code ships on the product itself, but it does get used for testing
 
     --dev --manual -s
 
-- test all QR code relates cases with:
+## Marked Test Cases
+
+- test all QR code related cases with:
+
     py.test -m qrcode
 
+- txn signing where an unfinalized PSBT is created (low-R tests)
+
+    py.test -m unfinalized
+
+- "bitcoind" which means test would be skipped if you don't have bitcoin core
+  running locally (on testnet)
+
 ## PSBT reference files
 
 - examples with `IN_REDEEM_SCRIPT`:

testing/conftest.py

@@ -586,6 +586,13 @@ def set_encoded_secret(sim_exec, sim_execfile, simulator, reset_seed_words):
     # - actually need seed words for all tests
     reset_seed_words()
 
+@pytest.fixture(scope="function")
+def use_mainnet(settings_set):
+    def doit():
+        settings_set('chain', 'BTC')
+    yield doit
+    settings_set('chain', 'XTN')
+
 @pytest.fixture(scope="function")
 def set_seed_words(sim_exec, sim_execfile, simulator, reset_seed_words):
     # load simulator w/ a specific bip32 master key
@@ -1033,10 +1040,15 @@ def end_sign(dev, need_keypress):
             # skip checks; it's text
             return psbt_out
 
+        sigs = []
+
         if not finalize:
-            if in_psbt:
-                from psbt import BasicPSBT
-                assert BasicPSBT().parse(in_psbt) != None
+            from psbt import BasicPSBT
+            tp = BasicPSBT().parse(psbt_out)
+            assert tp is not None
+
+            for i in tp.inputs:
+                sigs.extend(i.part_sigs.values())
         else:
             from pycoin.tx.Tx import Tx
             # parse it
@@ -1045,6 +1057,11 @@ def end_sign(dev, need_keypress):
             t = Tx.from_bin(res)
             assert t.version in [1, 2]
 
+            # TODO: pull out signatures from signed txn, but pycoin not helpful on that
+                    
+        for sig in sigs:
+            assert len(sig) <= 71, "overly long signature observed"
+
         return psbt_out
 
     return doit

testing/pytest.ini

@@ -6,3 +6,4 @@ markers =
     onetime: test cant be combined with any others, likely needs board reset
     veryslow: test takes more than 30 minutes realtime
     qrcode: test uses or tests QR related features
+    unfinalized: test cases produces an unfinalized PSBT

testing/test_msg.py

@@ -222,4 +222,47 @@ def test_sign_msg_microsd_fails(dev, sign_on_microsd, msg, concern, no_file, tra
 
     assert concern in story
 
+@pytest.mark.parametrize('msg,num_iter,expect', [ 
+    ('Test2', 1, 'IHra0jSywF1TjIJ5uf7IDECae438cr4o3VmG6Ri7hYlDL+pUEXyUfwLwpiAfUQVqQFLgs6OaX0KsoydpuwRI71o='),
+    ('Test', 2, 'IDgMx1ljPhLHlKUOwnO/jBIgK+K8n8mvDUDROzTgU8gOaPDMs+eYXJpNXXINUx5WpeV605p5uO6B3TzBVcvs478='),
+    ('Test1', 3, 'IEt/v9K95YVFuRtRtWaabPVwWOFv1FSA/e874I8ABgYMbRyVvHhSwLFz0RZuO87ukxDd4TOsRdofQwMEA90LCgI='),
+])
+def test_low_R_cases(msg, num_iter, expect, dev, set_seed_words, use_mainnet, need_keypress):
+    # Thanks to @craigraw of Sparrow for this test case, copied from:
+    # <https://github.com/sparrowwallet/drongo/blob/master/src/test/java/com/sparrowwallet/drongo/crypto/ECKeyTest.java>
+
+    set_seed_words('absent essay fox snake vast pumpkin height crouch silent bulb excuse razor')
+    use_mainnet()
+    path = "m/44'/0'/0'/0/0"            # first address, P2PKH
+    addr_fmt = AF_CLASSIC
+
+    #addr = dev.send_recv(CCProtocolPacker.show_address(path, addr_fmt), timeout=None)
+    #assert addr == '14JmU9a7SzieZNEtBnsZo688rt3mGrw6hr'
+
+    msg = msg.encode('ascii')
+    dev.send_recv(CCProtocolPacker.sign_message(msg, path, addr_fmt=addr_fmt), timeout=None)
+
+    need_keypress('y')
+
+    done = None
+    while done == None:
+        time.sleep(0.050)
+        done = dev.send_recv(CCProtocolPacker.get_signed_msg(), timeout=None)
+
+    assert len(done) == 2, done
+    got_addr, raw = done
+
+    assert got_addr == '14JmU9a7SzieZNEtBnsZo688rt3mGrw6hr'
+    assert 40 <= len(raw) <= 65
+
+    sig = str(b64encode(raw), 'ascii').replace('\n', '')
+
+    if num_iter != 1:
+        # I have gotten these cases to pass, but I didn't want to keep the code
+        # that grinded for low R in message signing... Ok for txn signing, but
+        # needless delay for message signing.
+        raise pytest.xfail('no code')
+
+    assert sig == expect
+
 # EOF

testing/test_multisig.py

@@ -1169,6 +1169,7 @@ def fake_ms_txn():
 
     return doit
 
+@pytest.mark.unfinalized
 @pytest.mark.parametrize('addr_fmt', [AF_P2SH, AF_P2WSH, AF_P2WSH_P2SH] )
 @pytest.mark.parametrize('num_ins', [ 2, 15 ])
 @pytest.mark.parametrize('incl_xpubs', [ False, True, 'no-import' ])
@@ -1196,6 +1197,7 @@ def test_ms_sign_simple(N, num_ins, dev, addr_fmt, clear_ms, incl_xpubs, import_
     else:
         try_sign(psbt)
 
+@pytest.mark.unfinalized
 @pytest.mark.parametrize('num_ins', [ 15 ])
 @pytest.mark.parametrize('M', [ 2, 4, 1])
 @pytest.mark.parametrize('segwit', [True, False])
@@ -1409,8 +1411,9 @@ def test_make_airgapped(addr_fmt, acct_num, goto_home, cap_story, pick_menu_item
     need_keypress('x')
 
 
-@pytest.mark.parametrize('addr_style', ["legacy", "p2sh-segwit", "bech32"])
+@pytest.mark.unfinalized
 @pytest.mark.bitcoind
+@pytest.mark.parametrize('addr_style', ["legacy", "p2sh-segwit", "bech32"])
 def test_bitcoind_cosigning(dev, bitcoind, import_ms_wallet, clear_ms, explora, try_sign, need_keypress, addr_style):
     # Make a P2SH wallet with local bitcoind as a co-signer (and simulator)
     # - send an receive various

testing/test_sign.py

@@ -124,6 +124,7 @@ def test_psbt_proxy_parsing(fn, sim_execfile, sim_exec):
     rb = BasicPSBT().parse(open(rb, 'rb').read())
     assert oo == rb
 
+@pytest.mark.unfinalized
 def test_speed_test(request, fake_txn, is_mark3, start_sign, end_sign, dev, need_keypress):
     import time
     # measure time to sign a larger txn
@@ -248,6 +249,7 @@ def test_real_signing(fake_txn, try_sign, dev, num_ins, segwit, decode_with_bitc
     if segwit:
         assert all(x['txinwitness'] for x in decoded['vin'])
 
+@pytest.mark.unfinalized        # iff we_finalize=F
 @pytest.mark.parametrize('we_finalize', [ False, True ])
 @pytest.mark.parametrize('num_dests', [ 1, 10, 25 ])
 @pytest.mark.bitcoind
@@ -386,6 +388,7 @@ def test_sign_example(set_master_key, sim_execfile, start_sign, end_sign):
 
     #assert 'require subpaths to be spec' in str(ee)
 
+@pytest.mark.unfinalized
 def test_sign_p2sh_p2wpkh(match_key, start_sign, end_sign, bitcoind):
     # Check we can finalize p2sh_p2wpkh inputs right.
 
@@ -420,6 +423,7 @@ def test_sign_p2sh_p2wpkh(match_key, start_sign, end_sign, bitcoind):
 
     assert network == signed
 
+@pytest.mark.unfinalized
 def test_sign_p2sh_example(set_master_key, sim_execfile, start_sign, end_sign, decode_psbt_with_bitcoind, offer_ms_import, need_keypress, clear_ms):
     # Use the private key given in BIP 174 and do similar signing
     # as the examples.
@@ -687,6 +691,7 @@ def test_sign_multisig_partial_fail(start_sign, end_sign):
 
     assert 'None of the keys involved' in str(ee)
 
+@pytest.mark.unfinalized
 def test_sign_wutxo(start_sign, set_seed_words, end_sign, cap_story, sim_exec, sim_execfile):
 
     # Example from SomberNight: we can sign it, but signature won't be accepted by
@@ -873,8 +878,9 @@ def KEEP_test_random_psbt(try_sign, sim_exec, fname="data/   .psbt"):
     assert 'led to wrong pubkey for input' in msg
 
 
-@pytest.mark.parametrize('num_dests', [ 1, 10, 25 ])
 @pytest.mark.bitcoind
+@pytest.mark.unfinalized
+@pytest.mark.parametrize('num_dests', [ 1, 10, 25 ])
 def test_finalization_vs_bitcoind(match_key, check_against_bitcoind, bitcoind, start_sign, end_sign, num_dests):
     # Compare how we finalize vs bitcoind ... should be exactly the same txn
 
@@ -1114,7 +1120,7 @@ def test_bip143_attack_data_capture(num_utxo, segwit_in, try_sign, fake_txn, set
     time.sleep(.1)
     title, story = cap_story()
     assert 'TXID' in title, story
-    txid = story.strip()
+    txid = story.strip().split()[0]
 
     assert hist_count() in {128, hist_b4+num_utxo+num_inp_utxo}
 
@@ -1170,7 +1176,7 @@ def test_txid_calc(num_ins, fake_txn, try_sign, dev, segwit, decode_with_bitcoin
     title, story = cap_story()
     assert '0' in story
     assert 'TXID' in title, story
-    txid = story.strip()
+    txid = story.strip().split()[0]
 
     if 1:
         # compare to PyCoin
@@ -1189,6 +1195,7 @@ def test_txid_calc(num_ins, fake_txn, try_sign, dev, segwit, decode_with_bitcoin
 
         assert decoded['txid'] == txid
 
+@pytest.mark.unfinalized            # iff partial=1
 @pytest.mark.parametrize('encoding', ['binary', 'hex', 'base64'])
 #@pytest.mark.parametrize('num_outs', [1,2,3,4,5,6,7,8])
 @pytest.mark.parametrize('num_outs', [1,2])
@@ -1212,6 +1219,7 @@ def test_sdcard_signing(encoding, num_outs, del_after, partial, try_sign_microsd
     _, txn, txid = try_sign_microsd(psbt, finalize=not partial,
                                         encoding=encoding, del_after=del_after)
 
+@pytest.mark.unfinalized
 @pytest.mark.parametrize('num_ins', [2,3,8])
 @pytest.mark.parametrize('num_outs', [1,2,8])
 def test_payjoin_signing(num_ins, num_outs, fake_txn, try_sign, start_sign, end_sign, cap_story):