bitcoinj
Address,SegwitAddress: Deprecate getHash()
- Deprecate Address.getHash() and SegwitAddress.getHash()
- Use SegwitAddress.getWitnessProgram() in the cases where SegwitAddress.getHash() was previously used.
It seems that both cases where a segwit address "hash" is being used are special-cases and we should not be providing "hash" has a general property of address. Especially, given that hash does not currently include witnessVersion.
Related:
- https://github.com/bitcoinj/bitcoinj/pull/1840
- https://github.com/bitcoinj/bitcoinj/issues/2697
- https://github.com/bitcoinj/bitcoinj/pull/2743
It's documented in Address.getHash():
Get either the public key hash or script hash that is encoded in the address.
So it's expected that it doesn't include the witnessVersion. And for that matter, it's not meant as a replacement to hashCode() or similar.
But indeed, it's debatable we need this as a general property – probably not. On the other hand, the 3 usages you refactored used it in a general way.
Perhaps is makes sense to just remove the method from the public API, but keep it for internal use? And/or renamed to something like getRepresentedHash()?
It's documented in
Address.getHash():Get either the public key hash or script hash that is encoded in the address.
I wasn't sure whether this comment had been updated for segwit. I suppose we could explicitly mention the segwit case and also distinguish from .hashCode().
My main motivation for exploring this was to eliminate the byte[] data in the parent Address class and to make Address into an interface.
Both of those goals can be achieved while leaving the Address.getHash() method.
So the question becomes: "is getHash a general property of all addresses?"
It seems there are (at least) two use cases for this information:
- Verifying message signatures
- Providing a unique binary lookup key for addresses (e.g. in a wallet)
I don't think either of these uses should be considered internal.
I'm not sure how a full Taproot implementation will affect this (if at all)
So it really comes down to: what is the best API for Address?
Both of those goals can be achieved while leaving the Address.getHash() method.
In that case I'd suggest to eliminate this commit from the other PRs so we can go ahead with the easy ones first.
I'll respond to your comment more thoroughly later today (hopefully).
Both of those goals can be achieved while leaving the Address.getHash() method.
In that case I'd suggest to eliminate this commit from the other PRs so we can go ahead with the easy ones first.
Yes, I was about to suggest that.
I'll respond to your comment more thoroughly later today (hopefully).
I'd like to hear your thoughts. However, I don't think there's a big hurry on changing this after we rebase the other PRs.
Converting to DRAFT. PR #2747 submitted to continue the PR-chain skipping this PR/commit.
I think you identified the right usecases and the necessary refactoring in the usages shows how much more complicated usages would become when removing getHash(). So I think its reasonable to have this method and indeed I don't see reason for it to become "internal" (private, whatever).
However based on our discussion I think we should…
- rename it to a more approprate name.
- make clear in the JavaDoc that this is not a hash (hashCode, whatever) of the address. Rather the hash is an element (field) of the address or like I said "the address represents a hash". I'm not sure about the best wording here.
However based on our discussion I think we should…
- rename it to a more approprate name.
- make clear in the JavaDoc that this is not a hash (hashCode, whatever) of the address. Rather the hash is an element (field) of the address or like I said "the address represents a hash". I'm not sure about the best wording here.
I agree. Let's put this off until we have resolved some other issues. I'll leave this PR open as a reminder.