-
Notifications
You must be signed in to change notification settings - Fork 138
docs: add docs re current stxo design #1830
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
Summary of ChangesHello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the documentation for the Taproot Assets protocol by introducing a detailed guide to the Spent Transaction Output (STXO) proof system. The newly added Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a comprehensive design document for the STXO proof system. The document is well-structured and detailed. I've provided a few suggestions to improve clarity, fix typos, and correct minor inaccuracies in diagrams and code snippets to make the documentation even better.
+Asset Asset | ||
+TaprootProof InclusionProof | ||
+[]TaprootProof ExclusionProofs | ||
+[]AltLeaf_Asset_ AltLeaves |
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.
|
||
Proof --> TaprootProof : has | ||
TaprootProof --> CommitmentProof : contains | ||
CommitmentProof --> Asset_STXO : STXOProofs map to |
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.
The relationship CommitmentProof --> Asset_STXO : STXOProofs map to
in the Mermaid diagram is a bit misleading. The STXOProofs
field is a map from a serialized key to a commitment.Proof
, not directly to an Asset_STXO
. A more accurate label would clarify that the proofs are used to prove the inclusion or exclusion of the STXO asset.
CommitmentProof --> Asset_STXO : STXOProofs map to | |
CommitmentProof --> Asset_STXO : STXOProofs prove |
|
||
```go | ||
// MakeSpentAsset creates an Alt Leaf with the minimal asset based on the | ||
// PrevId of the witness. |
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.
The critical decision is the `IsTransferRoot` check. An asset is a transfer root | ||
if it has at least one `PrevWitness` with a non-nil `PrevID`. This distinguishes | ||
transfer roots from genesis assets (no previous witnesses) and split leaves | ||
(created from a split commitment, don't reference inputs directly). Only |
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.
Using "don't" is a bit informal for technical documentation. It's generally better to use the full phrase "do not" for clarity and consistency.
(created from a split commitment, don't reference inputs directly). Only | |
(created from a split commitment, do not reference inputs directly). Only |
|
||
```go | ||
// CollectSTXO returns the assets spent by the given output asset in the form | ||
// of a minimal assets that can be used to create an STXO commitment. |
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.
// The firstPrevID must be the PrevID from the first input that is being spent | ||
// by the virtual transaction that contains the burn. | ||
func DeriveBurnKey(firstPrevID PrevID) *btcec.PublicKey { |
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.
The comment The firstPrevID must be the PrevID from the first input that is being spent
is confusing, as this function is called for each input's PrevID
. The parameter name firstPrevID
is also misleading. Consider renaming it to prevID
for clarity.
// The firstPrevID must be the PrevID from the first input that is being spent | |
// by the virtual transaction that contains the burn. | |
func DeriveBurnKey(firstPrevID PrevID) *btcec.PublicKey { | |
// The prevID must be the PrevID from an input that is being spent | |
// by the virtual transaction that contains the burn. | |
func DeriveBurnKey(prevID PrevID) *btcec.PublicKey { |
_ = wire.WriteOutPoint(&b, 0, 0, &firstPrevID.OutPoint) | ||
_, _ = b.Write(firstPrevID.ID[:]) | ||
_, _ = b.Write(firstPrevID.ScriptKey.SchnorrSerialized()) |
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.
To complete the renaming of firstPrevID
to prevID
suggested for the function signature, please update its usage here as well.
_ = wire.WriteOutPoint(&b, 0, 0, &firstPrevID.OutPoint) | |
_, _ = b.Write(firstPrevID.ID[:]) | |
_, _ = b.Write(firstPrevID.ScriptKey.SchnorrSerialized()) | |
_ = wire.WriteOutPoint(&b, 0, 0, &prevID.OutPoint) | |
_, _ = b.Write(prevID.ID[:]) | |
_, _ = b.Write(prevID.ScriptKey.SchnorrSerialized()) |
|
||
F --> G[ComputeTaprootOutputKey<br/>NUMSKey, Data] | ||
|
||
G --> H["TapTweak = SHA256<br/>TapTweak/elements concat NUMSKey concat Data"] |
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.
The formula for TapTweak
in the diagram is a simplification of the actual tagged hash mechanism used in Taproot. A more accurate representation would be TaggedHash('TapTweak', NUMSKey || Data)
, which clarifies that a specific tagged hash function is used.
G --> H["TapTweak = SHA256<br/>TapTweak/elements concat NUMSKey concat Data"] | |
G --> H["TapTweak = TaggedHash('TapTweak', NUMSKey || Data)"] |
### Split Transactions | ||
|
||
Split transactions exhibit nuanced STXO behavior. One input asset is divided | ||
into multiple outputs. The split root contains a `SplitCommitmentRoot`, a merkle |
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.
For consistency with other parts of the document (like "Merkle-Sum Sparse Merkle Tree"), "merkle tree" should be capitalized as "Merkle tree".
into multiple outputs. The split root contains a `SplitCommitmentRoot`, a merkle | |
into multiple outputs. The split root contains a `SplitCommitmentRoot`, a Merkle |
Pull Request Test Coverage Report for Build 18148030519Details
💛 - Coveralls |
No description provided.