Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: btcsuite and go-ethereum updates #1542

Merged
merged 5 commits into from May 25, 2022
Merged

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Mar 23, 2022

A number of updates in the btcsuite universe have created new sub-modules, and introduced breaking changes to several packages. This work adapts to the changes:

  • btcec/v2 was created as a new module. The was important so that go-ethereum, which imports btcec for it's no-cgo crypto package, could break free from the btcd mega-module that did not follow semantic import versioning, creating challenges with breaking changes with minor version updates. The migration of go-ethereum/crypto to the new btcec/v2 module was done in crypto: use btcec/v2 for no-cgo ethereum/go-ethereum#24533
  • Since btcec/v2 is primarily type aliases into dcrd/secp256k1/v4, a number of syntax changes are made similar to dcrec
  • btcutil was moved from it's own repo (github.com/btcsuite/btcutil) into a new submodule in the btcd repo (github.com/btcsuite/btcd/btcutil)
  • txscript was updated for taproot support, and there were a few breaking changes. Namely, NewTxSigHashes requires a PrevOutFetcher for taproot support, and ExtractPkScriptAddrs no longer returns an error for invalid/non-standard scripts.
  • github.com/btcsuite/btcwallet/wallet was updated to use the new txscript and there was a syntax change to wallet.(*Wallet).SendOutputs
  • github.com/btcsuite/btcwallet itself was updated for the above changes

There will be more btcsuite module changes in the near future, including the introduction of wire, chaincfg, btcutil/address, and txscript modules to btcd (see btcsuite/btcd#1825), as well as corresponding updates to btcwallet, but this starts the processes and moves to a go-ethereum version that does not require the btcd mega-module that is not friendly to Go consumers.

@JoeGruffins
Copy link
Member

Comment has go-ethereum in it, which is probably not the right thing?

@chappjc
Copy link
Member Author

chappjc commented Mar 23, 2022

The go-ethereum update to 1.10.17 is related to the btcec/v2 update since go-ethereum was using the btcd module before that patch. This is what I'm describing in the first bullet.

@JoeGruffins
Copy link
Member

Oh, I'm sorry. I guess I have a reading problem. The first bullet explained it.

@chappjc
Copy link
Member Author

chappjc commented Apr 6, 2022

There will be more to come with btcsuite/btcd#1825 and other module changes in the btcsuite repos, but this is working as is to allow us to freely update both btcsuite and go-ethereum modules.

@chappjc chappjc marked this pull request as ready for review April 6, 2022 21:37
@chappjc
Copy link
Member Author

chappjc commented Apr 15, 2022

Will update the go-ethereum require to use ethereum/go-ethereum#24649

@chappjc chappjc added this to the 0.5 milestone Apr 23, 2022
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't get the core live tests to pass (passing on master), but maybe just need a rebase? Everything else seems good to go.

Specifically, running TestNoMakerRedeem, first I got a

trade_simnet_test.go:937: client 2 reported 1 incomplete trades for order b4aefa89 after 2m0s

then subsequent tests gave

trade_simnet_test.go:299: error starting clients: dcr wallet authentication error: -4: encryption/decryption error: account is not encrypted with a unique passphrase

@chappjc
Copy link
Member Author

chappjc commented May 9, 2022

Yeah I think I need to rebase with #1582 merged.
I really should change this PR to draft though because I'm sorta waiting on geth 1.10.18 and maybe another btcsuite tag.

@chappjc chappjc marked this pull request as draft May 9, 2022 16:08
btcutil and btcec/v2 are now sub-modules in the btcd repo.
The updated txscript.NewTxSigHashes requires a PrevOutFetcher for
detecting a taproot output.  In this change, we provide a dummy fetcher
that always returns a wire.TxOut with a nil pkScript that so
txscript.IsPayToTaproot returns false.
The updated btcwallet/wallet.(*Wallet).SendOutputs method
has a CoinSelectionStrategy argument.  Pass CoinSelectionLargest to
keep existing behavior.  The other option is CoinSelectionRandom.
The updated txscript.ExtractPkScriptAddrs always returns a nil error.
Keep the error return on ExtractScriptAddrs for now since it was not
removed from ExtractPkScriptAddrs and it could have meaning again.
@chappjc
Copy link
Member Author

chappjc commented May 20, 2022

Rebased but not re-tested.

@chappjc chappjc marked this pull request as ready for review May 20, 2022 22:29
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing well. lgtm

@chappjc chappjc merged commit b2ebd32 into decred:master May 25, 2022
@chappjc
Copy link
Member Author

chappjc commented May 25, 2022

Sorry I know that creates more conflicts, again, but this one has been getting rebased for months

@chappjc chappjc deleted the btcsuite-deps branch May 25, 2022 14:29
@chappjc
Copy link
Member Author

chappjc commented May 25, 2022

Oh fft go-ethereum 1.10.18 was released this morning. 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants