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

Increase group witness test coverage #549

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Increase group witness test coverage #549

merged 8 commits into from
Jan 23, 2024

Conversation

jharveyb
Copy link
Contributor

@jharveyb jharveyb commented Oct 4, 2023 •
edited
Loading

Fixes #511. Tracking the canonical checklist here:

  • Manually generate group witness with a non-empty tapscript root (BIP86, key spend, script spend)
  • Verify custom witnesses in VM
  • Exercise new VM validation branches
  • Coverage for GroupPubKey(), DeriveGroupKey(), and new Has/NeedsGenesisWitnessForGroup helpers
  • Exercise group and group anchor verifiers + sortassets
  • Validate proof with assets with custom witnesses
  • Exercise new branches in proof.Verify() concerning group key validation
  • Validate group key reveal handling in committedProofs()

@jharveyb jharveyb force-pushed the group_witness_testing branch 3 times, most recently from 8880ecc to 6a438e6 Compare October 10, 2023 16:42
@jharveyb jharveyb changed the base branch from main to mint-rpc-fixes October 10, 2023 16:42
@jharveyb
Copy link
Contributor Author

Moved to mint-rpc-fixes as a base to inherit other fixes related to group key handling.

@Roasbeef
Copy link
Member

Would be good to thread through some of the new unit tests into test vectors as well.

@guggero guggero force-pushed the mint-rpc-fixes branch 6 times, most recently from cc203b1 to 975a969 Compare October 10, 2023 21:04
Base automatically changed from mint-rpc-fixes to main October 11, 2023 07:24
@jharveyb jharveyb force-pushed the group_witness_testing branch 2 times, most recently from 8d2d68e to 3deb243 Compare October 17, 2023 21:54
Roasbeef
Roasbeef reviewed Oct 18, 2023
View reviewed changes
asset/asset.go Outdated Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
tapdb/asset_minting_test.go Outdated Show resolved Hide resolved
Roasbeef
Roasbeef reviewed Oct 18, 2023
View reviewed changes
asset/asset.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the group_witness_testing branch from 3deb243 to aa088b1 Compare October 18, 2023 04:21
@jharveyb jharveyb mentioned this pull request Oct 18, 2023
Merged
@jharveyb
Copy link
Contributor Author

Split off fixes and simple test cases that don't make use of a custom group key to #598.

The taproot utils used for these tests should be made more general so they could be used in the eventually-exposed functions for making complex group keys.

@dstadulis dstadulis mentioned this pull request Nov 13, 2023
Closed
@dstadulis dstadulis added this to the v0.4 milestone Nov 13, 2023
@jharveyb jharveyb force-pushed the group_witness_testing branch from aa088b1 to a19ecb7 Compare November 14, 2023 17:30
@jharveyb
Copy link
Contributor Author

jharveyb commented Nov 14, 2023 •
edited
Loading

Rebased, this PR also has some unit test coverage unrelated to group key witnesses that will likely be split off into a separate PR.

The problematic group key witness tests are collectible_group_anchor_BIP86_key and normal_group_anchor_key_spend, still hunting down the cause.

@jharveyb jharveyb force-pushed the group_witness_testing branch from a19ecb7 to a1bd7ce Compare November 14, 2023 21:29
@jharveyb jharveyb force-pushed the group_witness_testing branch from a1bd7ce to 1a603f6 Compare December 4, 2023 23:14
@jharveyb jharveyb mentioned this pull request Dec 4, 2023
Merged
@jharveyb jharveyb force-pushed the group_witness_testing branch from ffa6a09 to 47baba3 Compare December 14, 2023 18:04
@jharveyb
Copy link
Contributor Author

Strangely, this failure only triggers for me when running tests from the IDE, not via Makefile.

@dstadulis
Copy link
Collaborator

@jharveyb What else needs to be added to this before the PR is out of draft phase?

@jharveyb
Copy link
Contributor Author

@jharveyb What else needs to be added to this before the PR is out of draft phase?

Need to fix my tapscript building test code so all these test cases pass; don't think we need new test cases outside of what I added here.

@jharveyb jharveyb force-pushed the group_witness_testing branch 2 times, most recently from d33459f to ca9a73b Compare December 19, 2023 06:38
@jharveyb jharveyb marked this pull request as ready for review December 20, 2023 18:02
@lightninglabs-deploy
Copy link

@Roasbeef: review reminder
@ffranr: review reminder
@GeorgeTsagk: review reminder

ffranr
ffranr requested changes Jan 8, 2024
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.

Looking good so far.

asset/asset.go Outdated Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
asset/asset.go Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the group_witness_testing branch 3 times, most recently from 418e4c1 to 24b0cf9 Compare January 9, 2024 20:24
@jharveyb
Copy link
Contributor Author

jharveyb commented Jan 9, 2024

Rebased and added a constructor that also does request validation, ready for re-review.

@jharveyb jharveyb requested a review from ffranr January 9, 2024 23:09
ffranr
ffranr approved these changes Jan 11, 2024
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, great opportunity for me to review a few concepts, thanks.

asset/mock.go Outdated Show resolved Hide resolved
asset/mock.go Outdated Show resolved Hide resolved
vm/vm_test.go Outdated Show resolved Hide resolved
@dstadulis dstadulis removed the request for review from Roasbeef January 16, 2024 18:19
@jharveyb
multi: explicitly pass asset versions in tests
b0ee68e 
In this commit we update multiple areas where we create an asset using
fields from a base asset, to explicitly pass through the version of the
base asset. This can cause an issue if asset versions are randomized.
@jharveyb
multi: refactor calls for group key derivation
35e7225 
In this commit, we introduce a new type to include the required fields
for a group key derivation. This helps readability and sanity checking,
especially with the complex group key derivation added in later commits.
@jharveyb
asset: support group key creation with tapscripts
d01237d
@jharveyb
asset+test+vm: extract common tapscript test code
a338f0e
@jharveyb
vm: test group anchors with script witnesses
d4f5e17
@jharveyb
asset: test group key tweaks with taptweak added
d5b2f89
@jharveyb
proof: test proof creation with custom group key
a1c3027
@jharveyb
tappsbt+vm: update test vectors
290eff1
@jharveyb jharveyb force-pushed the group_witness_testing branch from 24b0cf9 to 290eff1 Compare January 17, 2024 19:22
@Roasbeef Roasbeef merged commit 58bfe28 into main Jan 23, 2024
14 checks passed
@guggero guggero deleted the group_witness_testing branch January 23, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef left review comments

@ffranr ffranr ffranr approved these changes

@GeorgeTsagk GeorgeTsagk Awaiting requested review from GeorgeTsagk

Assignees

@jharveyb jharveyb

Labels
group_key testing
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Add script-path validation unit tests for group_key witness scenarios
5 participants
@jharveyb @Roasbeef @dstadulis @lightninglabs-deploy @ffranr