Various improvements not included in #1203

[evanlinjin]

Description

Adds various improvements to the work of #1203. These were missed out while cherry-picking, or review comments left in #1428 that were forgotten:

• Change keychains_to_descriptors to keychains_to_descriptor_ids which simplifies the field. This was mentioned here and included in Further work on #1203 #1428, but an older commit was cherry-picked.
• Rename KeychainTxOutIndex field descriptor_ids_to_keychain_set to descriptor_ids_to_keychains was missed out as an older commit was cherry-picked. This change to naming shows the direct relationship between keychains_to_desriptor_ids and descriptor_ids_to_keychains (one is directly a reverse lookup of the other).
• Change reveal_to_target_with_id to reveal_to_target_with_descriptor, reasoning mentioned here.

In addition to this, I changed the output signature of reveal_to_target, reveal_next_spk and next_unused_spk methods to return (Option<spk(s)>, changeset), whereas previously it was Option<(spk(s), changeset)>. This makes the API more consistent as the ChangeSet is always returned, and reveal_to_target and unbounded_spk_iter-esc methods all return Option<SpkIterator> (which we can .flatten()).

Notes to the reviewers

Not all changes in this PR are Changelog-worthy. I.e. renaming of internal variables to increase readability, changing code comments, refactoring private methods - are all excluded from the changelog.

Changelog notice

• Change KeychainTxOutIndex methods to always return a changeset. This makes the API more consistent.

Checklists

All Submissions:

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

[ValuedMammal]

In terms of super simple names you could have descriptor_ids_to_descriptors be named descriptors and descriptor_ids_to_keychains be keychains. With a method like get, it's still clear what the key-value relationship is.

[ValuedMammal]

I got tripped up at this comment. I think what's happening is we're removing this keychain from the old descriptor's set

bdk/crates/chain/src/keychain/txout_index.rs

Lines 509 to 516 in 78e3264

	// we should remove old descriptor that is associated with this keychain as the index
	// is designed to track one descriptor per keychain (however different keychains can
	// share the same descriptor)
	let _is_keychain_removed = self
	.descriptor_ids_to_keychains
	.get_mut(&old_desc_id)
	.expect("we must have already inserted this descriptor")
	.remove(&keychain);

[ValuedMammal]

Maybe more of a philosophical point, but I think an argument can be made that methods which previously returned K, such as index_of_spk et al, can return the descriptor id and let the caller match the id with a pre-determined label. If a user is interested in getting the set of keychains marking a descriptor, there can be a method for that called keychains_of_descriptor{_id}

[notmandatory]

Can you also add changes from #1341?

[evanlinjin]

Can you also add changes from #1341?

Good idea

[ValuedMammal]

Is it worth revisiting a discussion of returning Option vs empty iterators? I don't see an issue with returning bogus values (given the wrong inputs) if it means reducing the syntactic overhead in function signatures. We could expand this concept by returning ScriptBuf::new when we don't have an spk to reveal, and returning an empty SpkIterator (which we'd have to implement) for reveal_to_target.

[ValuedMammal]

Reviewed up through 78e3264. For a minute I thought all of #1341 was done but it looks like there's a few more places like spk_at_index, revealed_spks, unused_spks.

documentation nit: There's a missing backtick in the docs for keychain ChangeSet, which is strange because I think this was fixed in an older version.