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

Use next available key on duplicate coinbase commitment #2737

Conversation

de1acr0ix
Copy link
Contributor

If a coinbase commitment hits a duplication and there is no transactions to mine, it is possible that coinbase cannot be built for quite a long time. This change tries to build coinbase with an empty key identifier immediately in this case. The listening wallet will then use the next available key to build a coinbase commitment, which is different from the previous one and is unlikely to hit the duplication.

If a coinbase commitment hits a duplication and there is no transactions
to mine, it is possible that coinbase cannot be built for quite a long
time. This change tries to build coinbase with an empty key identifier
immediately in this case. The listening wallet will then use the next
available key to build a coinbase commitment, which is different from
the previous one and is unlikely to hit the duplication.
@garyyu
Copy link
Contributor

garyyu commented Apr 9, 2019

Thanks for your 1st PR 👍
I'm afraid I don't understand here it is possible that coinbase cannot be built for quite a long time., do you see the real case? and why quite a long time? I see there just a 100ms sleep there.

Could you please share more info?

@garyyu
Copy link
Contributor

garyyu commented Apr 9, 2019

perhaps #1491 related.

match e {
self::Error::Chain(c) => match c.kind() {
chain::ErrorKind::DuplicateCommitment(_) => {
debug!(
"Duplicate commit for potential coinbase detected. Trying next derivation."
);
// use the next available key to generate a different coinbase commitment
new_key_id = None;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@de1acr0ix
Copy link
Contributor Author

de1acr0ix commented Apr 9, 2019

Thanks for your 1st PR
I'm afraid I don't understand here it is possible that coinbase cannot be built for quite a long time., do you see the real case? and why quite a long time? I see there just a 100ms sleep there.

Could you please share more info?

We have met this in production some time (stratum server stucks for hours). The commitment seems only depend on the amount and the key itself (please correct me if I am wrong). If we hit the duplication and there is no transactions, the amount will always be just the coinbase reward itself. If we do not change the key, the commitment does not change until there is a transaction (amount changes as there are transaction fees). Hence the coinbase cannot be successfully built and stratum server stalls.

@ignopeverell ignopeverell changed the base branch from master to milestone/1.1.0 April 15, 2019 22:05
@ignopeverell ignopeverell changed the base branch from milestone/1.1.0 to master April 15, 2019 22:06
@ignopeverell ignopeverell merged commit 56fed50 into mimblewimble:master Apr 15, 2019
@de1acr0ix de1acr0ix deleted the next_key_id_on_duplicate_coinbase_commitment branch April 16, 2019 01:50
@antiochp antiochp added this to the 1.1.0 milestone Jun 5, 2019
@antiochp antiochp added the release notes To be included in release notes (of relevant milestone). label Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes To be included in release notes (of relevant milestone).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants