Implement Segwit for BSQ #5000
Implement Segwit for BSQ #5000
Conversation
|
Ah great to see work on that! I will have a look soon into it...
I assume some dev has created that tx. With the release code base its should not be possible to create a BSQ tx with segwit input. |
|
I reviewed the current state. Looks all good to me. Will do a more in depth review once its out of draft state. |
stejbac
force-pushed
the
implement-segwit-for-bsq
branch
from
5e14ee6
to
5611905
Compare
stejbac
force-pushed
the
implement-segwit-for-bsq
branch
from
e1b68da
to
d92867b
Compare
|
I'm thinking it may be better to put the actual BSQ wallet changes in a separate PR, so that anyone running a full DAO node has a chance to upgrade to a Bisq version including these RPC changes first, before segwit BSQ txs start appearing which would otherwise break their consensus with the rest of the network. (So the above changes should now be ready for review.) |
|
Have you done a full DAO rebuild from genesis with that branch and check if the hashes in the monitor are the same as those from current seeds? It takes a while, probably several hours or so.... Have you seen my ealrier commnet at a review? ("Wouldn't it be better to add that to the constructor so the TxInput is immutable?") |
|
I did a full DAO rebuild with an earlier version of the current branch. I should probably do another rebuild - I'll start that now. TxInput should be immutable in the current version. There was an experimental |
|
I've finished a DAO rebuild from genesis with the current branch and the hashes are still in agreement with the seed nodes. |
|
@stejbac I think it would be more safe if we fork the jsonrpc4j libray to our own repo and use that. What do you think? |
|
@stejbac Could you please resolve the conflict? I hope it will trigger Travis as well (seems to be stuck somehow) |
|
I resolved the conflict from the web interface this time. (Perhaps it would have been better to do a rebase - I'm not sure.) |
core/src/main/java/bisq/core/dao/node/full/rpc/dto/RawBlock.java
Outdated
Show resolved
Hide resolved
|
@stejbac It would be great for this PR to get some information on how to test this best during release testing if it is getting merged for v1.5.5. Thanks! |
|
@ripcurlx I think it was not intended to get it into 1.5.5 but next release. |
Add 'witness_v1_taproot' script type to the enum and proto.pb, so that it doesn't cause any problems when Taproot is activated and the new script type starts showing up in RPC getBlock(..) responses (including possibly BSQ transactions). Also change the Java enum order (which shouldn't cause any problems as the ordinal isn't used directly in hashCode calculations) and add the missing 'witness_unknown' enum value to pb.proto to bring it in sync.
Create a new 'BitcoindClient' interface and a corresponding builder, to replace the old 'com.neemre.btcdcli4j.core.client.BtcdClientImpl' class from the btcdcli4j library. This is instantiated by jsonrpc4j using a dynamic proxy. It provides only a cut down version of the bitcoind RPC API, exposing the methods 'getblock', 'getblockcount' & 'getblockhash', as they are the only ones currently being used by RpcService. Add corresponding Jackson-annotated DTO classes to model the JSON structures returned by bitcoind, very similar to the classes provided by btcdcli4j. Note that we use Double instead of BigDecimal to represent fractional fields (difficulties + coin amounts in BTC), as they have more consistent Jackson (de)serialisation and appear to be able to faithfully round-trip numeric fields produced by bitcoind. Also note that doubles can faithfully represent any valid decimal BTC amount (that is, with 8 d.p. of precision) up to 21 million. For now, keep the old BtcdClientImpl instance used by RpcService in place, as the btcdcli4j block notification daemon is dependent upon it and would also need to be replaced. Also add unit tests for BitcoindClient which test against sample regtest responses, using a mock HttpURLConnection.
Provide a new 'BitcoindDaemon' block notification socket server, to replace 'com.neemre.btcdcli4j.daemon.BtcdDaemonImpl'. This starts a single service thread to listen for raw block hashes on localhost port 512*, sent by the specified 'blocknotify' shell/batch script, delegating to a pool of worker threads to run the supplied BlockListener handler. Unlike the original BtcdDaemonImpl class, a call to the 'getblock' RPC method is not made automatically to supply a complete block to the handler, instead requiring a separate, manual BitcoindClient.getBlock invocation from within RpcService. Also provide unit tests using a mock ServerSocket + Socket. TODO: Use the new Bitcoind(Client|Daemon) implementations in RpcService, in place of btcdcli4j Btcd(Client|Daemon)Impl & remove the old library.
Wrap any exception that occurs during socket IO or within the supplied BlockListener with a new 'BlockNotificationException'. This brings the exception handling more in line with that of the old BtcdDaemonImpl and makes it easier to match them downstream in FullNode.handleError.
Migrate RpcService over to the new block notification daemon and RPC client based on jsonrpc4j. Drop in own DTO classes in place of the ones defined by btcd-cli4j and rename requestBtcBlock & addNewBtcBlockHandler to requestDtoBlock & addNewDtoBlockHandler respectively. Also remove now redundant filtering from the logback config and update grade-witness.
Factor out a new RpcService.extractPubKeyAsHex method, to take public keys from the inputs of the raw transactions returned by the RPC client, when building TxInput objects to incorporate into the DAO state. Enhance the method to additionally support segwit (P2WPKH & P2SH-P2WPKH) inputs (but only the first input for backwards compatibility - see code comment). Also fix a bug when handling non-SIGHASH_ALL input signatures. This will allow segwit BSQ to be used in proof-of-burn and issuance txs, which need a public key associated with the tx to establish ownership of it, when signing messages with a proof-of-burn or staking merit awarded from a compensation issuance, respectively. Also add unit tests for the factored-out method and add a missing RawTx toString() method, to aid debugging the TxInput fields within the processed block returned by RpcService.
Factor out shared construction logic to a new 'getBlockFromRawDtoBlock' method in RpcService. Also add some 'NOPMD' comments in an attempt to suppress unfixable Codacy warnings about qualified imports.
Change jsonrpc4j version from 1.5.3 to 1.6.0.bisq.1, forked to the Bisq repo from the recent 1.6.0 release. The forked version changes the class 'com.googlecode.jsonrpc4j.HttpException' to be public, instead of (prob. mistakenly) package private, so we can avoid using reflection to catch it and re-throw as a 'bisq.network.http.HttpException'. Remove the now unused constructors from the latter. As part of this, upgrade Jackson to the latest stable (2.12.1) release, since jsonrpc4j now depends on a newer version than the previous 2.8.10.
|
@stejbac Once you remove the draft tag I can merge this PR |
|
@sqrrm Lets coordinate before merge, we need to test that very well before it goes into master, also considering potential fork risks. Probably best to test it in a dedicated dev branch and only merge quickly before release to reduce time span where master and release have a difference. |
|
@chimp1984 that sounds reasonable. I'm still not able to review this PR but I will do when I'm back in business. |
|
Activation date is @stejbac What would you think? |
|
@stejbac @ripcurlx @sqrrm @jmacxx Thanks @stejbac for the great work! @ripcurlx Can you arrange the compensation from the remaining funds from the Segwit sponsors? It was agreed that @stejbac will get what remains for that Segwit BSQ implementaion. |
|
I think 4 weeks after the 1.5.7 release would be reasonable. Assuming the release is around a week from now, that would place the fork activation in the middle of the Cycle 23 proposal phase, away from the critical periods. What about the BSQ wallet changes in #5109? Perhaps it would be OK if that PR is merged only into master after the release but before the fork activation, but if #5109 is merged into the release before the fork activation then users will start generating segwit-only BSQ UXTOs, which could get selected as proposal tx inputs. So some DAO participants may encounter problems if they try to make compensation requests early in Cycle 23, as any segwit BSQ tx inputs will be missing the necessary |
|
Ah yes, good point regarding the wallet PR. To me it seems it has to be held back as long the activation time is not triggered. Actually merging this PR asap with short term activation time should be ok as it does not represent much risk. All BSQ txs are still legacy we only support the new RPC and detect pubkeys from segwit txs. After a few weeks when most users have updated we can release #5109 so that segwit addresses will be used. |
|
One possible issue with using a short activation time is that anyone running a DAO full node will have monitor hashes out of consensus with the rest of the network if they don't upgrade before the activation date, even though there would be no segwit BSQ txs yet. That's because the segwit BTC (non-BSQ) inputs will also start having nonnull |
|
Ah ok. So lets play safe with sufficient time for activation and then as second step the wallet release. |
|
General comms being made to prepare users? |
Push back the (2nd) DAO hard fork activation block height to 680300, which is 4 weeks after the planned 1.6.0 release around 2021/03/25. (Also push back the testnet block activation height to 1943000 - 2 weeks from now assuming an average block time of 10 minutes.)
|
@stejbac As I'm preparing everything for the v1.6.0 pre-release right now. Is it really necessary that all seednodes are already updated before the pre-release as well? As the activation date is in the future and we don't ship the other Wallet PR yet, it looks to me as if this comment by @chimp1984 is related to the state where this PR contained also the Wallet code, am I wrong? |
|
I don't think it's strictly necessary for the seed nodes to be updated at this point (but they should obviously be updated before the hard fork activation date). There shouldn't be any change in the network monitor hashes or any other loss of consensus before that date, unless there is a bug in the code - I didn't spot any such issues during testing on mainnet a month or two ago. I'll run another full DAO rebuild shortly to double check. |
|
My full DAO node (running against Bitcoin Core 0.21.0) is still in consensus with the seed nodes after doing a rebuild from genesis. (I'm not sure if there has been an issue with any of the seed nodes recently, though, as my lite node fell out of consensus with them a few days ago for some reason - it wasn't running any of the 1.6.0 changes at the time.) The current DAO state hash (from my full node) is 44d66d64... @ block height 676797. The proposal and blind vote hashes are also in sync with the seed nodes. |
|
I can confirm the DAO state hash on my local lite node. |
|
@chimp1984 @stejbac how should mempool.space display bech32 addresses? currently we add |
|
I think the Bisq UI will handle it the same (B prefix) |
This PR attempts to add support for native Segwit (p2wpkh) BSQ addresses.
The btcd-cli4j Json-RPC interface for bitcoind (which is used by DAO full nodes) is replaced, as that code appears to be unmaintained for about 5 years and only maintained compatibility up to bitcoind-0.10.3. In particular, it does not support the extra Segwit fields in the
getblockJson responses returned to the Java client, including thetxinwitnessfield of the raw tx inputs. These are needed to fill in theTxInput.pubKeyfields of the inputs of each BSQ tx that makes up the DAO state, used in turn to validate staked merit (from issuance txs) and messages signed for a proof-of-burn tx. Thus the RPC client needs upgrading for us to support compensation and proof-of-burn txs with native Segwit BSQ inputs.A new, home-grown RPC client is provided using the jsonrpc4j library. Additionally, a simple home-grown socket daemon is provided to receive block hashes sent by the blocknotify script run by bitcoind, replacing the btcd-cli4j version.
getnetworkinfomessage to bitcoind & check the version. [edit: now done]TxInput.pubKeyfield in the DAO state). I'm not sure yet if there are any other consensus issues. [edit: now done][edit: should be finished - created a separate PR Wallet changes for Segwit BSQ implementation #5109 for the changes]