OOR client 1/4: package primitives + validation baseline - #77
Conversation
Summary of ChangesHello @bhandras, 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 lays the groundwork for Off-chain Out-of-band Relay (OOR) transactions by implementing core building blocks. It focuses on creating and validating the structure of these specialized transactions using Partially Signed Bitcoin Transactions (PSBTs), ensuring they adhere to specific ordering rules and contain necessary metadata. The changes also involve refactoring existing transaction-related code to improve modularity and prepare for future extensions of the OOR protocol. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 primitives, builders, and validation for Off-chain to On-chain Refresh (OOR) transactions. It also refactors arktx and checkpoint primitives and their PSBT encodings, and extends test coverage. The changes are well-structured, with clear separation of concerns between transaction building, canonicalization, and validation. The introduction of package-level structs for SubmitPackage and FinalizePackage with JSON serialization is a good step towards durable and transportable transaction bundles.
I've found a critical issue in the tap tree decoding logic that would prevent scripts from being correctly decoded, and a couple of medium-severity issues related to efficiency and robustness. My detailed comments are below.
| if t, ok := parsedTypes[typeTapLeafScript]; ok { | ||
| if t == nil { | ||
| leaf.Script = script | ||
| } | ||
| } |
There was a problem hiding this comment.
There appears to be a bug in the logic for decoding the tapleaf script. The condition if t == nil will likely always be false for a known TLV type like typeTapLeafScript. When tlv.Stream.DecodeWithParsedTypes finds a known type, it returns a non-nil tlv.Record in the parsedTypes map. A nil value is used to indicate an unknown TLV type was encountered. Because typeTapLeafScript is a known type in the decoder stream, t will not be nil, and leaf.Script will never be assigned. This will result in decoded leaves having nil scripts.
The correct approach, as seen in btcwallet, is to just check for the presence of the key in the map. The script variable is populated by the DecodeWithParsedTypes call via the pointer passed to MakePrimitiveRecord.
if _, ok := parsedTypes[typeTapLeafScript]; ok {
leaf.Script = script
}There was a problem hiding this comment.
Fixed in split-1: tapleaf decode now sets leaf.Script whenever the tapleaf-script type is present, so decode no longer depends on the map entry value being nil.
| tapscript, err := scripts.CheckpointTapScript( | ||
| policy, in.OwnerLeafScript, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| encodedTapTree, err := EncodeTapTree(tapLeafScripts(tapscript.Leaves)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| checkpointPkScript, err := scripts.CheckpointPkScript( | ||
| policy, in.OwnerLeafScript, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The BuildPSBT function currently calls scripts.CheckpointTapScript and then scripts.CheckpointPkScript. However, scripts.CheckpointPkScript internally calls scripts.CheckpointTapScript again, leading to redundant computation.
You can optimize this by computing the checkpointPkScript directly from the tapscript variable that is already available. This avoids the second call to CheckpointTapScript.
tapscript, err := scripts.CheckpointTapScript(
policy, in.OwnerLeafScript,
)
if err != nil {
return nil, err
}
encodedTapTree, err := EncodeTapTree(tapLeafScripts(tapscript.Leaves))
if err != nil {
return nil, err
}
tapKey, err := tapscript.TaprootKey()
if err != nil {
return nil, fmt.Errorf("unable to compute taproot key: %w", err)
}
checkpointPkScript, err := txscript.PayToTaprootScript(tapKey)
if err != nil {
return nil, fmt.Errorf("unable to create p2tr script: %w", err)
}There was a problem hiding this comment.
Updated in latest push: simplified this path by deriving taproot key from tapscript once and building the output script directly (PayToTaprootScript), removing the redundant recomputation path.
| func GetTapTreePSBTInput(input psbt.PInput) ([]byte, error) { | ||
| for _, u := range input.Unknowns { | ||
| if bytes.Equal(u.Key, TapTreePSBTKey) { | ||
| if len(u.Value) == 0 { | ||
| return nil, fmt.Errorf( | ||
| "tap tree value is empty", | ||
| ) | ||
| } | ||
|
|
||
| return u.Value, nil | ||
| } | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("tap tree not found") | ||
| } |
There was a problem hiding this comment.
The GetTapTreePSBTInput function returns the value of the first taptree key it finds in the PSBT input's unknown fields. If multiple taptree keys are present, it will silently ignore the subsequent ones. This could lead to ambiguity if a PSBT is malformed with multiple taptree entries.
To make this more robust, the function should ensure that there is exactly one taptree key and return an error if multiple are found.
func GetTapTreePSBTInput(input psbt.PInput) ([]byte, error) {
var tapTreeValue []byte
var found bool
for _, u := range input.Unknowns {
if bytes.Equal(u.Key, TapTreePSBTKey) {
if found {
return nil, fmt.Errorf("multiple tap tree " +
"entries found")
}
if len(u.Value) == 0 {
return nil, fmt.Errorf(
"tap tree value is empty",
)
}
tapTreeValue = u.Value
found = true
}
}
if !found {
return nil, fmt.Errorf("tap tree not found")
}
return tapTreeValue, nil
}There was a problem hiding this comment.
Addressed: GetTapTreePSBTInput now rejects duplicate tap tree entries instead of accepting first-match.
bhandras
changed the title
57e967a to
7c870f8
Compare
bhandras
changed the title
d4fd6c0 to
7d7f9bf
Compare
f6e2018 to
5af35c8
Compare
|
@bhandras, remember to re-request review from reviewers when ready |
5af35c8 to
aee4ce1
Compare
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
nice work! great structure.
Main blocking questions are for the very last commit wrt checkpoint txs. But perhaps those questions will be answered as I make my way through the series
| // We treat this as part of the OOR PSBT profile so client and server | ||
| // implementations can deterministically attach, validate, and later use | ||
| // the same metadata during finalization. | ||
| TapTreePSBTKey = []byte("taptree") |
There was a problem hiding this comment.
wondering what is a good rule for knowing when to add data to PSBT kv pairs vs when to explicitly communicate it.
For example, for the VTX tree signing, I opted to explicitly communicate co-signers rather than embedding it in the PSBT like was done in arkade.
There was a problem hiding this comment.
Rule we’re using: put tx-bound signing material in PSBT; keep session/protocol control data explicit in RPC fields.
| // The checkpoint tree for v0 is a simple two-leaf tree: | ||
| // | ||
| // - an operator-controlled CSV unroll leaf (operator key + relative | ||
| // timelock), | ||
| // - an owner-controlled collaborative leaf (provided by the caller as raw | ||
| // script). | ||
| // |
There was a problem hiding this comment.
hmm im a bit confused here. in my mind, the checkpoint is "owned" by the operator since the operator is the party who can sweep after the CSV (so that is the operator-controlled CSV leaf path), then : the other leave is a collab multisig between operator and client. ie, it is important that that is the exact script
There was a problem hiding this comment.
Clarified the docs: checkpoint has operator CSV timeout leaf + collaborative operator/owner leaf, and this helper only commits the provided script bytes into the tree. Higher layers are responsible for enforcing that those bytes are the exact expected closure script.
acc8c5c to
2b7d0fa
Compare
bhandras
changed the title
24cbc6e to
9ffa9bb
Compare
(cherry picked from commit eadff97)
Add a v0 structural validator for OOR submit packages. The validator enforces canonical Ark PSBT ordering, ensures each Ark input spends a provided checkpoint tx output (vout=0), checks that Ark PSBT witness UTXOs match the referenced checkpoint output, and requires per-input `taptree` metadata for later finalization. Unit tests cover a happy path and common failure cases. (cherry picked from commit 1834eea)
Add a v0 structural validator for OOR finalize packages. The validator checks that the provided checkpoint PSBT set matches the Ark tx input checkpoint set (txid:vout=0), and requires each checkpoint PSBT to include some signature material (final witness/script or taproot sig fields). Unit tests cover a happy path and common failure cases. (cherry picked from commit 9f97816)
Add draft OOR checkpoint script helpers. This introduces a minimal CheckpointPolicy and helpers to deterministically construct a two-leaf checkpoint taproot tree (operator CSV unroll leaf plus caller-provided owner leaf) and derive the corresponding P2TR pkScript. The implementation lives in new files to minimize overlap with ongoing closure/vtxo refactors, and is intended to be swapped to closure-based building later. Unit tests assert the result is a valid P2TR pkScript and that the tapscript root hash and output key are computed consistently. (cherry picked from commit f6e2018)
9ffa9bb to
7a8ef31
Compare
Address lint failures introduced by rebasing `oor-client-split-1` onto the latest `main`. Fix `gocritic`'s append-assign warning in the OOR tap tree helper, shorten comment/tag lines to satisfy the custom `ll` 80-column rule, and remove the now-unneeded `nolint` directive in DB store config.
3 participants
Context
Companion client stack for OOR epic: lightninglabs/darepo#89.
This split introduces protocol primitives and validators that all later OOR FSM
and durability work depends on.
Scope
Included
lib/tx/oorprimitives + validators.Not Included
Testing
Stack