Skip to content

[develop] Descriptor validation for miniscript#75

Merged
odudex merged 4 commits intodiybitcoinhardware:developfrom
tadeubas:validate-desc-load-miniscript
May 6, 2025
Merged

[develop] Descriptor validation for miniscript#75
odudex merged 4 commits intodiybitcoinhardware:developfrom
tadeubas:validate-desc-load-miniscript

Conversation

@tadeubas
Copy link
Member

Descriptor validation for miniscript branches was not working properly

@tadeubas tadeubas force-pushed the validate-desc-load-miniscript branch from 0a8ef6c to ed9c205 Compare April 18, 2025 02:18
@tadeubas tadeubas force-pushed the validate-desc-load-miniscript branch from ed9c205 to f5df860 Compare April 18, 2025 02:22
@odudex
Copy link
Collaborator

odudex commented Apr 25, 2025

I think to better reflect real cases, test descriptors shouldn't have repeated keys. When they do, they should have increasing branches <2;3>, <4;5> ...

@tadeubas
Copy link
Member Author

I only adapted the tests that already existed. I don't want to alter those, so I will create new test cases to contemplate this, ok?

@odudex
Copy link
Collaborator

odudex commented Apr 26, 2025

Yes, if possible, please leave previous tests untouched, their keys and order; only change their outcomes if this was a result of the code changes. To test new scenarios, please add new descriptors, trying to reproduce usual exports of real coordinators.

@tadeubas
Copy link
Member Author

Done. Plz note that this branch fixes pre-existing validation only. There are unimplemented things in Descriptor class (See TODO here https://github.com/diybitcoinhardware/embit/blob/master/src/embit/descriptor/descriptor.py#L22 ), so I had to let some TODO in tests also.

Please, fell free to submit a PR to my branch if you want any other change and I will merge right away.

@odudex
Copy link
Collaborator

odudex commented Apr 28, 2025

I still see descriptors with repeated keys in tests (while I don't see in #76).
Can you elaborate on the commented TODO TR tests? What's needed for them to be activated, how can I recreate this descriptor with <0;1;3> branches (line 208) on Liana, and what's the use case for a key like this? How about others with keys that doesn't have any branches. (line 215, 222).

@tadeubas
Copy link
Member Author

I'll revert the old tests, only updating the outcome check since validation now disallows miniscripts with different num_branches.

Will update TODO to note the tests don't check tr miniscript.

These descriptors are mostly edge cases to test validation, but real-world examples are included too.

@tadeubas tadeubas changed the title Descriptor miniscript fix check branches [develop] Descriptor validation for miniscript May 6, 2025
@tadeubas tadeubas changed the base branch from master to develop May 6, 2025 14:34
@odudex odudex merged commit a9b9d8b into diybitcoinhardware:develop May 6, 2025
@odudex
Copy link
Collaborator

odudex commented May 6, 2025

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants