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.
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
As per the proposed changed in the sequence diagram.
The aim is to have a unique terminology per message instead of having
the same name for 2 consequent messages that share the same behaviour.
Note that the aim is to remove the shared `RequestResponse` behaviours.
Rust fmt automatically groups the imports (from top to bottom) as `pub use` `use crate` and `use`.
There is no need to introduce sections which cause annoyance when auto importing using the IDE.
149: Fix Alice redeem scenario r=da-kami a=da-kami
Follow up of #144, partial fix of https://github.com/comit-network/xmr-btc-swap/issues/137
Fix Alice redeem scenario
- 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)
Co-authored-by: Daniel Karzel <daniel@comit.network>
- 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.
Currently this code is actually not reachable, but that is semantically applied by the program's flow (the resume command includes the swap direction).
It is still preferred to have an error message rather than an unreachable statement.
- 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
145: Make lock-tx id available in redeem/punish state to be able to assert exact fees r=da-kami a=da-kami
We can do exact assertions for Bob's redeem as well, but have to store Bob's tx_lock id in the respective final state. Make tx_lock available in BtcRedeemed and BtcPunished to have better assertions / harmonize test behaviour.
Storing this information is strictly speaking not needed for the production environment. But it is static information that can be seen as additional information that can be handy for a user. We could potentially extract it inside the tests as well (for redeem without restart would be a bit tricky), but I think this solution is more elegant.
Co-authored-by: Daniel Karzel <daniel@comit.network>
Co-authored-by: Franck Royer <franck@coblox.tech>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
144: Test refactor r=da-kami a=da-kami
This PR is pure refactoring, keeping the logic of the tests we had before. No production code is touched besides re-exports in early commits (no logic changes).
In the follow ups improvements will be introduced, that touch the production code as well.
All remaining tasks actioned since Friday:
- [x] `happy_path_bob _restart` (trivial)
- [x] add refund assertions to harnesses (trivial)
- [x] convert all refund scenarios currently being tested (trivial)
- [x] remove dead test init code once all old tests are converted
- [ ] ~~(optional) move alice and bob harness code into separate files~~ -> might action this once re-using test code in production.
Out of scope, follow up:
- [x] https://github.com/comit-network/xmr-btc-swap/pull/145 - We can do exact assertions for Bob's redeem as well, but have to store Bob's `tx_lock` id in the respective final state. Make `tx_lock` available in `BtcRedeemed` and `BtcPunished` to have better assertions / harmonize test behaviour.
- [ ] update the production code to use the `Alice` and `Bob` structs to bundle the params - update tests to use the production struct.
- [ ] Re-use test swap setup in production (i.e. `Alice-/BobHarness::new`) to setup the swap.
- [ ] add additional tests
- [ ] re-try moving the tests from `test` to `src` (if the peer_id was the only problem this should be trivial now - but should be done after the refactor is finished)
- [ ] creating new wallets upon restart
- [ ] aborting the old event loop after restart
Co-authored-by: rishflab <rishflab@hotmail.com>
Co-authored-by: Daniel Karzel <daniel@comit.network>
If we wait for lock transaction confirmations immediately after sending the transaction without saving this state to the DB this might cause locking the money twice.
An additional state is needed for such a scenario.
This includes the introduction of the --data-dir parameter instead of the --database.
Both the seed file and the database are stored in the data-dir, the database in sub-folder `database`.
We already select waiting for this message with the cancellation expiry,
we do not need add another guard that tries to guess how long it would
for the Monero transaction to be finalised.
Created network, storage and protocol modules. Organised
files into the modules where the belong.
xmr_btc crate moved into isolated modulein swap crate.
Remove the xmr_btc module and integrate into swap crate.
Consolidate message related code
Reorganise imports
Remove unused parent Message enum
Remove unused parent State enum
Remove unused dependencies from Cargo.toml
