Repository navigation
fix(miniscript): compare thresh k and tree size in Terminal Eq/Ord - #1036
portlandhodl wants to merge 2 commits into
Conversation
| /// tree would compare equal to any other tree that has it as a | ||
| /// pre-order prefix, e.g. `thresh(2,A,B)` and `thresh(2,A,B,C)`. | ||
| fn eq(&self, other: &Self) -> bool { | ||
| if self.pre_order_iter().count() != other.pre_order_iter().count() { |
There was a problem hiding this comment.
In a187133:
This is super inefficient. Both these iterators are kinda costly, allocate stacks and do a bunch of accounting etc. I also don't believe it's possible for this check to fail without one of the below checks failing, since we are checking thresh.n() and every other combinator has a fixed number of children.
So we should drop this check.
There was a problem hiding this comment.
Ack, dropped; 100% the .count() comparison was redundant. Good catch.
There was a problem hiding this comment.
In 2476148:
There's another node count added in Terminal::cmp which is redundant.
7e6adb8 to
1edab6c
Compare
|
In f3417ad: Normally I'd complain about all this algorithmic detail in a doccomment, but in a docomment on |
995867c to
1edab6c
Compare
Thought: given LLMs could easily generate the same docs, want me to just remove to keep consistency? |
Yeah, I'd appreciate it. Good point that anybody curious can just ask a LLM how the comparison works (and ask whatever their "real" question is, e.g. how does it handle key equality checks or whether it uses recursion). |
Terminal::eq never compared the `k` threshold or child count of
`Thresh` fragments (they fell into the discriminant-only catch-all
arm), and both Terminal::eq and Terminal::cmp zipped the two pre-order
iterators without checking that they had the same length. As a result
structurally distinct miniscripts could compare equal, and could order
Ordering::Equal:
thresh(2,pk(A),s:pk(B)) == thresh(1,pk(A),s:pk(B))
thresh(2,pk(A),s:pk(B)) == thresh(2,pk(A),s:pk(B),s:pk(C))
while hashing differently (Hash includes k and the child count),
violating the Eq/Hash and Eq/Ord contracts for Miniscript, Descriptor
and TapTree, which all delegate to these impls, and corrupting sorted
and hashed collections of descriptors in downstream users.
Compare the `Thresh` threshold `k` and child count `n` in
PartialEq::eq. With `n` compared there, two trees whose pre-order
yields agree must have the same number of nodes — every other
fragment has a fixed number of children — so `eq` needs no separate
length check. Ord::cmp yields only `k` for `Thresh` nodes, so it
compares the number of pre-order nodes up front; a tree now orders
before any tree it is a strict pre-order prefix of.
Assisted-by: Kimi kimi-fast-latest
Regression tests for the Terminal equality/ordering fix: structurally distinct miniscripts and descriptors that differ only in a `thresh` threshold `k` or child count previously compared equal and ordered `Equal`, violating the Eq/Ord/Hash contracts and corrupting sorted and hashed collections of descriptors. Cover the reported failure modes permanently: - `thresh` differing in `k` or in child count compares unequal. - Eq, Ord and Hash are mutually consistent over a list of structurally distinct scripts (antisymmetry included). - Distinct scripts survive BTreeSet/HashSet insertion while exact duplicates still deduplicate. - Descriptors differing only in `thresh` policy compare unequal and survive sort + dedup. Assisted-by: Kimi kimi-fast-latest
1edab6c to
a9e0479
Compare
a9e0479: Complete |
|
I did some performance analysis on my host regarding the performance characteristics of this pull. This test harness was created outside this PR but should give an idea if any performance regressions would be considered unacceptable. Under normal use nothing sticks out as unacceptable in my opinion. Performance impactMicro-benchmark: Benchmark environment
|
|
Lol at the benchmarks showing massive speedups in cases where we now correctly notice that we're comparing different trees. |
Terminal::eqnever compared thekthreshold or child count ofThreshfragments (they fell into the discriminant-only catch-all arm), and bothTerminal::eqandTerminal::cmpzipped the two pre-order iterators without checking that they had the same length. As a result structurally distinct miniscripts could compare equal, and could orderOrdering::Equal:while hashing differently (
Hashincludeskand the child count), violating the Eq/Hash and Eq/Ord contracts forMiniscript,DescriptorandTapTree, which all delegate to these impls, and corrupting sorted and hashed collections of descriptors in downstream users.Fix: compare the number of pre-order nodes up front in both impls (a tree now orders before any tree it is a strict pre-order prefix of), and compare the
Threshthresholdkand child count ineq.Tests: regression tests cover
threshdiffering ink/child count, mutual Eq/Ord/Hash consistency (including antisymmetry and cross-instance equality) over a list of structurally distinct scripts, survival of distinct scripts inBTreeSet/HashSet, and descriptor-level equality/sort+dedup. All fail without the fix.