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

fix(block-sync): check coinbase maturity #4168

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jun 7, 2022

Description

  • Checks for correct coinbase maturity in block sync validator
  • Adds unit tests for check_coinbase_maturity and check_coinbase_reward helpers

Motivation and Context

Coinbase maturity should be checked on block sync.

How Has This Been Tested?

Additional unit tests pass. Manually by syncing on dibbler testnet.

@CjS77
Copy link
Collaborator

CjS77 commented Jun 7, 2022

It's a bit odd that we're asking the miners to add the maturity themselves.
I thought our code was such that if a coinbase output is being spent, we chack if it was mined > maturity blocks ago.

@sdbondi
Copy link
Member Author

sdbondi commented Jun 7, 2022

There are currently no special rules for spending a coinbase. We check that the maturity is correctly set at creation time as per consensus, however this check was missing from the block sync validation.

We could add a separate spend rule for coinbases, however, this would require tracking* and fetching the mined height for each coinbase** vs statelessly comparing the maturity to the current height.

It feels simpler to enforce the correct maturity for the miner at coinbase creation but open to changing the approach esp if there are downsides/oddness.

* UTXO mined_height is currently tracked but is informational and does not form part of the protocol/consensus rules
** Alternatively we could use the mmr pos of the coinbase input <= output_mmr_size in the header@{height-coinbase_maturity} to check maturity without tracking mined height.

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

I've been thinking on this. I agree with you that the pros outweigh the cons here. I initially balked because we have this policy of not adding redundant info to blocks (if it can be derived, let it be derived). But in this case, we're not adding a new field, we're using an existing one, for the purpose it was intended.

And in fact, if a miner wants to lock up a UTXO for longer than the minimum maturity (I don't know why they would, but 🤷) then they are free to do so.

@aviator-app aviator-app bot merged commit 38b4af7 into tari-project:testnet-dibbler Jun 8, 2022
@sdbondi sdbondi deleted the core-block-sync-full-validation branch June 8, 2022 08:36
aviator-app bot pushed a commit that referenced this pull request Jun 10, 2022
Description
---

Clean up duplicate maturity check introduced in #4168 

Motivation and Context
---
In PR #4168 I renamed`check_coinbase_reward` to `check_coinbase` but then decided that since `check_coinbase` doesn't fully check the coinbase, to split the checks into `check_coinbase_reward` and `check_coinbase_maturity` - however I forgot to take the code out of `check_coinbase_reward`.

[Duplicated code](https://github.com/tari-project/tari/pull/4168/files#diff-c76c3fe2827c9e3510483d26bf1c51ef22e25182e26719cfe26f123dfe598966R713)

How Has This Been Tested?
---
More tests are needed
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.

2 participants