8eefb4d68c unused and missing vars in jmbitcoin tx code (Adam Gibson)
Pull request description:
Just noticed these while reading the code for some reason or other.
The `btc.OP_RETURN` was presumably not fixed before because that code (burn addresses) is now entirely unused.
However, this should be in the tests somewhere. We are missing tests for a lot of the functions in `secp256k1_transaction`. A PR should be opened by someone at some point to add those.
ACKs for top commit:
kristapsk:
ACK 8eefb4d68c
Tree-SHA512: 6cd7f959c1f798f14536a17c672a7ededbb4f962183aed17cba46a81955d91f5499bbd550a2de7bf337ed81660d55d00ea2f8b8cb0936c905f49a14ad405a6b9
Fixes#1214.
Prior to this commit, if a broadcast of a transaction failed locally, or
failed after attempting remote broadcast then falling back to local via
the function jmclient.taker.Taker.handle_unbroadcast_transaction, then
the version of the on_finished_callback found in
jmclient.wallet_rpc.JMWalletDaemon incorrectly asserted that the fromtx
keyword argument must be True, thus failing to update the state of the
daemon to CJ_NOT_RUNNING.
After this commit that assertion is removed, and the coinjoin_in_process
value returned over the API is correctly False after either a
successful, or an unsuccessful, finishing of the coinjoin.
Fixes#1213.
Prior to this commit, if the YieldGeneratorService were started via a
call to /maker/start over the RPC-API, the early check of whether coins
were present in the wallet to allow starting the maker, incorrectly read
the contents of the dict returned by
jmclient.wallet.UTXOManager.get_balance_by_mixdepth, which returns
entries for mixdepths with coins which are all frozen, with a total
value of zero, even if the include_disabled keyword argument is set to
False. Thus only checking for whether the length of the returned dict
was zero was incorrect.
After this commit, we will stop early, as intended, for a wallet which
has no mixdepths with any non-frozen coins.
Fixes#1212.
Before this commit, transactions were kept monitored in
WalletService.transaction_monitor after first being seen, only if the
list of confirmed callbacks was non empty, but this meant that the logic
is process_new_tx was not executed, meaning the height field of the utxo
was never updated from INF_HEIGHT, so the utxo would continue to be seen
as having zero confirmations, even after blocks are mined.
After this commit, we add any transaction that was seen to the
active_txs dict, so that the height field, and thus the number of
confirmations reported, is correct even if the caller never registered a
confirmed callback.
2692f08d3b Update local Tor config (Kristaps Kaupe)
Pull request description:
* Change loglevel to "warn" to have less verbose output (it still outputs some "notice" level messages for me but less than without this).
* Add [stream isolation](https://github.com/JoinMarket-Org/joinmarket-clientserver/issues/583).
```
IsolateDestPort Don’t share circuits with streams targeting a different destination port.
IsolateDestAddr Don’t share circuits with streams targeting a different destination address.
```
* Cookie authentication.
Top commit has no ACKs.
Tree-SHA512: d006e541d250418656b7933ee2537b16897b449ae9418f3b7f2c1521bd964048d7ef62ed5cc3098c4d0f4aea4eb0af680c9e23ed9d18cb310c1bd5acfefdf58c
The former was a stale test that we never got to a properly
working state, so we replace it with test/e2e-coinjoin-test.py
which does the same job more effectively, using the RPC-API.
Also removed the old file from parameters of run_tests script.
8d7d82f15c Add local Tor autostart to missing scripts (Kristaps Kaupe)
Pull request description:
Some scripts were missed in #1061.
Top commit has no ACKs.
Tree-SHA512: ae6a87295c5d771cc44b8e78d05f20917ddaa7d59a5267811e0e04d4619c101efcb9cb55ed392411c126d1d82ded1ab32f2e668746a3663c0af5d19ce1807182
704ffcfb7e docs: Add required port for directory_nodes in doc (laanwj)
Pull request description:
`:80` needs to be provided otherwise parsing will fail with
Failed to load directory nodes: InvalidLocationStringError('3kxw6lf5vf6y26emzwgibzhrzhmhqiw6ekrek3nqfjjmhwznb2moonad.onion')
This is correct in the auto-generated configuration file, but not in the document.
Also mention that even for UNIX sockets, it's necessary to specify `tor_control_port`.
ACKs for top commit:
kristapsk:
ACK 704ffcfb7e. Checked that doc changes describes current reality.
Tree-SHA512: 32c41e0e0136c7d84f6fe988befbbf3e0ebcae0fa5f5d790c32c73e06c0b47296cfdacbb97fba4479c84c69bda436fa7382d01fb70dbfd1a00cf9d9c0653f6de
`:80` needs to be provided otherwise parsing will fail with
Failed to load directory nodes: InvalidLocationStringError('3kxw6lf5vf6y26emzwgibzhrzhmhqiw6ekrek3nqfjjmhwznb2moonad.onion')
This is correct in the auto-generated configuration file, but not in the
document.
Also mention that even for UNIX sockets, it's necessary to specify `tor_control_port`.
Original testing setup of onion message channels just sent messages on
the first directory node peer in our list. This fixes that TODO.
After this commit, we keep a map of which directory nodes we have seen a
given Joinmarket nick on, and we keep track of whether those directory
nodes are available (connected) or not. Then when we want to privmsg
that nick, if we do not currently have a direct connection, we choose
randomly from the set of directory nodes on which that nick has been
seen, and which are currently live/connected.
Prior to this commit, a taker bot that received the orderbook on the
onion message channel would attempt to connect to every maker bot that
had sent an offer, which is not just inefficient but appears to trigger
rate limiting in Tor itself, throwing errors in the SOCKS proxy, at
numbers of around 80 makers, in testing (but could easily happen at
lower levels).
After this commit, we only opportunistically attempt to connect to
makers (using the same OnionPeer.try_to_connect method as before) when
we have already decided to send them a message (e.g. chosen to fill
their offer). This is done by calling try_to_connect only in
OnionMessageChannel._privmsg, and only to the right kind of peer.
The downside of this is that we will send more messages via directory
(at least one additional, for each maker), but clearly the alternative
is not viable.
As part of this update, we ensure that connection attempts are not
accidentally duplicated (see OnionPeer.connecting).
Prior to this commit, running ob-watcher with
an onion-type message channel configured would result
in the bot attempting to connect to all the makers,
which is bad. This happened because the internal logic
of the onion message channel is that receivers of privmsgs
will be sent peer info from the directory, so that they can
immediately respond p2p if they succeed in outward connecting.
But for bots who do not intend to engage in a coinjoin interactive
protocol, like ob-watchers, this is absolutely not the desired outcome.
After this commit, a bot can specify mode "PASSIVE" in the call to
get_mchannels(), which results in the OnionMessageChannel object only
creating non-directory remote peer objects of type OnionPeerPassive,
instead of OnionPeer, which means they never try to connect to those
remote peers.
4e720406fe Abort with error if descriptor wallet configured in rpc_wallet_file (Kristaps Kaupe)
90ec479422 Document wallet creation for old Core versions (Kristaps Kaupe)
44b61a1162 Always use legacy Core wallet in tests (Kristaps Kaupe)
2bca836569 doc: Document use of legacy wallet in USAGE.md (W. J. van der Laan)
Pull request description:
This is the same as #1063, I wasn't allowed to add extra commits to that one. Original PR description:
Mention the creation of a legacy wallet in USAGE.md. This is currently important because of the use of `importmulti` RPC which doesn't exist for the new descriptor wallets.
Top commit has no ACKs.
Tree-SHA512: ec1f7d03117e6fc980b70921a33bb2d6865978c91fa2622919241933506f962576558d0cbb5375b494283dd9bf3fae7f2fdfa6bd4539552183a3bf4a0875b936
Also, exports JMMakerClientProtocol for custom directory node scripts
(stored in the custom-scripts repo).
Modify default config with 2 signet and mainnet directory nodes to
start.
Handles unreachable directory nodes with a human readable error and
adjusts connection timeouts to be realistic.
Changes wording in Qt notifications from "IRC" to message channel.
Updates docs, new directory node information.
Mention the creation of a legacy wallet in USAGE.md. This is currently
important because of the use of `importmulti` RPC which doesn't exist for
the new descriptor wallets.
5132342f57 update install.sh to build tor on macOS (jules23)
7a88781648 Add support to build and autostart local Tor instance in jmvenv (Kristaps Kaupe)
Pull request description:
See https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1000#issuecomment-957379435 for context.
This adds new option `--with-local-tor` to `install.sh` which builds Tor locally inside jmvenv. Then, when starting JM scripts that might need Tor connection, it is started automatically if nobody already listens on `127.0.0.1:9050`.
If no problems are found with his approach in testing, we could switch to this as a default and remove all clearnet IRC configs from default config.
Top commit has no ACKs.
Tree-SHA512: caa7fcac88e57321f65a3f9fc09def94f9659855977026a19e1ac8f2f160a01237fe90220b986561bd622b05534f201ea5a29a9cdbc71013e40301992ed59e4d
07cdd3a8d7 add twitter joinmarket onion link to readme (unofficial bisq contributor)
Pull request description:
since twitter got official onion now, link to it too
Top commit has no ACKs.
Tree-SHA512: 663a693a0365c91c3feb1759e6644610b66f52c14fed0404cfb67bd93d12c39d4a582ea70d2bb739e0bb619d4d9d6e4e48d39bc4a709d0538891ff2de18caeee
In the previous commit, all peers served an onion.
After this commit, taker client instances will automatically
send a config var to the jmdaemon backend that instructs
the OnionMessageChannel instance to not start an onion service,
and the handshake messages sent by these peers replace the
onion location with a placeholder string NOT-SERVING-ONION.
Directories and maker peers will not therefore to connect outbound
to them, but privmsging still happens p2p with connections from
takers to makers after the directory has communicated their
reachable .onion addresses.
This change reduces the configuration requirement for takers and
is better for their privacy and security (without sacrificing
the gain we get from having p2p connections).
The above comments re: takers also apply to ob-watcher bots.
This commit also fixes a large number of minor bugs and errors in
documentation, as well as many Python cleanups after review from
@PulpCattel. A few concrete items are:
It fixes the ob-watcher functionality to work with the new subclass
of MessageChannel (OnionMessageChannel).
It corrects the on_nick_leave trigger to make dynamic nick switching
between MessageChannels (as implemented in MessageChannelCollection)
work correctly.
It corrects the order of events in the add_peer workflow to ensure that
a handshake can always be sent so that the activation of the connection
always works.
It sets a default messaging config with onion, 2 active IRC servers and
one inactive IRC server. The onion config has 2 signet directory nodes,
so this will change to mainnet after the PR is merged to master.
Joinmarket bots run their own onion services allowing inbound connections.
Both takers and makers connect to other makers at the mentioned
onion services, over Tor.
Directory nodes run persistent onion services allowing peers to
find other (maker) peers to connect to, and also forwarding
messages where necessary.
This is implemented as an alternative to IRC, i.e. a new
implementation of the abstract class MessageChannel, in onionmc.py.
Note that using both this *and* IRC servers is supported; Joinmarket
supports multiple, redundant different communication methods,
simultaneously.
Messaging is done with a derived class of twisted's LineReceiver,
and there is an additional layer of syntax, similar to but not the
same as the IRC syntax for ensuring that messages are passed with
the same J5.. nick as is used on IRC. This allows us to keep the
message signing logic the same as before. As well as Joinmarket line
messages, we use additional control messages to communicate peer lists,
and to manage connections.
Peers which send messages not conforming to the syntax are dropped.
See https://github.com/JoinMarket-Org/JoinMarket-Docs/pull/12 for
documentation of the syntax.
Connections to directory nodes are robust as for IRC servers, in
that we use a ReconnectingClientFactory to keep trying to re-establish
broken connections with exponential backoff. Connections to maker
peers do not require this feature, as they will often disconnect
in normal operation.
Multiple directory nodes can and should be configured by bots.
* Migrate callbacks registered under provisional (tx-output tuples) keys
as soon as their txids are known. Leave a txid breadcrumb in place so
register_callbacks and check_callback_called can find it and the
migrated list of callbacks.
* Invoke callbacks via list comprehensions, retaining only the callbacks
that return False. The old code was buggy, as it was removing elements
from the callback lists while iterating over them, which would cause
callbacks to be skipped.
* The existing code would fail to call any "confirmed" callbacks for a
remove-only transaction if no "unconfirmed" callbacks had been
registered for that transaction, and it would discontinue monitoring
of a transaction after just one "confirmed" callback had returned
True, even if other "confirmed" callbacks returned False to remain
registered. This commit overhauls the logic to fix all of these bugs.
* Delete emptied callback lists from the dicts to be tidy.
The transaction_monitor function in WalletService was polling the most
recent 100 transactions from the BlockchainInterface every 5 seconds.
This was egregious and inefficient. Introduce a _yield_new_transactions
function that remembers the newest previously yielded transaction
between invocations and yields only transactions newer than that one.
Use the new function in transaction_monitor.
Avoid repeatedly deserializing confirmed transactions that remain in the
set of monitored transactions after being confirmed either due to
"confirmed" callbacks that return False or because callbacks have timed
out and have been unregistered without cleaning the set of monitored
transactions. Change the active_txids list to an active_txs dict whose
keys are the txids and whose values are the deserialized transactions.
Use this new dict as a cache to avoid repeatedly deserializing confirmed
transactions.
Alter the BitcoinCoreInterface._yield_transactions generator to be
nicer in cases where only a small number of transactions need to be
examined. Geometrically ramp the requested number of transactions in
each successive request, starting from 1. Since new transactions can
appear in the Bitcoin Core wallet between RPC calls, overlap successive
request ranges and resume yielding transactions after finding the
previously yielded transaction to avoid yielding duplicates.
The listtransactions RPC of Bitcoin Core can return the same (txid,vout)
as both a "category":"receive" and a "category":"send". Therefore, use
(txid,vout,category) as the sync key to ensure that all distinct
elements are yielded.
Delete the unused wallet_name parameter from _yield_transactions, per
suggestion by Adam Gibson.
Kristaps Kaupe
Adam Gibson
laanwj
bisqubutor
W. J. van der Laan
jules23
Jaime
Adam Gibson
Matt Whitlock