-
Notifications
You must be signed in to change notification settings - Fork 130
tapdb+universe: implement new Universe tree for 1st party burns #1456
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass, very nice! Nothing major, so should be pretty close.
Nice unit tests too 💯
CI failures seem to be flakes.
Pushed up a commit to consolidate a lot of the duplication between the ignore and burn trees. |
Pushed up a commit to solve the collision issues. We add a new interface for For the ignore tree, we use the hash of the prev ID as a key, which already includes the asset ID, so we're safe there. |
@guggero: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice refactor, code re-use and collision avoidance!
Code looks good to me (apart from a few mostly code style related nits, Aider still seems to struggle with our preferred code style. Or I struggle with its decisions, however you want to see it 😅 )
} | ||
// decodeAndBuildAuthBurnLeaf decodes the raw leaf, reconstructs the key, and | ||
// builds the AuthenticatedBurnLeaf. | ||
func decodeAndBuildAuthBurnLeaf(dbLeaf UniverseLeaf) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here we're using the:
func myFuncName(param, param) (
returnVal, returnVal) {
style, while further down we're using
func myFuncName(param, param
) (returnVal, returnVal) {
Can we please at least get consistency in terms of this decision? I way prefer the first one, for reasons previously stated.
type LeafKey struct { | ||
// LeafKey is an interface that allows us to obtain the universe key for a leaf | ||
// within a universe. | ||
type LeafKey interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice solution! Awesome to be able to leave behind the universe collision issue for new trees 💯
In this commit, we add a new interface, the BurnTree. This interface will be used by a future sub-system to maintain the on-chain asset commitment for a given asset group. The structure of this tree is very similar to the existing issuance tree. The main difference is that we'll only insert burn proofs into this tree.
This is a bug fix to ensure that randProof makes proofs that have unique outpoints. Otherwise, it's possbile that the outpoints collide if used as a map key.
In this commit, we add an option to skip multi-verse insertion. This is useful as for the new burn and ignore trees, we won't put them in a multi-verse of all the other ignore/burn trees. Instead, they'll go in a special tree for a given group, which contains these trees as sub-trees.
We're able to re-use a lot of code for the insertion routines here as the tree structure is identical to the issuance tree.
…rnTree Previously, the `BurnUniverseTree` and `IgnoreUniverseTree` implementations contained significant duplicated logic for common operations such as: - Deriving a universe identifier from an asset specifier. - Calculating the total sum of leaf values in a tree. - Querying specific leaves and generating their MS-SMT inclusion proofs. - Listing all leaves within a specific universe namespace. This commit introduces several generic helper functions within `tapdb/universe.go` to centralize this logic: - `specifierToIdentifier`: Creates a universe identifier for a given specifier and proof type. - `getUniverseTreeSum`: Calculates the sum of a universe tree root. - `queryUniverseLeavesAndProofs`: A generic function to query leaves, decode them, fetch MS-SMT proofs, and build authenticated results. - `listUniverseLeaves`: A generic function to list and decode all leaves in a namespace. The `BurnUniverseTree` and `IgnoreUniverseTree` methods (`Sum`, `QueryBurns`, `ListBurns`, `QueryTuples`, `ListTuples`) have been refactored to utilize these new helpers. This significantly reduces code duplication and improves the maintainability of universe tree interactions. The `IgnoreTree` interface and related tests were updated to reflect changes in the `ListTuples` return type.
This is a prep for fixing an issue with Universe keys that have an identical 2-tuple, but distinct 3-tuple.
We then also add a new concrete implementation of this leaf key. The interface will allow us to pass it in anywhere that the normal LeafKey is expected.
This ensures that we avoid collision issues when an output has multiple assets that have the same script key and same group key. The asset ID will now be the tie breaker here.
4a29c11
to
9668caa
Compare
Pushed up latest iteration. This is dependent on the ignore tree PR. |
I've changed base branch to |
|
||
// BurnTree sum is the response to a query of the total amount burned in a given | ||
// burn tree. | ||
type BurnTreeSum = SumQueryResp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably consider generalising the doc for SumQueryResp
in this PR, because it's specific to ignore right now.
// BurnProof is the burn proof that is stored within the burn leaf. | ||
BurnProof *proof.Proof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this field needs to be a pointer type.
type AuthenticatedBurnLeaf struct { | ||
*BurnLeaf | ||
|
||
// BurnTreeRoot is the root of the ignore tree that the burn leaf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore tree
-> burn tree
// decodeAndBuildAuthIgnoreTuple decodes the raw leaf, reconstructs the key, | ||
// and builds the AuthenticatedIgnoreTuple. | ||
func decodeAndBuildAuthIgnoreTuple(dbLeaf UniverseLeaf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe function name could be: parseDbSignedIgnoreTuple
} | ||
|
||
// buildAuthIgnoreTuple constructs the final AuthenticatedIgnoreTuple. | ||
func buildAuthIgnoreTuple(decodedLeaf *universe.SignedIgnoreTuple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this can become a universe.AuthenticatedIgnoreTuple
constructor called NewAuthIgnoreTuple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'm getting an error in my IDE because decodedLeaf
is a pointer where as field SignedIgnoreTuple
is not.
@@ -5276,7 +5276,7 @@ func (r *rpcServer) AssetLeaves(ctx context.Context, | |||
|
|||
// unmarshalLeafKey un-marshals a leaf key from the RPC form. | |||
func unmarshalLeafKey(key *unirpc.AssetKey) (universe.LeafKey, error) { | |||
var leafKey universe.LeafKey | |||
var leafKey universe.BaseLeafKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can follow the same pattern we used for the GroupKeyReveal
upgrade from V0 to V1. That would mean introducing a new interface, renaming universe.LeafKey
to universe.LeafKeyV0
, and adding a new universe.LeafKeyV1
, rather than layering changes on top of a shared base. Might help keep the original V0 implementation clearly identifiable.
And then in the doc for V0 we can explain why it's been depreciated.
In this commit, we add a new interface, the BurnTree. This interface
will be used by a future sub-system to maintain the on-chain asset
commitment for a given asset group.
The structure of this tree is very similar to the existing issuance
tree. The main difference is that we'll only insert burn proofs into
this tree.
We then implement a concrete implementation of this interface
using the existing universe tree routines.