Multisig wallet features, BCUR, native tpub support, and more #41

mflaxman · 2021-04-10T01:24:40Z

This is a big branch that I just keep adding to for months and it's getting out of hand. It might make sense to merge it in now just as a checkpoint (would squash down of course).

mflaxman · 2021-04-13T15:05:00Z

@jimmysong any thoughts on this? I'd like to merge it.

jimmysong

Generally pretty good, mostly nits, except for the bcur stuff. That feels like it should be an object. I don't know much about it, but would be interested in your thoughts.

jimmysong · 2021-04-13T15:52:08Z

buidl/bcur.py

+    cbor = bc32decode(data)
+    if checksum is not None:
+        h = bc32decode(checksum)
+        assert h == hashlib.sha256(cbor).digest()


maybe raise a helpful error about the checksum being wrong?

Good call (here and elsewhere when I was returning error messages)

jimmysong · 2021-04-13T15:54:03Z

buidl/bcur.py

+        y_int = int(xofy_parts[1])
+
+        if x_int > y_int:
+            return None, None, None, None, "x must be >= y (in xOFy)"


Why are you returning such a large number of things here? I would rather have it raise an exception or return an object if it makes sense. Same for other return statements here

jimmysong · 2021-04-13T15:56:06Z

buidl/bcur.py

+import hashlib
+
+
+def bcur_encode(data):


This whole file feels like it should be an object. The number of elements being passed back and forth seem messy to me.

You're right, it's messy.

I was trying to avoid extra classes, but it's ugly. Will rework.

jimmysong · 2021-04-13T15:59:25Z

buidl/helper.py

+    try:
+        int(int_as_string)
+        return True
+    except Exception:


except ValueError, Exception is too broad

jimmysong · 2021-04-13T16:00:06Z

buidl/psbt.py

+
+        self.validate()
+
+        TX_FEE_SATS = self.tx_obj.fee()


This code block is so complicated, and my thinking is that by marking it as all caps I'll know that it was treated as a constant (not edited). Perhaps that's a bad convention?

I usually reserve all-caps for global constants. What we have here is something that doesn't change within the function, which to me should be a normal variable. There may come some change in the future where the fee may need to be modified based on what condition from the taproot MAST tree gets executed, so I would change this to be a variable.

jimmysong · 2021-04-13T16:02:47Z

buidl/psbt.py

+                ),
+            }
+
+            if psbt_out.named_pubs:


I can see a case where you're naming multiple outputs, one which goes to one of the pubkeys of the quorum (maybe paying out a bet). I'm not sure this is a safe assumption.

Can you describe this a little more?

I want developers to be able to rely on battle-tested logic for building apps on top of. I'm trying to keep this simple, but it's a little unwieldy.

I'd like to also add support for batching (multiple outputs), but am concerned the code would get very hard to read.

You're assuming here that if the output is a bunch of named public keys (that is, known to the signers) that it's a change output. This may not be the case. If, for example you have a 2-of-2 output being spent where some amount of it goes back to the 2-of-2 but the rest goes to one of the parties (say in a bet that pays out multiple times depending on the conditions), then the spending output will be caught as a change output according to this logic. Does that make sense?

Hmm, I think I might be misunderstanding what you're saying :(

In order to be counted as change, the current code requires that the output shares the same xfps and m-of-n as every input*:

buidl-python/buidl/psbt.py

Lines 635 to 656 in 55e09f5

if psbt_out.named_pubs:

# Confirm below that this is correct (throw error otherwise)

output_desc["is_change"] = True

root_xfp_hexes = [] # for calculating msig fingerprint

for _, details in psbt_out.named_pubs.items():

root_xfp_hexes.append(details.root_fingerprint.hex())

quorum_m, quorum_n = psbt_out.witness_script.get_quorum()

if quorum_n != len(root_xfp_hexes):

raise SuspiciousTransaction(

f"Transaction claims {quorum_m}-of-{quorum_n} but has {len(root_xfp_hexes)} root fingerprints, which != {quorum_n}"

)

output_msig_id = calc_multisig_id(

quorum_m=quorum_m, root_xfp_hexes=root_xfp_hexes

)

if output_msig_id != inputs_desc[0]["msig_id"]:

# ALL inputs have the same msig_digest

raise SuspiciousTransaction(

f"Output #{cnt} is claiming to be change but has different multisig wallet(s)! Do a sweep transaction (1-output) if you want this wallet to cosign."

)

Shifting from an input controlled by the same group to a new output controlled by that same group does indeed sound like change to me (and the behavior that an end-user manually verifying would expect).

What am I missing? I don't want to introduce a vulnerability here!

* I am a little nervous relying on the PSBT class, but the logic in validate() seems solid as far as confirming that the xpubs in the PSBT correspond to the ones being used in the actual transactions. However, the xfp hex is the only part that can't be verified (it would be trivial to swap this out). I think in my calc_multisig_id method I should be using the xpubs (which are validated by the PSBT class) and not the xfps (which can only be validated by each Signer who holds the corresponding xpriv)!

jimmysong · 2021-04-13T16:03:37Z

buidl/script.py

@@ -428,3 +428,22 @@ def p2sh_address(self, testnet=False):
        redeem_script = self.script_pubkey().redeem_script()
        # return the p2sh address of the RedeemScript (remember testnet)
        return redeem_script.address(testnet)
+
+    def is_p2wsh(self):


shouldn't this be is_p2wsh_multisig?

Good catch.

jimmysong · 2021-04-13T16:04:06Z

buidl/script.py

+        Return the m-of-n of this multisig, as in 2-of-3 or 3-of-5
+        """
+
+        assert self.is_p2wsh(), f"Not a multisig witness script: {self}"


I would prefer a specific Exception not a bald assert.

I changed it to this locally:

if not self.is_p2wsh_multisig(): raise RuntimeError(f"Not a multisig witness script: {self}")

Is that what you had in mind? I could define an Exception class just for catching this error, but I didn't because I thought it too much clutter.

Maybe ValueError or SyntaxError? Those are built in and seem more appropriate here.

…code review

mflaxman · 2021-04-14T00:07:40Z

@jimmysong thanks for the code review!

I reworked bcur.py into two new objects BCURSingle and BCURMulti (single and multi BCUR's have some unique properties, my original attempt to shoehorn them together was too convoluted).

Look good?

To me, the one big outstanding issue is any potential security vulnerabilities in describe_basic_multisig_tx(). In retrospect, it would've been better to make this a separate PR. But, it's close and I'd really like to have that in the library.

jimmysong

I like the refactor of BCUR much better. I would like to see the exceptions renamed and the one logic issue with the change output addressed, but otherwise looks good.

jimmysong · 2021-04-14T00:15:46Z

buidl/bcur.py

+# BCUR Exceptions
+
+
+class INCONSISTENT_BCUR_STRING(RuntimeError):


Not a huge fan of all-caps exceptions. Generally, I've seen camel-cased exceptions ending with "Error" or "Exception" so you would have InconsistentStringError (or ValueError?), InvalidChecksumError, InvalidEncodingError (SyntaxError?) and InvalidStringError (or SyntaxError?). You can use some built in ones if they make sense, but I would like to see the classes follow naming conventions of the rest of Python.

Oof! clearly I haven't been writing a lot of python lately. Good catch.

jimmysong · 2021-04-14T00:42:12Z

LGTM, merge when you want.

Michael Flaxman added 16 commits February 7, 2021 16:13

STASHING changes for multisig wallet helper

e3db9c3

pass tests

1d1a5ae

stashing changes for bcur qr codes

fb959ac

blackify

c5213bd

flake8

bdec85c

flake8 again

c2c9a5f

add testnet flag and reorganize

5a8fc53

test fix

feeb967

blackify

457b433

xpub fix, add bcur gif logic, some helpers

461d5ba

blackify

711d335

lint

e97ed35

alphabetize

b2fab65

make base64 explicit and also add more tests

fe2c9e7

Merge branch 'main' into multisig-wallet

0d04c74

cleanup

e594a6c

jimmysong requested changes Apr 13, 2021

View reviewed changes

Break out BCUR module into nice objects, make changes from @jimmysong …

a3092b4

…code review

jimmysong reviewed Apr 14, 2021

View reviewed changes

switch to pythonic errors

bae229f

jimmysong approved these changes Apr 14, 2021

View reviewed changes

Michael Flaxman added 6 commits April 13, 2021 20:05

minor cleanup

55e09f5

clean up naming

773e1a9

missing error name change

0440c72

stashing changes

2d46444

more cleanup

2542fc9

mb ready to merge

9448686

mflaxman merged commit 5e6f848 into main Apr 16, 2021

mflaxman mentioned this pull request Apr 17, 2021

Better way to handle multisig change validation? #42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multisig wallet features, BCUR, native tpub support, and more #41

Multisig wallet features, BCUR, native tpub support, and more #41

mflaxman commented Apr 10, 2021

mflaxman commented Apr 13, 2021

jimmysong left a comment

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021

jimmysong Apr 13, 2021

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021

jimmysong Apr 13, 2021

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021

jimmysong Apr 14, 2021

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021 •

edited

Loading

jimmysong Apr 14, 2021

mflaxman Apr 14, 2021

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021

jimmysong Apr 13, 2021

mflaxman Apr 13, 2021

jimmysong Apr 14, 2021

mflaxman commented Apr 14, 2021

jimmysong left a comment

jimmysong Apr 14, 2021

mflaxman Apr 14, 2021

jimmysong commented Apr 14, 2021

	if psbt_out.named_pubs:
	# Confirm below that this is correct (throw error otherwise)
	output_desc["is_change"] = True

	root_xfp_hexes = [] # for calculating msig fingerprint
	for _, details in psbt_out.named_pubs.items():
	root_xfp_hexes.append(details.root_fingerprint.hex())

	quorum_m, quorum_n = psbt_out.witness_script.get_quorum()
	if quorum_n != len(root_xfp_hexes):
	raise SuspiciousTransaction(
	f"Transaction claims {quorum_m}-of-{quorum_n} but has {len(root_xfp_hexes)} root fingerprints, which != {quorum_n}"
	)

	output_msig_id = calc_multisig_id(
	quorum_m=quorum_m, root_xfp_hexes=root_xfp_hexes
	)
	if output_msig_id != inputs_desc[0]["msig_id"]:
	# ALL inputs have the same msig_digest
	raise SuspiciousTransaction(
	f"Output #{cnt} is claiming to be change but has different multisig wallet(s)! Do a sweep transaction (1-output) if you want this wallet to cosign."
	)

		# BCUR Exceptions


		class INCONSISTENT_BCUR_STRING(RuntimeError):

Multisig wallet features, BCUR, native tpub support, and more #41

Multisig wallet features, BCUR, native tpub support, and more #41

Conversation

mflaxman commented Apr 10, 2021

mflaxman commented Apr 13, 2021

jimmysong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mflaxman Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mflaxman commented Apr 14, 2021

jimmysong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmysong commented Apr 14, 2021

mflaxman Apr 13, 2021 •

edited

Loading