-
Notifications
You must be signed in to change notification settings - Fork 20
Context-Aware Witness Deserialization with Optional Comments #190
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: master
Are you sure you want to change the base?
Conversation
|
Cringe at the AI-generated PR description. Also, I'm not a fan of the diluted database structure using underscores. Real-world users wouldn't write witness files themselves; tools would generate the files for them. |
|
I appreciate the feedback. Two quick questions:
I really want to see improvements on the witness files beacause I find them pretty inconvenient to use - and yes, I do write them myself - I am a user not a machine - I need it user readable. |
|
|
Thanks for the clarification - that's helpful. Regarding 1) I appreciate you now explaining the actual concern about tests. However, in your first feedback you just said the PR description was "hard to read" without mentioning that explicit Rust tests were missing. That would be easier for me to understand - I thought my comments on the testing was ok. I tested the implementation with Python (created a wheel from libsimplicityhl.rlib and ran comprehensive tests), which validated the functionality. For next time, if you point me to the right location for Rust tests in this repo, I'll add them there directly. Regarding 2 & 3) I have a suggestion: Use YAML instead of JSON for witness files JSON doesn't support comments natively - that's the core issue with my approach. YAML does: value:
# I put a comment on witness variable x here
x: 0x0001
ignored_section:
description: |
I write here what ever I want.The key point: the compiled witness should end up with "AAE=" (base64) in both the JSON AND the YAML case. This means you can let the compiler accept both formats transparently. Process:
Benefits:
I'm ready to implement this. Should I send a PR? |
It puts an unreasonable effort on reviewers who need to sift through reams of slop to identify ambiguities and incorrectness, while taking nearly zero effort for the poster to produce. It's a denial-of-service attack. It's also extremely grating to read. |
|
Uncomputable demonstrated more professionalism in his feedback than you did, Andrew. You're right that the PR description is long, but your characterization as a 'denial-of-service attack' is inappropriate. Consider the context:
More importantly: I'm primarily a user of SimplicityHL. When I identify a problem (witness file format), I make the effort to implement a solution myself rather than just complaining. That deserves respect, not dismissal—especially when I'm using tools like AI to bridge gaps in my own expertise. Thank you for your understanding. |
|
It seems like CI is failing, so let's fix that. @coremoon Personally, I am ok with removing types from witness files, if we cannot come up with scenarios where wallets have issues with ambiguous data. Does anybody have thoughts on this? I think we should consolidate on 1 format, so let's remove the old one. I think comments should not be part of witness files. Right now witness files are plain old data, just like JSON. Do we want to change that? |
|
I understand the concerns. As I mentioned yesterday, let me propose a compromise: support both JSON and YAML for witness files. JSON (current): Strict, no comments, plain data format The existing Process: (same as already suggested above)
Both formats produce identical binary-encoded witness data. The choice is purely about developer experience during development and testing. This way:
Regarding the failing CI checks: I now understand where to properly place the Rust tests in this project. Would this approach address your concerns? |
What This Solves
Closes #177 - Witness files should not require explicit type annotations
Related Discussion
This PR directly addresses the discussion in issue #177:
Key points from the issue discussion:
@apoelstra (Issue Creator):
Our Implementation Aligns With:
@roconnor-blockstream (Pruned Values Discussion):
Our Implementation Handles:
@uncomputable (Type Validation):
Our Implementation Agrees:
Summary
This PR implements optional witness type annotations by introducing compiler-provided type context to the deserialization process. Users can now write simplified witness JSON files without redundant type fields.
Additionally, users can add optional "description" fields and hint comments to their witness files without affecting compilation.
Before (Old Format):
{ "x": { "value": "1", "type": "u16" }, "signature": { "value": "Left(0x...)", "type": "Either<Signature, Signature>" } }After (New Format):
{ "x": "1", "signature": "Left(0x...)", "description": "Witness data for Bitcoin transaction", "_x": "16-bit counter value", "_signature": "ECDSA signature" }Benefits:
Changes Made
Core Implementation (5 files)
Tests
New Feature: Optional Description and Comment Fields
Description Field
Users can add a "description" field at the top level to document the witness file:
{ "description": "Witness data for Bitcoin multisig transaction #12345", "pubkey1": "0x...", "pubkey2": "0x...", "signature": "Left(0x...)" }Hint Fields (Convention)
Fields starting with "_" are treated as comments/hints and are ignored by the compiler:
{ "pubkey": "0x79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "_pubkey": "secp256k1 base point", "signature": "Left(0xabcd...)", "_signature": "ECDSA signature (r, s) encoded as Either", "description": "Example witness with documented fields" }Implementation Details
The deserialization logic automatically ignores:
Example with multiple comments:
{ "amount": "1000", "_amount": "Amount in satoshis", "_amount_note": "This is the transaction amount", "nonce": "42", "_nonce": "Random nonce for replay protection", "description": "Complete witness with detailed comments" }The compiler sees only: {"amount": "1000", "nonce": "42"}
All comment fields are silently ignored.
Test Results
Architecture
Data Flow:
Key Innovation:
Type information flows from compiler to deserialization, not from user JSON input.
This is the correct architecture because:
Code Quality
Backward Compatibility
Fully backward compatible - No breaking changes
New Public API
In CompiledProgram:
In WitnessValues:
In Arguments:
New Error Variants:
Usage Example
Benefits
Addresses @apoelstra's Point
Handles @roconnor's Concern
Satisfies @uncomputable's Requirement
Simpler User Experience
Reduced Errors
Cleaner Format
Compilation Status
Files Modified
Total: approximately 56 lines added, 191 lines refactored
Key Highlights
Addressing Issue Discussion Points
This PR directly responds to the discussion:
Questions for Reviewers
Related Issues
Checklist
Ready for review!
See discussion: #177