[bisq-network/proposals] [WIP] Segwit support plan (#226)
Oscar Guindzberg
notifications at github.com
Fri May 22 15:33:03 UTC 2020
> _This is a Bisq Network proposal. Please familiarize yourself with the [submission and review process](https://docs.bisq.network/proposals.html)._
<!-- Please do not remove the text above. -->
-
- Summary
- This is a proposal to add segwit support to bisq. Bitcoinj has to be updated to 0.15, bisq has to be adapted to bitcoinj 0.15 and bisq has to use the segwit features present in bitcoinj 0.15. There are a couple of different places where segwit could be used in bisq, they are described as “phases” below. Migrating existing bisq users and data to segwit is one of the major challenges.
- Once a phase is implemented, it could be released to users and does not require the next phases to be implemeted. I would recommend including phase 1 (and maybe phase 2) in one user release and phase 3 in another user release.
- Goals
- btc miner fees become cheaper
- eliminate a potential transaction malleability exploit that stejbac and sq have identified.
- Allowing users to extract to bc1 addresses
- Migrate out from bitcoinj 0.14 because is not being maintained.
- Get access to the non-segwit-related features in bitcoinj 0.15 in case a dev wants to use them in the future.
- Context
- Past work
- Bisq has a custom version of bitcoinj. It is currently based on bitcoinj 0.14.
- In may 2019 Oscar wrote a PR to update bisq’s bitcoinj from 0.14 to 0.15 (https://github.com/bisq-network/bitcoinj/commits/bisq_0.15.1) and to use it in bisq (https://github.com/bisq-network/bisq/pull/2772).
- The (unmerged) PR does not bring segwit to bisq. It just brings the latest bitcoinj version with segwit support. To add segwit support, bisq should be updated to use the segwit features in bitcoinj 0.15.
- The PR was not merged because it did not bring any value to users (still not segwit support) and high risk (no bitcoinj dev to provide support if bugs popped up)
- Status report as of May 2019 here https://github.com/bisq-network/bitcoinj/issues/33
- Segwit BIPs
- https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki
- https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki
- https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki
- Related bisq open proposals
- https://github.com/bisq-network/projects/issues/15
- https://github.com/bisq-network/projects/issues/30
- Done
- Oscar updated the work done in May 2019 to May 2020 world:
- Oscar updated bisq’s bitcoinj to the latest bitcoinj 0.15 as of may 2020 i.e. adapted “bisq’s bitcoinj 0.15.1” to “bitcoinj 0.15.8”: https://github.com/bisq-network/bitcoinj/commits/bisq_0.15.8
- Oscar updated the bisq code to use bitcoinj 0.15.8, i.e. he rebased https://github.com/bisq-network/bisq/pull/2772 from bisq master and updated it for bitcoinj 0.15.8. See: https://github.com/bisq-network/bisq/pull/4270
- implementation doc
- 4 phase are described to bring segwit to bisq. I would implement them in the order described here, but it might also be possible to alter the order.
- Using segwit for BSQ txs is out of scope and could be subject of a future phase.
- Phase 1: Migrate to bitcoinj 0.15.8
- TODO
- Review and merge https://github.com/bisq-network/bisq/pull/4270
- Phase 2: Allow users to extract to bc1 addresses
- TODO
- Search for usages of LegacyAddress.fromBase58 and replace it with org.bitcoinj.core.Address.fromString (which works with “1xxx”, “3xxx” and native segwit addresses). Do it only when it makes sense (eg for btc addresses but not for bsq addresses)
- Some relevant cases to mention:
- bisq.asset.coins.Bitcoin should stop using Base58BitcoinAddressValidator that is based on LegacyAddress.fromBase58() and use another validator that checks org.bitcoinj.core.Address.fromString(). Keep in mind Base58BitcoinAddressValidator is also used for many altcoins that don’t have segwit support.
- bisq.core.util.validation.BtcAddressValidator
- Phase 3: Segwit for bisq btc wallet and for the Trade protocol
- Segwit for bisq btc wallet
- This is just for the btc wallet, the bsq wallet is out of scope.
- Native segwit (aka bech32, aka P2WPKH, aka bc1 addresses) should be used (I mean instead of segwit over old “3xxxx” addresses). See note below.
- bitcoinj wallets can have several keychains. A keychain is a group of private key/bitcoin addresses of the same type (Actually a Wallet has a KeyChainGroup that has a list of KeyChain)
- Each user will have a native segwit keychain (the new default, or in bitcoinj terms “active”) and a second legacy P2PKH (aka “1xxxx” addresses) keychain.
- The legacy P2PKH keychain is for
- Btc received before segwit implementation in bisq.
- Support users that use old external btc wallets that can’t send to native segwit addresses.
- TODO
- Search for usages of org.bitcoinj.core.Address and org.bitcoinj.core.LegacyAddress. Those are possible places where code needs to be adapted.
- Make bitcoinj newly created wallets have 2 keychains
- WalletConfig.preferredOutputScriptType is currently set to p2pkh. Set to it to p2wpkh for btc wallet (make sure to keep p2pkh for the bsq wallet)
- Support users with old external btc wallets
- On the receive funds screen, the “generate new address” button should have an associated dropdown/radio button to select segwit vs native.
- It eventually should end up calling Wallet.freshReceiveAddress(ScriptType). Maybe a new method should be added to the Wallet class that returns a fresh private/public key given a ScriptType.
- When requested a key or address, the bitcoinj Wallet class provides a from the active keychain. Once segwit is implemented, the active key chain is native segwit . Bisq code that gets key/addresses from the bitcoinj wallet should be reviewed. If a legacy address is required, it should be obtained via one of the Wallet methods that accept a ScriptType parameter e.g. Wallet.freshReceiveAddress(Script.ScriptType.P2PKH). Maybe more methods for obtaining key/addresses with a Script.ScriptType parameter should need to be added to the bitcoinj Wallet class.
- Migration
- Existing bisq btc wallets have only 1 keychain: p2pkh. The BIP32 keychain path is 0h
- The first time a user uses bisq with bitcoinj 0.15, her bsq and btc wallets are migrated to bip44 standard path. This is already implemented in https://github.com/bisq-network/bisq/pull/4270 (See BisqKeyChainGroupStructure and BisqKeyChainFactory)
- Migrated wallets should be added a native segwit keychain that uses the same seed used by the old p2pkh. The new native segwit keychain will become the default one. Implementation that needs to be reviewed: https://github.com/oscarguindzberg/bisq/commit/bfd545f89388f2b7672ed974f9111df7352e1e73
- Segwit for the Trade protocol
- P2SH mulstisig (https://bitcoin.org/en/transactions-guide#pay-to-script-hash-p2sh) is currently used for the 2of2 multisig between buyer and seller. P2WSH should be used instead (see https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wsh).
- P2PKH is currently used for related addresses created to be used for the trade protocol (e.g. reserved for trade address). P2WPKH should be used instead.
- New addresses created to be used as part of the trade protocol should be native segwit (either single-sig p2wpkh or multisig p2wsh). Old trades created before segwit migration used legacy addresses (either single-sig p2pkh or multisig p2sh) and users should be able to see those trades as part of the trade history.
- bitcoinj P2WSH support in bitcoinj probably has some missing parts, but I guess it would be easy to complete what is missing. eg ScriptBuilder class.
- It should be implemented as a bisq hard fork. i.e. old bisq clients should stop working. If not a bisq hard fork, at least have a new trade protocol version to avoid old and new clients trading with each other. People that worked on the new trade protocol (https://github.com/bisq-network/proposals/issues/52) migration might have a say about this. See https://bisq.community/t/critical-notice-finish-trades-before-upgrading-to-v1-2/8576
- TODO
- Search for usages of org.bitcoinj.core.Address and org.bitcoinj.core.LegacyAddress. Those are possible places where code needs to be adapted. Also, code that signs transactions probably needs to be adapted.
- Main parts of the source code that needs to be adapted
- AddressEntry
- new field “boolean segwit” should be added to mark addresses as segwit or legacy.
- getAddress() return type should remain as Address, but the actual instance returned should be either a SegwitAddress or LegacyAddress depending on the boolean value.
- Protobuf persistence should be adjusted: See AddressEntry in pb.proto
- We should make sure all AddressEntry in the bisq db created before the segwit upgrade have the segwit boolean flag set to false. Maybe “false” is the default value for new attribute and no migration is required. If that is not the case, a migration should be done to set it to false.
- Tasks
- CreateMakerFeeTx
- fundingAddress make it a SegwitAddress
- reservedForTradeAddress make it a SegwitAddress
- changeAddress make it a SegwitAddress
- CreateTakerFeeTx
- fundingAddress make it a SegwitAddress
- reservedForTradeAddress make it a SegwitAddress
- changeAddress make it a SegwitAddress
- TakerSendInputsForDepositTxRequest
- SellerAsMakerCreatesUnsignedDepositTx
- BuyerAsMakerCreatesAndSignsDepositTx
- DelayedPayoutTxValidation
- BuyerSetupDepositTxListener
- SetupPayoutTxListener
- Services
- TradeWalletService, WalletService and BtcWalletService have to be reviewed in detail and will require fixes in many places. I list some examples below.
- TradeWalletService
- All the methods that sign transactions should be adapted.
- get2of2MultiSigRedeemScript and get2of2MultiSigOutputScript
- Make them P2WSH multisig
- signInput
- Add support to sign P2WPKH
- createBtcTradingFeeTx
- sellerSignsAndFinalizesPayoutTx
- makerCreatesDepositTx
- WalletService
- signTransactionInput
- if input is spending a legacy address, then current code works.
- if input is spending a segwit address, code should be adapted. TransactionInput scriptSig and witness should be set.
- BtcWalletService
- getOrCreateAddressEntry and getFreshAddressEntry
- newly created AddressEntry should mostly use segwit addresses unless a legacy address is needed.
- A new method parameter should be added ScriptType
- wallet.freshReceiveKey() usage should be replaced by a method that allows the selection of the ScriptType.
- AddressEntry.segwit boolean value should be set depending on ScriptType.
- BsqWalletService
- Probably requires little or no change, but have a look at it just in case.
- keep in mind BSQ wallet is not being moved to segwit
- Make sure partially signed txs sent from/to buyer/seller have are serialized/deserialized with witness data. I guess nothing has to be coded, but just check that happens. See RawTransactionInput.
- Migration
- In progress trades when the migration occurs. It won’t be possible to complete them.
- Closed trades when the migration occurs: Users will be able to view them.
- Open offers: Funding address? Either adapt to use legacy/segwit when needed or open offers would not work.
- Phase 4: Use segwit as recipient of BTC trade fees
- bisq.core.dao.governance.param.Param.RECIPIENT_BTC_ADDRESS change with a harcoded a bc1 address
- whoever control this address, should move to a segwit wallet
- The value is also harcoded in DelayedPayoutTxValidation
- Dictionary
- Bsq wallet: The bitcoinj based bsq wallet embedded in the Bisq software.
- Bisq Btc wallet: The bitcoinj based btc wallet embedded in the Bisq software.
- External Btc wallet: The btc wallet the user normally uses (e.g. Electrum, Mycellium, etc).
- Notes
- Segwit “flavors”
- native segwit (aka bech32, aka P2WPKH, aka bc1 addresses)
- Pros:
- Already implemented in bitcoinj
- Smaller tx size (thus fee)
- Cons
- Old bitcoin wallets can not send to “bc1” addresses, so those users would not be able to fun their bisq btc wallets
- segwit over old “3xxxx” addresses (aka P2WPKH nested in BIP16 P2SH)
- Pros:
- Old bitcoin wallets can send to “3xxx” addresses, no disruption to those users.
- Cons
- Needs to be implemented in bitcoinj
- Bigger tx size (thus fee)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/proposals/issues/226
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200522/1c84a44f/attachment-0001.html>
More information about the bisq-github
mailing list