Segwit support plan

[oscarguindzberg]

This is a Bisq Network proposal. Please familiarize yourself with the submission and review process.

Summary

This is a plan to add segwit support to bisq. Bisq's custom bitcoinj has to be rebased on top of bitcoinj 0.15, bisq has to be adapted to use 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. Using segwit for BSQ txs is out of scope and could be subject to a future proposal.

Once a phase is implemented, it could be released to users and does not require the next phases to be implemented. I would recommend including phase 1 (and optionally phase 2) in one user release and phase 3 in another user release.

Goals

• Reduce btc miner fees.
• Allow users to extract btc to native segwit addresses.
• Migrate out from bitcoinj 0.14 because it 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 (https://github.com/bisq-network/bitcoinj/commits/bisq_0.14.7).
• Bisq’s custom bitcoinj is based on bitcoinj 0.14.7. In May 2019 I wrote the code needed to port bisq's bitcoinj to 0.15.1 (https://github.com/bisq-network/bitcoinj/commits/bisq_0.15.1) and to use it in bisq ([WIP] Use bisq's bitcoinj 0.15.1 bisq#2772).
• The (unmerged) PR was not meant to bring segwit to bisq. It just brings the latest bitcoinj version as of May 2019. 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 bisq's bitcoinj status bitcoinj#33

Segwit BIPs

• Segwit: https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki
• Segwit address format: https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki
• Segwit tx serialization: https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki

Related bisq open proposals

• Support sending to SegWit/Bech32 addresses: Support sending to SegWit/Bech32 addresses projects#15
• Introduce interface between Bisq and BitcoinJ: Introduce interface between Bisq and BitcoinJ projects#30

Done

• I updated the work done in May 2019 to May 2020 world:
• I 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
• I updated the bisq code to use bitcoinj 0.15.8, i.e. he rebased [WIP] Use bisq's bitcoinj 0.15.1 bisq#2772 from bisq master and updated it for bitcoinj 0.15.8. See: [WIP] Use bisq's bitcoinj 0.15.8 bisq#4270

Phases

Phase 1: Migrate to bitcoinj 0.15.8

• Review and merge [WIP] Use bisq's bitcoinj 0.15.8 bisq#4270

Phase 2: Allow users to extract to native segwit addresses

• This is just a matter of parsing and validating properly addresses entered by the user.
• Search for usages of org.bitcoinj.core.LegacyAddress.fromBase58() and replace it with org.bitcoinj.core.Address.fromString() (which supports both legacy and native segwit addresses). Do it only when it makes sense (eg for btc addresses but not for bsq addresses)
• Some relevant cases to fix:
  • bisq.asset.coins.Bitcoin should stop using Base58BitcoinAddressValidator that is based on LegacyAddress.fromBase58() and use another validator that checks 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 the Trade protocol

Segwit for bisq btc wallet

• This is just for the bisq btc wallet, the bsq wallet is out of scope.
• Native segwit should be used (I mean as opposed to segwit nested in old “3xxxx” addresses). See "Notes" 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 instances)
• Each user will have a native segwit keychain (the new default, or in bitcoinj terms “active”) and a second legacy P2PKH keychain.
• The legacy P2PKH keychain is for
  • Btc received before segwit implementation in bisq.
  • Supporting 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 newly created bisq btc wallets have 2 keychains
    • In WalletConfig, the preferredOutputScriptType variable is currently set to P2PKH. Set to it to P2WPKH for the bisq btc wallet (make sure to keep P2PKH for the bsq wallet).
  • When requested a key/address, the bitcoinj Wallet class provides a key/address from the active keychain. Once segwit is implemented, the active keychain for the btc wallet is native segwit. Bisq code that gets key/addresses from the btc 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 Wallet class.
  • Support users with old external btc wallets
    • They should be allowed to obtain a btc legacy address to fund their bisq btc wallets.
    • On the "Funds / Receive funds" screen, the "Generate new address" button should have a related dropdown/radio button to select native segwit address (default) or legacy address.
    • It eventually should end up calling Wallet.freshReceiveAddress(ScriptType) or a newly created method that accepts a ScriptType.
  • Migration
    • Existing btc and bsq 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 a BIP44 standard path. This is already implemented in [WIP] Use bisq's bitcoinj 0.15.8 bisq#4270 (See BisqKeyChainGroupStructure and BisqKeyChainFactory).
    • A native segwit keychain that uses the same seed used by the old P2PKH keychain should be added to migrated wallets. The new native segwit keychain will become the default one. Draft implementation: oscarguindzberg/bisq@bfd545f

Segwit for the Trade protocol

• P2SH mulstisig is currently used for the 2of2 multisig between buyer and seller. P2WSH multisig should be used instead.
• P2PKH is currently used for other addresses created to be used as part of the trade protocol (e.g. reserved for trade address). P2WPKH should be used instead.
• bitcoinj P2WSH support probably has some missing parts, but I guess it would be easy to add what is missing. e.g. add more methods to ScriptBuilder class.
• It should be implemented as a Bisq hard fork. i.e. old bisq clients should stop working. Alternatively it should be implemented as a different trade protocol version to avoid old and new clients trading with each other. People that have worked on the new trade protocol migration might have a say about this. See the "migration to new trade protocol message to users".
• 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 the AddressEntry records in the bisq db created before the bisq segwit upgrade have the segwit boolean flag set to false. Maybe false is the default value for a new attribute and no migration is required. If that is not the case, it should set to false during migration.
    • Tasks
      • CreateMakerFeeTx
        • fundingAddress, reservedForTradeAddress and changeAddress make them a SegwitAddress.
      • CreateTakerFeeTx
        • fundingAddress, reservedForTradeAddress and changeAddress make them 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 work with P2WSH multisig.
        • signInput()
          • Add support to sign P2WPKH.
        • createBtcTradingFeeTx()
        • sellerSignsAndFinalizesPayoutTx()
        • makerCreatesDepositTx()
      • WalletService
        • signTransactionInput()
          • TransactionInput scriptSig and witness data 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 of type ScriptType should be added.
          • Wallet.freshReceiveKey() usage should be replaced by a method that accepts a ScriptType.
          • AddressEntry.segwit boolean flag should be set depending on the script type.
      • BsqWalletService
        • It probably requires little or no changes, but have a look at it just in case.
        • Keep in mind BSQ wallet is not being converted to segwit.
    • Make sure partially signed txs sent from/to buyer/seller are serialized/deserialized with witness data. I guess nothing has to be coded, but just check that is the case. See RawTransactionInput.
  • Migration
    • In progress trades when the migration occurs: It won’t be possible to complete them.
    • Open offers 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.

Phase 4: Use a segwit native address as the recipient of BTC trade fees

• Whoever controls this address, should move to a native segwit enabled wallet and generate a new address.
• bisq.core.dao.governance.param.Param.RECIPIENT_BTC_ADDRESS value should be changed the new address.
• The value is also hardcoded in DelayedPayoutTxValidation and should be changed there too.

Dictionary

• Bisq 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).
• Native BTC segwit address: A segwit btc address in bech32 format. In mainnet, both single sig addresses (P2WPKH) and multisig over script hash addresses (P2WSH) start with bc1.
• Legacy BTC address: A BTC address in Base58 format. In mainnet, single sig addresses (P2PKH) start with 1, and multisig over script hash addresses (P2SH) start with 3.

Notes

• Segwit “flavors”
  • Native segwit (the one I recommend to be used)
    • Pros:
      • Already implemented in bitcoinj.
      • Smaller tx size (thus fee).
    • Cons
      • Old bitcoin wallets can not send to native segwit addresses, those users need an alternative solution.
  • Segwit nested in old “3xxxx” addresses (aka P2WPKH nested in BIP16 P2SH)
    • Pros:
      • Old bitcoin wallets can send to “3xxx” addresses, no alternative solution needed for users with old bitcoin wallets.
    • Cons
      • Needs to be implemented in bitcoinj.
      • Bigger tx size (thus fee).

[chimp1984]

@oscarguindzberg
Thanks a lot for that excellent proposal!

May I throw in a few considerations/ideas/remarks?

Regarding hardfork:

I think we should only consider a hardfork if we are sure that there is no feasible alternative. A hardfork would require that existing offers are removed and created newly which is a usability penalty and causes loss of maker fee and tx fee. The lost maker fee could be reimbursed but that causes quite a bit of administrative overhead and again is a usability penalty. If users update when they have an open trade it would lead to failed trades. With the trade protocol v2 hard fork we have learned that telling people to not update in such cases is not sufficient as most do not read warnings....It was quite a support mess as a result to deal with all those failed trades.

I would like to sketch out quickly the option for a non-hard fork solution:
We keep the existing wallet methods and use if for non-segwit trades. For segwit trades we use the new methods. A flag indicates which method to be used.
The maker fee tx and taker fee tx can be used to convert legacy address to segwit or the other way round. So we are not dependent on the way ho the user has funded his wallet.

A user who has updated will create new offers with a BECH 32 address. Such offers get an extra data field to mark them and a taker who requests to take the offer has to send a flag to signal that he has segwit support, otherwise he will get a reject message from the maker. If we roll out early enough the flag in the offer, we can detect that also in the offerbook so an not-updated user sees such offers as deactivated (with a popup to tell the user to update), avoiding wait time for the response of the maker. Only users who have not updated for longer time would get the reject message. We have even the version number as part of the offer ID so we could even use that to detect if the offer was created after segwit activation....
If an updated users wants to take an offer from an not-updated user he can still do that and the old wallet methods are used (legacy addresses).
If an updated user has an open trade which was started before the update, they can still continue using the old wallet methods.

Once most users have updated, most offers will be segwit enabled and thus there is more pressure on not-updated users. Additionally we could support an offer-renewal method to make it easier for users to switch their existing offers to segwit. The maker fee and tx fee would still be lost, but I assume some users would at least not care too much for smaller offers (fiat).

As said we can support the transition with early shipping of options which will help us at segwit activation time.
I have not spent much time to think about all so I might miss some critical problems, but I have the feeling it is less of a pain overall to support both address types rather to enforce a hard fork with all negative consequences. The 2 hard forks Bisq had in the past cause each time a big disruption and loss of trade volume for several months.

Tools for dealing with backward compatibility:

We have several "tools" to deal with backward compatibility and its a quite complex field. I will list here a few I remember but there might be more...

• Activation date: Use a defined date/time to activate certain behaviour. Roll it out earlier and make sure most users have updated to switch to the new behaviour at activation time.
• Filter: A core dev can publish P2P network data which triggers some actions like preventing trade if the user has not updated to a certain version. If needed we can add new fields there.
• Capabilities: Peers exchange at certain P2P network messages their capabilities. As implementation is not guaranteeing to have exchange the capabilities, this feature might be of limited usage in the current state.
• Trade messages: At take offer time trade messages could be used to detect if the peer has an updated version (e.g. if a certain field in the PB object is set or not).

Alternatives/BitcoinJ future

I have not followed BitcoinJ but a year back it seemed its a "dead horse" and we should be careful to invest too much to stick with it. The past investigations for alternatives ended negative but I think it might be useful to make an update for that investigation for alternatives. Also as Bitcoin Core has deactivated Bloom filters by default we are even more locked up with our federated Bitcoin Core nodes which makes the whole wallet infrastructure much more a "federated server model" as it was intended initially. I am wondering if a federated Electrum server model would be a feasible alternative? But I am aware that this is another huge multi-month project. So nothing we should take lightly and we have to consider the benefits/costs specially with out budget constraints now. How difficult and risky it would be to migrate the BSQ wallet would be another hard challenge.

[dmos62]

To add to what @chimp1984 said about handling backwards compatibility and migration under "Tools for dealing with backward compatibility", BitcoinJ 0.15.7 got service bit filtering for DnsDiscovery [0], (which is capabilities filtering you mentioned) letting you not connect to incompatible peers for example. Further, we can already check each peer's service bit and version in Bisq; it's only a matter of wiring that information through to where we need it. We can even disconnect a peer if we don't like its service bit (simulating the mentioned new feature in 0.15.7), but it's not as neat as doing it on DnsDiscovery's level.

Even now BitcoinJ is still limited in how well it can discriminate peers (service bit filtering is not version filtering for example). We can expand BitcoinJ's facilities for our migration plans (and general robustness) if needed. I think that will often be preferable to doing something elaborate Bisq-side (like implementing parts of Bitcoin handshake on top of trade messages).

[0] https://bitcoinj.github.io/release-notes#version-0157

[chimp1984]

@dmos62

BitcoinJ 0.15.7 got service bit filtering for DnsDiscovery [0], (which is capabilities filtering you mentioned)

Thanks for the info. But the capabilities I referred to is our own P2P network based capability feature.
As Bisq nodes are connected only to the provided Bitcoin nodes but not to other Bisq nodes on the Bitcoin layer we cannot use the Bitcoin service bits for any of our needs for the segwit upgrade.

[ricardosaurio]

i´ll ask out of sheer ignorance, coulndt bisq just use the bitcoin daemon in localhost, if avaliable?

[chimp1984]

@ricardosaurio
That would require downloading the blockchain (pruned mode only helps with limiting required disc space not with bandwidth and validation time). We hoped on the "hybrid full block SPV mode" version which would have optimised some things to make it possible for Bisq to use Bitcoin Core but seems that project did not get enough support and is dead ;-(.

[ricardosaurio]

@chimp1984 how about opt-in for users who do run such a full node with bloom filters activated? I am actually running such a setup in the same machine as my Bisq app. I dont know nothing about coding, im just spitballing. And I know running a full node is not what grandma would do.

[chimp1984]

If you have a local full node Bisq is using it anyway (via BitcoinJ and bloom filters). I was thinking that you refer to use Bitcoin Core as replacement for BitcoinJ.

[ricardosaurio]

yes i meant that, bitcoin core as replacement for BitcoinJ... I can see how it wouldnt help if someone without a node is sending from legacy to native segwit from bitcoinj, so that would be a problem.

[MwithM]

Closed as approved.

[wiz]

@oscarguindzberg since this proposal was approved, many users are asking about the implementation timeline. i understand all the budget concerns have been addressed. do you have any ETA on when you can begin working on this project?

[chimp1984]

@oscarguindzberg
After more though about the Segwit implementation roadmap I would suggest following handling of legacy/segwit mixed cases.

Offers:
If offer was created pre segwit release the reserved funds sit on a legacy address.
If offer was created post segwit release the reserved funds sit on a segwit address.
I don't see any problems here.

Open trades when one user has updated and the other not:
The Multisig and delayed payout tx are created pre segwit. Once the payout tx is created they send the funds to the defined legacy payout addresses. If delayed payout gets published it goes to the legacy burningman address.

Trade between not updated and updated users:
Case: Maker has pre segwit verions and taker has post segwit version.
Taker uses funds from segwit address and create the taker fee tx with reserved funds to a segwit address. Maker had alreayd created maker fee tx and reserved funds are sitting on a legacy address.
Both create the 2of2 P2SH Multisig and use mixed inputs (maker has legacy utxo, taker has segwit utxo) and send those to the P2SH multisig. The delayed payout tx and later the payout tx is also handled in legacy style, though the taker can receive the funds on a segwit address (we need to check if there is no address validation at makers side which would fail for segwit addresses for old users, is so we need to send funds to legacy address for taker as well).
Case: Maker has post segwit verions and taker has pre segwit version.
Same as above, just swapped roles when it comes to segwit address support. MultiSig is P2SH.

The trade messages at creating the multisig need a new flag for indicating segwit support. If the flag on at user is missing P2SH multisig is used.

Only if both users have updated they can use P2WSH.

I am aware that handling both might be a bit more dev effort but if we dont support that we would run into lot of backward compatibility issues. Specially open trades cannot be handled by a hard fork (see comments above).

For BSQ I do not see any issue as BTC inputs can be either segwit or legacy. By default segwit should be used. BTC change address would also be by default segwit (I think no option for user to change that is required).

PS: Instead using flags for indicating segwit support we could also add a capability. But not sure if the capability is exchanged between the peers before the trade protocol starts. Probably more safe to add a flag.

[chimp1984]

I had a call with @oscarguindzberg and we agreed that a enforced update (hardfork) is feasible and the intended strategy. If we find during development that support for a non-hardfork is feasible we might change that.

[sqrrm]

I think making a hard fork is the better option. The issues we had going to the new trade protocol were quite broad and costly, both from a user experience perspective and definitely from a developer/support perspective.