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

feat: Allow miner add an arbitrary message into the cellbase #1000

Merged
merged 3 commits into from
Jun 15, 2019

Conversation

driftluo
Copy link
Collaborator

@driftluo driftluo commented Jun 13, 2019

Allow miner add an arbitrary message into the cellbase

closed #888

@driftluo driftluo requested a review from a team June 13, 2019 05:57
@nervos-bot
Copy link

nervos-bot bot commented Jun 13, 2019

@yangby-cryptape is assigned as the chief reviewer

@driftluo driftluo force-pushed the allow-miner-arbitrary-message branch from e663d6d to 60fea84 Compare June 13, 2019 06:26
@@ -117,8 +117,16 @@ cell_output_cache_size = 128
#
# ckb cli secp256k1-lock <pubkey>
#
# Also, ckb allows the miner to add any data to the cellbase that he has dug out.
# The data must be A 0x-prefixed hex string. **Please note** that the data cannot be
# larger than the capacity value of the current cellbase. If it is larger,
Copy link
Contributor

Choose a reason for hiding this comment

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

The normal users don't know the capacity value of the cellbase, this data size may be confused for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. First of all, it is not a required field here. If you don’t understand it, you will not fill it out.
  2. The capacity of cellbase is dynamically changed. It is impossible to determine the size of data by a static value.
  3. I considered a lot of processing solutions, and finally decided to write the details in the comment, and verify the value when generating the block template to ensure that the user can find the filling error as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to truncate the data when it is too long. And add a warning here to tell users that do not rely on the data here, it may be truncated when the block reward capacity is not enough to hold the data.

Copy link
Contributor

@u2 u2 Jun 13, 2019

Choose a reason for hiding this comment

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

It should never be panic if the data size overflow sometimes, for the user view it's a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current processing will not panic if overflow RPC will return an error when getting the block template.

Copy link
Collaborator Author

@driftluo driftluo Jun 14, 2019

Choose a reason for hiding this comment

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

I changed the implementation to forced truncate, need to review if the calculation of capacity is correct

test/src/specs/mining/bootstrap.rs Outdated Show resolved Hide resolved
resource/ckb.toml Outdated Show resolved Hide resolved
@@ -117,8 +117,16 @@ cell_output_cache_size = 128
#
# ckb cli secp256k1-lock <pubkey>
#
# Also, ckb allows the miner to add any data to the cellbase that he has dug out.
# The data must be A 0x-prefixed hex string. **Please note** that the data cannot be
# larger than the capacity value of the current cellbase. If it is larger,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to truncate the data when it is too long. And add a warning here to tell users that do not rely on the data here, it may be truncated when the block reward capacity is not enough to hold the data.

@driftluo driftluo force-pushed the allow-miner-arbitrary-message branch from 1227071 to 0d204f5 Compare June 14, 2019 03:49
miner/src/block_assembler.rs Outdated Show resolved Hide resolved
@doitian doitian merged commit 0a9520e into nervosnetwork:develop Jun 15, 2019
@driftluo driftluo deleted the allow-miner-arbitrary-message branch June 15, 2019 04:21
This was referenced Jun 21, 2019
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.

Allow miner add an arbitrary message into the cellbase
5 participants