-
Notifications
You must be signed in to change notification settings - Fork 5.9k
BIP375: Add test vectors + runner #2046
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?
BIP375: Add test vectors + runner #2046
Conversation
d540a48 to
88dec03
Compare
- Add constants.py with BIP 375 PSBT field type definitions - Add parser.py with PSBT v2 structure parsing functions - Add minimal test_runner.py for basic structure validation - Validates PSBT magic bytes and parse-ability - All 17 test vectors pass basic structure checks - - Basic structure validation complete: 4 passed, 13 failed
- Add inputs.py with input type validation helpers - Add dependencies from previous work - - deps/bip352_utils.py - - deps/secp256k1_374.py - Validate eligible input types (P2PKH, P2WPKH, P2TR, P2SH-P2WPKH) - Check segwit version restrictions (v0-v1 only) - Verify P2SH inputs are P2SH-P2WPKH - Update test_runner.py to validate inputs Input validation complete: 6 passed, 11 failed
- Add dleq.py with DLEQ proof validation functions - Extract public keys from BIP32 derivation fields - Validate global and per-input DLEQ proofs using BIP-374 - Verify scan key consistency between ECDH and DLEQ fields - Prevent conflicting global/per-input ECDH for same scan key - Check ECDH shares exist for silent payment outputs - Update test_runner.py with comprehensive DLEQ validation DLEQ validation complete: 14 passed, 3 failed
- Add validator.py with comprehensive BIP-375 validation - Validate output field requirements (SCRIPT or SP_V0_INFO) - Check SP_V0_INFO field size (66 bytes) - Validate SP_V0_LABEL requires SP_V0_INFO - Enforce SIGHASH_ALL requirement for silent payments - Integrate all previous validation (inputs, DLEQ, ECDH) - Simplify test_runner.py to use validator module - Validation complete: 16 passed, 1 failed
- Add validate_bip352_outputs() for output script verification - Support both global and per-input ECDH share modes - Update test_runner.py to pass test material to validator
- Details core files and dependencies - Link to test_generator.py for vector generation - Provide usage instructions for test_runner.py
…l=0)" Fix compute output script ‘k’ bug - now increments for each output sharing the same scan key
88dec03 to
4510632
Compare
bip-0375/test_runner.py
Outdated
| # Check magic bytes | ||
| if len(psbt_data) < 5 or psbt_data[:5] != b"psbt\xff": | ||
| return False, "Invalid PSBT magic" |
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.
This check is also in parse_psbt_structure. I would keep it only in one of both places.
|
|
||
| # Extract field type and handle key-value pairs | ||
| if key_data: | ||
| field_type = key_data[0] |
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 extraction of field_type from key_data is odd. I would use de-structure assignment to keep them separated when assigning key_data, and also use the names stated in BIP 174, e.g.:
key_type, *key_data = data[offset : offset + key_len]| return False, f"Input {i} uses segwit version > 1 with silent payments" | ||
|
|
||
| # Eligible input type requirement | ||
| # When silent payment outputs exist, ALL inputs must be eligible types |
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.
What does ALL inputs must be eligible types mean in this context?
This is checking all inputs are valid for shared secret derivation, which is not a condition all inputs must fulfill in order to create a silent payment transaction.
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.
Good catch - will address in the next revision
| "comment": "Silent payment outputs require SIGHASH_ALL signatures only" | ||
| }, | ||
| { | ||
| "description": "Mixed segwit versions with silent payments", |
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.
I would prefer explicitness rather than conciseness in test case descriptions: Input with segwit script version greater than 1, even if it is more verbose.
| "sp_label": null | ||
| } | ||
| ], | ||
| "comment": "Output script doesn't match BIP-352 computed address" |
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.
What's the difference between comment and description? Could we keep only one?
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.
I agree these test vectors don't need both - will consolidate in next revision
nymius
left a comment
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.
cACK 4510632
-
I haven't been able to decode the PSBT's using
bitcoin-cli decodepsbtin a per test basis. Did you have any issues? -
I would consider copying the approach taken in #2084, and use
secp256k1labhere. -
As each PSBT change is reflected in BIP 174, maybe is worth moving the parsing, fields and PSBT validation logic to BIP 174 directory, and keep the role logic of each protocol on its own directory, like here.
Details in comment above - PSBT v2 not supported
I agree that approach is better than the current bip-0374 import. The current import process would have to adapt to that change regardless. I'll put that on the list for the next revision.
Interesting idea. Are you suggesting a storage change for this PR or expand the scope of the PSBT parsing, fields and validation to v0 and other v2 fields and move to other BIPS directories? |
I'm suggesting the second one, but as it seems to involve more components, I wouldn't pursue it in this PR. |
This PR adds the test runner
test_runner.pyandtest_vectors.jsonfor demonstrating BIP 375 behavior.Care was taken to minimize dependencies required to evaluate the test vectors.
Dependencies are referenced at runtime from bip-0374 instead of cloning locally.
Test generation tools moved to external repo
Feedback welcome @andrewtoth @achow101 @josibake
Test runner output