Implement descriptor checksums#130
Conversation
|
Concept ACK.
|
| @@ -1,5 +1,54 @@ | |||
| import re | |||
|
|
|||
| # From: https://github.com/bitcoin/bitcoin/blob/master/src/script/descriptor.cpp | |||
There was a problem hiding this comment.
why not just copy test/functional/test_framework/descriptors.py which I think has more review than transliteration of the C++?
There was a problem hiding this comment.
Because I didn't know that file existed. I don't think it really matters either way. At least the C++ has code comments explaining how it works.
There was a problem hiding this comment.
meh ok as long as we're generally relying on the C++ code for generation/validation we should be fine.
There was a problem hiding this comment.
I also prefer using the upstream Python code, and copying comments. Not only does it have more review, it's used in the test suite to check against the c++ implementation. Our own test suite, since it clones Bitcoin Core anyway, could do a diff to make sure our implementation remains identical.
There was a problem hiding this comment.
Worst case let's just swap it out in a future PR.
|
@Sjors we should make sure we have a test covering that case as well |
This change does that. Checkums are currently optional for |
| @@ -1,5 +1,54 @@ | |||
| import re | |||
|
|
|||
| # From: https://github.com/bitcoin/bitcoin/blob/master/src/script/descriptor.cpp | |||
There was a problem hiding this comment.
I also prefer using the upstream Python code, and copying comments. Not only does it have more review, it's used in the test suite to check against the c++ implementation. Our own test suite, since it clones Bitcoin Core anyway, could do a diff to make sure our implementation remains identical.
|
For anyone else having difficulty fetching this branch after the repo move, this works too: wget https://patch-diff.githubusercontent.com/raw/bitcoin-core/HWI/pull/130.patch && git apply 130.patch |
2d62204 Use descriptors with checksums in TestSignTx (Andrew Chow) c2ff20b Use descriptor class in getkeypool (Andrew Chow) f2aff4d Add descriptor checksums and tests (Andrew Chow) Pull request description: Adds checksums to the Descriptor class. Checksums are optional, but if provided, must be valid. `getkeypool` is also changed to use the descriptor class to return checksums. Tests are updated so that all descriptors used have checksums. Closes #129 Tree-SHA512: f77b44a1edbc02d4dc6130fb4f6a97aadf2f12fef90f4b93e734df7374decb060e5486f0a5a03074aa696f6af3c772a52380bbce3ef045042e00597791aacbd1
3 participants
Adds checksums to the Descriptor class. Checksums are optional, but if provided, must be valid.
getkeypoolis also changed to use the descriptor class to return checksums. Tests are updated so that all descriptors used have checksums.Closes #129