pool icon indicating copy to clipboard operation
pool copied to clipboard

Upgrade trader accounts to Taproot/MuSig2

Open guggero opened this issue 3 years ago • 6 comments

Depends on https://github.com/lightninglabs/pool/pull/370.

Pull Request Checklist

  • [X] LndServices minimum version has been updated if new lnd apis/fields are used.

guggero avatar Jun 16 '22 18:06 guggero

I addressed all comments and got almost all itests working on the server side. This is now officially not WIP anymore :tada:

guggero avatar Jul 01 '22 19:07 guggero

@positiveblue: review reminder

lightninglabs-deploy avatar Jul 25 '22 21:07 lightninglabs-deploy

I addressed all comments except for the additional unit tests. I agree they should be added, but I'm going to work on them next week.

guggero avatar Jul 29 '22 14:07 guggero

might need to extend unit test frame work

dstadulis avatar Aug 02 '22 16:08 dstadulis

I added a set of unit tests for the new spend paths. And I also ran the fuzzer locally for almost 2 hours and didn't find anything so far.

guggero avatar Aug 03 '22 16:08 guggero

Ready for deployment modulo ~~when the subasta PR is merged.~~ Do not deploy until then

Requirements for merge:

  • deploy the server side
  • gated by lnd 0.15.1 to certify that bitcoind has taproot availability

dstadulis avatar Aug 08 '22 15:08 dstadulis

⚠️ PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

Deliverable outcome:

  • [ ] Modify the implemented MuSig2 spec to incoporate recent changes which will promote ISP

dstadulis avatar Aug 26 '22 22:08 dstadulis

warning PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

Deliverable outcome:

* [ ]  Modify the implemented MuSig2 spec to incoporate recent changes which will promote ISP

Are we sure we want to wait for that? Updating to the latest spec means:

  • Implement new API in btcd, merge.
  • Implement changes to MuSig2 API in lnd, merge, release
  • Use new API in Pool.

I think it would make more sense to instead add a version parameter to the MuSig2 API when we implement this, so we can upgrade later.

guggero avatar Aug 29 '22 07:08 guggero

PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

No, it can proceed as defined. As mentioned, those changes themselves are still in a draft state, with no other implementations currently tracking the change (other than the python ref implementation used to generate the test vectors). Correctness is still retained without the changes. It would matter more in a p2p setting as you need cross implementation compatibility.

Ofc, we could delay things, but then what if we hit another change in serialization somewhere that is "better" but doesn't affect correctness. Would we then do the same and pause to target the newer change? The tradeoff of being one of the first in the ecosystem to utilize these new features is that we sort of need to commit to having the proper versioning to handle future updates.

Roasbeef avatar Aug 29 '22 19:08 Roasbeef

@guggero agreed, iiuc we already have the versioning in the proper places as well. An account version is the most apt, since the serialization changes will change the actual aggregate key, so making the jump a future version would be an actual account version bump.

Roasbeef avatar Aug 29 '22 19:08 Roasbeef

Agree that versioning should be incorporated here to facilitate it being upgraded. Appears there was some misinterpretation about the code etc constrained/delayed by MuSig2 spec, which won't be the plan.

dstadulis avatar Aug 30 '22 16:08 dstadulis