306: Fix logging and retrying of Monero transaction watching r=thomaseizinger a=thomaseizinger
Hopefully, this should also reduce the load because I am not asking the node every second.
Related: https://github.com/comit-network/xmr-btc-swap/issues/202
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Daniel Karzel <daniel@comit.network>
Instead, we use a regular loop and extract everything into a function
that can be independently tested.
`backoff` would be useful to retry the actual call to the node.
This config setting makes backoff stop retrying if we didn't get an
error within this timeframe.
For us, this results in backoff not actually doing anything.
The connection to kraken is very long-running. It might be active
for hours without failing. However, the default value for
`max_elapsed_time` is set to 15 minutes. As such, once the connection
fails any time after that, backoff doesn't actually retry the operation
but just gives up.
Fixes#303.
In order to be able to re-connect on certain errors, we model
connection errors separately from parsing errors. We also change
the API of the whole module to no longer forward all errors to
the subscribers but instead, only update the subscribers with
either a latest rate or a permanent failure in case we exhausted
all our options to re-connect the websocket.
To model all of this properly, we introduce to sub-modules so that
each submodule can have their own `Error` type.
Resolves#297.
First, we tell the user that we are now waiting for Alice to lock
the monero. Additionally, we tell them once we received the
transfer proof which will lead directly into the
"waiting for confirmations" function.
The type hints are generated from the field names. This has the
unfortunate consequence of the config field becoming file_path which
does not really make sense people working on the codebase.
The bitcoin::Wallet::sync_wallet function doesn't do anything else
other than delegating. As such, we have just as much information
about what went wrong inside this function as we have outside.
By moving the .context call into the function, we can avoid repeating
us on every call-site.
288: Switch to public stagenet node that works r=rishflab a=rishflab
The xmr.to node has been unreliable lately. The exan.tech node seems to
working.
@da-kami is following up with making this configurable. Lets get this in so we can get a release on Friday.
Co-authored-by: rishflab <rishflab@hotmail.com>
Instead of leaking the tokio::sync:⌚:Receiver type in our
return value, we create a newtype that implements the desired
interface. This allows us to get rid of the `RateService` structs
and instead implement `LatestRate` directly on top of this struct.
Given that `LatestRate` is only used within the event_loop module,
we move the definition of this type into there.
271: Bob can verify that the XMR lock tx was published r=da-kami a=da-kami
The Monero `txhash` log was removed. I feel the user should have the possibility to verify that the transaction was actually published so I added the tx-hash to the confirmation output.
We could potentially print the tx-hash when receiving the transfer proof already, but that might not add much value compared to printing it with the confirmations.
Additionally we should allow the user to at least know when the XMR can be expected in the user's wallet, otherwise the swap ends like this:
```
2021-03-04 13:49:19 INFO Monero lock tx received 5 out of 5 confirmations
```
This is just not very informative - yes, the final transaction is an implementation detail, but I don't think we should hide the transactions from the user. By printing the tx-hash for spending from the lock-tx into the user wallet we ensure the user knows that the XMR can now be expected in the user wallet.
---
To add context, here the complete log (with debug enabled) **before** this change:
```
2021-03-04 13:30:46 DEBUG Database and seed will be stored in /Users/dakami/Library/Application Support/xmr-btc-swap
2021-03-04 13:30:46 DEBUG Starting monero-wallet-rpc on port 56145
2021-03-04 13:30:51 DEBUG Requesting quote
2021-03-04 13:30:51 INFO Received quote: 1 XMR = 0.00433500 BTC
2021-03-04 13:30:51 INFO Still got 0.01018746 BTC left in wallet, swapping ...
2021-03-04 13:30:51 INFO Spot price for 0.00500000 BTC is 1.153402537485 XMR
2021-03-04 13:30:52 DEBUG Starting execution setup with 12D3KooWCdMKjesXMJz1SiZ7HgotrxuqhQJbP5sgBm2BwP1cqThi
2021-03-04 13:30:55 INFO Published Bitcoin 3a6690a962191529892318819fb20e7f1ac4625400e64ee734056a9b2a17ad8f transaction as lock
2021-03-04 13:41:13 DEBUG Received Transfer Proof from 12D3KooWCdMKjesXMJz1SiZ7HgotrxuqhQJbP5sgBm2BwP1cqThi
2021-03-04 13:42:11 INFO Monero lock tx received 1 out of 5 confirmations
2021-03-04 13:45:33 INFO Monero lock tx received 2 out of 5 confirmations
2021-03-04 13:47:49 INFO Monero lock tx received 3 out of 5 confirmations
2021-03-04 13:48:56 INFO Monero lock tx received 4 out of 5 confirmations
2021-03-04 13:49:19 INFO Monero lock tx received 5 out of 5 confirmations
2021-03-04 13:49:19 DEBUG Encrypted signature sent
2021-03-04 13:49:19 DEBUG Alice acknowledged encrypted signature
2021-03-04 13:49:19 DEBUG watching for tx: e5569d3f0bcccac95252dffaebe74ead0360c09b76bc762de890aaa0e51afbcf
2021-03-04 13:49:20 DEBUG Received protocol error "missing transaction" from Electrum, retrying...
2021-03-04 13:49:22 DEBUG Received protocol error "missing transaction" from Electrum, retrying...
```
Co-authored-by: Daniel Karzel <daniel@comit.network>
Print tx-hashes for monero transactions to allow Bob to look the transaction up in block explorer.
The story of Bab:
Our famous actor Bob has a brother named Bab.
In school they were often mixed up, because their names were so similar.
Eventually Bab renamed himself into Barbara, but that was even more confusing for now he
carried a female name even though he was not female. Bob wanted to help his brother and told him he
could just go for Bub. But that did not solve anything. Fun fact: Bub is actually married to Alice.
Previously, the user neither knew the price nor the maximum quantity
they could trade. We now request a quote from the user and display
it to them.
Fixes#255.
This reduces the overall amount of LoC that imports take up in our
codebase by almost 100.
It also makes merge-conflicts less likely because there is less
grouping together of imports that may lead to layout changes which
in turn can cause merge conflicts.
265: Replace quote with spot-price protocol r=thomaseizinger a=thomaseizinger
This is essentially functionally equivalent but includes some
cleanups by removing a layer of abstraction: `spot_price::Behaviour`
is now just a type-alias for a request-response behaviour.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
261: Sweep xmr funds from generated temp wallet r=da-kami a=da-kami
Fixes#252
Please review by commit :)
Did a few cleanups before actually doing the feature.
Please note the comment that influenced this solution: https://github.com/comit-network/xmr-btc-swap/issues/252#issuecomment-789387074
Co-authored-by: Daniel Karzel <daniel@comit.network>
This is essentially functionally equivalent but includes some
cleanups by removing a layer of abstraction: `spot_price::Behaviour`
is now just a type-alias for a request-response behaviour.
Container initialization and wallet initialization have to ensure to use the same wallet name.
In order to avoid problems constants are introduced to ensure we use the same wallet name.
Prefixing docker-containers and -networks is a necessity to be able to spin up multiple containers and networks.
However, there is no reason to prefix the wallet names that live inside a container. One cannot add a wallet with
the same name twice, so the prefixing of wallets does not bring any advantage. When re-opening a wallet by name
the wallet name prefix is cumbersome and was thus removed.
The wallet is an instance of a wallet that has a name.
When we use `CreateWalletForOutputThenReloadWallet` we actually unload the wallet.
It would be cleaner to create a new instance that does that swap, but I did not go that far.
Instead of instantiating the `EventLoop` within the builder, we only
pass in the necessary arguments (which is the `EventLoopHandle`) to
the Builder upon `new`.
This is work towards #255 which will require us to perform network
communication (which implies having the `EventLoop`) before starting
a swap.
If our expression directly evaluates to a future, we don't need to
create an async block.
This requires us to have `EventLoopRun::run` consume the instance
instead of just taking a mutable reference (otherwise we run into
lifetime issues). However, that is better anyway because `run` is
an endless loop so you never get to use the handle afterwards
anyway.
Previously we were ignoring if the monero wallet rpc was not found and
unpacked from archive leading to a failure down the line when trying to
run a non-existent executable. Bail when the executable is no found in
the archive.
1. We can generalize the signing interface by passing a PSBT in
instead of the `TxLock` transaction.
2. Knowing the transaction ID of a transaction that we are about
to sign is not very useful. Instead, it is much more useful to know
what failed. Hence we add a `.context` to the call of `sign_and_finalize`.
3. In case the signing succeeds, we will immediately broadcast it
afterwards. The new broadcasting interface will tell us that we broadcasted
the "lock" transaction.
We eliminate unnecessary layers of indirection for broadcasting logic
and force our callers to provide us with the `kind` of transaction
that we are publishing.
Eventually, we can replace this string with some type-system magic
we can derive the name from the actual transaction. For now, we just
require the caller to duplicate this information because it is faster
and good enough TM.
This struct is a wallet. The only thing it can meaningfully broadcast
are transactions. The fact that they have to be signed for that is
implied. You cannot broadcast unsigned transactions.
Abstracting over the individual bits of functionality of the wallet
does have its place, especially if one wants to keep a separation
of an abstract protocol library that other people can use with their
own wallets.
However, at the moment, the traits only cause unnecessary friction.
We can always add such abstraction layers again once we need them.
If the user doesn't pass `--debug`, we only show `INFO` logs but
without time and level to make it clearer that it is meant to be
read by the user.
Without `--debug`, the user sees:
Still got 0.00009235 BTC left in wallet, swapping ...
With `--debug`, they see:
2021-03-01 12:21:07 DEBUG Database and seed will be stored in /home/thomas/.local/share/xmr-btc-swap
2021-03-01 12:21:07 DEBUG Starting monero-wallet-rpc on port 40779
2021-03-01 12:21:11 INFO Still got 0.00009235 BTC left in wallet, swapping ...
2021-03-01 12:21:11 DEBUG Dialing alice at 12D3KooWCdMKjesXMJz1SiZ7HgotrxuqhQJbP5sgBm2BwP1cqThi
2021-03-01 12:21:12 DEBUG Requesting quote for 0.00008795 BTC
Previously, the time was formatted as ISO8601 timestamps which is
barely readable by humans. Activating the `chrono` feature allows
us to format with a different format string. The output now looks
like this:
2021-03-01 11:59:52 DEBUG Database and seed will be stored in /home/thomas/.local/share/xmr-btc-swap
2021-03-01 11:59:52 DEBUG Starting monero-wallet-rpc on port 40673
2021-03-01 11:59:59 DEBUG Still got 0.00009235 BTC left in wallet, swapping ...
2021-03-01 11:59:59 DEBUG Dialing alice at 12D3KooWCdMKjesXMJz1SiZ7HgotrxuqhQJbP5sgBm2BwP1cqThi
2021-03-01 11:59:59 DEBUG Requesting quote for 0.00008795 BTC
There is a double space after the time which is already fixed in
tracing-subscriber but not yet released.
See https://github.com/tokio-rs/tracing/issues/1271.
Log messages are ideally as close to the functionality they are talking about, otherwise we might end up repeating ourselves on several callsites or the log messages gets outdated if the behaviour changes.
These intermediate structs were creating unnecessary noise. The peer id
and multiaddr fields are going to be removed in the future further
reducing the need to have seperate structs for cancel, resume and
refund.
If communication with the other party fails the program should stop and the user should see the respective error.
Communication errors are handled in the event-loop. Upon a communication error the event loop is stopped.
Since the event loop is only stopped upon error the Result returned from the event loop is Infallible.
If one of the two futures, event loop and swap, finishes (success/failure) the other future should be stopped as well.
We use tokio::selec! to stop either future if the other stops.
Failure does not express what the error represents. It is only used for communication
errors for quote requests, receiving the XMR transfer proof and sending the encryption signature.
If the current balance is 0, we wait until the user deposits money
to the given address. After that, we simply swap the full balance.
Not only does this simplify the interface by removing a parameter,
but it also integrates the `deposit` command into the `buy-xmr`
command.
Syncing a wallet that is backed by electrum includes transactions
that are part of the mempool when computing the balance.
As such, waiting for a deposit is a very quick action because it
allows us to build our lock transaction on top of the yet to be
confirmed deposit transactions.
This patch introduces another function to the `bitcoin::Wallet` that
relies on the currently statically encoded fee rate. To make sure
future developers don't forget to adjust both, we extract a function
that "selects" a fee rate and return the constant from there.
Fixes#196.
These traits were only used once within the `TxLock` constructor.
Looking at the rest of the codebase, we don't really seem to follow
any abstractions here where the protocol shouldn't know about the
exact types that is being passed in.
As such, these types are just noise and might as well be removed in
favor of simplicity.
The only reason we need this argument is because we need to access
the output descriptor. We can save that one ahead of time at when
we construct the type.
BDK already has a log line for the sync that we could enable if we
wanted such a log.
Additionally, _we_ are not actually syncing the wallet, bdk is so our
log line was lying. It should have said "calling bdk to sync wallet".
231: Error only on close message when fetching the rate r=thomaseizinger a=da-kami
Ping/Pong messages disturb the rate requests quite frequently resulting in failed swap setup because there is no rate available.
As a result messages Ping, Pong and Binary are now ignored and not reported as error.
Co-authored-by: Daniel Karzel <daniel@comit.network>
If the monero wallet rpc has not already been downloaded we download the monero cli package and extract the wallet rpc. The unneeded files are cleaned up. The monero wallet rpc is started on a random port which is provided to the swap cli.
We added a fork of tokio-tar via a git subtree because we needed a tokio-tar version that was compatible with tokio 1.0. Remove this subtree in favor of a regular cargo dependency when this PR merges: https://github.com/vorot93/tokio-tar/pull/3.
For transitioning to state4 we either go into a redeem or a cancellation scenario.
The function name state4 is misleading, because it is only used for cancellation scenarios.
This TDOO is misleading, because - to our current knowledge - it is impossible for
Bob to retrieve the exact inclusion block-height of the lock transaction (send by Alice).
The wallet RPC is only capable of retrieving the inclusion block height of a transaction
through `get_payments` and `get_bulk_payments` which requires the `payment_id`.
The `payment_id` can be retrieved through `get_transfer_by_txid` which states
"Show information about a transfer to/from this address." - however the address that the
transfer goes to is not part of Bob's wallet yet! Thus, it is impossible for Bob to use
`get_transfer_by_txid` which in turn means Bob is unable to use `get_payments`.
The only possible way for Bob to know the exact inclusion block/height of the lock transaction
would be if Alice sends it over to Bob. But for that Alice would have to extract it she would have
to wait for confirmation - which she currently does not and might never do. Even if she does await
the first confirmation before sending the transfer proof the solution for retrieving the inclusion
block-height is not fleshed out on her side yet.
In order to ensure that we can atomically generate_from_keys and then reload a wallet,
we have to wrap the client of the monero wallet RPC inside a mutex.
When introducing the Mutex I noticed that several inner RPC calls were leaking to the
swap crate monero wallet. As this is a violation of boundaries I introduced the traits
`GetAddress`, `WalletBlockHeight` and `Refresh`.
Note that the monero wallet could potentially know its own public view key and
public spend key. If we refactor the wallet to include this information upon wallet
creation we can also generate addresses using `monero::Address::standard`.
By updating `tracing_log`, we can access the re-export. That we need
to initialize the `tracing_log` adaptor.
The usage of `log::LevelFilter` for the `init_tracing` function was
conceptually incorrect. We should be using a type from the `tracing`
library here.
The automated swap backend (asb) requires Monero funds, because Alice is selling Monero.
We use a hardcoded default wallet named asb-wallet. This wallet is opened upon startup.
If the default wallet does not exist it will be created.
This allows us to use .context instead of .map_err when calling
`latest_rate()`. For the static rate module, we simply fill in
`Infallible` which is actually better suited because it describes
that we are never using this error.
Note that because we are using `watch` channel, only a reference to the
channel value can be returned.
Hence, using custom Error that can be cloned to be able to
pass `Result` through the channel.
209: Upgrade to bdk 0.4 r=thomaseizinger a=thomaseizinger
Effectively, this also means:
- Upgrading to rust-bitcoin 0.26
- Upgrading to miniscript 5
- Upgrading monero to 0.10
- Upgrading curve25519-dalek to 3
- Upgrading bitcoin-harness to rust-bitcoin 0.26 (https://github.com/coblox/bitcoin-harness-rs/pull/21)
- Upgrade `ecdsa_fun` to latest version
- Replace `cross_curve_dleq` with `sigma_fun` (to avoid an upgrade dance on that library)
I refrained from specifying `rev`s in the Cargo.toml because we have a lock-file anyway. This should allow us to update those dependencies easier in the future by just running `cargo update -p <dependency>`.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Once the transaction was included into a block it has one confirmation - before inclusion it has zero.
current-block-height - transaction-block-height = zero; but that means one confirmation.
Hence, the confirmation calculation was adapted to: Current-block-height - (transaction-block-height - 1).
To achieve this we also:
- upgrade rust-bitcoin to 0.26
- upgrade bitcoin-harness to latest version (which also depends bitcoin 0.26)
- upgrade to latest edcsa-fun
- replace cross_curve_dleq proof with sigma_fun (to avoid an upgrade dance over there)
200: Wait for refund if insufficient Monero is locked up r=da-kami a=da-kami
In a scenario where Alice does not lock up sufficient funds Bob should properly transition to refunds. At the moment the CLI just panics.
I noticed this when Alice accidentally had a different amount set than Bob. In the future this should not happen, because Alice provides the amount for Bob. However, in case Alice is malicious Bob should still transition correctly.
Co-authored-by: Daniel Karzel <daniel@comit.network>
206: Remove misplaced wallet sync call r=rishflab a=rishflab
These bdk wallet sync calls must of gotten lost during a rebase. Removed the call in build TxLock and added one when nectar starts up
Co-authored-by: rishflab <rishflab@hotmail.com>
The bitcoind wallet required the user to run a bitcoind node. It was replaced with a bdk wallet which allows the user to connect to an electrum instance hosted remotely. An electrum and bitcoind testcontainer were created to the test the bdk wallet. The electrum container reads the blockdata from the bitcoind testcontainer through a shared volume. bitcoind-harness was removed as bitcoind initialisation code was moved into test_utils. The bdk wallet differs from the bitcoind wallet in that it needs to be manually synced with an electrum node. We synchronise the wallet once upon initialisation to prevent a potentially long running blocking task from interrupting protocol execution. The electrum HTTP API was used to get the latest block height and the transaction block height as this functionality was not present in the bdk wallet API or it required the bdk wallet to be re-synced to get an up to date value.
190: Do not pass Monero amount to the CLI r=D4nte a=D4nte
The CLI user only pass the Bitcoin amount they want to sell.
The CLI then do a quote request to nectar which provides the Monero amount the taker can get.
Co-authored-by: Franck Royer <franck@coblox.tech>
188: Tor cleanup r=da-kami a=da-kami
We never removed Tor install from CI. I don't think it should be necessary given that Tor was removed in code.
Co-authored-by: Daniel Karzel <daniel@comit.network>
To allow the related timelock to be defined with the
transaction that uses it. This will allow the access to the
timelock's struct inner field with defining `From` impl.
Hence, reducing complexity of the codebase. Note that the seed will be
used by both nectar and the cli whereas the config mod will be different
so this changes helps with the next step of having a dedicated config
module for each binary.
The punish test needs re-work due to the fact that Alice runs continuously
Currently focusing on the CLI (Bob), so we can re-introduce this test
once we want to ensure that nectar (Alice) punishes.
The test do not work without acks as we stop the event loop as soon
as a message is considered as "sent" when actually the event loop
and swarm may not have yet sent the message.
The ack allow to avoid this issue as the message was considered "sent"
only once the other party sent a response. However, the ack brings
other issue so a review needs to be done to select the appropriate
solution.
We are aware of issues of timeouts when waiting for acknowledgements.
Also, to properly supports acks in a multiple swap context, we need to
revert to doing event processing on the behaviour so that we can link
leverage the `RequestResponse` libp2p behaviour and link the messages
requests ids to swap ids when receiving an ack or response.
Acks are usefully for specific scenarios where we queue a message on the
behaviour to be sent, save as sent in the DB but crash before the
message is actually sent. With acks we are able to resume the swap,
without ack, the swap will abort (refund).
`alice::swap::run_until` will be called once the execution setup is
done. The steps before are directly handled by the event loop,
hence no channels are needed for said steps: connection established,
swap request/response & execution setup.
The `EventLoop` will use the `Builder` interface to instantiate a
`Swap` upon receiving a `SwapRequest` and successfully doing an
execution setup.
Before this change, the `EventLoop` would have to hold the path to the
db and re-open the db everytime it wants to construct a swap.
With this change, we can open the DB once and then hold a
`Arc<Database>` in the `EventLoop` and pass it to new `Swap`s structs.
This was introduced due to a CI run, where Bob included tx_refund, but Alice had waited until T2 had expired,
and then went for punishing Bob instead of refunding.
Weirdly, Alice's punich transaction did not fail in that scenario.
If dialing Bob fails Alice waits for the acknowledgement of the transfer proof indefinitely.
The timout prevents her execution from hanging.
Added a ToDo to re-visit the ack receivers. They don't add value at the moment and should be removed.
