python-bitcointx backend for jmbitcoin + bip78 and snicker. #536
Conversation
|
Summary for reviewers: MotivationWe need the bitcoin code underlying the project to be robust and flexible, to support future new transaction types (including not just native segwit, which was already supported, but to support future segwit v1 and multisig of various types (might be potentially important for coinjoinxt or variants etc), and things like psbt). We also want it to support proper script verification, albeit that is not critical for basic coinjoin. The above explains the desire to move to
The other element of the motivation for this PR is touched on in the above - formatting mess. This specifically means mixed use of hex and binary across the codebase, but also the difficult to reason about serialization and deserialization routines in Also, not a completely trivial point, but a simple reduction in the amount of code is a motivation - Caveat
Implementation
|
|
Update on status: apart from a huge mess around testing, which actually has nothing to do with this PR, and which I'm working on fixing right now, the status here is:
If people want to do other testing that'd be appreciated. I realise that code review is a bit daunting but you could always just review a small part (I originally intended to separate this into multiple functionally separate commits but it proved impossible to make work). |
|
Rebased on master as of 73d1039 Qt tests are working so far but more will be done. |
|
Rebased on master again as of 2a10e86 |
|
PSBT support as of 8bf0346 ; commit notes tell most of the story. This is a beginning only, but it is still probably the bulk of the work in terms of what we need. I see the main goal as manual coinjoins, but it of course will also allow the SNICKER code to work, and also the Payjoin proposal to work, although for the latter in particular, I'm going to have to extend considerably to make sure that all input and output script types are functioning correctly. |
|
Now that the PSBT code is included, it was much cleaner and easier for me to implement SNICKER. The base code is in jmbitcoin, and the basic functionality (create a half-signed proposal, receive a proposal and complete) is now in a |
|
381423b supports the first element of the PayJoin workflow: the direct_send function can now optionally output a PSBT of the created transaction (without broadcasting, of course), instead of a raw transaction and broadcast. |
If such thing is added for coinjoins too, this will make #414 obsolete and can potentially be useful in future for LN channel opening and other more complicated stuff. |
Yes. To be clear, I am being cautious in not adding the most obvious use-case yet: as you say, to create a partially signed coinjoin using the wallet, or co-sign an existing PSBT, because there are security concerns if you do that the wrong way. So I'll need to add a few checks before supporting that. But I don't mean that it's difficult, it will be fairly easy to add this. (For now the workflow is just: add |
For this I think we need to look what is latest "stable" in most popular Linux distributions and also macOS. For Gentoo, which I use, it's 3.7.7, so this is fine, but I'm minority here. Feedback from others on this would definitely no hurt. |
More on this - I actually read #414 again and reminded myself of the details. So ... for a single coinjoin, on the command line only for now (Qt later), I agree we could do this here. But is PSBT actually preferable for that use-case? I guess not, as you're developing essentially a testing workflow, so you just need the tx itself (considering that it is fully signed), to pass across via inter-process-communication, or manually, or whatever suits your test. The PSBT way of doing things will be more useful for manual coinjoin than this, right? Or did I miss something. |
|
@kristapsk so as of 9cc0333 I think we now have everything we need to write a BIP79++ payjoin script (and indeed on Qt). I'll review your #560 now and then we can start doing that. Once we have the basic script working we can start testing it against btcpayserver server-side implementation. |
|
See Simplexum/python-bitcointx#31 - this combined with 30350f0 (which may need some touchups later) allows automatic installation of a local libsecp256k1 library without touching the system installation (so no |
python-bitcointx may be opinionated in more than one sense. For example
openssl in python-bitcointx is optional, and used only in
Correct. PR welcome, though (IIUC that no extra dependencies is needed for that, if this is possible with libsecp256k1. Additive tweaks were obvious to do via libsecp, so I added them, but didn't look into multiplicative) |
|
I am currently attempting to run the test suite at d34c53b, and I am getting the error below. This also causes python to crash. My machine is mac os 10.15.5. It looks to me like this is a problem with loading the openssl library. @dgpv Adam told me this is something you might be able to help with? I do have openssl installed. stack tracejmbitcoin/test/test_tx_signing.py Fatal Python error: AbortedCurrent thread 0x00000001163c1dc0 (most recent call first): |
|
This is a known issue with OSX Catalina release: Simplexum/python-bitcointx#32 Homebrew/homebrew-core#44996 <-- (this issue in Homebrew has some workarounds that can be done on the host machine) This issue can occur in python-bitcointx if |
Without this flag set, the python-bitcointx Script verification will use openssl for non-strict encoding, which requires a libopenssl dependency. Moreover non- strict encoding is now out of consensus so is not needed for our purpose.
|
Thanks @dgpv , that's what I was hoping for :) Indeed we don't of course need the non-strict version of verify, so added the SCRIPT_VERIFY_STRICTENC flag as you mention. |
|
@Jules23 great that sounds perfect. i guess some simple bundling of the libsecp install in another shell script might fit the bill. So far we've just provided that 4 or 5 step recipe for Mac users, so I don't think another step like that will hurt (although clearly it'd be nicer if we had everything in a one-step runnable script like we do for Linux). |
|
@AdamISZ I can provide a script for the libsecp install. How do you want me to provide it? Branch off your pblwrap? |
|
Something like that sounds great, sure. |
|
ok! definitely i agree the end goal is a the one script like in linux. I'll be working towards that. |
|
Re-tested:
|
As per above comment, I'm doing the BIP78 changes now, I'll commit that new code once I get it passing on regtest and against Kukks' server. |
This is now tested as compatible with BIP78 as implemented by BTCPayServer. An additional config section [PAYJOIN] is added to manage settings for fee control in payjoin as described in the BIP. These settings are marked as advanced usage as they're rather complex for users to understand and the defaults should be very safe.
73b0edc Update macOS installation instructions (Jules Comte) Pull request description: This should be merged after #536, and is a followup to #629 It includes build instructions for libsecp256k1 for those who don't use `install.sh` @AdamISZ I was wrong, it turns out that `install.sh` does need some modifications to work on mac os from scratch. My biggest question at this point is whether I have the list of darwin dependencies correct in `install.sh: line 44` With the current list, I was able to build, and pass the testsuite on fresh installations of Mac OS Catalina with Apple Command Line Tools and Homebrew installed. Top commit has no ACKs. Tree-SHA512: f9feed9d6052cefd043a025212e27347669aa6a082a4543d2f19a64f74ceb51694fa33efb5a92b79f7fc15a70efead4ac32cee3ab78acb6badd894f8b616aede
Prior to this commit, users setting the POLICY config option `tx_broadcast` to anything other than `self` would cause a crash after the merge of #536 due to a bin/hex conversion failure (before this merge, the tx would simply fail to broadcast). This commit adds a `JMTXBroadcast` AMP command so that makers can send arbitrary transactions from daemon to client, for broadcast via the blockchain interface. This allows the existing code in `taker.push()` to function correctly, after fixing the bin/hex conversion bug. Hence users can now select `random-peer` or `not-self` and the transaction will be broadcast as expected according to the comments, and the WalletService will react to the broadcast just as it does currently for self-broadcast. Note that this change will be ineffective if the counterparties do not support it; the transaction will simply remain un-broadcast.
(See second comment for overview of what this is)
Replaces core transaction, address, serialization
and sign functionality for Bitcoin with
python-bitcointx backend.
Removes bech32 and btscript
modules from jmbitcoin. Removes all string,
hex, binary conversion routines. A generic
hex/binary conversion now is added to jmbase.
Removes all transaction serialization and
deserialization routines. Removes the now
irrelevant test modules.
Remaining functions in jmbitcoin remove any parsing of
hex format, requiring callers to use binary only.
One additional test added, testing the remaining
function in secp256k1_transaction.py: the signing
of transactions. Deserialized form is now
bitcointx.CMutableTransaction.
For jmbase, in addition to the above, generic conversions
for utxos to and from strings is added, and a dynamic conversion
for AMP messages to binary-only. Within the code, utxos are
now only in (binarytxid, int) form, except where converted
for communcation.
Tthe largest part of the changes are
the modifications to jmbitcoin calls in jmclient;
as well as different encapsulation with CMutableTransaction,
there is also a removal of some but not all hex parsing;
it remains for rpc calls to Core and for AMP message
parsing. Backwards compatibility must be ensured so some
joinmarket protocol messages still use hex, and it is
also preserved in persistence of PoDLE data.
As part of this, some significant simplification of
certain legacy functions within the wallet has been done.
jmdaemon is entirely unaltered (save for one test which
simulates jmclient code).