6a8149fe96 Removes utxo field from non-receiver inputs (Adam Gibson)
Pull request description:
Prior to this commit, the payjoin receiver code
was signing a PSBT containing the utxo field
for every input, including the ones it did not
own, and transferring this to the sender.
However BIP78 specifies that, for inputs belonging
to the sender, no utxo field should be included.
This is corrected in this commit.
ACKs for top commit:
kristapsk:
Tested this with BTCPay Server and it succeeded! ACK 6a8149fe96
Tree-SHA512: df5b8770ef9398ede622a4584edc75d3da2665936136b2705c0b72ba108d6268c20e238cb635a97799e1279d468fcd94d47c0b4d87af5e26c4f2790b952f68bc
The handle_unbroadcast_transaction fallback in Taker
is useful because Makers may not cooperate in broadcasting
a transaction, but it is only appropriate for the
random-peer option of `tx_broadcast`; for not-self the
user has chosen to avoid using their own IP, so this
commit simply abandons the attempt to broadcast the
transaction in that case.
The comments added to the config emphasize the problematic
aspect of this, which is that the tx may need to be
manually broadcast via another channel.
Prior to this commit, the payjoin receiver code
was signing a PSBT containing the utxo field
for every input, including the ones it did not
own, and transferring this to the sender.
However BIP78 specifies that, for inputs belonging
to the sender, no utxo field should be included.
This is corrected in this commit.
Also, ensure witness_utxo field is populated,
plus minor bugfixes related to presence of
NONWITNESS_UTXO field in provided payment PSBT.
Tested as being functional either with or without
NONWITNESS_UTXO field, for all-segwit inputs.
03075a047b Fix libsecp256k1 build on FreeBSD (Kristaps Kaupe)
Pull request description:
Fixes#697.
Top commit has no ACKs.
Tree-SHA512: ae93a22c9a34270c31c60384902ba41271b9c81562345b75797b43a0c313d8fb5648693b88c9052e8ab2bae8b8065c9b39fc1f398557e3a88b1418385ee57eb2
Also manually fire order creation in coinjoin tests.
This clarification and test change is required due
to the fact that LoopingCalls are designed to fire
immediately by default, before the reactor is
initialized (and therefore in a `running` state),
making it not possible to shutdown the reactor as
a result of events happening in that first call;
so we delay the first call of the maker's orderbook
populating code, so that if a no-coins error
occurs, it will actually shut down the reactor and
hence the whole yield generator program, as intended.
Prior to this commit, in case an RPC failure occurred when
accesing the block height, the program would continue but the
wallet would be in an un-writeable state (for command line
programs, specifically yield generators; for Qt the shutdown
would occur).
This commit slightly cleans up the process of shutting down,
ensuring that duplicate shutdown calls do not result in
stack traces. It also ensures that also for command line
programs, the application will immediately shutdown if the
regular heartbeat call to query the block height fails, as this
risks inconsistencies in the wallet (though the previous
situation luckily did not result in this as the call to
BaseWallet.close() resulted in the wallet being read only).
A future PR should develop a more sophisticated approach to
RPC call failures that may allow the program to wait.
stopservice
Prior to this commit, the cancel button remained
activated when the BIP78 payjoin processing had
completed, either successfully or unsuccessfully
which could be confusing for the user.
After this commit, when the processing is complete,
the JMBIP78ReceiverManager object fires the shutdown
callback, which Qt uses to signal the dialog, which
then updates to disable the Cancel button and show
the Close button.
Additionally, line breaks were added to make tooltips
more readable.
This commit implements a command line script and a GUI
dialog to receive a payment using the BIP78 protocol,
by setting up an ephemeral hidden service.
It also deprecates the pre-existing inter-Joinmarket
protocol for payjoin payments, since we now have
both sending and receiving support for BIP78. Thus,
much code in Maker, Taker and client-daemon protocol
is removed, as is some documentation in docs/PAYJOIN.md.
Also the script `sendpayment.py` is altered to support
only the BIP78 variant.
The test in jmclient/test/test_payjoin now implements
BIP78 over a TCP connection, while the custom tests in
test/payjoinserver.py can support hidden service based
tests, but the latter is not included in the test suite
and may not always work (it is only for manual
investigations).
The following features of BIP78 are supported:
minfeerate
additionalfeeoutputindex - but *only* for single
change output transactions
maxadditionalfeecontribution
The receiver does not have nor request payment
output substitution.
Utxo selection is no longer sophisticated, instead
we only choose a single utxo to keep the size
increase of the transaction minimal. Thus UIH is
not addressed at the moment.
Errors returned are in line with BIP78.
Sequence numbers are checked by receiver, and
kept identical if uniform, otherwise respected.
Receiver uses transaction monitor to shut down
when the payment is seen.
The workflow is almost entirely implemented in
jmclient/payjoin.py and the command line script
is in scripts/receive-payjoin.py. The setup, including
configuration changes for Tor, are documented in
docs/PAYJOIN.md, including a user guide video linked.
Prior to this commit, if non-self broadcast was enabled
but the counterparty chosen did not broadcast, the transaction
would remain unbroadcast.
After this commit, the Taker checks, after the configured
value of TIMEOUT.unconfirm_timeout_sec (default 90s), the
Taker will broadcast the transaction.
Also amended default config comment for this function.
5c85a3d979 Log sendrawtransaction errors as warning not debug (Kristaps Kaupe)
Pull request description:
Before this change `sendrawtransaction` errors in `pushtx()` are logged as debug, not warning. But default `console_log_level` is `INFO`. This is important enough information for the user to change log level here.
Before:
```
Would you like to push to the network? (y/n):y
2020-09-09 23:54:26,199 [ERROR] Transaction broadcast failed!
```
After:
```
Would you like to push to the network? (y/n):y
2020-09-09 23:54:26,198 [WARNING] error pushing = -26 dust (code 64)
2020-09-09 23:54:26,199 [ERROR] Transaction broadcast failed!
```
Somewhat related to #600.
Top commit has no ACKs.
Tree-SHA512: 0148e38ec7ae32156c02db76e52ce26920c2815665ced9b7779c1543adb66beead02b32d8891149f848dcd0bd9ff0b6a9a08c22ebfec55b6ad2ace026e1c9b58
0e5eb2f3b1 More verbose absurd fee exception message (Kristaps Kaupe)
Pull request description:
Display both sat/vkB and sat/vB values as we already do in other places, otherwise units may cause confusion for new users, and output both absurd fee value as well as estimated fee rate. This may help in supporting other users via Telegram, IRC, etc.
Top commit has no ACKs.
Tree-SHA512: 4c4c67399f8c427bd7e9aef71fbe6b7f748855b1163f97daa665c689e39be3bf33202ec44a4e2678f81ebb5c17d8b1a47524d86ea68410c096c39e98b0fda028
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.
a70329126b Bump coveragepy to fix pytest on python 3.8 (Alessio Caiazza)
Pull request description:
Python 3.8 was not supported before coveragepy v5.0a4
https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst#version-50a4-----2018-11-25
The bump is big, but tests are green
ACKs for top commit:
jules23:
TestedACK a703291 I am now able to run the test suite with Python 3.8.5. Thanks for taking care of this!
kristapsk:
ACK a70329126b. Tested that it fixes tests with Python 3.8.2 and didn't break anything with Python 3.6.10.
Tree-SHA512: 97c2c17f79943d0d3fb1ff50051712a7116bec4769672081f5d270c4b868c083c0c9c99fdccbf40d43f1650d9dc092157414a3aa1c6c375aaaa5220cac8adf7c
Kristaps Kaupe
Adam Gibson
jleo84
nixbitcoin
Tim Akinbo
Alessio Caiazza