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

Block<T>.ProtocolVersion & BlockHeader.ProtocolVersion #1147

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Jan 6, 2021

This implements #1142.

@dahlia dahlia self-assigned this Jan 6, 2021
@dahlia dahlia changed the title WIP: Block<T>.ProtocolVersion & BlockHeader.ProtocolVersion Block<T>.ProtocolVersion & BlockHeader.ProtocolVersion Jan 7, 2021
@dahlia dahlia marked this pull request as ready for review January 7, 2021 05:27
@longfin longfin requested a review from junorouse January 7, 2021 05:37
@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #1147 (849e3c0) into main (9934f3a) will decrease coverage by 0.19%.
The diff coverage is 75.47%.

@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
- Coverage   87.60%   87.41%   -0.20%     
==========================================
  Files         346      347       +1     
  Lines       30049    30292     +243     
==========================================
+ Hits        26324    26479     +155     
- Misses       2092     2112      +20     
- Partials     1633     1701      +68     
Impacted Files Coverage Δ
...net/Blocks/InvalidBlockProtocolVersionException.cs 35.71% <35.71%> (ø)
Libplanet.Tests/Common/Action/DumbAction.cs 79.29% <38.46%> (-2.87%) ⬇️
Libplanet.Tests/Blocks/BlockHeaderTest.cs 63.03% <51.75%> (-9.00%) ⬇️
Libplanet/Blocks/Block.cs 88.40% <53.33%> (-0.83%) ⬇️
Libplanet.Tests/TestUtils.cs 83.64% <77.77%> (-0.07%) ⬇️
...sts/Blockchain/BlockChainTest.ValidateNextBlock.cs 94.80% <94.73%> (-0.12%) ⬇️
Libplanet.Tests/Blocks/BlockFixture.cs 100.00% <100.00%> (ø)
Libplanet.Tests/Blocks/BlockTest.cs 98.31% <100.00%> (+0.01%) ⬆️
Libplanet/Blockchain/BlockChain.cs 90.97% <100.00%> (+0.13%) ⬆️
Libplanet/Blocks/BlockHeader.cs 95.85% <100.00%> (+2.06%) ⬆️
... and 6 more

@dahlia
Copy link
Contributor Author

dahlia commented Jan 8, 2021

@planetarium/libplanet PTAL.

@@ -126,9 +135,11 @@ public Block(Bencodex.Types.Dictionary dict)
{
}

// FIXME: Should this be
Copy link
Member

Choose a reason for hiding this comment

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

Something is missing I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed the sentence.

@dahlia
Copy link
Contributor Author

dahlia commented Jan 8, 2021

@planetarium/libplanet Rebased. PTAL.

@limebell limebell self-requested a review January 8, 2021 06:31
junorouse
junorouse previously approved these changes Jan 8, 2021
Copy link

@junorouse junorouse left a comment

Choose a reason for hiding this comment

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

I understand that libplanet separately control the flow of actions through protocol version management (this patch) instead of creating a new action file (i.e., Buy {2,3,4}.cs), am I right? -- for proper patching of planetarium/lib9c#248

If so, this PR looks OK for me. However, from an attacker's point of view, if an attacker has enough mining power, the block's previous version can continue to be mined even if the new protocol version introduces.

There are some mitigations for it:

  1. Set the maximum number of blocks(=limit) allowed with the previous protocol version.
  2. Like lib9c, use an authorized miner to introduce a new version.
  3. Change to a consensus algorithm to that selecting the next block proposer randomly.
  4. ...

@dahlia
Copy link
Contributor Author

dahlia commented Jan 8, 2021

@junorouse Thanks for your review and suggestions!

However, from an attacker's point of view, if an attacker has enough mining power, the block's previous version can continue to be mined even if the new protocol version introduces.

Even if we don't merge this PR, that attackers can outpace honest nodes is the critical problem in itself and we need to address it as soon as possible. Therefore, IMHO we'd better deal with it in the separate issue/patches.

riemannulus
riemannulus previously approved these changes Jan 8, 2021
@dahlia dahlia merged commit 8d2e815 into planetarium:main Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants