util/address: make address encoding more modular by benma · Pull Request #643 · rust-bitcoin/rust-bitcoin

benma · 2021-08-29T23:30:02Z

This allow library clients to plug their own encoding parameters in a
backwards compatible manner.

sanket1729

concept ACK. I don't see any reason not to support this. There is a CI failure on 1.29 MSRV

src/util/address.rs

apoelstra · 2021-09-08T21:14:25Z

concept ACK.

I'd like the bech32 hrp to be &'static and to also have an AddressEncodingOwned type (maybe even this should be called Address?) which does not have any lifetimes and directly holds the payload.

src/util/address.rs

benma · 2021-09-10T12:18:58Z

concept ACK.

I'd like the bech32 hrp to be &'static and to also have an AddressEncodingOwned type (maybe even this should be called Address?) which does not have any lifetimes and directly holds the payload.

Imho it would be better to defer AddressEncodingOwned to another PR or issue. Since Address already exists and owns the payload, the goal is not quite clear.

I added a separate lifetime for bech32_hrp, see the other comment above.

dr-orlovsky

Concept ACK. Unfortunatelly will need a refactor once taproot addresses will get merged from #563

dr-orlovsky · 2021-09-27T06:19:22Z

Needs rebase & editing since we've got P2TR into master

benma · 2021-11-14T12:14:09Z

Needs rebase & editing since we've got P2TR into master

@dr-orlovsky Sorry for the delay. I finally rebased this PR. I also snuck in another small fix here which I extracted into another PR: #698

dr-orlovsky

Found several ways of improving the code. This is not related to the PR and rather affects the code we had before, but since we are moving/touching that code anyway I propose also to improve its readibility

src/util/address.rs

Kixunil · 2021-11-17T11:52:37Z

src/util/address.rs

Could this be represented in type?

I couldn't work out a way, did you have a concrete idea?

I do: pls see my commend below

I meant taking struct CompressedPublicKey(secp256k1::PublicKey) which guarantees the key being compressed. We could then push checking to the boundary and also have a helpful compress(self) -> CompressedPublicKey method. (Might make sense to implement in secp256k1.)

benma · 2021-11-22T13:35:32Z

I think this is ready to be merged. @sanket1729

sanket1729

Left a few comments. I think rest is good to go

sanket1729 · 2021-11-22T17:13:28Z

src/util/address.rs

This should be a reference to Secp256k1<C>

The other part is being resolved here: #707

Rebased this PR, should be good to go now!

dr-orlovsky · 2021-11-22T21:27:00Z

LGTM. Agree with @sanket1729 on having Secp by ref

sanket1729 · 2021-12-02T00:34:14Z

src/util/address.rs

nit: There is a to_bytes() method for doing this. I don't like using psbt specific utilities for this.

sanket1729

Just one more small thing.

tcharding · 2021-12-02T02:52:16Z

Thanks for your continued contributions @benma.

I'd like the bech32 hrp to be &'static

Could you address this suggestion please, either by implementing it or by commenting on why you think it should not be implemented.

If you want to implement it as suggested its just a quick fix

    /// hrp used in bech32 addresss (e.g. "bc" for "bc1..." addresses).
    pub bech32_hrp: &'static str,

Legend,
Cheers.

benma · 2021-12-02T08:28:16Z

I'd like the bech32 hrp to be &'static

Could you address this suggestion please, either by implementing it or by commenting on why you think it should not be implemented.

This was addressed here already:

#643 (comment)

tcharding · 2021-12-03T01:34:34Z

Thanks @benma, I'll try to be more careful next time and look through all the comments before messaging :)

tcharding · 2021-12-03T01:36:41Z

GitHub is weird these days

Did you mean to suggest we should move rust-bitcoin off GitHub and start using patches to a mailing list - sir, I concur!

dr-orlovsky

I just recalled that we should use existing functions PublicKey::(w)pubkey_hash() and Script::(w)script_hash() everywhere and get rid of the most of this code duplicating business logic which is already present there (BTW, unlike this code, it does no allocations)

src/util/address.rs

dr-orlovsky · 2021-12-03T09:51:15Z

src/util/address.rs

I do: pls see my commend below

benma · 2021-12-10T21:50:50Z

@dr-orlovsky

I just recalled that we should use existing functions PublicKey::(w)pubkey_hash() and Script::(w)script_hash() everywhere and get rid of the most of this code duplicating business logic which is already present there (BTW, unlike this code, it does no allocations)

I did this in all places I could find, and put it into a new commit for easier review. Please check it out.

dr-orlovsky

utACK f53c9b7b956a94eef47d4c929fa896577cf140ab

sanket1729 · 2021-12-12T05:44:58Z

ACK f53c9b7

sanket1729

ACK f53c9b7

dr-orlovsky · 2021-12-12T06:33:53Z

Oh, now it requires rebase :(

benma · 2021-12-12T10:20:39Z

Oh, now it requires rebase :(

Rebased

dr-orlovsky · 2021-12-12T11:28:06Z

First commit fails to build, pls check & fix the history with

git rebase -x 'AS_DEPENDENCY=true TOOLCHAIN=stable ./contrib/test.sh' master &&
  PIN_VERSIONS=true AS_DEPENDENCY=true TOOLCHAIN="${BITCOIN_MSRV:-1.29.0}" ./contrib/test.sh &&
  AS_DEPENDENCY=true DO_FUZZ=true TOOLCHAIN=nightly ./contrib/test.sh

This allow library clients to plug their own encoding parameters in a backwards compatible manner.

Simpler code, less duplication.

benma · 2021-12-12T12:12:40Z

@dr-orlovsky good catch - fixed

dr-orlovsky

tACK f53c9b7b956a94eef47d4c929fa896577cf140ab

All tests are passing for each of the commits in the history

benma · 2021-12-15T14:36:17Z

Please merge.

sanket1729

ACK f53c9b7

sanket1729 · 2021-12-15T14:48:52Z

Sorry, it took this long. Merging this since this has been open for quite some time. If there are other comments from other reviewers we can address those in follow-up PRs.

benma mentioned this pull request Aug 29, 2021

Make address encoding more modular / flexible #642

Closed

benma force-pushed the address_params branch from af368aa to 3951ee9 Compare August 29, 2021 23:39

sanket1729 reviewed Sep 7, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

benma force-pushed the address_params branch from 3951ee9 to a8209e8 Compare September 10, 2021 12:10

benma commented Sep 10, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

benma force-pushed the address_params branch from a8209e8 to f8739d1 Compare September 10, 2021 21:42

dr-orlovsky reviewed Sep 10, 2021

View reviewed changes

dr-orlovsky mentioned this pull request Sep 13, 2021

Error in Address:p2sh if the script is more than MAX_SCRIPT_ELEMENT_SIZE #650

Closed

apoelstra mentioned this pull request Sep 13, 2021

Should Address maintain validity invariants? #648

Open

dr-orlovsky mentioned this pull request Sep 19, 2021

Taproot P2TR address #563

Merged

dr-orlovsky added this to the 0.28.0 milestone Sep 25, 2021

benma force-pushed the address_params branch from f8739d1 to 1dca495 Compare November 14, 2021 12:12

dr-orlovsky suggested changes Nov 14, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

Kixunil reviewed Nov 14, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

benma force-pushed the address_params branch 2 times, most recently from 37d1ec4 to f01e7f6 Compare November 16, 2021 22:35

Kixunil reviewed Nov 17, 2021

View reviewed changes

sanket1729 reviewed Nov 22, 2021

View reviewed changes

benma force-pushed the address_params branch 2 times, most recently from fa9a8ef to ec776ff Compare November 30, 2021 23:17

sanket1729 reviewed Dec 2, 2021

View reviewed changes

dr-orlovsky suggested changes Dec 3, 2021

View reviewed changes

benma force-pushed the address_params branch from 41f88bc to f53c9b7 Compare December 10, 2021 21:49

dr-orlovsky previously approved these changes Dec 11, 2021

View reviewed changes

sanket1729 previously approved these changes Dec 12, 2021

View reviewed changes

benma dismissed stale reviews from sanket1729 and dr-orlovsky via 87a75d2 December 12, 2021 10:20

benma force-pushed the address_params branch from f53c9b7 to 87a75d2 Compare December 12, 2021 10:20

Kixunil added the waiting for author This can only progress if the author responds to a request. label Dec 12, 2021

benma added 3 commits December 12, 2021 13:10

util/address: make address encoding more modular

ad83f6a

This allow library clients to plug their own encoding parameters in a backwards compatible manner.

util/address: avoid .expect/panic

f826316

util/address: use hash functions of PublicKey/Script

506e03f

Simpler code, less duplication.

benma force-pushed the address_params branch from 87a75d2 to 506e03f Compare December 12, 2021 12:12

dr-orlovsky approved these changes Dec 12, 2021

View reviewed changes

sanket1729 approved these changes Dec 15, 2021

View reviewed changes

sanket1729 merged commit 36f3d23 into rust-bitcoin:master Dec 15, 2021

Kixunil removed the waiting for author This can only progress if the author responds to a request. label Dec 15, 2021

benma deleted the address_params branch December 15, 2021 16:14

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Kixunil mentioned this pull request Jun 23, 2023

Address's payload shouldn't be used by users #1908

Closed

benma mentioned this pull request Jul 4, 2024

Make Payload and AddressEncoding private #1979

Merged

Conversation

benma commented Aug 29, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apoelstra commented Sep 8, 2021

Uh oh!

Uh oh!

benma commented Sep 10, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Sep 27, 2021

Uh oh!

benma commented Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Nov 22, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Dec 2, 2021

Uh oh!

benma commented Dec 2, 2021

Uh oh!

tcharding commented Dec 3, 2021

Uh oh!

tcharding commented Dec 3, 2021

Uh oh!

dr-orlovsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Dec 10, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Dec 12, 2021

benma commented Nov 14, 2021 •

edited

Loading

dr-orlovsky left a comment •

edited

Loading