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/coinbase pay to contract #3164

Merged
merged 41 commits into from
Jun 29, 2022
Merged

Feat/coinbase pay to contract #3164

merged 41 commits into from
Jun 29, 2022

Conversation

jcnelson
Copy link
Member

@jcnelson jcnelson commented Jun 8, 2022

EDIT: this PR now supports building coinbase transactions that pay to either a smart contract or an alternative standard principal. The latter permits the miner to pay to a cold storage address automatically.

This PR implements #3101 by altering the TransactionPayload::Coinbase(..) type variant to include an optional smart contract address alternative recipient address, be it a contract or standard principal address. If given, the encoded transaction payload will have ID 0x05, and will include a serialized smart contract identifier principal. When processed in epoch 2.1, the block reward will be transferred to this smart contract address instead of the miner. In addition, I have updated the node software to accept a pay_to_contract contract contract or standard address under the [miner] heading called block_reward_address, which if given, will cause the node to create pay-to-contract alternative recipient coinbases for blocks mined after epoch 2.1 (the node ignores this field in epoch 2.05 and earlier).

I have added test coverage for:

  • the new transaction encoding
  • the static transaction validation tests (which are now epoch-gated to check for this new variant)
  • the account crediting logic
  • the miner's ability to produce a valid history of blocks that pay to a smart contract OR standard principal
  • the relayer's ability to reject pay-to-contract OR pay-to-alt-principal blocks that are mined before epoch 2.1
  • the node's ability to mine pay-to-contract OR pay-to-alt-principal blocks only once epoch 2.1 goes live

The code includes the relevant feature gating to ensure that a pay-to-contract pay-to-alt-principal block can only be successfully preprocessed in epoch 2.1. This prevents a Stacks 2.1-series node from accepting a pay-to-contract pay-to-alt-principal block at all before epoch 2.1 goes live. But, this differs slightly from how a 2.05-series node will handle such a block -- the 2.05-series node will not even be able to decode it, thereby making any attempt to push such a block to it fail with either an HTTP 400 (if sent via HTTP) or a temporary ban (if sent via p2p). I can make the 2.1-series node just as strict by adding feature gates to the p2p and HTTP endpoints, but I'm not sure it will make an appreciable difference in safety. Also, I'm not sure it's a good idea to make 2.1-series nodes ban each other for this, since if it turns out there's an off-by-one error somewhere in the gating logic that causes nodes to produce these blocks before they're supported, the nodes could accidentally all ban each other, leading to a catastrophic network partition. Open to feedback on this.

…ress for a block, not the address that receives the coinbase
…inbase variant. Add the codec logic for it. Add epoch 2.1 gating in the static transaction validation logic so that a block that gets preprocessed before 2.1 with a pay-to-contract coinbase will not be treated as valid (which stops a 2.1-series node from even accepting such a block into the staging DB before the feature is supported).
…tely from the miner address, and update the reward disbursment code to send the block reward to this address if it is given. Expand the unit tests to cover this case.
…tic transaction validation logic so that we never store a pay-to-contract block mined before 2.1. Also, API sync with the new coinbase payload type. In addition, when rejecting a block during pre-processing mask NotFound errors so that the code path returns InvalidStacksBlock.
…t the contract, not the miner, receives the expected block rewards
…ses, and update all uses of the coinbase type variant to include the optional contract ID
… the config file, and the current epoch is 2.1
…-contract blocks after epoch 2.1 starts, if configured to do so
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #3164 (e8c85f8) into next (00ca64b) will decrease coverage by 0.41%.
The diff coverage is 89.81%.

❗ Current head e8c85f8 differs from pull request most recent head 6376520. Consider uploading reports for the commit 6376520 to get more accurate results

@@            Coverage Diff             @@
##             next    #3164      +/-   ##
==========================================
- Coverage   84.19%   83.78%   -0.42%     
==========================================
  Files         268      268              
  Lines      212573   213534     +961     
==========================================
- Hits       178981   178902      -79     
- Misses      33592    34632    +1040     
Impacted Files Coverage Δ
clarity/src/vm/docs/mod.rs 86.30% <ø> (ø)
src/chainstate/stacks/db/mod.rs 87.26% <ø> (ø)
src/cost_estimates/fee_medians.rs 93.27% <ø> (ø)
src/cost_estimates/fee_scalar.rs 90.78% <ø> (ø)
src/main.rs 0.08% <0.00%> (ø)
src/net/download.rs 32.43% <0.00%> (-8.74%) ⬇️
src/net/rpc.rs 31.50% <0.00%> (-0.21%) ⬇️
testnet/stacks-node/src/tests/epoch_21.rs 90.85% <19.67%> (-5.84%) ⬇️
testnet/stacks-node/src/config.rs 56.02% <20.00%> (-0.25%) ⬇️
src/chainstate/stacks/auth.rs 97.82% <80.00%> (-0.03%) ⬇️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 875eb49...6376520. Read the comment docs.

… it has a microblock parent stream and crosses an epoch boundary) should be an InvalidStacksBlock error, not a NotFoundError
Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -132,6 +132,7 @@ pub struct StacksAccount {
#[derive(Debug, Clone, PartialEq)]
pub struct MinerPaymentSchedule {
pub address: StacksAddress,
pub recipient_contract: Option<QualifiedContractIdentifier>,
Copy link
Member

Choose a reason for hiding this comment

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

Is there any use for specifying a different StacksAddress as the recipient, rather than a contract? As in, what if miner Alice wanted coinbase rewards to go to Bob even if Bob isn't a contract?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this too, but I can't think of a compelling use-case. If Alice wanted to pay Bob the block reward, then why can't Alice just fund Bob's block-commit transaction?

Copy link
Member

Choose a reason for hiding this comment

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

Because they don't share keys -- like one situation could be that the coinbase rewards are going to a "cold" Stacks wallet, which you wouldn't want to use as the miner address (because the miner address must be hot).

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I've made it so that the coinbase can pay to either a contract or a standard principal.

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This looks good to me, I had mostly superficial comments, plus wanted to discuss whether or not we should allow the coinbase to specify a general principal or only contracts.

@gregorycoppola
Copy link
Contributor

Removing myself since there'll be two approvals when kantai's comments are addressed.

@gregorycoppola gregorycoppola removed their request for review June 22, 2022 19:03
@jcnelson jcnelson requested a review from kantai June 28, 2022 18:03
@jcnelson jcnelson mentioned this pull request Jun 28, 2022
…dules, grouped by functional requirements tested
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM!

@jcnelson
Copy link
Member Author

Thanks for the approvals! Let's get #3180 merged to this PR, and then I'll merge this all into next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants