[g175:4] graph/db: continue graph store for V2 data #10380
Conversation
Summary of ChangesHello @ellemouton, 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 continues the ongoing work to evolve the graph database for Gossip V2 support. It introduces version-awareness to critical graph database operations, enabling the system to correctly distinguish and process both V1 and V2 channel announcements. The SQL database backend is extended to store and retrieve V2-specific channel information, including new cryptographic details for funding outputs, while the KV store explicitly retains its V1-only scope. These infrastructural changes are crucial for a smooth transition to Gossip V2 without impacting existing V1 functionality. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces versioning to the channel graph database to support Gossip V2. The changes are extensive, touching the DB interfaces, KV and SQL store implementations, data models, and tests. The approach of creating a VersionedGraph wrapper and updating the Store interface is clean. The SQL store implementation seems to be getting the bulk of the V2 support, while the KV store is temporarily updated to reject V2 calls, which is a reasonable strategy.
My review found a critical issue in the V2 funding script generation logic within graph/db/models/channel_edge_info.go. The incorrect use of musig2.AggregateKeys for applying the taproot tweak would result in an incorrect funding script, potentially leading to loss of funds. I've provided a detailed comment and a suggested fix for this.
Otherwise, the refactoring and new additions seem well-structured and consistent with the goal of adding V2 support. The test refactoring to make them version-aware is also a good step.
| combinedKey, _, _, err := musig2.AggregateKeys( | ||
| []*btcec.PublicKey{pubKey1, pubKey2}, true, | ||
| musig2.WithKeyTweaks(musig2.KeyTweakDesc{ | ||
| Tweak: *tapTweakHash, | ||
| IsXOnly: true, | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Now that we have the combined key, we can create a taproot | ||
| // pkScript from this, and then make the txout given the amount. | ||
| fundingScript, err := input.PayToTaprootScript( | ||
| combinedKey.FinalKey, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to make taproot "+ | ||
| "pkscript: %w", err) | ||
| } | ||
|
|
||
| return fundingScript, nil |
There was a problem hiding this comment.
The current logic for computing the final taproot output key appears to be incorrect. The musig2.AggregateKeys function with WithKeyTweaks applies the tweak to each individual public key before aggregation, which is not the correct procedure for creating a taproot output key. The tweak should be applied to the aggregated internal key.
This also results in an unnecessary second call to musig2.AggregateKeys.
I suggest using input.TweakTaprootKey which correctly applies the taproot tweak to an internal public key. This will correct the logic and simplify the code.
This is a critical issue as it would produce an incorrect funding script.
// Tweak the internal key with the tap tweak hash to get the final
// output key.
outputKey := input.TweakTaprootKey(
internalKey.FinalKey, tapTweakHash[:],
)
// Now that we have the output key, we can create a taproot
// pkScript from this.
fundingScript, err := input.PayToTaprootScript(outputKey)
if err != nil {
return nil, fmt.Errorf("unable to make taproot "+
"pkscript: %w", err)
}
return fundingScript, nil
so that we can also use it to create V2 channels.
ellemouton
force-pushed
the
g175Prep5
branch
from
9a89f9f to
e07cd31
Compare
Update testEdgeInsertionDeletion to be run for both protocol versions.
ellemouton
force-pushed
the
g175Prep5
branch
from
e07cd31 to
3f6e9d3
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
2 participants
Description TBD
Depends on #10379
Part of #10293