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

Change BlockChain to implement IReadOnlyList #205

Merged
merged 1 commit into from Apr 16, 2019
Merged

Change BlockChain to implement IReadOnlyList #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGES.md
View file
Open in desktop
Expand Up @@ -29,11 +29,18 @@ To be released.
given.
- Fixed a bug that TURN relay had been disconnected when being connected for longer than 5
minutes.
- `BlockChain<T>` became to implement `IReadOnlyList<Block<T>>`. [[#205]]
- `BlockChain<T>.Validate()` became to receive `IReadOnlyList<Block<<T>>`
instead of `IEnumerable<Block<T>>`. [[#205]]
- `IBlockPolicy<T>.ValidateBlocks()` and
`IBlockPolicy<T>.GetNextBlockDifficulty()` became to receive
`IReadOnlyList<Block<<T>>` instead of `IEnumerable<Block<T>>`. [[#205]]

[#185]: https://github.com/planetarium/libplanet/pull/185
[#187]: https://github.com/planetarium/libplanet/issues/187
[#190]: https://github.com/planetarium/libplanet/pull/190
[#193]: https://github.com/planetarium/libplanet/pull/193
[#205]: https://github.com/planetarium/libplanet/pull/205


Version 0.2.2
Expand Down
4 changes: 2 additions & 2 deletions Libplanet.Tests/Blockchain/BlockChainTest.cs
View file
Open in desktop
Expand Up @@ -354,11 +354,11 @@ public void EvaluateActions()
private sealed class NullPolicy<T> : IBlockPolicy<T>
where T : IAction, new()
{
public int GetNextBlockDifficulty(IEnumerable<Block<T>> blocks) =>
public int GetNextBlockDifficulty(IReadOnlyList<Block<T>> blocks) =>
blocks.Any() ? 1 : 0;

public InvalidBlockException ValidateBlocks(
IEnumerable<Block<T>> blocks,
IReadOnlyList<Block<T>> blocks,
DateTimeOffset currentTime
) => null;
}
Expand Down
44 changes: 17 additions & 27 deletions Libplanet.Tests/Blockchain/Policies/BlockPolicyTest.cs
View file
Open in desktop
@@ -1,6 +1,5 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Security.Cryptography;
using Libplanet.Blockchain.Policies;
Expand Down Expand Up @@ -62,27 +61,27 @@ public void GetNextBlockDifficulty()

Assert.Equal(
0,
policy.GetNextBlockDifficulty(blocks.Take(0))
policy.GetNextBlockDifficulty(blocks.Take(0).ToList())
);
Assert.Equal(
1,
policy.GetNextBlockDifficulty(blocks.Take(1))
policy.GetNextBlockDifficulty(blocks.Take(1).ToList())
);
Assert.Equal(
2,
policy.GetNextBlockDifficulty(blocks.Take(2))
policy.GetNextBlockDifficulty(blocks.Take(2).ToList())
);
Assert.Equal(
3,
policy.GetNextBlockDifficulty(blocks.Take(3))
policy.GetNextBlockDifficulty(blocks.Take(3).ToList())
);
Assert.Equal(
2,
policy.GetNextBlockDifficulty(blocks.Take(4))
policy.GetNextBlockDifficulty(blocks.Take(4).ToList())
);
Assert.Equal(
3,
policy.GetNextBlockDifficulty(blocks.Take(5))
policy.GetNextBlockDifficulty(blocks.Take(5).ToList())
);
Assert.Equal(
2,
Expand All @@ -104,8 +103,7 @@ public void ValidateBlocks()
{
fields.Index = 1;
return fields;
}
),
}).ToList(),
FixtureEpoch + new TimeSpan(0, 1, 0)
)
);
Expand All @@ -119,8 +117,7 @@ public void ValidateBlocks()
{
fields.PreviousHash = default(HashDigest<SHA256>);
return fields;
}
),
}).ToList(),
FixtureEpoch + new TimeSpan(0, 1, 0)
)
);
Expand All @@ -134,8 +131,7 @@ public void ValidateBlocks()
{
fields.Index = fields.Index == 0 ? 0 : 2;
return fields;
}
),
}).ToList(),
FixtureEpoch + new TimeSpan(1, 1, 0)
)
);
Expand All @@ -148,32 +144,28 @@ public void ValidateBlocks()
Assert.Null(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (1, 1), (3, 2) }
),
new[] { (0, 0), (1, 1), (3, 2) }).ToList(),
FixtureEpoch + new TimeSpan(3, 1, 0)
)
);
Assert.IsType<InvalidBlockDifficultyException>(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (1, 1), (3, 1) }
),
new[] { (0, 0), (1, 1), (3, 1) }).ToList(),
FixtureEpoch + new TimeSpan(3, 1, 0)
)
);
Assert.Null(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (1, 1), (5, 2), (9, 1) }
),
new[] { (0, 0), (1, 1), (5, 2), (9, 1) }).ToList(),
FixtureEpoch + new TimeSpan(9, 1, 0)
)
);
Assert.Null(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (1, 1), (5, 2), (9, 5) }
),
new[] { (0, 0), (1, 1), (5, 2), (9, 5) }).ToList(),
FixtureEpoch + new TimeSpan(9, 1, 0)
)
);
Expand All @@ -193,8 +185,7 @@ public void ValidateBlocks()
}

return fields;
}
),
}).ToList(),
FixtureEpoch + new TimeSpan(5, 1, 0)
)
);
Expand All @@ -203,8 +194,7 @@ public void ValidateBlocks()
Assert.IsType<InvalidBlockTimestampException>(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (2, 1), (1, 2) }
),
new[] { (0, 0), (2, 1), (1, 2) }).ToList(),
FixtureEpoch + new TimeSpan(2, 1, 0)
)
);
Expand All @@ -213,8 +203,8 @@ public void ValidateBlocks()
Assert.Null(
policy.ValidateBlocks(
MineBlocks(
new[] { (0, 0), (1, 1), (3, 2), (7, 3), (9, 2) }
),
new[] { (0, 0), (1, 1), (3, 2), (7, 3), (9, 2) })
.ToList(),
FixtureEpoch + new TimeSpan(9, 1, 0)
)
);
Expand Down
14 changes: 11 additions & 3 deletions Libplanet/Blockchain/BlockChain.cs
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@
[assembly: InternalsVisibleTo("Libplanet.Tests")]
namespace Libplanet.Blockchain
{
public class BlockChain<T> : IEnumerable<Block<T>>
public class BlockChain<T> : IReadOnlyList<Block<T>>
where T : IAction, new()
{
private readonly ReaderWriterLockSlim _rwlock;
Expand Down Expand Up @@ -83,8 +83,15 @@ public Block<T> Tip
get; private set;
}

/// <inheritdoc/>
int IReadOnlyCollection<Block<T>>.Count =>
checked((int)Store.CountIndex(Id.ToString()));

internal IStore Store { get; }

/// <inheritdoc/>
public Block<T> this[int index] => this[(long)index];
Copy link
Contributor

Choose a reason for hiding this comment

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

@earlbread Although it's late, we should make it throw ArgumentOutOfRangeException instead of IndexOutOfRangeException, because collection types in System.Collections.Generic throw ArgumentOutOfRangeException and only arrays throw IndexOutOfRangeException. See also this SO answer:

Note that List<T> throws ArgumentOutOfRangeException for the same cases where arrays use IndexOutOfRangeException.

Could you change this in the next patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change it in the next patch!


public Block<T> this[long index]
{
get
Expand All @@ -111,7 +118,7 @@ public Block<T> Tip
}

public void Validate(
IEnumerable<Block<T>> blocks,
IReadOnlyList<Block<T>> blocks,
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is a public method, this change also should be written in the changelog.

DateTimeOffset currentTime
)
{
Expand Down Expand Up @@ -236,8 +243,9 @@ public void Append(Block<T> block, DateTimeOffset currentTime)
block.Validate(
currentTime,
a => GetStates(new[] { a }, tip).GetValueOrDefault(a));
Validate(Enumerable.Append(this, block), currentTime);
Enumerable.Append(this, block);
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it's late, this seems a miss; Enumerable.Append() method returns a new IEnumerable<T> that has one more item, but this code doesn't use its return value. According to the docs:

This method does not modify the elements of the collection. Instead, it creates a copy of the collection with the new element.

To me, the code before changed seems correct. @earlbread Could you fix this in the pull request #210?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this code was changed because the type of Validate() method's first parameter was changed from IEnumerable<Bock<T>> to IReadOnlyList<Block<T>> in this pull request. To fix this quickly, it could be like Validate(Enumerable.Append(this, block).ToList(), currentTime);, I'm skeptic if it's good though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems not needed anymore. I'll remove it in #210.


Validate(this, currentTime);
_rwlock.EnterWriteLock();
try
{
Expand Down
6 changes: 3 additions & 3 deletions Libplanet/Blockchain/Policies/BlockPolicy.cs
View file
Open in desktop
Expand Up @@ -54,15 +54,15 @@ public BlockPolicy(TimeSpan blockInterval)
/// An appropriate interval between consecutive <see cref="Block{T}"/>s.
/// It is usually from 20 to 30 seconds.
/// <para>If a previous interval took longer than this
/// <see cref="GetNextBlockDifficulty(IEnumerable{Block{T}})"/> method
/// <see cref="GetNextBlockDifficulty(IReadOnlyList{Block{T}})"/> method
/// raises the <see cref="Block{T}.Difficulty"/>. If it took shorter
/// than this <see cref="Block{T}.Difficulty"/> is dropped.</para>
/// </summary>
public TimeSpan BlockInterval { get; }

/// <inheritdoc />
public InvalidBlockException ValidateBlocks(
IEnumerable<Block<T>> blocks,
IReadOnlyList<Block<T>> blocks,
DateTimeOffset currentTime
)
{
Expand Down Expand Up @@ -129,7 +129,7 @@ DateTimeOffset currentTime
}

/// <inheritdoc />
public int GetNextBlockDifficulty(IEnumerable<Block<T>> blocks)
public int GetNextBlockDifficulty(IReadOnlyList<Block<T>> blocks)
{
return ExpectDifficulties(blocks, yieldNext: true)
.First(t => t.Block == null)
Expand Down
4 changes: 2 additions & 2 deletions Libplanet/Blockchain/Policies/IBlockPolicy.cs
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@ public interface IBlockPolicy<T>
/// <em>invalid</em>, or <c>null</c> if <paramref name="blocks"/> are
/// <em>valid</em>.</returns>
InvalidBlockException ValidateBlocks(
IEnumerable<Block<T>> blocks,
IReadOnlyList<Block<T>> blocks,
DateTimeOffset currentTime
);

Expand All @@ -43,6 +43,6 @@ DateTimeOffset currentTime
/// followed by a new <see cref="Block{T}"/> to be mined.</param>
/// <returns>A right <see cref="Block{T}.Difficulty"/>
/// for a new <see cref="Block{T}"/> to be mined.</returns>
int GetNextBlockDifficulty(IEnumerable<Block<T>> blocks);
int GetNextBlockDifficulty(IReadOnlyList<Block<T>> blocks);
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes in this file also break API backward compatibility, we need to write the corresponding changelogs.

}
}