update btcec dependency

[brianathere]

, Proposing this specifically to update to btcec v2 and avoid ambiguous import of chainhash

[marten-seemann]

Can we only update the btcec dependency in this PR please?

[brianathere]

Yes, will do

[brianathere]

When I reverted the go.mod and go .sum changes, and then made the change to v2, ran go mod tidy it updated a lot of versions to the semantically compatible updated version. Are you suggesting I manually maintain the files instead of using the tools? Or is there a way of using the tools to only make very specific changes? (I'm reading the manuals, but if there's a pointer I'm happy to follow it)

[marten-seemann]

I can't reproduce this.

go get github.com/btcsuite/btcd/btcec/v2
go mod tidy

works perfectly fine.

[brianathere]

I'm still seeing a bit more change than I expect following the same steps.

[marten-seemann]

This looks fine now. Thanks.

[marten-seemann]

Can you point us to the release notes for the v2? I didn't find anything in the repo.

[brianathere]

Can you point us to the release notes for the v2? I didn't find anything in the repo.

I was unable to find it either when I started looking into how to avoid the ambiguous import that was coming into our project. This thread pointed me in this direction, and with minimal changes, it works.

btcsuite/btcd#1839 (comment)

[marten-seemann]

Looks like there were quite a few changes between v0.22.0-beta and v2.1.3. Not really sure how to review this (or if this is even necessary). The lack of release notes is disturbing though.

 ~/src/go/src/github.com/btcsuite/btcd | @fc36cb25 >
❯ git diff --stat v0.22.0-beta -- btcec/**.go
 btcec/bench_test.go     |  169 ++++++++++++++++----------
 btcec/btcec.go          |  961 ++-----------------------------------------------------------------------------------------------------------------------------------------------
 btcec/btcec_test.go     |  256 +++++++++++++++++++--------------------
 btcec/ciphering.go      |  204 +------------------------------
 btcec/ciphering_test.go |  147 +----------------------
 btcec/curve.go          |   63 ++++++++++
 btcec/error.go          |   19 +++
 btcec/field.go          | 1395 +++++++------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 btcec/field_test.go     | 1048 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------
 btcec/modnscalar.go     |   45 +++++++
 btcec/privkey.go        |   58 ++-------
 btcec/pubkey.go         |  173 +++------------------------
 btcec/pubkey_test.go    |    6 +-
 13 files changed, 906 insertions(+), 3638 deletions(-)

[brianathere]

Looks like there were quite a few changes between v0.22.0-beta and v2.1.3. Not really sure how to review this (or if this is even necessary). The lack of release notes is disturbing though.

 ~/src/go/src/github.com/btcsuite/btcd | @fc36cb25 >
❯ git diff --stat v0.22.0-beta -- btcec/**.go
 btcec/bench_test.go     |  169 ++++++++++++++++----------
 btcec/btcec.go          |  961 ++-----------------------------------------------------------------------------------------------------------------------------------------------
 btcec/btcec_test.go     |  256 +++++++++++++++++++--------------------
 btcec/ciphering.go      |  204 +------------------------------
 btcec/ciphering_test.go |  147 +----------------------
 btcec/curve.go          |   63 ++++++++++
 btcec/error.go          |   19 +++
 btcec/field.go          | 1395 +++++++------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 btcec/field_test.go     | 1048 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------
 btcec/modnscalar.go     |   45 +++++++
 btcec/privkey.go        |   58 ++-------
 btcec/pubkey.go         |  173 +++------------------------
 btcec/pubkey_test.go    |    6 +-
 13 files changed, 906 insertions(+), 3638 deletions(-)

Totally agree, I found the lack of documentation about V2 to be uncomfortable.

I believe the test cases in libp2p-core do cover this change, and that gave me confidence this is a working change.

Beyond that, I don't have any deeper insight into how to review this.

[brianathere]

@marten-seemann Do you understand why the "API Compatibility" test is failing? I'm not familiar with this type of test, I will fix, but again, pointers would be appreciated if you can provide them.

[Stebalien]

Needs a rebase, but otherwise this looks fine. It's technically a breaking change because Secp246k1{Public,Private}Key is a newtype (and we're changing the underlying type). However, I don't personally consider that to be a part of the API.

[Stebalien]

rename to btcecdsa (there's an ecdsa in the stdlib).

[brianathere]

Needs a rebase, but otherwise this looks fine. It's technically a breaking change because Secp246k1{Public,Private}Key is a newtype (and we're changing the underlying type). However, I don't personally consider that to be a part of the API.

I agree that this didn't feel like part of the public interface. I don't know how to reset this to be the new, accepted interface. Can you point me in the right direction to get that test to pass.

[Stebalien]

I agree that this didn't feel like part of the public interface. I don't know how to reset this to be the new, accepted interface. Can you point me in the right direction to get that test to pass.

It won't pass, but that's fine. It's just a way to let us know very clearly that this is a breaking change. Ideally we'd have something fancier than a failing test... but it works.

[brianathere]

Thanks! I don't have the permissions to merge or release, how do I request that?

[Stebalien]

@marten-seemann has the final decision here.

[brianathere]

Ah, thanks.

[marten-seemann]

Can you please clean up the commit history? Ideally, just rebase this on top of the current master and have a single commit.