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

Fix the duplicate connection bug #499

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix the duplicate connection bug #499

wants to merge 2 commits into from

Conversation

MSNTCS
Copy link
Contributor

@MSNTCS MSNTCS commented Sep 4, 2020

In this pr, we try to address this issue #274.

@MSNTCS MSNTCS requested a review from sgkim126 September 7, 2020 09:04
majecty
majecty reviewed Sep 7, 2020
View reviewed changes
sync/src/block/extension.rs
@@ -476,22 +476,27 @@ impl NetworkExtension<Event> for Extension {
cinfo!(SYNC, "New peer detected #{}", id);
self.send_status(id);

let t = self.connected_nodes.insert(*id);
debug_assert!(t, "{} is already added to peer list", id);
if !self.connected_nodes.contains(id) {
Copy link
Contributor

@majecty majecty Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@MSNTCS
It seems this patch is avoiding the error case. Not fixing the error.
When the on_node_added is called, it should not be included in connected_nodes.
Could you try to fix this problem fundamentally?

@majecty
Copy link
Contributor

majecty commented Sep 8, 2020

I met this error while running yarn mocha -r ts-node/register --timeout 10000 src/e2e.long/discovery5.test.ts in 0db816d5f0dbb2600edf840b06ce7e6314578683.
I can find it when I run the test a few tens of times.

I'll run the same test in the current master.


stack backtrace:
   0:     0x5640209e77b3 - backtrace::backtrace::libunwind::trace::h389b7f78776dda1f
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/backtrace/libunwind.rs:53
                         - backtrace::backtrace::trace::h35e146709d259c8b
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/backtrace/mod.rs:42
   1:     0x5640209e4b23 - backtrace::capture::Backtrace::new_unresolved::h2acfb8aeff36f932
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/capture.rs:88
   2:     0x5640209e4a96 - backtrace::capture::Backtrace::new::h32feb04c4b84be54
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/capture.rs:63
   3:     0x56401f03761a - panic_hook::panic_message::h4db40ff8aee25363
                        at util/panic_hook/src/lib.rs:74
   4:     0x56401f036e55 - panic_hook::panic_hook::h332960d4ef09703e
                        at util/panic_hook/src/lib.rs:43
   5:     0x56401f039f87 - core::ops::function::Fn::call::ha78be1dacd140af8
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:72
   6:     0x564020a1fb9b - std::panicking::rust_panic_with_hook::ha0c7ca5b39baa9d5
                        at src/libstd/panicking.rs:490
   7:     0x564020a1f77a - rust_begin_unwind
                        at src/libstd/panicking.rs:388
   8:     0x564020a474a0 - core::panicking::panic_fmt::h106d7128cbbc7cd2
                        at src/libcore/panicking.rs:101
   9:     0x564020a472a2 - core::option::expect_none_failed::hfcafefc70a7975bb
                        at src/libcore/option.rs:1272
  10:     0x564020215824 - core::result::Result<T,E>::unwrap::hdb946cd6d9a894ea
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/result.rs:1005
  11:     0x56402020aa2e - codechain_crypto::aes::decrypt::h21e077a0f16a0afa
                        at /home/juhyung/.cargo/git/checkouts/rust-codechain-crypto-81d89da97780f22a/3002ed3/src/aes.rs:43
  12:     0x56401f927383 - codechain_network::routing_table::decrypt_nonce::h9ce36753c2c1983a
                        at network/src/routing_table.rs:620
  13:     0x56401f92593b - codechain_network::routing_table::RoutingTable::set_initiator_establish::ha9eabc7afaa7d5df
                        at network/src/routing_table.rs:513
  14:     0x56401f9f43cf - <codechain_network::p2p::handler::Handler as codechain_io::IoHandler<codechain_network::p2p::handler::Message>>::stream_readable::h489585ca5efd3a37
                        at network/src/p2p/handler.rs:946
  15:     0x56401f946875 - codechain_io::worker::Worker::do_work::h3f4d6699c74bee95
                        at /mnt/sonnyexternal/juhyung/code/kodebox/foundry/util/io/src/worker.rs:122
  16:     0x56401f947f7c - codechain_io::worker::Worker::work_loop::h283ccb40ec5dd2c1
                        at /mnt/sonnyexternal/juhyung/code/kodebox/foundry/util/io/src/worker.rs:110
  17:     0x56401f946622 - codechain_io::worker::Worker::new::{{closure}}::h193f3065f32fdca7
                        at /mnt/sonnyexternal/juhyung/code/kodebox/foundry/util/io/src/worker.rs:84
  18:     0x56401fa14812 - std::sys_common::backtrace::__rust_begin_short_backtrace::h812eb05e4b5329b8
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:130
  19:     0x56401fa08472 - std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}::he631a29fe360477a
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/thread/mod.rs:475
  20:     0x56401fa14b80 - <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h6eaff2580ca0094c
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:318
  21:     0x56401fa15533 - std::panicking::try::do_call::h815471931bf1a096
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:297
  22:     0x56401fa156ec - __rust_try
  23:     0x56401fa15303 - std::panicking::try::h28f2ba4d50eb9f37
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:274
  24:     0x56401fa14c52 - std::panic::catch_unwind::he845036214da02b9
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394
  25:     0x56401fa07ed8 - std::thread::Builder::spawn_unchecked::{{closure}}::h4eb589dd385e1965
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/thread/mod.rs:474
  26:     0x56401f968b7e - core::ops::function::FnOnce::call_once{{vtable.shim}}::h4630174f3ad4646d
                        at /home/juhyung/.rustup/toolchains/1.45.2-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232
  27:     0x564020a27a59 - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::hf311c88f1fadb9b8
                        at /rustc/d3fb005a39e62501b8b0b356166e515ae24e2e54/src/liballoc/boxed.rs:1076
                         - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h8cfb7235b81393ef
                        at /rustc/d3fb005a39e62501b8b0b356166e515ae24e2e54/src/liballoc/boxed.rs:1076
                         - std::sys::unix::thread::Thread::new::thread_start::hf745c8cf29a89648
                        at src/libstd/sys/unix/thread.rs:87
  28:     0x7f7aff4d5608 - start_thread
  29:     0x7f7aff3df102 - __clone
  30:                0x0 - <unknown>

Thread 'P2P Worker #1' panicked at 'called `Result::unwrap()` on an `Err` value: BlockModeError', /home/juhyung/.cargo/git/checkouts/rust-codechain-crypto-81d89da97780f22a/3002ed3/src/aes.rs:43

This is a bug. Please report it at:

    https://github.com/CodeChain-io/codechain/issues/new

@majecty
Copy link
Contributor

majecty commented Sep 8, 2020 •
edited
Loading

Since I met the error in the master(bc771a4), I'll create an issue for it. This error seems not related to this PR.

I created an issue: #501

sgkim126
sgkim126 requested changes Sep 8, 2020
View reviewed changes
network/src/p2p/handler.rs Outdated
let t = inbound_connections.insert(token, connection);
assert!(t.is_none());
io.register_stream(token);
let mut can_insert: bool = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

                    let can_insert = inbound_connections.values().all(|in_connection| {
                        in_connection.peer_addr() != connection.peer_addr()
                    });
                    let is_not_out = outbound_connections.values().all(|out_connection| {
                        out_connection.peer_addr() != connection.peer_addr()
                    });

@MSNTCS MSNTCS mentioned this pull request Sep 9, 2020
Open
@majecty majecty mentioned this pull request Dec 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majecty majecty majecty left review comments

@sgkim126 sgkim126 sgkim126 requested changes

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@MSNTCS @majecty @sgkim126