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>
136: Testnet resume fixes r=da-kami a=da-kami
Add a few log statements on Bob's side to make the user experience better.
Update / remove ToDos.
I set the log level to `Info` in main again, `Debug` heavily clutters the output. In order to make `Debug` more usable we might want to review printing all those `rpc` messages. But this goes beyond the scope of this PR.
Co-authored-by: Daniel Karzel <daniel@comit.network>
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.
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
We have overridden a number of clippy warnings such as
"large enum variant".
Considering that we have a number of issues
with the stack size in CI, it is more prudent to follow clippy's advice
and box larger items so that the enum does not take larger space.
Do note that an instance of the enum always takes as much space as its
largest variant.
There are no refund timelock, only a cancellation timelock and punish
timelock.
Refund can be done as soon as the cancellation transaction is published.
107: Ensure that Bob can cancel correctly if T1 expired and Alice did not … r=da-kami a=da-kami
Bob has to check for the possibility to cancel in every state after he locked the BTC.
Otherwise Bob will try to perform actions that don't have any point and it might be impossible to use the `resume` command because it will always fail in trying to go on with Alice even though that might not be possible.
Co-authored-by: Daniel Karzel <daniel@comit.network>
Bob has to check for the possibility to cancel in every state after he locked the BTC.
Otherwise Bob will try to perform actions that don't have any point.
As Bob is dialing Alice, we now ensure that we are connected to Alice
at each step that needs communication.
If we are not connected, we proceed with dialing.
In an attempt to improve libp2p usage, we also add known address of
Alice first and only use peer_id to dial.
This ensures that we use the expected peer id.
98: Remove tor module r=da-kami a=da-kami
This removes the currently unused `tor module`.
Different `tokio` versions have been causing issues with the `tor` module in the past (i.e. `Cargo.lock` broken problem...). It started causing issues again when adding a dependency to `jsonrpc_client` working on https://github.com/comit-network/xmr-btc-swap/pull/97
We don't support `tor` at the moment and are no planning to add this feature initially as it is not super important to users.
The functionality can easily added again at a later point.
Co-authored-by: Daniel Karzel <daniel@comit.network>
This module was intended to contain helper functions for each step.
However, those are not needed except for the negotiate step.
A dedicated module is not needed for one function.
The usage of the peer id is incorrect as we do not even check it when
dialing. For now, we can ignore it.
We can then re-introduce it and use it properly at a later stage.
Reworked Alice XmrLocked state transition handler to handle the
scenario when Alice received the encsig but Bob refunds.
Previously Alice was trying to redeem after receiving the encsig
without checking if t1 had elapsed.
Previously state0 had to be set after creating Alice's behaviour.
With the event loop we no longer has access to the swarm so
set_state0() has to be called indirectly through a channel. This
means it is difficult to guarantee state0 is being set due to the
asynchronous nature of channels. This was solved by initialising
Alice with state0.
Previously the libp2p swarm had to be manually polled within the
protocol execution code to execute actions such as sending a
message. The swarm is now wrapped in SwarmDriver which polls the
swarm in a seperate task
Tracing should be initialized by test and the `_guard` kept alive within the test.
Re-using this code in different tests does not really have any additional value.
Instead of specifying what messages we want to include, I went for a filter that excludes noise.
That way we get more useful logging.
Use reusable test init functions for happy path test
Extract tracing setup to reusable function
Move test initialization to seperate functions
Increase stack size in CI
Fix monero max finality time
Force Bob swarm polling to send message 2
Run Bob state to xmr_locked in punish test to force the sending of
message2. Previously Bob state was run until btc_locked. Although
this was the right thing to do, message2 was not being sent as the
swarm was not polled in btc_locked. Alice punish test passes.
Add info logging to executor
