Conversation
Add DefaultVTXOTapScript function that constructs a waddrmgr.Tapscript from the closure-based VTXO system. This bridges the new closure architecture with code that requires the waddrmgr.Tapscript type. The function is explicitly named "Default" to indicate it creates the standard 2-of-2 VTXO structure (exit + collab paths). Custom VTXO scripts with additional closures should use the closure package directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @sputn1ck, 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 refactors the VTXO scripting mechanism by introducing a modular, closure-based architecture. This change moves away from rigid, hardcoded script constructions towards a more adaptable system that can support a wider array of custom spend conditions, including various timelocks and conditional scripts. The new design enhances the flexibility and extensibility of VTXO functionality, allowing for more complex and tailored transaction logic while maintaining essential validation checks. 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
|
Pull Request Review: ClosuresThis is an excellent architectural improvement that replaces hardcoded VTXO script construction with a flexible, modular closure-based system. The implementation is well-designed, thoroughly tested, and follows the project's development guidelines. Below is my detailed review. ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Test CoverageThe PR includes ~2,100 lines of test code across three test files:
Test quality highlights:
3. Strong Code Quality
4. Security Considerations
🔍 Issues & Recommendations1. Critical: Missing Function Comments
|
![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 a significant and well-executed refactoring to a flexible, closure-based system for VTXO scripts. The new lib/closure package is a great addition, providing a modular and extensible way to construct and validate complex spending conditions. The migration of existing code to use this new system is thorough and consistent. The comprehensive test suite for the new functionality is also commendable and inspires confidence in the changes. I've identified a few correctness and robustness issues in the new library code, which are detailed in the specific comments. Once these are addressed, this will be an excellent contribution to the codebase.
| case sequenceNum&wire.SequenceLockTimeDisabled == wire.SequenceLockTimeDisabled: | ||
| return nil, true | ||
| case sequenceNum&wire.SequenceLockTimeIsSeconds == wire.SequenceLockTimeIsSeconds: | ||
| timeLockSeconds := (relativeLock << wire.SequenceLockTimeGranularity) - 1 |
There was a problem hiding this comment.
The subtraction of 1 here seems incorrect according to BIP-68. BIP-68 specifies that the time-based lock-time is the 16-bit value multiplied by 512 (2^9). There is no subtraction of 1 mentioned. This could lead to off-by-one errors in locktime calculations. The other decoding function BIP68DecodeSequenceFromBytes in this file correctly implements this without subtracting 1. The test TestBIP68DecodeSequence in locktime_test.go should also be updated to reflect this correction.
| timeLockSeconds := (relativeLock << wire.SequenceLockTimeGranularity) - 1 | |
| timeLockSeconds := (relativeLock << wire.SequenceLockTimeGranularity) |
| finalStack := vm.GetStack() | ||
|
|
||
| if len(finalStack) != 0 { | ||
| return false, fmt.Errorf( | ||
| "script must return zero value on the stack, got %d", | ||
| len(finalStack), | ||
| ) | ||
| } | ||
|
|
||
| return true, nil |
There was a problem hiding this comment.
The logic for EvaluateScriptToBool seems incorrect. A script that evaluates to 'true' for opcodes like OP_VERIFY should leave a single non-zero value on the stack. This implementation checks for an empty stack and returns true, which is the opposite of the desired behavior. It should check for a single stack item that evaluates to true.
finalStack := vm.GetStack()
if len(finalStack) == 0 {
// An empty stack is considered false.
return false, nil
}
if len(finalStack) > 1 {
return false, fmt.Errorf(
"script must return a single value on the stack, got %d",
len(finalStack),
)
}
return txscript.AsBool(finalStack[0]), nil| if !valid { | ||
| return false, nil | ||
| } | ||
|
|
||
| d.Locktime = *locktime | ||
| d.MultisigClosure = *multisigClosure | ||
|
|
||
| return valid, nil |
There was a problem hiding this comment.
The Decode method for CSVMultisigClosure is missing a final verification step. Other Decode methods in this file (e.g., CSVSigClosure.Decode, ConditionMultisigClosure.Decode) rebuild the script from the parsed components and compare it to the original script to ensure canonical encoding and prevent extraneous data. This check should be added here for consistency and correctness.
if !valid {
return false, nil
}
d.Locktime = *locktime
d.MultisigClosure = *multisigClosure
// Verify the script matches what we would generate to ensure canonical encoding.
rebuilt, err := d.Script()
if err != nil {
return false, err
}
return bytes.Equal(rebuilt, script), nil| if !valid { | ||
| return false, nil | ||
| } | ||
|
|
||
| d.Locktime = AbsoluteLocktime(locktime) | ||
| d.MultisigClosure = *multisigClosure | ||
|
|
||
| return valid, nil |
There was a problem hiding this comment.
Similar to CSVMultisigClosure.Decode, the Decode method for CLTVMultisigClosure is missing the final script rebuild and comparison step. This is important for ensuring the script is canonically encoded and doesn't contain extra data. This should be added for consistency with other Decode methods in this file.
if !valid {
return false, nil
}
d.Locktime = AbsoluteLocktime(locktime)
d.MultisigClosure = *multisigClosure
// Verify the script matches what we would generate to ensure canonical encoding.
rebuilt, err := d.Script()
if err != nil {
return false, err
}
return bytes.Equal(rebuilt, script), nil| func UnspendableKey() *btcec.PublicKey { | ||
| pubBytes, _ := hex.DecodeString(arkNUMSHex) | ||
| pub, _ := btcec.ParsePubKey(pubBytes) | ||
| return pub | ||
| } |
There was a problem hiding this comment.
Ignoring errors from hex.DecodeString and btcec.ParsePubKey is not robust. While arkNUMSHex is a constant, it's better practice to handle potential parsing errors. A good pattern for this is to parse the key in an init() function and panic if it fails. This ensures the key is valid when the package is initialized and avoids ignoring errors in the function itself. For example:
var unspendableKey *btcec.PublicKey
func init() {
pubBytes, err := hex.DecodeString(arkNUMSHex)
if err != nil {
panic(fmt.Sprintf("invalid arkNUMSHex: %v", err))
}
pub, err := btcec.ParsePubKey(pubBytes)
if err != nil {
panic(fmt.Sprintf("invalid arkNUMSHex: %v", err))
}
unspendableKey = pub
}
// UnspendableKey returns the NUMS (nothing up my sleeves) key used as the
// internal key for taproot outputs where the key path should be unspendable.
func UnspendableKey() *btcec.PublicKey {
return unspendableKey
}
| type Closure interface { | ||
| Script() ([]byte, error) | ||
| Decode(script []byte) (bool, error) | ||
| Witness(controlBlock []byte, opts map[string][]byte) (wire.TxWitness, error) |
There was a problem hiding this comment.
What's opts here? Any reason to not use functional options? Unclear what the string key value is here, etc.
| // Closure represents a single tapscript leaf that can be spent. | ||
| type Closure interface { | ||
| Script() ([]byte, error) | ||
| Decode(script []byte) (bool, error) |
There was a problem hiding this comment.
What's the first return value indicate?
Is this Decode actually useful on the interface level?
| for _, t := range types { | ||
| scriptCopy := make([]byte, len(script)) | ||
| copy(scriptCopy, script) | ||
| valid, err := t.closure.Decode(scriptCopy) |
There was a problem hiding this comment.
Why not have some sort of framing layer here instead? So like a type prefix, so then you know exactly what you're attempting to decode?
Alternatively, you can add a type param here, than do like:
var closure T
and then decode into that.
| } | ||
|
|
||
| valid, err = f.decodeChecksigAdd(script) | ||
|
|
|
|
||
| } | ||
|
|
||
| func (f *MultisigClosure) decodeChecksigAdd(script []byte) (bool, error) { |
There was a problem hiding this comment.
Same here re just declaring the type upfront. Haven't seen how it's used yet in the wild though, so perhaps I'm missing something.
| } | ||
|
|
||
| // Create a new script engine with the fake tx | ||
| vm, err := txscript.NewEngine( |
There was a problem hiding this comment.
How can this run w/o all the other required inputs like the prev output fetcher, etc?
| return false, fmt.Errorf("failed to create script engine: %w", err) | ||
| } | ||
|
|
||
| vm.SetStack(witness) |
There was a problem hiding this comment.
This is odd....so it's a higher level VM on top of existing VTXOs?
| ) | ||
|
|
||
| // ReadTxWitness deserializes a witness from a byte slice. | ||
| func ReadTxWitness(witnessSerialized []byte) (wire.TxWitness, error) { |
| package scripts | ||
|
|
||
| // VTXO Taproot Tree Structure: | ||
| // VTXO Closure System: |
There was a problem hiding this comment.
Wouldn't it be possible to leave most of the existing scripts in place, but then add on custom scripts implemented as an && then an arbitrary script after that?
This PR as is breaks everything built on top of lib as is.
| // These can be decoded into Closure objects using closure.ParseVtxoScript(). | ||
| // The scripts define the spending conditions (exit paths, collab paths, | ||
| // etc.) for this VTXO. | ||
| Scripts []string |
agents: add initial draft of agent files, and CI work flows to sync them
|
@bhandras: review reminder |
3 participants
Summary
This PR introduces a flexible closure-based system for VTXO (Virtual Transaction Output) scripts, replacing the hardcoded script
construction with a modular architecture that supports custom spend conditions.
ConditionMultisigClosure, ConditionCSVMultisigClosure) with Script/Decode/Witness methods
Motivation
The previous VTXO implementation had hardcoded 2-of-2 multisig + CSV timeout structure. This PR enables:
Key Changes
New lib/closure/ Package
Closure Types
Validation Rules