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>
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.
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.
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.
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.
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.
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>
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.
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.
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.
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`.
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)
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.
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.
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.
Alice was attempting to create a new event loop using the same listen addr as the old one which was still running. This commit aborts the event loop before creating a new one.
Upgrade bitcoin harness dependency to latest commit
Upgrade backoff to fix failing tests. The previous version of backoff had a broken version of the retry function. Upgraded to a newer comit which fixes this problem.
Upgrade hyper to 0.14 as the 0.13 was bringing in tokio 0.2.24
Upgraded bitcoin harness to version that uses tokio 1.0 and reqwest 0.11
Upgrade reqwest to 0.11. Reqwest 0.11 uses tokio 1.0
Upgrade libp2p to 0.34 in preparation for tokio 1.0 upgrade
- Properly check the timelocks before trying to redeem
- Distinguish different failure scenarios and reactions to it.
- if we fail to construct the redeem transaction: wait for cancel.
- if we fail to publish the redeem transaction: wait for cancel but let the user know that restarting the application will result in retrying to publish the tx.
- if we succeed to publish the tx but then fail when waiting for finality, print error to the user (secreat already leaked, the user has to check manually if the tx was included)
This is not really a factory as a factory design pattern is about
producing several instances.
In the current usage, we are only interested in one swap instance. Once
the swap instance is created, the factory becomes useless. Hence, it is
more of a builder pattern.
- Introduce Test abstraction instead of tow harnesses, move test specific data into Test
- Change the abstraction from actors to swap, because we are creating swaps, not actors
- rename actor::swap to run, because we are running a swap
This makes the redeem assertion reusable for all tests with a redeem scenario.
Since the factory was not a clean factory before and is now doing even more it was renamed to harness.
