fix(iter): translate_pk to iterate in left-to-right order

[trevarj]

First commit is the actual conversion. Deleted all the
RtlPostOrderIter stuff, which is a breaking change. Let me know if you
want that in a separate commit.

Second commit with the Ext trait can be dropped, but I didn't want to
ruin the readability of Terminal mappings.

Closes #1022

[apoelstra]

In 66c6c9e:

Let's keep rtl_post_order_iter. It's part of the API and implementors of TreeLike get it for free. (And I use TreeLike in a bunch of downstream projects, though I don't know that I ever use the rtl iterator..)

[trevarj]

In 66c6c9e:

Let's keep rtl_post_order_iter. It's part of the API and implementors of TreeLike get it for free. (And I use TreeLike in a bunch of downstream projects, though I don't know that I ever use the rtl iterator..)

restored it and amended into the first commit.

[apoelstra]

In b413b93:

I'd suggest squashing these two commits since the second largely overwrites the first. I like the idea of pulling this into a StackExt trait. Though all the methods should be #[inline] I think.

Then, I don't like the use of split_off, which allocates a whole new vector. You should be able to use Vec::drain to "directly" turn the end of the stack vector into an iterator.

[trevarj]

In b413b93:

I'd suggest squashing these two commits since the second largely overwrites the first. I like the idea of pulling this into a StackExt trait. Though all the methods should be #[inline] I think.

Then, I don't like the use of split_off, which allocates a whole new vector. You should be able to use Vec::drain to "directly" turn the end of the stack vector into an iterator.

roger that. i had it separate in case i needed to drop it off

[apoelstra]

ACK ec491c4; successfully ran local tests

[portlandhodl]

ACK ec491c4: tests pass.

Notes: For stateless translators output is bit-identical, but a translator whose result depends on visitation order — e.g. one that assigns keys from a list by counter — will now attach the same keys to different leaves, silently producing a different descriptor/scriptPubKey/address for identical inputs (verified: filling wsh(and_v(v:pk(A),pk(B))) from a fixed key list yields bcrt1qqx5nrr8… before, bcrt1qkr42lc9… after, no error anywhere). I am not aware of any project that implements a custom Translator, but it might be worth noting.

Nit: I think it would be worth adding a CHANGELOG entry calling out the behavior change explicitly.

[apoelstra]

Yeah, agreed on the changelog entry. Ok to wait until we actually cut a release. I'll notice this PR title as a "meaningful behavior change" worth calling out, and try to remember to bold it.

I think that if anybody was depending on the order, they'd probably have already complained about it. But even most kinds of stateful translators (e.g. ones that are maintaining a counter) are still order independent, I think.