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.

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

Option to reuse states calculated by trusted peers #343

Merged
merged 21 commits into from Jul 22, 2019

Conversation

@dahlia
Copy link
Member

commented Jul 9, 2019

This patch implements (in a quick-and-dirty way) the idea suggested from #272. There are direct accesses to IStore from Swarm<T>; these should be fixed in the near future, as they have possibility to cause unexpected behaviors on race conditions.

Closes #272.

@dahlia dahlia added this to the 0.5.0 milestone Jul 9, 2019

@dahlia dahlia self-assigned this Jul 9, 2019

@dahlia dahlia force-pushed the dahlia:trusted-peers branch from 0a39ade to 22452bf Jul 10, 2019

@dahlia dahlia force-pushed the dahlia:trusted-peers branch from 22452bf to 6e4e4f3 Jul 11, 2019

@dahlia dahlia force-pushed the dahlia:trusted-peers branch from 6e4e4f3 to 9553863 Jul 11, 2019

@dahlia dahlia changed the title WIP: Option to reuse states calculated by trusted peers Option to reuse states calculated by trusted peers Jul 11, 2019

@dahlia dahlia requested review from longfin and earlbread Jul 11, 2019

Libplanet/Net/Messages/RecentStates.cs Show resolved Hide resolved
Libplanet/Net/Messages/RecentStates.cs Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Show resolved Hide resolved
Libplanet.Tests/Net/Messages/RecentStatesTest.cs Outdated Show resolved Hide resolved
Libplanet/Net/Messages/RecentStates.cs Outdated Show resolved Hide resolved
Libplanet/Net/Messages/RecentStates.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Messages/RecentStates.cs Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
).ToArray();
if (evaluateActions)
{
ValidateNonce(block);

This comment has been minimized.

Copy link
@longfin

longfin Jul 12, 2019

Member

it skips ValidateNonce() when evaluationActions is false. is it intended?

This comment has been minimized.

Copy link
@dahlia

dahlia Jul 12, 2019

Author Member

It was intended, but I'm going to reconsider this. Until yesterday, I thought tx nonces cannot be made because these require the entire actions to be evaluated, but it turns out they probably do not really.

This comment has been minimized.

Copy link
@longfin

This comment has been minimized.

Copy link
@dahlia

dahlia Jul 16, 2019

Author Member

I made tx nonces completely independent from action evaluation, and now these are not included in RecentStates message, but always calculated by each node instead, because these are merely adding numbers. See also: de75e11

Libplanet/Net/Messages/RecentStates.cs Outdated Show resolved Hide resolved
try
{
reply = await socket.ReceiveMultipartMessageAsync(
timeout: TimeSpan.FromSeconds(30),

This comment has been minimized.

Copy link
@longfin

longfin Jul 12, 2019

Member

How about make TimeSpan.FromSeconds(30) to configurable?

This comment has been minimized.

Copy link
@dahlia

dahlia Jul 15, 2019

Author Member

I'd tried parametrizing this, but there were sufferings. First of all, this private method is called by several layers of other internal/private methods, and then this parameter turns out too trivial case from the perspective of the lowest public API. Also, TimeSpan, a value type, cannot have a complex default value (such as “30 seconds”), so I had to made a pair of overloaded internal/private method to make it optional, and to me it's like overkill.

So IMHO your suggestion seems not that cost-effective as it has only small benefit.

Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
@codecov

This comment has been minimized.

Copy link

commented Jul 12, 2019

Codecov Report

Merging #343 into master will increase coverage by 5.41%.
The diff coverage is 94.72%.

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage    82.6%   88.01%   +5.41%     
==========================================
  Files         165      192      +27     
  Lines       12122    13402    +1280     
==========================================
+ Hits        10013    11796    +1783     
+ Misses       1887     1337     -550     
- Partials      222      269      +47
Impacted Files Coverage Δ
Libplanet/Blockchain/BlockChain.cs 95.7% <100%> (+0.09%) ⬆️
Libplanet/Net/Messages/Message.cs 84.21% <100%> (+0.57%) ⬆️
Libplanet/Net/Messages/GetRecentStates.cs 100% <100%> (ø)
Libplanet/Net/Messages/RecentStates.cs 100% <100%> (ø)
Libplanet/Net/Swarm.cs 78.77% <90.33%> (+1.93%) ⬆️
Libplanet.Tests/Net/Messages/RecentStatesTest.cs 91.52% <91.52%> (ø)
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.12% <92.52%> (-0.66%) ⬇️
Libplanet.Tests/Net/SwarmTest.cs 93.05% <98.92%> (+0.66%) ⬆️
....Stun.Tests/Stun/Messages/ConnectionAttemptTest.cs 100% <0%> (ø)
... and 58 more
@codecov

This comment has been minimized.

Copy link

commented Jul 12, 2019

Codecov Report

Merging #343 into master will increase coverage by 5.36%.
The diff coverage is 95.07%.

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage    82.6%   87.96%   +5.36%     
==========================================
  Files         165      192      +27     
  Lines       12122    13394    +1272     
==========================================
+ Hits        10013    11782    +1769     
+ Misses       1887     1336     -551     
- Partials      222      276      +54
Impacted Files Coverage Δ
Libplanet/Blockchain/BlockChain.cs 95.76% <100%> (+0.15%) ⬆️
Libplanet/Net/Messages/Message.cs 84.21% <100%> (+0.57%) ⬆️
Libplanet/Net/Messages/GetRecentStates.cs 100% <100%> (ø)
Libplanet/Net/Messages/RecentStates.cs 100% <100%> (ø)
Libplanet.Tests/Net/Messages/RecentStatesTest.cs 88.6% <88.6%> (ø)
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.21% <93.4%> (-0.58%) ⬇️
Libplanet/Net/Swarm.cs 78.64% <93.91%> (+1.79%) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 91.89% <98.79%> (-0.5%) ⬇️
... and 59 more
dahlia and others added 2 commits Jul 12, 2019
Missing links to issue/pull request
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
dahlia and others added 4 commits Jul 15, 2019
Reword docs
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>

@dahlia dahlia requested review from longfin and earlbread Jul 16, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

@earlbread @longfin I fixed things you'd addressed in review. Please review this patch again.

Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Libplanet/Net/Messages/RecentStates.cs Show resolved Hide resolved

/// <summary>
/// A reply to <see cref="GetRecentStates"/>.
/// Contains the calculated recent states, transaction nonces, and state references.

This comment has been minimized.

Copy link
@earlbread

earlbread Jul 17, 2019

Member
Suggested change
/// Contains the calculated recent states, transaction nonces, and state references.
/// Contains the calculated recent states and state references.

This comment has been minimized.

Copy link
@earlbread

earlbread Jul 22, 2019

Member

Shouldn't we change this?

Libplanet/Net/Swarm.cs Show resolved Hide resolved
Libplanet/Net/Swarm.cs Show resolved Hide resolved

@dahlia dahlia force-pushed the dahlia:trusted-peers branch from 2c9fd75 to 8a99042 Jul 17, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

I conducted a small bench (with 580 blocks); here's the results:

  • Preloading time before this patch: 00:01:15.1941430
  • Preloading time after this patch, without trusted peers: 00:00:37.8942200
  • Preloading time after this patch, with trusted peers: 00:00:34.7963090

It's unintended and strange that trusting peers does not seem to affect the performance (37 sec vs 34 sec), but other changes in this patch seem to have some effect (1 min 15 sec → 37 sec). 🤔

@dahlia

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

There were two bugs:

  1. that serializing a RecentStates message fails where more than one states were updated at a block (i.e., multiple accounts shares the same state ref) — 46f4b96, and
  2. that PreloadAsync() does not set block states even if trustedStateValidators are empty or unused — 3a335b2.

The reason the last bench was so strange is because those two bugs happened at a time, which led no action evaluation, hence null states.

Anyway I fixed these bugs, and conducted a similar bench again (with 326 blocks). Here's the results, which are more sane 🙄:

  • Preloading time before this patch: 00:00:34.2664440
  • Preloading time after this patch, without trusted peers: 00:00:33.0748250
  • Preloading time after this patch, with trusted peers: 00:00:22.7336880

It seems trusting peers makes IBL about 1.5 times faster.

@dahlia dahlia requested review from earlbread and longfin Jul 20, 2019


/// <summary>
/// A reply to <see cref="GetRecentStates"/>.
/// Contains the calculated recent states, transaction nonces, and state references.

This comment has been minimized.

Copy link
@earlbread

earlbread Jul 22, 2019

Member

Shouldn't we change this?

@dahlia dahlia dismissed stale reviews from earlbread and longfin via 19b6eab Jul 22, 2019

@dahlia dahlia force-pushed the dahlia:trusted-peers branch from 3a335b2 to 19b6eab Jul 22, 2019

@dahlia dahlia requested review from longfin and earlbread Jul 22, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

@earlbread I fixed and amended the commit.

@dahlia dahlia merged commit 4100811 into planetarium:master Jul 22, 2019

16 checks passed

WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/patch 94.72% of diff hit (target 82.6%)
Details
codecov/project 88.01% (+5.41%) compared to f418686
Details
docs Libplanet docs generated by DocFX
Details
license/cla Contributor License Agreement is signed.
Details
planetarium.libplanet Build #20190722.1 had test failures
Details
planetarium.libplanet (Linux_Mono) Linux_Mono succeeded
Details
planetarium.libplanet (Linux_NETCore) Linux_NETCore succeeded
Details
planetarium.libplanet (Windows_Mono) Windows_Mono succeeded
Details
planetarium.libplanet (Windows_NETCore) Windows_NETCore succeeded
Details
planetarium.libplanet (Windows_NETCore_coverage) Windows_NETCore_coverage succeeded
Details
planetarium.libplanet (Windows_NETFramework) Windows_NETFramework succeeded
Details
planetarium.libplanet (macOS_Mono) macOS_Mono succeeded
Details
planetarium.libplanet (macOS_NETCore) macOS_NETCore succeeded
Details
planetarium.libplanet (macOS_Unity) macOS_Unity succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.