Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

farcaster-swapd-syncer race-free initiation interplay: improve sequence of msgs #574

Merged
merged 39 commits into from
Jul 20, 2022
Merged

farcaster-swapd-syncer race-free initiation interplay: improve sequence of msgs #574

merged 39 commits into from
Jul 20, 2022

Conversation

zkao
Copy link
Member

@zkao zkao commented Jul 14, 2022

No description provided.

@zkao zkao force-pushed the farcasterd_improve_sequence branch 4 times, most recently from 4ffd2c9 to 443c26a Compare July 15, 2022 12:16
@zkao zkao changed the title Farcasterd improve sequence farcaster-swapd-syncer race-free initiation interplay: improve sequence of msgs Jul 15, 2022
@zkao zkao force-pushed the farcasterd_improve_sequence branch from 443c26a to a534be0 Compare July 15, 2022 13:03
@zkao
Copy link
Member Author

zkao commented Jul 16, 2022 

2022-07-15T16:40:11.9309543Z thread 'main' panicked at 'Checked above', src/swapd/runtime.rs:1239:30

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2022 •
edited
Loading

Codecov Report

Merging #574 (7cccff6) into main (0f80924) will decrease coverage by 0.0%.
The diff coverage is 0.0%.

❗ Current head 7cccff6 differs from pull request most recent head cb27381. Consider uploading reports for the commit cb27381 to get more accurate results

@@           Coverage Diff           @@
##            main    #574     +/-   ##
=======================================
- Coverage   12.0%   12.0%   -0.0%     
=======================================
  Files         34      34             
  Lines      10601   10643     +42     
=======================================
  Hits        1277    1277             
- Misses      9324    9366     +42
Impacted Files Coverage Δ
src/farcasterd/runtime.rs 0.0% <0.0%> (ø)
src/rpc/request.rs 17.6% <0.0%> (+0.1%) ⬆️
src/swapd/runtime.rs 0.0% <0.0%> (ø)
src/swapd/swap_state.rs 0.0% <0.0%> (ø)
src/swapd/syncer_client.rs 0.0% <0.0%> (ø)
src/syncerd/runtime.rs 0.0% <0.0%> (ø)
src/walletd/runtime.rs 0.0% <0.0%> (ø)
src/error.rs 0.0% <0.0%> (ø)
... and 1 more

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 0f80924...cb27381. Read the comment docs.

@zkao zkao force-pushed the farcasterd_improve_sequence branch from 1e12b84 to 38ad945 Compare July 18, 2022 15:59
zkao
zkao commented Jul 18, 2022
View reviewed changes
src/swapd/runtime.rs Outdated Show resolved Hide resolved
@zkao zkao force-pushed the farcasterd_improve_sequence branch from 38ad945 to 544afb0 Compare July 18, 2022 18:08
@zkao zkao marked this pull request as ready for review July 18, 2022 18:44
@zkao
Copy link
Member Author

zkao commented Jul 18, 2022

closes #552

@zkao
Copy link
Member Author

zkao commented Jul 19, 2022 

2022-07-18T19:44:19.3211711Z [2022-07-18T19:44:19Z DEBUG microservices::esb::controller] swap<0xf1d5…45c0> -> Bitcoin (Local) syncer: take_swap(peerd<02b29b33ec35931d740e55e3a22cbeef8303df3cd432c76655c1ab0f1ba77
8ece6@127.0.0.1:9376>, 0xf1d5…45c0)
2022-07-18T19:44:19.3213042Z [2022-07-18T19:44:19Z ERROR farcaster_node::syncerd::runtime] Request is not supported by the CTL interface req: take_swap(peerd<02b29b33ec35931d740e55e3a22cbeef8303df3cd432c76655c1ab0f1ba778ece6@127.0.0.1:9376>, 0xf1d5…45c0), source: swap<0xf1d5…45c0>

@zkao zkao marked this pull request as draft July 19, 2022 12:30
This was referenced Jul 19, 2022
Merged
Merged
@zkao zkao added the mainnet label Jul 19, 2022
zkao added 16 commits July 19, 2022 17:21
swapd: improve unhandled request logging
57698c0
swapd: improve RefundSigA display
83ba271
wallet: remove unfit TODO, permission done at the start of handler
9ddad86
disambiguate Msg Reveal, variant RevealProof was impossible to find
2ab7fa6
swapd: small comments, logging
ebfade3
farcaster-swapd-syncer race-free initiation interplay
6e5bf73
request: rm request SyncerRunning, not needed anymore
f136d6b
syncer_client: fn on_btc_syncer_launch subs for fees & block height
009cc1f
swapd: bug using bitwise OR
da916d6
cargo-fmt
2220965
farcasterd: remove sleep
73d9705
swapd: checkme, fixed pattern match to avoid failing unwrap
3889460 
treating as if the bug is on the pop unwrap, since the expect is checked

2022-07-17T13:13:23.6366705Z [2022-07-17T13:13:23Z DEBUG microservices::esb::controller] syncer<(Monero,Local)> -> swap<0x8478…a469>: syncer_event(TransactionConfirmations(TransactionConfirmations { id: TaskId(1
1), block: [178, 67, 8, 101, 245, 54, 2, 17, 91, 42, 117, 121, 49, 102, 1, 0, 114, 0, 189, 96, 58, 32, 16, 52, 132, 231, 198, 83, 77, 42, 7, 187], confirmations: Some(20), tx: [] }))
2022-07-17T13:13:23.6368099Z thread 'main' panicked at 'Checked above', src/swapd/runtime.rs:1239:30
2022-07-17T13:13:23.6368705Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
syncer: improve catch all unsupported request logging
faec54a
farcasterd: fix source/dest of Take/Make swap req
1928647
swapd: add log error when no request processed
f185745
swapd: fix warnings
4ab7a39
@zkao zkao force-pushed the farcasterd_improve_sequence branch from cb27381 to 4ab7a39 Compare July 19, 2022 15:22
@zkao zkao marked this pull request as ready for review July 19, 2022 15:22
@zkao zkao linked an issue Jul 19, 2022 that may be closed by this pull request
grep sleep and remove them all #552
Closed
@zkao
Copy link
Member Author

zkao commented Jul 19, 2022

this PR includes bits of noise, related to me going through the first 2/3 of the sequence diagram, and recognizing the patterns on the code. Some bits were not recognizable on the code, and I slightly modified them. Some pieces were not good and I tagged them with FIXME or something. Additionally I added unrelated logging while debugging.

Since the final diffs are not too bad, I kept it like that without removing commits

TheCharlatan
TheCharlatan approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was hard to review, but I think I understood the changes. The race condition should indeed be fixed now.

@zkao
Copy link
Member Author

zkao commented Jul 20, 2022

there is still a race condition if monero syncer takes longer than bitcoin syncer to come online, but it requires creating new datatypes to handle that, and this PR was already overwhelming

@TheCharlatan TheCharlatan merged commit abc86cb into farcaster-project:main Jul 20, 2022
@zkao zkao mentioned this pull request Jul 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCharlatan TheCharlatan TheCharlatan approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

grep sleep and remove them all
3 participants
@zkao @codecov-commenter @TheCharlatan