swapd: fee estimate from syncer (electrum)

[zkao]

No description provided.

[codecov-commenter]

Codecov Report

Merging #533 (559571d) into main (17a5687) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##            main    #533     +/-   ##
=======================================
- Coverage   12.1%   12.1%   -0.1%     
=======================================
  Files         34      34             
  Lines      10529   10573     +44     
=======================================
  Hits        1277    1277             
- Misses      9252    9296     +44     

Impacted Files	Coverage Δ
src/swapd/runtime.rs	0.0% <0.0%> (ø)
src/swapd/syncer_client.rs	0.0% <0.0%> (ø)
src/syncerd/bitcoin_syncer.rs	0.0% <0.0%> (ø)
src/syncerd/syncer_state.rs	86.0% <0.0%> (-0.1%)	⬇️
src/syncerd/types.rs	24.6% <0.0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17a5687...559571d. Read the comment docs.

[zkao]

The syncer is the one that is interacting externally, so we can't rely messages will be delivered in time. There are potential race conditions there, since we use Events to trigger actions in swapd (the sequence diagrams is the place to search for them). Especially at launch when too many messages are flying around. As you probably know from the sequence diagrams, we do rely that for a given service bus, its msgs are received in the order they are sent. And if there will be a race condition because one msg is being sent through Msg bus and the other through Ctl bus, then we have to put the two in the same bus, so that they're ordered correctly, and thus received in the correct order

…

------- Original Message -------

On Monday, July 4th, 2022 at 10:56 PM, Reproducibility Matters ***@***.***> wrote: @TheCharlatan commented on this pull request. --------------------------------------------------------------- In [src/swapd/runtime.rs](#533 (comment)): > + + // syncer takes too long to give a fee + std::thread::sleep(Duration::from_secs_f32(5.0)); + The caching can be done, yes. Nevertheless it would be nice to not rely on the fee estimator to return in time. Do we make similar assumptions for other requests? — Reply to this email directly, [view it on GitHub](#533 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AIQJTBPTTQQUASOGWL66D3LVSNFXHANCNFSM52UD27IA). You are receiving this because you authored the thread.Message ID: ***@***.***>

[zkao]

Most of the tasks are watch tasks. So those are easy to handle, because u subscribe very well ahead of time.

While coding, one simplification that I did and that might still be around is, for example, that the act of sending a broadcast task actually broadcasts a tx. Haven't searched for those.

[zkao]

So far, fee estimation happens only once, FIXME

[zkao]

discussion on handling bitcoin fees happened on #534

[TheCharlatan]

From the logs:

[2022-07-11T08:38:45Z INFO  farcaster_node::swapd::runtime] 0xe2ad…44ae | Accepting swap as Maker from Taker through peerd peerd<03ee5e61d42bc0f932c0e7d09e0df6ab8b2dde43d86131645cd4b074a738a56803@0.0.0.0:9376>
[2022-07-11T08:38:45Z DEBUG microservices::esb::controller] swap<0xe2ad…44ae> -> farcasterd: progress: Accepting swap 0xe2ad…44ae as Maker from Taker through peerd peerd<03ee5e61d42bc0f932c0e7d09e0df6ab8b2dde43d86131645cd4b074a738a56803@0.0.0.0:9376>
[2022-07-11T08:38:45Z ERROR microservices::esb::controller] ESB request processing error: error sending message from swap<0xe2ad…44ae> to Bitcoin (Local) syncer. Details: service is offline or not responding
[2022-07-11T08:38:45Z DEBUG syncerd] MSG RPC socket ipc://tests/fc1/msg.rpc
[2022-07-11T08:38:45Z DEBUG syncerd] CTL RPC socket ipc://tests/fc1/ctl.rpc
[2022-07-11T08:38:45Z DEBUG syncerd] Starting runtime ...

Seems like the problem is when Bob is Maker, the swap daemon receives MakeSwap before the syncers are up. Either this requires another sleep before the EstimateFee Task is spawned, or we wait for some kind of Hello from the syncer before spawning syncer tasks in the swap daemon, or we spawn the syncers once the offers are open instead of when the swap is initialized.

[zkao]

i had an embarrassing number of bugs -- hopefully this is the last

[TheCharlatan]

The parallel swap test is failing now, no idea why, upon investigation it seems like 0.5 seconds are too fast for it. I bumped it to 2.0 and it is working again.

[zkao]

2022-07-11T12:46:55.3239843Z [2022-07-11T12:46:55Z ERROR farcaster_node::swapd::runtime] Incorrect amount funded. Required: 1.00000124 BTC, Funded: 1.00000123 BTC. Do not fund this swap anymore, will abort and atttempt to sweep the Bitcoin to the provided address.
2022-07-11T12:58:13.9790261Z [2022-07-11T12:58:13Z DEBUG microservices::esb::controller] swap<0x575d…5dc0> -> farcasterd: progress: Too big amount funded. Required: 1.000000000000 XMR, Funded: 1.000000000001 XMR. Do not fund this swap anymore, will attempt to refund.

[zkao]

i understand u guys dont want epsilon, but then how do i ensure all amounts are always exact at the sat or atomic unit level? reference from this discussion: #534

[zkao]

@TheCharlatan @Lederstrumpf

[zkao]

picked the dropped commits with epsilon for amounts, i know that is heavily objected, and the intend here is to move forward, not to challenge anyone

those commits can be dropped or later reverted as far a another solution is provided

[zkao]

i changed the epsilon to 2 sat or 2 pnero

test for monero fixed, did not find test for btc

[TheCharlatan]

2022-07-11T12:46:55.3239843Z [2022-07-11T12:46:55Z ERROR farcaster_node::swapd::runtime] Incorrect amount funded. Required: 1.00000124 BTC, Funded: 1.00000123 BTC. Do not fund this swap anymore, will abort and atttempt to sweep the Bitcoin to the provided address.
2022-07-11T12:58:13.9790261Z [2022-07-11T12:58:13Z DEBUG microservices::esb::controller] swap<0x575d…5dc0> -> farcasterd: progress: Too big amount funded. Required: 1.000000000000 XMR, Funded: 1.000000000001 XMR. Do not fund this swap anymore, will attempt to refund.

@zkao , from which tests are these logs? Aren't they from the overfunding tests and therefore expected and correct behavior?
There are two overfunding tests, one for Alice, one for Bob, so in the entire log, you should see exactly these two occurences once.

The Bob incorrect funding test is here: https://github.com/farcaster-project/farcaster-node/blob/main/tests/functional-swap.rs#L75
The Alice overfunding funding test is here: https://github.com/farcaster-project/farcaster-node/blob/main/tests/functional-swap.rs#L259

Please state which test is failing because of these errors before proceeding.

i understand u guys dont want epsilon, but then how do i ensure all amounts are always exact at the sat or atomic unit level? reference from this discussion: #534

In which scenario is the funding amount not exact right now?

[zkao]

oh, thats great, there are parallel new error that i did not undertand, will drop commits

[zkao]

have to understand what is wrong in bff0ec5

[zkao]

ah, it still is the syncer too late thingy

[zkao]

i will add to pending requests, but i'd want a more general solution later

[zkao]

oh, boy, am i stupid, when syncer becomes online and triggers the pending requests, we already hit Msg::Reveal, too late

not using pending request

just left a longer sleep for bob maker to wait for the bitcoin syncer -- will grep sleep eventually and clean them all

[TheCharlatan]

Thank you for bearing through this one, I am happy we are this far now :)