Taproot wallet support #6035

instagibbs · 2023-02-24T18:57:44Z

This switches CLN to issuing p2tr addresses for deposits optionally, and also by default sending change to p2tr outputs.

Open questions:

should we deprecate wrapped segwit first?
I'm not happy with how many places in the code I have to check things like chainparams->is_elements(12 times looks like), bet it would be good to have some simpler way to not forget what version we're doing?

instagibbs · 2023-05-16T18:49:44Z

one more libwally update to go....

wallet/walletrpc.c

instagibbs · 2023-05-23T13:58:45Z

wallet/reservation.c

@@ -395,7 +404,7 @@ static struct command_result *finish_psbt(struct command *cmd,
 		excess = AMOUNT_SAT(0);
 		/* Add additional weight of output */
 		weight += bitcoin_tx_output_weight(
-				BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN);
+				chainparams->is_elements ? BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN : BITCOIN_SCRIPTPUBKEY_P2TR_LEN);


since script is known b32script we could just tal_count it, but there are other cases where we don't have a script on hand already. would rather we sync up behavior

rustyrussell

One minor weird delete...

It's unfortunate that we can't simply reuse "bech32" as the JSON field name for these addresses: I guess some software won't be up-to-date.

However, I think it's naive to use "p2tr" as well, since we're hoping that THIS TIME FOR SURE we have a universal address format: next change it would be great not to have everyone have to parse a new field. So, do we optimistically go for "bech32m"?

I may remove p2sh as part of the deprecations PR, let's not get bogged down behind that!

bitcoin/script.h

rustyrussell · 2023-05-25T04:30:36Z

common/psbt_keypath.c

+												 ext->pub_key + 1, sizeof(ext->pub_key) - 1,
+												 NULL, 0,
+												 fingerprint, sizeof(fingerprint),
+												 path, 1) != WALLY_OK)


Guessing your local tabs are 4 spaces? Because this is very indented!

ok im in libwally mode; what's the expected indent level :)

plugins/spender/multifundchannel.c

instagibbs · 2023-05-25T10:49:21Z

However, I think it's naive to use "p2tr" as well, since we're hoping that THIS TIME FOR SURE we have a universal address format: next change it would be great not to have everyone have to parse a new field. So, do we optimistically go for "bech32m"?

If someday there's witness output v2 that also uses bech32m, we'll have to un-camp it? p2tr just seems more explicit to me.

instagibbs · 2023-05-25T11:04:35Z

last question I'd have: is it better to not switch to taproot change outputs, just to limit blast radius if something were to go wrong, at least for a release?

pushed a couple fixes to tests as well

edit: can't replicate tests/test_connection.py::test_feerate_stress failures locally

rustyrussell · 2023-05-30T02:46:08Z

last question I'd have: is it better to not switch to taproot change outputs, just to limit blast radius if something were to go wrong, at least for a release?

Unfortunately, I don't think we'd find any bugs without this, and it adds another step change.

However, I do note that you need to add two Changelog-Added lines, and edit the newaddr man page (which doesn't mention p2tr as an option for addresstype:

Changelog-Added: JSON-RPC: newaddr: p2tr option to create taproot addresses.
Changelog-Changed: Wallet: we now use taproot change addresses.

pushed a couple fixes to tests as well

edit: can't replicate tests/test_connection.py::test_feerate_stress failures locally

That's a known flake, ignore...

rustyrussell · 2023-05-30T02:47:37Z

Also, please change all v23.05 to v23.08!

rustyrussell

See comments...

instagibbs · 2023-05-30T13:45:14Z

Changelog-Added: JSON-RPC: newaddr: p2tr option to create taproot addresses.
Changelog-Changed: Wallet: we now use taproot change addresses.

Added

Also, please change all v23.05 to v23.08!

done!

also added description for arg in doc/lightning-newaddr.7.md

instagibbs · 2023-05-30T16:00:42Z

now that dual funding is back in CI matrix.... time to fix tests :)

instagibbs · 2023-05-31T20:46:40Z

I'm going to look into storing the feature bits shared by the peers to remove this race in tests that I've run into. We'd like to know that we've previously seen a peer share anysegwit, and assume this when proposing a close.

instagibbs · 2023-06-15T20:36:30Z

rebased on the now-ACK'd #6308

removed the random gating in tests, things should be passing now...

instagibbs · 2023-06-15T20:47:52Z

not sure what this error means https://pipelines.actions.githubusercontent.com/serviceHosts/e5392bd7-7462-4d15-b5db-89c31513dbe4/_apis/pipelines/1/runs/16019/signedlogcontent/2?urlExpires=2023-06-15T20%3A48%3A10.7086093Z&urlSigningMethod=HMACV1&urlSignature=nIBcuC2cCYA9AqGaMYb0165s4SZR5Rgmu45hVnaRRSQ%3D

rustyrussell · 2023-07-10T03:43:35Z

Rebased on master, and folded the fixups into a single commit. Also did some indent fixups.

Ack: 3d97754

Changelog-Added: JSON-RPC: newaddr: p2tr option to create taproot addresses. Changelog-Changed: Wallet: we now use taproot change addresses.

When modifying the schema, regenerating the file can be challenging. This change sets the autogenerate file as the default target, m aking the process more convenient and simplifying our workflow. Fixes ElementsProject#6365 Report-by: Dusty Daemon Changelog-None Signed-off-by: Vincenzo Palazzo <[email protected]>

rustyrussell · 2023-07-10T20:00:39Z

Trivial rebase for generated file clash.

Ack 2c1d579

niftynei · 2023-07-11T02:15:50Z

Ayyyy!!!

…

On Mon, Jul 10, 2023 at 21:11 Rusty Russell ***@***.***> wrote: Merged #6035 <#6035> into master. — Reply to this email directly, view it on GitHub <#6035 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIMAKN2GKIZWOMAPDOVWPDXPSY47ANCNFSM6AAAAAAVHHMQ3Q> . You are receiving this because you commented.Message ID: ***@***.***>

harding · 2023-07-16T19:30:49Z

Regarding field names, @sipa previously had this advice for me:

<sipa> harding: i think output types should be distinguished based on sender's abilities to
    pay them, as you may need to construct different addresses for older/new sender software
<sipa> and in that regard, i think legacy/p2sh/bech32, and unfortunately, bech32m are
   probably the right choices
[...]
<harding> sipa: you're saying that (in December 2021) if I run `getnewaddress bech32m` I
    should get a P2TR address, and (years from now) if I run `getnewaddress bech32m` I should
    get a P2FOO address because all I care about is whether or not the person who will be spending
    to me can support bech32m?
<sipa> yes

Source: https://www.erisian.com.au/bitcoin-core-dev/log-2021-06-28.html#l-89

instagibbs force-pushed the taproot_wallet_support branch from e08a21b to 7023e45 Compare February 24, 2023 19:26

instagibbs force-pushed the taproot_wallet_support branch from ac1a6ba to e65af25 Compare April 19, 2023 16:49

instagibbs force-pushed the taproot_wallet_support branch from 9a2f1f7 to 58c55f1 Compare May 22, 2023 15:37

instagibbs changed the title ~~WIP Taproot wallet support~~ Taproot wallet support May 22, 2023

instagibbs marked this pull request as ready for review May 22, 2023 15:59

instagibbs requested a review from cdecker as a code owner May 22, 2023 15:59

instagibbs force-pushed the taproot_wallet_support branch from 58c55f1 to 92c7176 Compare May 22, 2023 16:08

instagibbs commented May 22, 2023

View reviewed changes

wallet/walletrpc.c Outdated Show resolved Hide resolved

instagibbs commented May 23, 2023

View reviewed changes

instagibbs force-pushed the taproot_wallet_support branch 3 times, most recently from 83e529b to 4ced65f Compare May 23, 2023 19:44

rustyrussell reviewed May 25, 2023

View reviewed changes

instagibbs force-pushed the taproot_wallet_support branch from 4ced65f to d63f23f Compare May 25, 2023 11:03

instagibbs force-pushed the taproot_wallet_support branch from d63f23f to a08dce3 Compare May 26, 2023 16:27

rustyrussell added this to the v23.08 milestone May 30, 2023

rustyrussell requested changes May 30, 2023

View reviewed changes

instagibbs force-pushed the taproot_wallet_support branch from a08dce3 to 366e9b5 Compare May 30, 2023 13:44

instagibbs force-pushed the taproot_wallet_support branch 4 times, most recently from 4bfd9be to e39f019 Compare May 30, 2023 14:55

instagibbs force-pushed the taproot_wallet_support branch from e304a4e to 50b913b Compare May 31, 2023 19:56

instagibbs force-pushed the taproot_wallet_support branch 2 times, most recently from 746c41c to cc3820e Compare May 31, 2023 20:32

instagibbs mentioned this pull request Jun 5, 2023

Persist feature bits across restarts #6308

Merged

instagibbs force-pushed the taproot_wallet_support branch from cc3820e to f799269 Compare June 15, 2023 20:36

instagibbs force-pushed the taproot_wallet_support branch 2 times, most recently from a66e29c to 4a99000 Compare June 26, 2023 15:06

ddustin mentioned this pull request Jun 28, 2023

make default target not include the target to generate code from the schema #6365

Closed

instagibbs marked this pull request as draft July 2, 2023 00:19

instagibbs marked this pull request as ready for review July 2, 2023 00:20

rustyrussell approved these changes Jul 10, 2023

View reviewed changes

rustyrussell force-pushed the taproot_wallet_support branch from 79750d6 to 3d97754 Compare July 10, 2023 03:42

instagibbs and others added 8 commits July 11, 2023 01:53

psbt_input_add_pubkey: support taproot

6da8c72

p2tr script utilities

cc64ac8

encode_scriptpubkey_to_addr: support p2tr

95ba82c

is_segwit_output: properly detect all known segwit script types

bad004d

txfilter_add_derkey: track p2tr scripts

fe7d96f

cleanup comments, variables with respect to bech32(m)

b4c1fc8

Support p2tr deposit addresses

d7b961d

Changelog-Added: JSON-RPC: newaddr: p2tr option to create taproot addresses. Changelog-Changed: Wallet: we now use taproot change addresses.

rustyrussell force-pushed the taproot_wallet_support branch from 3d97754 to 2c1d579 Compare July 10, 2023 20:00

rustyrussell merged commit da91e70 into ElementsProject:master Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taproot wallet support #6035

Taproot wallet support #6035

instagibbs commented Feb 24, 2023 •

edited

Loading

instagibbs commented May 16, 2023

instagibbs May 23, 2023

rustyrussell left a comment

rustyrussell May 25, 2023

instagibbs May 26, 2023

instagibbs commented May 25, 2023

instagibbs commented May 25, 2023 •

edited

Loading

rustyrussell commented May 30, 2023

rustyrussell commented May 30, 2023

rustyrussell left a comment

instagibbs commented May 30, 2023 •

edited

Loading

instagibbs commented May 30, 2023

instagibbs commented May 31, 2023

instagibbs commented Jun 15, 2023

instagibbs commented Jun 15, 2023

rustyrussell commented Jul 10, 2023 •

edited

Loading

rustyrussell commented Jul 10, 2023

niftynei commented Jul 11, 2023 via email

harding commented Jul 16, 2023

Taproot wallet support #6035

Taproot wallet support #6035

Conversation

instagibbs commented Feb 24, 2023 • edited Loading

instagibbs commented May 16, 2023

instagibbs May 23, 2023

Choose a reason for hiding this comment

rustyrussell left a comment

Choose a reason for hiding this comment

rustyrussell May 25, 2023

Choose a reason for hiding this comment

instagibbs May 26, 2023

Choose a reason for hiding this comment

instagibbs commented May 25, 2023

instagibbs commented May 25, 2023 • edited Loading

rustyrussell commented May 30, 2023

rustyrussell commented May 30, 2023

rustyrussell left a comment

Choose a reason for hiding this comment

instagibbs commented May 30, 2023 • edited Loading

instagibbs commented May 30, 2023

instagibbs commented May 31, 2023

instagibbs commented Jun 15, 2023

instagibbs commented Jun 15, 2023

rustyrussell commented Jul 10, 2023 • edited Loading

rustyrussell commented Jul 10, 2023

niftynei commented Jul 11, 2023 via email

harding commented Jul 16, 2023

instagibbs commented Feb 24, 2023 •

edited

Loading

instagibbs commented May 25, 2023 •

edited

Loading

instagibbs commented May 30, 2023 •

edited

Loading

rustyrussell commented Jul 10, 2023 •

edited

Loading