Wallet changes for Segwit BSQ implementation #5109
Conversation
boring-cyborg
bot
added
in:altcoins
is:no-priority
|
Maybe it would be more safe to convert the 2 segwit PRs to draft until we are ready for merging it? (on the right side GH added a feature recently to convert a PR to draft) |
cd2357
added
in:dao
in:trade-process
and removed
in:altcoins
is:no-priority
|
@stejbac Is that PR ready for the upcomig release? |
|
I just checked the ACTIVATE_HARD_FORK_2_HEIGHT_MAINNET is at block 680300 which was 3 days ago (2021-04-23 11:33). Proposal phase ends May 12th, so if we release soon we do not risk to fall into the voting period which carries a bit more risk. |
|
I just tried merging with master and there are some slightly nontrivial conflicts in |
Hi Steven! Just let me know when it is ready to be merged and tested. Please point the PR directly to the v1.6.3 release branch so we can test it there a bit before it is merged into master. Thanks! |
Use flatMap(Optional::stream) instead of filter(..isPresent).map(..get) and avoid a redundantly nested Optional in OpReturnType. Also replace some unnecessary stream().forEach(..) invocations on lists in BtcWalletService, as forEach is already part of the List interface.
Remove the restriction to base58 (P2SH & P2PKH) addresses when parsing, formatting & validating BSQ addresses, by replacing o.b.c.LegacyAddress with its superclass o.b.c.Address throughout. Also remove restriction to LegacyAddress in BsqTxListItem and BsqTransfer(Service|Model). The bech32 BSQ addresses follow the same format as the old base58 BSQ addresses, namely 'B' + <btc-address>.
Uncomment & enable the m/44'/142'/1' native segwit BSQ account path and add code to migrate the user's BSQ wallet to segwit upon startup, along the same lines as the existing BTC wallet segwit migration logic. That is, set P2WPKH as the default output type, add a native segwit key chain (set to active) to the BSQ wallet and back up the old 'bisq_BSQ.wallet' file to 'pre_segwit_bisq_BSQ.wallet.backup'. Also filter out legacy addresses coming from the original keychain from BsqWalletService.get(Unused|Change)Address.
stejbac
force-pushed
the
implement-segwit-for-bsq-wallet
branch
from
cf9d162
to
c8e847c
Compare
boring-cyborg
bot
added
in:altcoins
is:no-priority
|
I've rebased onto de8213a (2021/04/20 - shared by master and release/1.6.3). |
Move consecutive maybeAddSegwitKeychain(..) calls for the BTC & BSQ wallets from BisqSetup to WalletsManager, as the latter class is used for operations which should be applied to both wallets and this moves the logic closer to the wallet domain. Also migrate a BSQ address validator in one of the test mock classes from Base58AddressValidator to BitcoinAddressValidator (missed earlier).
stejbac
force-pushed
the
implement-segwit-for-bsq-wallet
branch
from
c8e847c
to
9760526
Compare
|
(Amended the last commit of the rebase to remove a missed unused constructor parameter from |
|
Some important changes in I cannot attend to the details until later today, but I the current master's |
|
It appears the only difference between |
That |
|
I had locally merged yesterday and my branch matches exactly the rebased one. But would be good if @ghubstan can double check the changes in The |
Looks OK. (I just checked the diff between this PR's |
There was a problem hiding this comment.
utACK based on reviews from @chimp1984. I'll test it as part of release testing for v1.6.3.
Add the new account path "44'/142'/1'" for segwit BSQ to the wallet info view, which was missed from PR bisq-network#5109 making the wallet & UI changes to implement segwit BSQ. Also format the paths from the constants defined in 'BisqKeyChainGroupStructure', instead of using string literals, so that they are only defined in one place. (Though it is extremely unlikely the paths would ever change.)
Add the new account path "44'/142'/1'" for segwit BSQ to the wallet info view, which was missed from PR bisq-network#5109 making the wallet & UI changes to implement segwit BSQ. Also format the paths from the constants defined in 'BisqKeyChainGroupStructure', instead of using string literals, so that they are only defined in one place. (Though it is extremely unlikely the paths would ever change.)
This is a continuation from #5000, which replaces Bisq's Json RPC client to allow Segwit data (
"txinwitness"fields) to be retrieved by DAO full nodes, in order to fill in the pubkeys of BSQ tx Segwit inputs (as necessary to support fully Segwit compensation and proof-of-burn txs).This PR makes the necessary changes to the user's wallet and address formatting + validation to support Segwit BSQ, migrating the wallet upon startup in an analogous way to the Segwit BTC wallet upgrade (#4568). With these changes, newly generated BSQ addresses are native Segwit (p2wpkh) and take the form 'B' + <bech32-address>, e.g.
They should be valid everywhere the old base58 (p2pkh) BSQ addresses are, which continue to be supported (just never selected as new change or recipient addresses).
--
Note that this PR probably shouldn't be merged into master until the technical hard fork introduced by #5000 activates on mainnet, as otherwise upgraded users will have difficultly generating valid compensation requests or proof-of-burn txs. This is because native Segwit BSQ coins from their wallet may be arbitrarily selected as tx inputs.