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

Cancel mining if blockchain's tip index is changed #517

Merged
merged 5 commits into from Sep 23, 2019

Conversation

@limebell
Copy link
Contributor

commented Sep 18, 2019

This patch implements and closes #460. Some XML descriptions and test cases are included in this PR.

@limebell limebell requested a review from planetarium/libplanet Sep 18, 2019
@limebell limebell self-assigned this Sep 18, 2019
@limebell limebell force-pushed the limebell:mine-cancel branch from fed45f1 to 88496db Sep 18, 2019
@limebell limebell changed the title Cancel mining if blockchain's tip index in changed Cancel mining if blockchain's tip index is changed Sep 18, 2019
@limebell limebell force-pushed the limebell:mine-cancel branch from 88496db to e277dab Sep 18, 2019
@dahlia

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Tests in several environments failed due to timeout.

@limebell limebell force-pushed the limebell:mine-cancel branch 3 times, most recently from 54d44a1 to 512f5bf Sep 19, 2019
@codecov

This comment has been minimized.

Copy link

commented Sep 19, 2019

Codecov Report

Merging #517 into master will increase coverage by 0.02%.
The diff coverage is 96.57%.

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage    90.3%   90.32%   +0.02%     
==========================================
  Files         201      201              
  Lines       14928    15013      +85     
==========================================
+ Hits        13481    13561      +80     
- Misses       1170     1174       +4     
- Partials      277      278       +1
Impacted Files Coverage Δ
Libplanet/Blocks/Block.cs 90.68% <100%> (+0.03%) ⬆️
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.66% <100%> (+0.13%) ⬆️
Libplanet.Tests/Blockchain/NullPolicy.cs 100% <100%> (ø) ⬆️
Libplanet/Hashcash.cs 100% <100%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 93.95% <91.42%> (-0.24%) ⬇️
Libplanet.Tests/Net/SwarmTest.cs 98.87% <92.85%> (-0.23%) ⬇️
Libplanet.Tests/Net/SwarmTest.cs Outdated Show resolved Hide resolved
Libplanet.Tests/Net/SwarmTest.cs Outdated Show resolved Hide resolved
Libplanet/Hashcash.cs Outdated Show resolved Hide resolved
Libplanet/Blocks/Block.cs Outdated Show resolved Hide resolved
Assert.Throws<InvalidBlockIndexException>(() => _blockChain.Append(block));
Assert.False(called);

// TODO: Add test cases for swap

This comment has been minimized.

Copy link
@earlbread

earlbread Sep 19, 2019

Member

It would be good to have an issue for this.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@limebell limebell force-pushed the limebell:mine-cancel branch from 512f5bf to afa139d Sep 23, 2019
@limebell limebell force-pushed the limebell:mine-cancel branch from afa139d to 9494bee Sep 23, 2019
@limebell limebell requested review from libplanet and planetarium/libplanet and removed request for libplanet Sep 23, 2019
@limebell limebell force-pushed the limebell:mine-cancel branch 3 times, most recently from c48869d to 5184241 Sep 23, 2019
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Libplanet/Hashcash.cs Outdated Show resolved Hide resolved
Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Libplanet/Hashcash.cs Outdated Show resolved Hide resolved
throw new OperationCanceledException(
"Mining canceled due to change of tip index");
}
else

This comment has been minimized.

Copy link
@longfin

longfin Sep 23, 2019

Member

How about just re-throw it?

Suggested change
else
throw;

This comment has been minimized.

Copy link
@limebell

limebell Sep 23, 2019

Author Contributor

I used TaskCanceledException here because other tasks used in Swarm<T> throw TaskCanceledException when cancel is requested.

@limebell limebell force-pushed the limebell:mine-cancel branch from 5184241 to 838655e Sep 23, 2019
@limebell limebell force-pushed the limebell:mine-cancel branch from 838655e to bf8de58 Sep 23, 2019
@limebell limebell requested review from dahlia and moreal Sep 23, 2019
@dahlia
dahlia approved these changes Sep 23, 2019
@dahlia dahlia merged commit 6cf0c16 into planetarium:master Sep 23, 2019
15 checks passed
15 checks passed
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
docs Libplanet docs generated by DocFX
Details
license/cla Contributor License Agreement is signed.
Details
planetarium.libplanet Build #20190923.10 succeeded
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_Benchmark) Windows_NETCore_Benchmark 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
4 participants
You can’t perform that action at this time.