[wallet] Improve address API

[ValuedMammal]

Improvements to the wallet address API, see commit messages for details.

Notes to the reviewers

The logic of getting addresses is roughly the same as before when using AddressIndex, following this mapping:

• New -> reveal_next_address
• LastUnused -> next_unused_address (assuming this is what LastUnused really means)
• Peek -> peek_address

Wondering whether it makes sense to expose is_used for Wallet as well.

fixes #898

Changelog notice

Added:

• Added Wallet methods:
  • peek_address
  • reveal_next_address
  • next_unused_address
  • reveal_addresses_to
  • list_unused_addresses
  • mark_used
  • unmark_used

Removed:

• Removed Wallet methods:

  • get_address
  • get_internal_address
  • try_get_address
  • try_get_internal_address

• Removed type AddressIndex

Checklists

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Feature

• I've added tests for the new feature
• I've added docs for the new feature
• This pull request breaks the existing API
• I'm linking the issue being fixed by this PR

[evanlinjin]

If peek_address and get_address does the same thing, can we get rid of get_address?

[ValuedMammal]

If peek_address and get_address does the same thing, can we get rid of get_address?

Yes. I also feel that replacing get_address with peek_address(KeychainKind::External, 0) is slightly less convenient than before. get_address appears quite often in wallet tests and (although tests shouldn't be a driver of API choices) I think should be replaced by something similarly convenient.

How about something like this:

    /// Get the first derived address of the external keychain without revealing it.
    ///
    /// Shorthand for calling [`peek_address`](Self::peek_address) at index 0.
    pub fn first_address(&self) -> AddressInfo {
        self.peek_address(KeychainKind::External, 0)
    }

[evanlinjin]

@ValuedMammal Do you mean inconvenient as in we need to refactor many of the tests? As you mentioned, I don't think avoiding large test refactors should be a driving factor in API design.

For the first_address method you proposed, is the main motivation for non-wildcard descriptors? I think it makes sense if it was marketed as such.

[ValuedMammal]

Do you mean inconvenient as in we need to refactor many of the tests? As you mentioned, I don't think avoiding large test refactors should be a driving factor in API design.

For the first_address method you proposed, is the main motivation for non-wildcard descriptors? I think it makes sense if it was marketed as such.

"Inconvenient" in the sense that it requires an additional function arg compared to the current get_address(New).

I think any descriptor is capable of producing a first address, wildcard or not. Alternatively, we drop get_address and use peek_address everywhere. Simpler the better. Actually, next_unused_address works just fine here as well!

[ValuedMammal]

• Rebased on 52f3955
• Finally disposed of get_address

[evanlinjin]

LGTM, just a small addition to docs and I will be happy to ACK/merge!

[ValuedMammal]

The last push includes documentation improvements and the fix for map_keychain.

📌 We're using expect when converting an spk to a bitcoin Address under the assumption that the script will be valid. Is this something we can always guarantee? Food for thought I guess.

[LagginTimes]

Everything looks pretty good to me. 👍

[evanlinjin]

ACK d39b319