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

Calculate and validate founders reward addresses #1170

Closed

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Oct 16, 2020

TODO

See #338

Motivation

Part of the bock subsidy. We merged the validation of founders reward amounts and general block subsidy framework at #1051 and we have a RFC for the whole process being written at #1129.

This PR deals with the founder reward addresses that should be present in the coinbase outputs that must match a calculated address according to current height.

  • Calculation of the address is done at founders_reward_address() and tested at test_founders_address().
  • Given a coinbase transaction and a calculated address find_output_with_address() will look into the outputs trying to get the address. Test at subsidy_is_valid_test().

Problem: Calculated address are transparent address while what we have inside outputs is P2SH scripts. We need to decode(or whatever is the right word here) the script to get an address. Resolved

This is a draft PR and currently need lot of adjustments but it is posted to get help with the above problem. Still, i will probably continue on top of it so feel free to make comments/suggestions for any part of it.

@oxarbitrage

This comment has been minimized.

@oxarbitrage oxarbitrage force-pushed the founders_reward_addresses branch from 560845f to 91434a9 Compare October 16, 2020 12:39
@yaahc yaahc requested a review from teor2345 October 16, 2020 22:10
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It looks like you're still working on this, here are some comments on the draft.

Comment on lines +67 to +76
if height >= blossom_height {
adjusted_height = Height(
blossom_height.0
+ ((height.0 - blossom_height.0) / (BLOSSOM_POW_TARGET_SPACING_RATIO as u32)),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we do post-blossom height adjustments in a few different parts of the code.

Is there a way to abstract them into a new function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a follow up ticket for this: #1311

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement S-blocked Status: Blocked on other tasks labels Oct 21, 2020
@teor2345

This comment has been minimized.

@teor2345 teor2345 removed the S-blocked Status: Blocked on other tasks label Oct 21, 2020
@oxarbitrage oxarbitrage force-pushed the founders_reward_addresses branch from 5bd294d to 0cb4a0e Compare October 22, 2020 19:42
@oxarbitrage oxarbitrage force-pushed the founders_reward_addresses branch from d2f6bc6 to 6c62321 Compare February 28, 2021 14:32
@oxarbitrage
Copy link
Contributor Author

I synchronized the Mainnet using this PR in 3 hours.

Comment on lines +133 to 135
if matching_values != matching_address {
return Err(SubsidyError::FoundersRewardDifferentOutput)?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a set intersection test, not an equality test.

It is ok to:

  • send the correct value to a different address
  • send other values to the correct address
  • send multiple correct values to the correct address

As long as at least one correct value is sent to the correct address.

Please also add tests for all these cases.

As an alternative, we could checkpoint on Canopy, and delete this code. (But we'll probably want to re-use this code in the Funding Streams implementation, so we might as well fix it now.)

@teor2345
Copy link
Contributor

teor2345 commented Mar 8, 2021

I synchronized the Mainnet using this PR in 3 hours.

This PR isn't a high priority, because we're going to checkpoint on Canopy, so we won't need to validate the founders reward.

We also need to be sure that this PR is correct before we merge it, and we're still finding bugs.

@teor2345 teor2345 added the P-Low label Mar 8, 2021
@teor2345
Copy link
Contributor

teor2345 commented Mar 8, 2021

Since we're checkpointing on Canopy, we can ignore Founders Reward validation. So this ticket is a low priority.

@teor2345
Copy link
Contributor

We decided to close this draft PR, and re-open it when we start working on ticket #338 again.

@teor2345 teor2345 closed this Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants