Move PRs from Foundry by majecty · Pull Request #1979 · CodeChain-io/codechain

majecty · 2020-07-30T05:54:22Z

@dynaxis @sgkim126 @byeongjee @junbeomlee @MSNTCS Could you check the commits you made? I moved commits from Foundty to CodeChain. Please tell me if there are omitted commits that should be moved to CodeChain or if I moved commits that should not be moved.

I run tests on my local machine. There are some flaky tests. If I run the tests multiple times, they eventually succeed.

I've moved refactoring and bug fix PRs from Foundry to CodeChain

Execute self-nomination transactions on a new thread Execute self-nomination transactions on a new thread foundry#98 @MSNTCS
Refactor BodyDownloader Refactor BodyDownloader foundry#116 @sgkim126
Add RPCs for checking peer best blocks & target blocks Add RPCs for checking peer best blocks & target blocks foundry#117 @byeongjee
Fix auto-self-nonimante Fix auto-self-nonimante foundry#118 @majecty
Add an RPC getting minimum fees confiugred by each machine Add an RPC getting minimum fees confiugred by each machine foundry#127 @byeongjee
Remove duplicated TermInfo Remove duplicated TermInfo foundry#135 @majecty
Change constants to enumerations Change constants to enumerations foundry#136 @sgkim126
Fix typos Fix typos foundry#138 @byeongjee
Upgrade codechain-crypto to 0.2.0 Upgrade codechain-crypto to 0.2.0 foundry#141 @sgkim126
Fix typo Fix typo foundry#152 @byeongjee
Report double votes in commit messages. Report double votes in commit messages. foundry#181 @byeongjee
Fix typo in test helper's constants Fix typo in test helper's constants foundry#224 Seonpyo
Assign unbound port numbers in test instances Assign unbound port numbers in test instances foundry#255 @byeongjee
Fix a wrong argument name Fix a wrong argument name foundry#270 @byeongjee
Remove ChainNotify::transactions_received() Remove ChainNotify::transactions_received() foundry#282 @sgkim126
Fix wrong debug message Fix wrong debug message foundry#293 @byeongjee
Change the error message that Foundry prints when account is not unlocked Change the error message that Foundry prints when account is not unlocked foundry#294 @majecty
Disconnect a socket If Foundry meets EOF while reading the socket Disconnect a socket If Foundry meets EOF while reading the socket foundry#309 @majecty
Remove unused dependencies in the sync module Remove unused dependencies in the sync module foundry#313 @majecty
Cleanup code by removing unnecessary code Cleanup code by removing unnecessary code foundry#315 @sgkim126
Use ctypes::Header instead of encoded::Header in HeaderDownloader Use ctypes::Header instead of encoded::Header in HeaderDownloader foundry#325 @majecty
Remove unnecessary initialization of genesis header extra data Remove unnecessary initialization of genesis header extra data foundry#329 @byeongjee
Remove unused verification code Remove unused verification code foundry#330 @majecty
Remove unnecessary serialize/deserialize while calling import_header Remove unnecessary serialize/deserialize while calling import_header foundry#331 @majecty
Rename import functions Rename import functions foundry#332 @majecty
Simplify miner, client and block. Simplify miner, client and block. foundry#333 @sgkim126
Run mock tests Run mock tests foundry#369 @majecty
Fix header RLP encoding in mock code Fix header RLP encoding in mock code foundry#376 @majecty
Fix few things in test code Fix few things in test code foundry#398 @majecty
Make the client hold the miner directly Make the client hold the miner directly foundry#410 @sgkim126
Upgrade parking_lot to 0.11.0 Upgrade parking_lot to 0.11.0 foundry#435 @junha1
Upgrade vergen 3 Upgrade vergen 3 foundry#436 @dynaxis

dynaxis · 2020-08-05T11:53:41Z

My commit for upgrading vergen is good to move to CodeChain. And looks ok.

junha1 · 2020-08-06T01:08:06Z

I think this error in https://travis-ci.com/github/CodeChain-io/codechain/jobs/368532029 is not just timeout

3) sync 3 nodes
       Connected in a circle
         All diverged by two nodes in the opposite
           It should be synced when the first node becomes ahead:
      AssertionError: expected { Object (value) } to deeply equal { Object (value) }
      + expected - actual
       {
      -  "value": "0dd0d442ca395b545ca10db2514dd926204986ff2c25b9876d44ac8374fbd293"
      +  "value": "ba7d733b1207f702c84134022782c474b8ba9880ea358e462657a144a32bf073"
       }

MSNTCS

My Commits on foundry for auto self nomination is good to be included in codechain. SGTM

majecty · 2020-08-06T04:56:23Z

@junha1
The failed test is testing sync. It looks like some blocks are not synced in time. I think it can be a timing issue and it succeeded in my local machine multiple times.

I'll try to make the test more consistent.

junha1 · 2020-08-06T04:57:18Z

Ok. I'm ok with my commit

majecty · 2020-08-06T09:57:52Z

Fixed by this PR: #1982

~~I found divide by zero panic. I'm investigating the cause. I'm not sure the bug was cause by this PR.~~

stack backtrace:
   0:     0x55f432f4c2d3 - backtrace::backtrace::libunwind::trace::h231104a04b7a2956
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/backtrace/libunwind.rs:53
                         - backtrace::backtrace::trace::hcf799d709bfe775d
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/backtrace/mod.rs:42
   1:     0x55f432f3d743 - backtrace::capture::Backtrace::new_unresolved::hf674184481555072
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/capture.rs:88
   2:     0x55f432f3d69d - backtrace::capture::Backtrace::new::hc53b747fb37ff970
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.7/src/capture.rs:63
   3:     0x55f4316ae47d - panic_hook::panic_message::hcf20e5160dbfff82
                        at util/panic_hook/src/lib.rs:76
   4:     0x55f4316add0a - panic_hook::panic_hook::h12a32b36292f2d30
                        at util/panic_hook/src/lib.rs:45
   5:     0x55f4316af5d7 - core::ops::function::Fn::call::h3f865c0a3aed5556
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/ops/function.rs:69
   6:     0x55f4337156ef - std::panicking::rust_panic_with_hook::hbe174577402a475d
                        at src/libstd/panicking.rs:468
   7:     0x55f4337151bd - std::panicking::continue_panic_fmt::h4d855dad868accf3
                        at src/libstd/panicking.rs:373
   8:     0x55f4337150a5 - rust_begin_unwind
                        at src/libstd/panicking.rs:302
   9:     0x55f43373686d - core::panicking::panic_fmt::hdeb7979ab6591473
                        at src/libcore/panicking.rs:139
  10:     0x55f4337367b9 - core::panicking::panic::hb5daa85c7c72fc62
                        at src/libcore/panicking.rs:70
  11:     0x55f431fbb555 - num_rational::Ratio<T>::new::hce449f4d68463c80
                        at /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/num-rational-0.2.1/src/lib.rs:75
  12:     0x55f431d2703b - codechain_core::consensus::tendermint::engine::give_additional_rewards::{{closure}}::he9c03dbfc6dcb88f
                        at core/src/consensus/tendermint/engine.rs:469
  13:     0x55f431dcd925 - core::iter::adapters::map_fold::{{closure}}::h71236903ffc62f82
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/adapters/mod.rs:694
  14:     0x55f431a07591 - core::iter::traits::iterator::Iterator::fold::ok::{{closure}}::h9f938db10e5bb926
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:1828
  15:     0x55f4319a63ed - core::iter::traits::iterator::Iterator::try_fold::h303f6dc3acb3ffbe
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:1709
  16:     0x55f4319a55e4 - core::iter::traits::iterator::Iterator::fold::h25dddc385bd91e0c
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/traits/iterator.rs:1831
  17:     0x55f431dd796a - <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold::hcce643894de4016a
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/iter/adapters/mod.rs:727
  18:     0x55f431d269ff - codechain_core::consensus::tendermint::engine::give_additional_rewards::he6f5989ed0d7c6c6
                        at core/src/consensus/tendermint/engine.rs:467
  19:     0x55f4319ae130 - codechain_core::consensus::tendermint::engine::calculate_pending_rewards_of_the_term::h3d9d18415328eb0d
                        at core/src/consensus/tendermint/engine.rs:421
  20:     0x55f431eec08c - codechain_core::consensus::tendermint::engine::<impl codechain_core::consensus::ConsensusEngine for codechain
_core::consensus::tendermint::Tendermint>::on_close_block::h28969988be56a055
                        at core/src/consensus/tendermint/engine.rs:203
  21:     0x55f431afab94 - codechain_core::block::OpenBlock::close_impl::hafb958d4d6a3ed0a
                        at core/src/block.rs:218
  22:     0x55f431afb5ba - codechain_core::block::OpenBlock::close::h3da16d92cde7aae9
                        at core/src/block.rs:245
  23:     0x55f431b9d865 - codechain_core::miner::miner::Miner::prepare_block::h6689ff7b84ea1ad7
                        at core/src/miner/miner.rs:707
  24:     0x55f431ba107c - <codechain_core::miner::miner::Miner as codechain_core::miner::MinerService>::update_sealing::hadc8e620c0b2c2
17
                        at core/src/miner/miner.rs:976
  25:     0x55f431d3a889 - codechain_core::client::client::Client::update_sealing::h44435d4f3fcc1518
                        at core/src/client/client.rs:206
  26:     0x55f431a3c8ee - <codechain_core::service::ClientIoHandler as codechain_io::IoHandler<codechain_core::service::ClientIoMessage
>>::message::hb022c01be00d98b9
                        at core/src/service.rs:105
  27:     0x55f431a4c950 - codechain_io::worker::Worker::do_work::ha782f9c7fad94b36
                        at /mnt/sonnyexternal/juhyung/code/kodebox/codechain/util/io/src/worker.rs:141
  28:     0x55f431a4d31b - codechain_io::worker::Worker::work_loop::h9b6527d5d66fce9b
                        at /mnt/sonnyexternal/juhyung/code/kodebox/codechain/util/io/src/worker.rs:109
  29:     0x55f431a4b7b2 - codechain_io::worker::Worker::new::{{closure}}::h0dce92fd14971a42
                        at /mnt/sonnyexternal/juhyung/code/kodebox/codechain/util/io/src/worker.rs:83
  30:     0x55f431f28840 - std::sys_common::backtrace::__rust_begin_short_backtrace::h95eeccc291e8f144
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/sys_common/backtrace.rs:129
  31:     0x55f431ecb8e2 - std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}::hd70f9416e6cfd93f
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/thread/mod.rs:469
  32:     0x55f431ef5672 - <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::hdf2eb0202bc07839
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/panic.rs:317
  33:     0x55f431f2de4d - std::panicking::try::do_call::hbca511378a28d0d4
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/panicking.rs:287
  34:     0x55f43371e439 - __rust_maybe_catch_panic
                        at src/libpanic_unwind/lib.rs:78
  35:     0x55f431f2d047 - std::panicking::try::h47aa68b0ace5918c
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/panicking.rs:265
  36:     0x55f431ef58a2 - std::panic::catch_unwind::hfcc45c8a7aa795de
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/panic.rs:396
  37:     0x55f431ecab8d - std::thread::Builder::spawn_unchecked::{{closure}}::hb64e46c01fe1a787
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/thread/mod.rs:468
  38:     0x55f431e5ffa3 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h5e9a0bfb62da7117
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/ops/function.rs:227
  39:     0x55f43370355e - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h3534c64212330b0c
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
  40:     0x55f43371d4ff - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h338c10574a337ece
                        at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
                         - std::sys_common::thread::start_thread::h761ac6d57710d65d
                        at src/libstd/sys_common/thread.rs:13
                         - std::sys::unix::thread::Thread::new::thread_start::h61c012ef60f933c0
                        at src/libstd/sys/unix/thread.rs:79
  41:     0x7f08d34dd608 - start_thread
  42:     0x7f08d33e7102 - __clone
  43:                0x0 - <unknown>

Thread 'Client Worker #2' panicked at 'denominator == 0', /home/juhyung/.cargo/registry/src/github.com-1ecc6299db9ec823/num-rational-0.2
.1/src/lib.rs:75

This is a bug. Please report it at:

https://github.com/CodeChain-io/codechain/blob/master/core/src/consensus/tendermint/engine.rs#L474

sgkim126

codechain-stratum also uses parking_lot. You should upgrade it too. Others look good to me.

majecty · 2020-08-18T02:31:22Z

@sgkim126 I updated stratum! Thanks.

majecty · 2020-08-18T02:31:52Z

@byeongjee Could you check this PR?
Please check your commits.

Currently, we have a tool written in TypeScript to run auto-self-nominate. In this new patch, codechain is able to run auto-self-nominate in a different thread. In order to test the functionality of auto-self-nominate, a new `selfnomination` test is added in `e2e.dynval/2/selfnomination.test.ts`.

The purpose of the encoded type is to read a few fields from binary data when we don't want to full deserialize it. In the header sync, we already deserialized the whole struct in the verification process. We don't need to encoded Header when we have ctypes::Header.

We gain nothing from initializing extra data of genesis header with hash of common params, because the check will be done by the same node, and it will always succeed. We can just use extra_data written in scheme file.

CodeChain was using only the VerifierType::Canon.

CodeChain is using only CanonVerifier. We are not using the `Verifier` abstraction.

Client::import_header function receives a header that is not verified. Importer::import_headers assumes that received headers are verified. To reduce misunderstanding, I renamed the function.

The name `import_sealed_block` does not give enough context.

The h is always None.

Currently, AccountProvider in the miner is optional to support unit tests who cannot use the account provider which uses the permanent storage. This commit makes the miner uses memory back-end AccountProvider for test.

I changed the test code only. The tests were deprecated. I updated RLP encoded data from CodeChain code and it worked.

The seal is an array of any RLP data. Wrapping them with Buffer makes invalid RLP encoding.

Since Header has a cache of its hash using RefCell, the existence of the cache changes the compare result.

This will make CodeChain in tests not to increase view to high. Before this commit, some tests (for example dv.nomination.test.ts) sometimes creates a block with a very high view like 9. It was because blocks were created so fast and the timestamp of a block is too future for CodeChain.

Currently, the client access the miner through the importer. It's better to give a method to access the miner directly.

majecty force-pushed the f/copy-from-foundry branch from b51e019 to 00475f0 Compare August 5, 2020 06:55

majecty requested review from MSNTCS, byeongjee, dynaxis, junha1 and sgkim126 August 5, 2020 09:57

majecty changed the title ~~[WIP] Move PRs from Foundry~~ Move PRs from Foundry Aug 5, 2020

majecty marked this pull request as ready for review August 5, 2020 10:00

MSNTCS previously approved these changes Aug 6, 2020

View reviewed changes

majecty dismissed MSNTCS’s stale review via d19564d August 6, 2020 04:51

majecty force-pushed the f/copy-from-foundry branch from 00475f0 to d19564d Compare August 6, 2020 04:51

junha1 previously approved these changes Aug 6, 2020

View reviewed changes

majecty dismissed junha1’s stale review via 01076c7 August 6, 2020 06:23

majecty force-pushed the f/copy-from-foundry branch from d19564d to 01076c7 Compare August 6, 2020 06:23

majecty mentioned this pull request Aug 6, 2020

Make sync tests not to propagate transactions #1981

Merged

majecty force-pushed the f/copy-from-foundry branch from 01076c7 to ac45dc1 Compare August 6, 2020 07:43

sgkim126 requested changes Aug 13, 2020

View reviewed changes

majecty force-pushed the f/copy-from-foundry branch 2 times, most recently from dc5aad1 to 43bb362 Compare August 18, 2020 02:30

byeongjee previously approved these changes Aug 18, 2020

View reviewed changes

junha1 previously approved these changes Aug 19, 2020

View reviewed changes

MSNTCS previously approved these changes Aug 19, 2020

View reviewed changes

Seulgi Kim and others added 24 commits August 19, 2020 11:36

Remove unnecessarry attributes

6a9a485

Add document in the fields of HeaderDownloader struct

c2e7e8d

Remove unnecessary initialization of genesis header extra data

f5debd6

We gain nothing from initializing extra data of genesis header with hash of common params, because the check will be done by the same node, and it will always succeed. We can just use extra_data written in scheme file.

Remove verification_type

3255b03

CodeChain was using only the VerifierType::Canon.

Remove Verifier trait

0f75c4d

CodeChain is using only CanonVerifier. We are not using the `Verifier` abstraction.

Remove unnecessary serialize/deserialize while calling import_header

bef8d75

Make on_header_response receive moved Header instead of reference

8f2695e

Rename import_headers to import_verified_headers

c7e423c

Client::import_header function receives a header that is not verified. Importer::import_headers assumes that received headers are verified. To reduce misunderstanding, I renamed the function.

Rename import_sealed_block to import_generated_block

021506e

The name `import_sealed_block` does not give enough context.

Remove the unused argument of push_transaction

4287c1b

The h is always None.

Make the miner always have AccountProvider

80a7d6e

Currently, AccountProvider in the miner is optional to support unit tests who cannot use the account provider which uses the permanent storage. This commit makes the miner uses memory back-end AccountProvider for test.

Run mock tests

0f87e27

Fix mock tests

cc4f56a

I changed the test code only. The tests were deprecated. I updated RLP encoded data from CodeChain code and it worked.

Change mock's Header::default function to static function

32ce4f7

Change seal serialize code in mock

13fc568

The seal is an array of any RLP data. Wrapping them with Buffer makes invalid RLP encoding.

Add Header RLP encoding test in mock

1512b81

Remove PartialEq derive from Header and Block

56db6e9

Since Header has a cache of its hash using RefCell, the existence of the cache changes the compare result.

Add a hint comment that helps programmer to manage tests

d1caafd

Make typescript check type errors in test files

6d7d682

Make the client hold the miner directly

164321d

Currently, the client access the miner through the importer. It's better to give a method to access the miner directly.

Upgrade parking_lot to 0.11.0

6aea3b8

Upgrade vergen 3

62cded4

majecty dismissed stale reviews from MSNTCS, junha1, and byeongjee via 62cded4 August 19, 2020 02:37

majecty force-pushed the f/copy-from-foundry branch from 43bb362 to 62cded4 Compare August 19, 2020 02:37

majecty merged commit 9141174 into CodeChain-io:master Aug 19, 2020

majecty deleted the f/copy-from-foundry branch August 19, 2020 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move PRs from Foundry#1979

Move PRs from Foundry#1979
majecty merged 55 commits intoCodeChain-io:masterfrom
majecty:f/copy-from-foundry

majecty commented Jul 30, 2020 •

edited

Loading

Uh oh!

dynaxis commented Aug 5, 2020

Uh oh!

junha1 commented Aug 6, 2020

Uh oh!

MSNTCS left a comment

Uh oh!

majecty commented Aug 6, 2020

Uh oh!

junha1 commented Aug 6, 2020

Uh oh!

majecty commented Aug 6, 2020 •

edited

Loading

Uh oh!

sgkim126 left a comment

Uh oh!

majecty commented Aug 18, 2020

Uh oh!

majecty commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

majecty commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dynaxis commented Aug 5, 2020

Uh oh!

junha1 commented Aug 6, 2020

Uh oh!

MSNTCS left a comment

Choose a reason for hiding this comment

Uh oh!

majecty commented Aug 6, 2020

Uh oh!

junha1 commented Aug 6, 2020

Uh oh!

majecty commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgkim126 left a comment

Choose a reason for hiding this comment

Uh oh!

majecty commented Aug 18, 2020

Uh oh!

majecty commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

majecty commented Jul 30, 2020 •

edited

Loading

majecty commented Aug 6, 2020 •

edited

Loading