Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

universe: RPC fixes #328

Merged
merged 3 commits into from
Jun 16, 2023
Merged

universe: RPC fixes #328

merged 3 commits into from
Jun 16, 2023

Conversation

jharveyb
Copy link
Contributor

Display 33-byte script keys instead of 32 bytes. We should audit the current structs are parsing to make it internally consistent, so that copy & paste works on the CLI.

@jharveyb jharveyb requested review from Roasbeef and guggero May 26, 2023 22:48
Roasbeef
Roasbeef reviewed May 26, 2023
View reviewed changes
rpcserver.go Outdated Show resolved Hide resolved
Roasbeef
Roasbeef requested changes May 26, 2023
View reviewed changes
universe/auto_syncer.go Outdated Show resolved Hide resolved
@jharveyb
Copy link
Contributor Author

From offline discussion: A better approach would be accepting both 33 and 32 byte script keys when possible. In the context of universes, we only store script keys as 32 bytes, but a user may provide the same key in both formats as other CLI commands will show asset script keys in 33 byte form.

Will update this PR to add handling for both script key sizes.

@guggero guggero removed their request for review June 6, 2023 09:51
@jharveyb jharveyb force-pushed the universe_rpc_fixes branch 2 times, most recently from d46b466 to 4a6c32c Compare June 8, 2023 00:10
@jharveyb jharveyb marked this pull request as ready for review June 8, 2023 01:49
@lightninglabs-deploy
Copy link

@jharveyb, remember to re-request review from reviewers when ready

@jharveyb jharveyb requested review from Roasbeef and ffranr June 8, 2023 15:31
@jharveyb jharveyb mentioned this pull request Jun 8, 2023
Closed
Roasbeef
Roasbeef requested changes Jun 8, 2023
View reviewed changes
universe/auto_syncer.go Outdated Show resolved Hide resolved
cmd/tapcli/universe.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
itest/universe_test.go Outdated Show resolved Hide resolved
itest/universe_test.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the universe_rpc_fixes branch 2 times, most recently from 5fb7f89 to 9597bc0 Compare June 9, 2023 22:37
ffranr
ffranr requested changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🍰

rpcserver.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@jharveyb
Copy link
Contributor Author

We have the same problem with group keys, since they are shown as 33-byte on the CLI in the outpoint of assets list, but the universe ID is constructed from the 32-byte version of the key. IIUC, those should be safe to just truncate since they will always have an even parity byte.

@jharveyb
Copy link
Contributor Author

Discussed offline with @guggero to clear up some confusion:

  • Tweaked keys are stored internally in the compressed form, along with the corresponding internal key and any non-empty tweak information, so that can produce signatures by calling out to the LND signer via RPC.
  • We don't display this internal key or key tweak for asset script keys, but we do display some of this information for the asset group key.
  • Tweaked asset script keys, and the asset group key, will always end up with even parity due to schnorr serialization before storage; this does not affect our ability to sign with these keys.

Given that these keys will always have even parity, I think for this PR, we can just check the user-provided key for the even parity byte and then remove it / truncate the key before passing it along further.

@jharveyb jharveyb force-pushed the universe_rpc_fixes branch from 9597bc0 to 61d9108 Compare June 13, 2023 17:50
@jharveyb jharveyb mentioned this pull request Jun 14, 2023
Closed
@jharveyb jharveyb force-pushed the universe_rpc_fixes branch from 61d9108 to 6576c28 Compare June 14, 2023 19:26
jharveyb added 2 commits June 14, 2023 16:10
@jharveyb
rpcserver: accept both forms of script key
c3199f7 
In this commit, we update RPCs that accept script keys as an argument to
accept both 32 and 33 byte keys. Since we display both formats in parts
of the CLI, we also want to accept both formats. We also refactor
outpoint unmarshalling.
@jharveyb
itest: use 33 byte script key in universe test
8c49fcb
@jharveyb jharveyb force-pushed the universe_rpc_fixes branch from 6576c28 to 8c49fcb Compare June 14, 2023 20:10
@jharveyb
Copy link
Contributor Author

Switched to key truncation + tested as working locally with an odd-parity group key.

@jharveyb jharveyb requested review from ffranr and Roasbeef June 14, 2023 20:11
guggero
guggero approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM 🎉

ffranr
ffranr approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍

itest/universe_test.go Show resolved Hide resolved
Roasbeef
Roasbeef approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐲

@Roasbeef Roasbeef added this pull request to the merge queue Jun 16, 2023
Merged via the queue into main with commit 7ab8a57 Jun 16, 2023
@guggero guggero deleted the universe_rpc_fixes branch June 16, 2023 07:23
@guggero guggero mentioned this pull request Jun 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffranr ffranr ffranr approved these changes

@guggero guggero guggero approved these changes

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jharveyb @lightninglabs-deploy @Roasbeef @guggero @ffranr @dstadulis