Fix derive_next_unused

[rajarshimaitra]

Fixes #110

I think there was an internal logic problem previously where we were calling next twice. Once to check if next is available, and then in the return call in else block. This would progress the iterator twice on each call..

Restructured the logic to only call next once, and also solve the lifetime problem. This also allows us to remove redundant clones elsewhere.

Renamed derive_next_unused to next_unused.

[codecov-commenter]

Codecov Report

Merging #117 (fb52728) into master (fbc88da) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   59.39%   59.39%           
=======================================
  Files           8        8           
  Lines         266      266           
=======================================
  Hits          158      158           
  Misses        108      108           

Impacted Files	Coverage Δ
bdk_chain/src/keychain/keychain_txout_index.rs	0.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[evanlinjin]

I think there was an internal logic problem previously where we were calling next twice. Once to check if next is available, and then in the return call in else block. This would progress the iterator twice on each call..

The keychain_unused method is non-mutating, and the iterator returned on each call is different.

Restructured the logic to only call next once, and also solve the lifetime problem. This also allows us to remove redundant clones elsewhere.

I like this. I would prefer if derive_new uses the old new_spk (instead of calling self.inner.script_pubkeys().get(...).

Could you rebase your work on top of #116 ? Maybe create a PR onto my fork, thank you.

[rajarshimaitra]

I like this. I would prefer if derive_new uses the old new_spk (instead of calling self.inner.script_pubkeys().get(...).

Updated..

Could you rebase your work on top of https://github.com/LLFourn/bdk_core_staging/pull/116 ? Maybe create a PR onto my fork, thank you.

Rebased on top of #116

[LLFourn]

I think we should not clone the spks out here. It's easy for the caller to do this and it indicates that his was not just derived but also stored in the thing itself which is difficult to convey through the name of the method itself. I think maybe you can do it in a less awkward way without double inserting with the Entry API?

[rajarshimaitra]

I think we should not clone the spks out here.

The major hurdle I was facing for returning the reference is, that enforces the lifetime within the function scope to be specified such that two different references doesn't fall under same life time. This is a very edgy case of lifetimes and documented in this rustnomicon post. We cannot take the 2nd mutable reference, without dropping the 1st immutable one, or some how define the lifetime in the scope such that 2 refs don't occur in the same lifetime.. Or else compiler won't allow passing out the ref..

I think maybe you can do it in a less awkward way without double inserting with the Entry API?

That was my initial approach but it didn't seem to solve the above lifetime problem..

Thus so far "scoping out and copying data" seemed like the only non nasty fix to me.. But ya, this essentially doesn't allow us to pass out the ref any more.. The other option was what you had before,. have double calls and let go of the first call.

Another reason I went with copying was we were doing that in most of our call sites anyway, because that's generally is the downstream requirements for many script APIs.

[LLFourn]

Another reason I went with copying was we were doing that in most of our call sites anyway, because that's generally is the downstream requirements for many script APIs.

I think the main API we care about here is the Address API which takes a reference. I'd be in favor of just doing the rename in this PR.

I'm not sure why @evanlinjin asked you to rebase on top of his draft PR? Can we just drop the changes other than the rename here and merge it.

[rajarshimaitra]

@LLFourn reverted back to just renaming..

[evanlinjin]

I'm not sure why @evanlinjin asked you to rebase on top of his draft PR? Can we just drop the changes other than the rename here and merge it.

The reason is that #116 already takes care of #110 so this is essentially a duplicate effort, except for the changes to reference/no-reference.

[LLFourn]

Ok well since #116 has a different focus we'll just merge this now and move on.