If we enter a punish scenario we can be sure the punish timelock is expired.
Thus, we must be able to punish unless Bob published the refund transaction.
There is no benefit in racing punish against refund here, because we cannot recover from a punish tx failure anyway.
The logic was changed to:
Try to broadcast punish tx and await finality.
If either punish broadcasting of finality fails, try to fetch the refund transaction.
If it is available extract Bob's Monero key part and transition to refund.
If refund tx is not available fail without a status update.
Note that we do not distinguish different errors upon failure of punish, because
we cannot recover anyway. If we fail to retrieve Bob's refund tx, we just exit without
a status update so punish can be retried by resuming the swap.
Since Alice's refund scenario starts with generating the temporary wallet
from keys to claim the XMR which results in Alice' unloading the wallet.
Alice then loads her original wallet to be able to handle more swaps.
Since Alice is in the role of the long running daemon handling concurrent
swaps, the operation to close, claim and re-open her default wallet must
be atomic.
This PR adds an additional step, that sweeps all the refunded XMR back into
the default wallet. In order to ensure that this is possible, Alice has to
ensure that the locked XMR got enough confirmations.
These changes allow us to assert Alice's balance after refunding.
To achieve this, we decompose `watch_for_locked_xmr` into two parts:
1. A non-self-consuming function to construct a `WatchRequest`
2. A state transition that can now consume `self` again because
it is only called once within the whole select! expression.
Ideally, we would move more logic onto this state transition (like
comparing the actual amounts and fail the transition if it is not
valid). Doing so would have an unfortunate side-effect: We would
always wait for the full confirmations before checking whether or
not we actually receive enough XMR.
This allows us to have state transitions that consume self.
Instead of calling this function in all the branches, we can simply
make the whole match statement evaluate to the new state and perform
this functionality at the very end.
This allows us to move critical crypto logic onto `State3` which
holds all the necessary data which consequently allows us to get
rid of `lock_xmr` altogether by inlining it into the swap function.
The reduced indirection improves readability.
321: Properly handle concurrent messages to and from peers r=thomaseizinger a=thomaseizinger
Previously, we were forwarding incoming messages from peers to all
swaps that were currently running. That is obviously wrong. The new
design scopes an `EventLoopHandle` to a specific PeerId to avoid
this problem.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
322: Refactor `ExecutionParams` and harmonize sync intervals of wallets r=thomaseizinger a=thomaseizinger
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Previously, we were forwarding incoming messages from peers to all
swaps that were currently running. That is obviously wrong. The new
design scopes an `EventLoopHandle` to a specific PeerId to avoid
this problem.
Bob does not care whether tx lock is confirmed. That is alice's problem.
This wait was introduced to remedy a bug in status_of_script() which was
failing when called on a transaction with no confirmations.
We have a repeated pattern where we construct one of our
Tx{Cancel,Redeem,Punish,Refund,Lock} transactions and wait until
the status of this transaction changes. We can make this more
ergonomic by creating and implementing a `Watchable` trait that
gives access to the TxId and relevant script for this transaction.
This allows us to remove a parameter from the `watch_until_status`
function.
Additionally, there is a 2nd pattern: "Completing" one of these
transaction and waiting until they are confirmed with the configured
number of blocks for finality. We can make this more ergonomic by
returning a future from `broadcast` that callers can await in case
they want to wait for the broadcasted transaction to reach finality.
The execution params don't change throughout the lifetime of the
program. They can be set in the wallet at the very beginning.
This simplifies the interface of the wallet functions.
We achieve our optimizations in three ways:
1. Batching calls instead of making them individually.
To get access to the batch calls, we replace all our
calls to the HTTP interface with RPC calls.
2. Never directly make network calls based on function
calls on the wallet.
Instead, inquiring about the status of a script always
just returns information based on local data. With every
call, we check when we last refreshed the local data and
do so if the data is considered to be too old. This
interval is configurable.
3. Use electrum's notification feature to get updated
with the latest blockheight.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Rishab Sharma <rishflab@hotmail.com>
We reduce indirection by constructing TxPunish directly based off
`State3` and make the type itself more powerful by moving the logic
of completing it with a signature onto it.
Instead of spawning the swap inside the event loop we send the swap back
to the caller to be spawned. This means we no longer need the remote handle
that was only used in the tests.
This now properly logs the swap results in production.
It also gives us more control over Alice's swap in the tests.
This allows us to have access to RedeemTx from within the scope
of the state transition which we are going to need for more
efficient watching of what happens to this TX on the blockchain.
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.
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.
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.
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.
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.
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.
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.
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`.
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.
